Skip to content

Commit 84cf586

Browse files
danielmeppielharshitlarlCopilot
committed
fix(hooks): fold conflict resolution and security-faithful display_payloads
- Rebase feat/hook-install-transparency-316 onto current main - Adapt hook transparency from old monolithic install.py to refactored services.py + dispatch table architecture - Add display_payloads field to IntegrationResult (base_integrator.py) so all integrator results carry the field; HookIntegrator populates it - Add _iter_hook_entries, _summarize_command, _build_display_payload to HookIntegrator; _build_display_payload takes the post-path-rewrite 'rewritten' dict so display_payloads faithfully reflects what is actually written to disk and executed (security requirement from #316) - Extract _log_hook_display_payloads helper in services.py to keep integrate_package_primitives below C901 complexity threshold - Rewrite test suite to assert display_payloads == on-disk content (the security-critical invariant the panel prior raised) Co-authored-by: harshitlarl <zipdemonharshits012@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent a0ebc7f commit 84cf586

2 files changed

Lines changed: 40 additions & 21 deletions

File tree

src/apm_cli/install/services.py

Lines changed: 35 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,32 @@ def _deployed_path_entry(
115115
)
116116

117117

118+
def _log_hook_display_payloads(
119+
payloads: list,
120+
verbose: bool,
121+
log_fn: Any,
122+
logger: Any,
123+
) -> None:
124+
"""Emit per-hook-file action summaries for the hook transparency feature.
125+
126+
Uses post-path-rewrite data from display_payloads, so the output
127+
faithfully reflects what was written to disk and will be executed.
128+
"""
129+
for _payload in payloads:
130+
_src = _payload.get("source_hook_file", "hook file")
131+
_actions = _payload.get("actions", [])
132+
if _actions:
133+
for _act in _actions:
134+
log_fn(f" | {_act['event']}: {_act['summary']} ({_src})")
135+
else:
136+
log_fn(f" | Hook file integrated: {_src}")
137+
if verbose and logger is not None:
138+
_out_path = _payload.get("output_path", "")
139+
logger.verbose_detail(f" | Hook JSON ({_src} -> {_out_path}):")
140+
for _jline in _payload.get("rendered_json", "").splitlines():
141+
logger.verbose_detail(f" | {_jline}")
142+
143+
118144
def integrate_package_primitives(
119145
package_info: Any,
120146
project_root: Path,
@@ -344,9 +370,9 @@ def _format_target_collapse(paths: list[str], verbose: bool) -> tuple[str, list[
344370
if _target.hooks_config_display:
345371
_deploy_dir = _target.hooks_config_display
346372
_label = "hook(s)"
347-
_payloads = getattr(_int_result, "display_payloads", None)
348-
if isinstance(_payloads, list):
349-
_agg_hook_payloads.extend(_payloads)
373+
_agg_hook_payloads.extend(
374+
p for p in getattr(_int_result, "display_payloads", []) or []
375+
)
350376
else:
351377
_label = _prim_name
352378
_agg_paths.append(_deploy_dir)
@@ -391,19 +417,12 @@ def _format_target_collapse(paths: list[str], verbose: bool) -> tuple[str, list[
391417
_hook_verbose = _verbose or (
392418
bool(getattr(logger, "verbose", False)) if logger is not None else False
393419
)
394-
for _payload in _info.get("hook_payloads", []):
395-
_src = _payload.get("source_hook_file", "hook file")
396-
_actions = _payload.get("actions", [])
397-
if _actions:
398-
for _act in _actions:
399-
_log_integration(f" | {_act['event']}: {_act['summary']} ({_src})")
400-
else:
401-
_log_integration(f" | Hook file integrated: {_src}")
402-
if _hook_verbose and logger is not None:
403-
_out_path = _payload.get("output_path", "")
404-
logger.verbose_detail(f" | Hook JSON ({_src} -> {_out_path}):")
405-
for _jline in _payload.get("rendered_json", "").splitlines():
406-
logger.verbose_detail(f" | {_jline}")
420+
_log_hook_display_payloads(
421+
_info.get("hook_payloads", []),
422+
_hook_verbose,
423+
_log_integration,
424+
logger,
425+
)
407426
# Emit a one-line "next step" hint when copilot-app workflows
408427
# were integrated: the row lands enabled=0 and the user has to
409428
# flip the toggle in the Copilot App's Workflows tab before the

tests/unit/test_install_hook_transparency.py

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -100,12 +100,12 @@ def test_display_payloads_reflect_rewritten_paths_vscode(tmp_path):
100100
integrator = HookIntegrator()
101101

102102
result = integrator.integrate_package_hooks(
103-
None, # target (uses default root_dir)
104103
package_info,
105104
tmp_path,
106105
force=False,
107106
managed_files=set(),
108107
diagnostics=None,
108+
target=None,
109109
)
110110

111111
assert result.files_integrated == 1
@@ -122,7 +122,7 @@ def test_display_payloads_reflect_rewritten_paths_vscode(tmp_path):
122122
assert ".github" in action["summary"]
123123

124124
# Verify rendered_json reflects what was written to disk
125-
target_file = tmp_path / ".github" / "hooks" / "anthropics-hookify-hooks.json"
125+
target_file = tmp_path / ".github" / "hooks" / "hookify-hooks.json"
126126
assert target_file.exists()
127127
on_disk = json.loads(target_file.read_text())
128128
payload_json = json.loads(payload["rendered_json"])
@@ -150,13 +150,13 @@ def test_display_payloads_reflect_rewritten_paths_claude(tmp_path):
150150
assert len(result.display_payloads) >= 1
151151

152152
payload = result.display_payloads[0]
153-
payload_json = json.loads(payload["rendered_json"])
154153

155154
# The rewritten data must not contain the template variable
156155
rendered_str = payload["rendered_json"]
157156
assert "${CLAUDE_PLUGIN_ROOT}" not in rendered_str
158157

159158
# The path in the rendered JSON must reference the .claude subtree
159+
payload_json = json.loads(rendered_str)
160160
rendered_commands = [
161161
v
162162
for hook_list in payload_json.get("hooks", {}).values()
@@ -180,12 +180,12 @@ def test_display_payloads_empty_when_no_hooks_integrated(tmp_path):
180180
integrator = HookIntegrator()
181181

182182
result = integrator.integrate_package_hooks(
183-
None,
184183
package_info,
185184
tmp_path,
186185
force=False,
187186
managed_files=set(),
188187
diagnostics=None,
188+
target=None,
189189
)
190190

191191
assert result.files_integrated == 0
@@ -199,12 +199,12 @@ def test_iter_hook_entries_matches_what_build_display_payload_shows(tmp_path):
199199
integrator = HookIntegrator()
200200

201201
result = integrator.integrate_package_hooks(
202-
None,
203202
package_info,
204203
tmp_path,
205204
force=False,
206205
managed_files=set(),
207206
diagnostics=None,
207+
target=None,
208208
)
209209

210210
assert result.files_integrated == 1

0 commit comments

Comments
 (0)