Skip to content

ci: delete unwired pr-linkage-mcp-gate and assert hook wiring liveness - #3098

Merged
kyle-sexton merged 2 commits into
mainfrom
cursor/2959-2960-hook-liveness-cfcf
Aug 21, 2026
Merged

ci: delete unwired pr-linkage-mcp-gate and assert hook wiring liveness#3098
kyle-sexton merged 2 commits into
mainfrom
cursor/2959-2960-hook-liveness-cfcf

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Closes #2959
Closes #2960

Summary

Delete the dead repo-local hook .claude/hooks/pr-linkage-mcp-gate.sh (and its unused test) and add a hygiene-lane wiring-liveness check so the same class cannot ship green again.

#2188 stripped project hook wiring as a bare-baseline reset: an instruction returns only with ledger evidence. #2655 restored SessionStart only. The leftover script kept claiming it loaded in every session while .claude/settings.json no longer named it. Policy enforcement already survives via the source-control plugin hook plus required CI pr-issue-linkage. This PR does not rewire the stripped hook.

The new check requires every .claude/hooks/*.sh except *.test.sh to be referenced by settings.json hook commands (or args) or its env block, which is how hook-telemetry-sink.sh stays live via HOOK_TELEMETRY_SINK.

Fix

  • Delete .claude/hooks/pr-linkage-mcp-gate.sh and .claude/hooks/pr-linkage-mcp-gate.test.sh. Nothing else invoked the test, and the plugin copy plus required CI remain the enforcement path.
  • Add scripts/check-hook-wiring-liveness.sh in the hygiene lane: every non-test .claude/hooks/*.sh must appear in settings.json hook commands/args or env, with a bounded path-segment match so not-gate.sh does not satisfy gate.sh.
  • Self-test first (scripts/check-hook-wiring-liveness.test.sh), then the gate, then feed hook-wiring-liveness into the existing hygiene aggregator.

Test plan

  • bash scripts/check-hook-wiring-liveness.sh on the pre-delete tree (dead script still present) exits 1 and names .claude/hooks/pr-linkage-mcp-gate.sh.
  • After deletion, the same command exits 0.
  • bash scripts/check-hook-wiring-liveness.test.sh — all assertions passed, including the pre-delete replica, *.test.sh exclusion, command- and args-form wiring, bounded basename match, fail-closed missing/invalid settings, and a live-checkout pin.
  • bash plugins/source-control/hooks/pr-linkage-mcp-gate.test.sh — 24/24 (plugin copy unchanged).
  • bash .claude/hooks/hook-telemetry-sink.test.sh — PASS.
  • shellcheck on the new scripts — clean.
  • bash scripts/affected-tests.sh .github/actionlint.yaml — empty selection (the new suite no longer names a file that chains from that probe).

Verification

Pre-delete run of the new gate exited 1 naming pr-linkage-mcp-gate.sh. Post-delete run exited 0. The contract suite passed locally, including the #2959 replica and the bounded-match case. Plugin MCP-gate tests stayed 24/24. The first CI cycle failed on typos (unparseable), changelog-parity (comment-only plugin edits), plugin-gate (affected-tests R3/R4 via a bootstrap filename), and PR-body ## Fix / ## Verification sections; those are the fixes in this update.

Related

#2188 stripped repo-local hook wiring; the leftover script kept claiming
enforcement. Delete it (policy already lives in the plugin hook + required
CI) and fail hygiene when a .claude/hooks/*.sh is not referenced by
settings.json commands or env.

Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 20s —— View job


Claude finished the code review

  • Gather context (PR diff, changed files, CLAUDE.md/REVIEW.md)
  • Review new scripts/check-hook-wiring-liveness.sh + test
  • Review CI workflow changes (.github/workflows/ci.yml)
  • Review plugin hook changes (pr-linkage-mcp-gate.sh, pr-linkage-validator.sh)
  • Post findings (inline + summary)

Summary

No blocking or high-signal findings. This is the code-review lane (security findings are out of scope here since claude-security-review.yml exists in this repo) and the change is CI/hook tooling with no security surface anyway.

Reviewed:

  • scripts/check-hook-wiring-liveness.sh (blob) — jq-based extraction of env values + recursive .hooks command/args strings, then a literal-substring (grep -F) match per hook script against full path and basename. Fails closed on missing/unparseable settings.json (exit 2) and on any unwired script (exit 1). Logic is sound; scope is intentionally non-recursive (.claude/hooks/*.sh, no subdirs), matching the header comment.
  • scripts/check-hook-wiring-liveness.test.sh — good black-box coverage: happy path, the exact Dead repo-local hook: .claude/hooks/pr-linkage-mcp-gate.sh is unwired since #2188 but its header still claims enforcement #2959 pre-delete failure replica, *.test.sh exclusion, command-form and exec-form (args) wiring, fail-closed missing/unparseable settings, and a live-checkout pin (test 9) that reruns the SUT in place rather than only against fixtures.
  • .github/workflows/ci.yml — new self-test step (no continue-on-error, matching the repo's "self-test-first" pattern for other detectors) followed by the gate step (id: hook_wiring, continue-on-error: true, fed into aggregate-hygiene-results.sh via CHECK_RESULTS), consistent with every other whole-repo scanner already wired the same way (shellcheck, exec_bit, etc.). Comment updates accurately describe the new unconditional (non-docs-gated) scanner.
  • plugins/source-control/hooks/pr-linkage-mcp-gate.sh / pr-linkage-validator.sh — comment-only updates reflecting that the marketplace repo no longer carries a repo-local copy; the defer-guard logic itself is unchanged and still correct (it still checks settings.json generically for any consuming repo that does wire a repo-local copy, not just this one).

Both new scripts carry the 100755 exec bit in the tree, satisfying the repo's exec-bit gate.

One non-blocking note: this sandbox's checkout has stray untracked copies of the two deleted files (.claude/hooks/pr-linkage-mcp-gate.sh, .claude/hooks/pr-linkage-mcp-gate.test.sh) still present on disk (confirmed via git ls-files — they are not tracked). That's an artifact of this review environment's checkout, not of the commit itself (git show HEAD --stat confirms both are cleanly deleted in fd233cc), so I did not treat it as a finding — flagging only in case it's useful signal about the runner image.

I did not independently execute check-hook-wiring-liveness.sh/.test.sh or shellcheck myself (no shell-exec tool access in this pass) — the PR's Test Plan results are author-claimed and unverified by me.

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 19s —— View job


Security Review

  • Gather context (PR diff, changed files)
  • Check skip-gate conditions
  • Review changed files for security-relevant issues
  • Adversarial validation of any candidate findings
  • Post final findings

No security issues found.

This PR is CI/tooling-only: it deletes an already-unwired repo-local hook (.claude/hooks/pr-linkage-mcp-gate.sh + its test) and adds a new hygiene-lane check (scripts/check-hook-wiring-liveness.sh) that fails CI if any .claude/hooks/*.sh script (other than *.test.sh) isn't referenced in .claude/settings.json.

Verification performed:

  • Confirmed via .claude/settings.json that pr-linkage-mcp-gate.sh was in fact not referenced in any hook command, args, or env entry before this PR — its deletion removes no live enforcement. PR linkage policy still runs via the plugins/source-control plugin hook and the required CI pr-issue-linkage check, as the PR description states.
  • scripts/check-hook-wiring-liveness.sh only reads repo-controlled inputs (.claude/settings.json, filenames under .claude/hooks/) — no untrusted/external data, no command construction from variable content beyond grep -F literal matching, no injection surface.
  • .github/workflows/ci.yml changes add two static run: steps (no ${{ github.event.* }} or other untrusted-context interpolation into run: bodies) and wire the new check's outcome into aggregate-hygiene-results.sh, which already fails closed on any non-success outcome — so the new check can't be silently ignored.
  • plugins/source-control/hooks/pr-linkage-mcp-gate.sh and pr-linkage-validator.sh changes are comment-only (updating the defer-guard rationale now that the repo-local copy is gone); no logic changed.
  • Workflow permissions remain contents: read at the top level; nothing in this diff widens permissions, touches secrets, or adds pull_request_target/workflow_run triggers.

No CRITICAL, IMPORTANT, or SUGGESTION findings to report.

@github-actions

Copy link
Copy Markdown

Last security-reviewed head: fd233ccfb97c980d30297e21dff1249fdaf32193. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fd233ccfb9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/check-hook-wiring-liveness.sh Outdated
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

kyle-sexton added a commit that referenced this pull request Aug 21, 2026
…3100)

## Summary

Compress the session-flow handoff instruction walk (`save-point.md`,
`structure.md`, `skills/handoff/SKILL.md`) under docs-hygiene
flavor-only discipline. Re-review the Stop-hook escalation from #3018;
no defect found, hook not shipped.

## Fix

Author-time compression of the three named files: drop filler and
verbose verbs that do not carry contract meaning. Preserve find-handoff
detection contract (rails, copy instruction, `Read @…-handoff-…`
directive, `Re-arm <i> of <n> — <L> lines:`), redaction rules,
rooted-path / `Handoff origin:` rationale, and STOP / output-order
contract. Bump session-flow 0.32.2 → 0.32.3. Stop-hook re-review: 0.26.1
already shipped the STOP/output-order contract; one observed occurrence;
find-handoff rung 1 still recovers file-mode; the 0.22% cut does not
change occupancy. No hook change.

## Test plan

- [x] `markdownlint-cli2` on the three compressed files + CHANGELOG: 0
issues
- [x] `scripts/changelog-parity.sh --check --check-bump origin/main
--check-order --check-preserved origin/main`
- [x] Contract tokens remain: rails, copy instruction, Read
@…-handoff-…, Re-arm header, redaction markers, STOP / output-order
- [x] Byte delta vs author-time snapshots: 188 B / 0.22% (save-point 152
B, structure 27 B, SKILL 9 B)

## Verification

- `markdownlint-cli2` on the three files + CHANGELOG: 0 issues.
- Changelog-parity `--check`, `--check-bump origin/main`,
`--check-order`, `--check-preserved origin/main`: all pass.
- Contract-token grep: rails, copy instruction, `Read @…-handoff-…`,
`Re-arm <i> of <n> — <L> lines:`, redaction markers, STOP / output-order
all still present.
- Byte savings vs `/tmp/wt-3018-snap`: save-point 40831→40679 (152 B),
structure 23296→23269 (27 B), SKILL 19641→19632 (9 B); total 188 B /
0.22%.

## Related

- Closes #3018
- Follows #3097 / #3098 / #3099
- Stop-hook re-review; no defect — hook not shipped

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Drop the comment-only source-control edits that reused a published
version. Bound hook-name matching so a longer sibling cannot satisfy a
shorter basename. Stop naming the SessionStart bootstrap in the new
scripts (it chained affected-tests off actionlint.yaml). Use "invalid
JSON" instead of the typos-flagged spelling.

Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown

Claude finished @cursor[bot]'s task in 2m 32s —— View job


Code review

  • Gather context (PR diff, changed files, CLAUDE.md/REVIEW.md)
  • Review diff since last reviewed head (fd233ccc94b7d1)
  • Check whether Codex's bounded-match finding was addressed
  • Review new/changed scripts and CI wiring
  • Post findings (inline + summary)

Summary

No blocking or high-signal findings. This PR was already reviewed by this lane at fd233cc (comment above); commit c94b7d1 adds no CI-workflow changes and only touches the three files below to close review feedback, so I re-scoped to that diff rather than re-reviewing the whole PR.

Codex's P2 finding (unbounded grep -F substring match letting not-gate.sh wiring satisfy an unwired gate.sh) is fixed. scripts/check-hook-wiring-liveness.sh#L57-L69 replaces the raw substring checks with hook_ref_matches(): an exact full-path substring check, falling back to a basename match bounded by (^|[/\\ "'])...([/\\ "']|$). I traced this by hand against the new fixture in check-hook-wiring-liveness.test.sh#L135-L150 (gate.sh + not-gate.sh, only the latter wired via env): the full-path check correctly fails to match (.claude/hooks/gate.sh is not a substring of .claude/hooks/not-gate.sh), and the bounded regex correctly rejects gate.sh since the character preceding it in not-gate.sh (-) isn't a boundary character — so gate.sh is still reported unwired as intended. I did not execute the test myself (no shell-exec tool access in this pass); this is static verification of the fixture logic, not a run.

Also checked:

  • .github/workflows/ci.yml has no diff between the two heads — no re-review needed there.
  • No dangling references to the deleted .claude/hooks/pr-linkage-mcp-gate.sh/.test.sh remain in any script logic; scripts/run-plugin-tests.sh discovers .claude/hooks/*.test.sh dynamically via glob, so the deletion doesn't break it (the file's mention there is historical comment text only).
  • This sandbox's checkout again has stray untracked copies of the two deleted files on disk (git ls-files confirms untracked) — same checkout artifact noted in the prior review pass, not a defect in the commit itself.

@kyle-sexton
kyle-sexton merged commit dd86804 into main Aug 21, 2026
50 checks passed
@kyle-sexton
kyle-sexton deleted the cursor/2959-2960-hook-liveness-cfcf branch August 21, 2026 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants