feat: surface installed hook actions during apm install#409
feat: surface installed hook actions during apm install#409harshitlarl wants to merge 3 commits intomicrosoft:mainfrom
Conversation
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Adds install-time transparency for hook packages so developers can see what hook actions will run and (in verbose mode) inspect the deployed hook JSON content, addressing supply-chain visibility concerns from #316.
Changes:
- Emit per-event hook action summaries during
apm installfor integrated hooks. - In
--verbosemode, print the fully rewritten hook JSON that will be deployed/merged. - Add focused unit tests covering the new hook transparency output.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/apm_cli/integration/hook_integrator.py |
Adds hook action flattening/summarization and returns CLI display payloads alongside integration results. |
src/apm_cli/commands/install.py |
Logs hook action summaries and verbose hook JSON content during install. |
tests/unit/test_install_hook_transparency.py |
New unit tests validating the new install-time hook transparency output (normal + verbose). |
| return { | ||
| "target_label": target_label, | ||
| "output_path": output_path, | ||
| "source_hook_file": source_hook_file.name, | ||
| "actions": actions, | ||
| "rendered_json": json.dumps(rewritten, indent=2, sort_keys=True), | ||
| } |
There was a problem hiding this comment.
rendered_json is precomputed for every integrated hook via json.dumps(...), even when apm install is not in --verbose mode. This adds avoidable work/memory on the hot install path. Consider deferring JSON rendering until verbose logging is actually requested (e.g., store the rewritten dict, or store a callable/None and render in the logger code only when needed).
| "output_path": output_path, | ||
| "source_hook_file": source_hook_file.name, | ||
| "actions": actions, | ||
| "rendered_json": json.dumps(rewritten, indent=2, sort_keys=True), |
There was a problem hiding this comment.
The verbose display payload uses json.dumps(..., sort_keys=True), but the deployed JSON files are written with json.dump(..., indent=2) (no sort_keys). This means --verbose will show a key order that can differ from what was actually deployed, which undermines the goal of letting developers review the exact hook content. Align the verbose rendering with the on-disk serialization (or reuse the same serialization helper for both).
| "rendered_json": json.dumps(rewritten, indent=2, sort_keys=True), | |
| "rendered_json": json.dumps(rewritten, indent=2), |
| display_payloads.append( | ||
| self._build_display_payload( | ||
| ".github/hooks/", | ||
| target_filename, |
There was a problem hiding this comment.
For VSCode/GitHub hooks, the display payload sets output_path to target_filename only. This makes the verbose line render as hooks.json -> <filename> without the .github/hooks/ prefix, unlike the Claude/Cursor cases that include the full target path. Consider storing the full relative destination path (e.g. .github/hooks/<filename>) or using target_label when formatting the destination so the output is unambiguous.
| target_filename, | |
| rel_path, |
| if logger.verbose: | ||
| logger.verbose_detail( | ||
| f" Hook JSON ({source_name} -> {payload['output_path']}):" | ||
| ) | ||
| for line in payload["rendered_json"].splitlines(): | ||
| logger.verbose_detail(f" {line}") |
There was a problem hiding this comment.
The new verbose hook transparency (Hook JSON ... + full rewritten JSON) changes what --verbose emits during apm install, but the CLI docs currently describe verbose mode only in terms of file paths + diagnostic details. Please update the Starlight docs (e.g. docs/src/content/docs/reference/cli-commands.md under apm install) to mention that verbose mode also prints the full rewritten hook JSON so users can review deployed hook content.
|
Great work on this, @harshitlarl! Hook transparency is a P0 security feature (#316) and we want to get this merged. A few things to address: Rebase required
Code feedback
We can helpWe know the rebase is non-trivial given the architecture changes. If you'd like, we're happy to push the rebase to your branch directly (maintainer edits are enabled). Just let us know — otherwise, take your time and reach out if you hit any blockers. |
…t webhooks (#865) * ci: add merge-gate orchestrator (shadow) + stuck-PR watchdog PR #856 surfaced a structural CI fragility: required status checks are satisfied by two independent webhook channels (pull_request emits 'Build & Test (Linux)', pull_request_target emits the four Tier 2 stubs). When the pull_request delivery is dropped, 4/5 stubs go green and the 5th hangs in 'Expected -- Waiting' forever -- ambiguous yellow indistinguishable from a slow build. Recovery is folklore. This PR ships two safety nets in shadow mode: * .github/workflows/merge-gate.yml + scripts/ci/merge_gate_wait.sh Single orchestrator that polls the Checks API for 'Build & Test (Linux)' on the PR head SHA and aggregates into one verdict. Triggers on BOTH pull_request and pull_request_target so a single dropped delivery is recoverable; concurrency dedupes. Times out cleanly with a clear error message if Tier 1 never dispatched -- turning the invisible failure into a loud red check. NOT YET REQUIRED -- shadow observation first, ruleset flip after merge. * .github/workflows/watchdog-stuck-prs.yml + scripts/ci/watchdog_scan.sh Cron every 15 min. For open non-draft PRs with no ci.yml run on the head SHA AND non-paths-ignored files, posts one recovery comment. Backstop for any required check that stops dispatching. Tested live (dry-run) against microsoft/apm: watchdog correctly distinguishes stuck PRs (#853, #409) from docs-only PRs (#864, #461, #825). Both scripts shellcheck-clean. merge_gate_wait.sh tested end-to-end against PR #856 head SHA (success path, exit 0) and a non-existent SHA (timeout path, exit 2 with clear error annotation). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci(merge-gate): handle self-bootstrap and use checkout on pull_request Two fixes for the script-fetch step: 1. On 'pull_request' the runner has no secrets and a read-only token, so actions/checkout of PR head is safe -- use it for simplicity. We only need API-fetch under 'pull_request_target' where checkout would be a security risk. 2. On 'pull_request_target', when the script does not yet exist on base (i.e. THIS PR is the one adding it), curl returns 404 and we degrade to a self-bootstrap no-op pass instead of failing. Once the script lands on main, the gate becomes real. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: address Copilot review feedback on PR #865 - merge_gate_wait.sh: use $EXPECTED_CHECK in failure annotation instead of hardcoded 'Build & Test (Linux)' so the orchestrator stays generic. - merge-gate.yml: add curl --retry/--max-time on the script bootstrap fetch so a transient GitHub API blip does not redden the gate. - watchdog_scan.sh: fail loudly with stderr capture if 'gh pr list' errors out, instead of silently treating it as zero PRs (which would hide auth regressions or rate limiting). - watchdog_scan.sh: paginate the changed-files API call so PRs touching >100 files cannot be misclassified as docs-only and skipped. - CHANGELOG: append (#865) to follow the repo convention. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: drop watchdog -- gate's dual-trigger redundancy is sufficient The watchdog (cron every 15min, posts recovery comments on stuck PRs) was originally justified for the shadow-mode transition window. Since we are flipping to required immediately after this PR merges, that justification disappears. The merge-gate workflow already triggers on both 'pull_request' and 'pull_request_target' with concurrency dedup, so a single dropped webhook still fires the gate. The watchdog only added value for the double-drop case (both webhook channels fail for the same PR), which is vanishingly rare. We can add it back later if we ever observe one. Removes: - .github/workflows/watchdog-stuck-prs.yml - .github/scripts/ci/watchdog_scan.sh Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Testing
Closes #316