feat(rewrite-guard): deepen the content-mutation disclosure protocol into a shared vendored lib

[kyle-sexton]

Closes #3405, closes #3406, closes #3409

Summary

Six mutating hook plugins each hand-rolled the content-mutation disclosure protocol (snapshot, compare, disclose, release), and the copies drifted defect by defect: go-format leaked its snapshot on both error arms (#3405) and ruff/biome/bash printed the rewrite disclosure as a second JSON document on rewrite-plus-findings runs (#3406). This PR deepens the protocol into one vendored module and adopts it in all six hooks, closing both defect classes through the shared interface.

Fix

• New ADR-0019 vendored cluster hooks/rewrite-guard.sh (canonical lib/rewrite-guard.sh, sync gate scripts/sync-rewrite-guard.sh, registry row, CI job rewrite-guard-sync). Three entry points: hook::rewrite_guard_begin (best-effort snapshot; arms an EXIT trap so release is structural on every arm, including the cp-failure orphan every hand-rolled copy leaked), hook::rewrite_take_disclosure (destructive read: sets HOOK_REWRITE_MESSAGE, releases the snapshot; callers pass it as the systemMessage of their one hook::emit_channels call), and hook::rewrite_disclose (take-and-emit for arms with no context of their own).
• Adopted in all six hooks: go-format's per-arm blocks (fixing the go-format: _go_before snapshot temp file leaks on the syntax-error and tool-break arms #3405 leak and composing the error arms' output as one document), ruff/biome's immediate-emit blocks and bash's mid-run emit (fixing ruff/biome/bash-format: two JSON documents on stdout when one run both rewrites and reports findings #3406 by taking the disclosure at each exit arm and composing it with that arm's context), powershell's inline fix(powershell-format): release the _ps_before snapshot and disclose a rewrite on the tool-break arm #3401 template (generalized, behavior unchanged), and eol-normalizer's hand-rolled cmp (the taken message doubles as the EFFECTIVE_ACTION verdict).
• Version bumps and changelog entries for all six carrying plugins.

Verification

• bash lib/rewrite-guard.test.sh — 13/13 (contract: release on every arm incl. non-zero exits, destructive read, no orphan on failed snapshot, single-document composition).
• Plugin suites with new regression cases: go-format 54/54 (snapshot-hygiene cases under isolated TMPDIR for the syntax-error and tool-break arms), ruff-format 56/56, biome-format 46/46, bash-format 51/51 (each with a rewrite-plus-findings case asserting exactly one JSON document via jq -s), powershell-format 84/84 (full behavioral run with PSScriptAnalyzer), eol-normalizer 36/36.
• Mutation checks: the pre-change go hook leaks exactly 1 file on the syntax-error arm and the pre-change ruff hook emits exactly 2 JSON documents on a rewrite-plus-findings run, proving the new assertions catch the old behavior.
• scripts/affected-tests.sh --run — 121 shell suites passed; the selected Python and Node lanes run green locally (the knowledge vitest suite needs its own lane's deps and is untouched by this change).
• scripts/sync-rewrite-guard.sh --check / --check-bump, scripts/sync-hook-utils.sh --check, scripts/check-cross-plugin-source-drift.sh --check, scripts/check-changelog-parity.sh --check + --check-bump + --check-order, actionlint, shellcheck, shfmt -d, typos, markdownlint-cli2 — all clean, re-verified after merging origin/main (6ec8455).

Related

• Refs formatter hooks: nine siblings duplicate the PostToolUse prologue as structure the drift gate cannot see (deepening candidate, top recommendation) #3408 (formatter-prologue deepening, the scan's top recommendation — this cluster establishes the vendoring pattern it needs), fix(powershell-format): release the _ps_before snapshot and disclose a rewrite on the tool-break arm #3401/powershell-format: _ps_before temp file leaks on the trust-gate and tool-break arms, and a rewrite is undisclosed on tool break #3366 (the powershell instance this generalizes), ADR 0019 (vendoring-with-sync-gate mechanism).
• Found and shaped by the /architecture:improve deepening scan; candidate record in the topic's memory slice.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TwGsRVvv8izGrnpQXnDB4B

---

Generated by Claude Code

[claude]

Claude finished @kyle-sexton's task in 2m 40s —— View job

---

Code review — /review:code-review

• Gather context (diff, changed files, CLAUDE.md/AGENTS.md conventions)
• Invoke /review:code-review and follow its instructions
• Post inline findings (if any) via inline comments
• Final summary update

This repository carries a .github/workflows/claude-security-review.yml lane, so per REVIEW.md this pass stays scoped to correctness/design/maintainability and omits security findings.

Scope reviewed: lib/rewrite-guard.sh + lib/rewrite-guard.test.sh (new shared lib and its 13-case unit suite), the six adopting hooks (bash-format, biome-format, eol-normalizer, go-format, powershell-format, ruff-format), scripts/sync-rewrite-guard.sh, the scripts/cross-plugin-source-registry.txt entry, and the rewrite-guard-sync CI job.

Findings: none. I read the vendored lib's snapshot/take/release lifecycle, traced every exit arm in all six hooks against it, and diff'd each hook against its pre-change version:

• All six plugins/*/hooks/rewrite-guard.sh copies share the same blob SHA as lib/rewrite-guard.sh (dc06502e), consistent with the sync-gate contract mirrored from sync-hook-utils.sh.
• None of the six hooks set their own EXIT trap, so the guard's structural release trap is the only one in play — no conflict with the "a hook with its own EXIT trap must chain the release" caveat documented in the lib.
• go-format.sh's three arms (clean, syntax-error, tool-break) and ruff-format.sh's three arms each call hook::rewrite_take_disclosure/hook::rewrite_disclose exactly once before their single hook::emit_channels/exit, closing the go-format: _go_before snapshot temp file leaks on the syntax-error and tool-break arms #3405 leak class the new snapshot-hygiene test cases (go-format.test.sh Case 7b/7c) assert directly.
• biome-format.sh and ruff-format.sh's previous hand-rolled paths emitted the mutation disclosure via a bare hook::emit_system_message call and a separate hook::ctx_flush, i.e. two JSON documents on a rewrite-plus-findings run — confirmed by diffing against the pre-change hook. The new code takes the disclosure into a variable and composes it through one hook::emit_channels call per arm, matching the new jq -s 'length' == 1 regression cases added to biome-format.test.sh/ruff-format.test.sh.
• eol-normalizer.sh's refactor is behavior-preserving: the old inline cmp -s "$FILE" "$_eol_tmp" check and the new hook::rewrite_take_disclosure both compare the same pre/post snapshot, and EFFECTIVE_ACTION is still derived from "did the bytes actually change," not from the tool's echoed action alone (idempotent normalization can echo lf/crlf without changing bytes).
• powershell-format.sh generalizes its existing fix(powershell-format): release the _ps_before snapshot and disclose a rewrite on the tool-break arm #3401 take/compose pattern without behavior change; every non-formatter-reached exit arm (3/5/6) correctly relies on the guard's EXIT trap since the trust gate and missing-pwsh/module checks run before Invoke-Formatter.
• CI wiring (rewrite-guard-sync job) and the registry entry exactly mirror the existing hook-utils-sync job/entry pattern, including the --check, --check-bump, and shared-lib-test steps, and the job is added to the aggregate needs: gate.

Not independently verified: I could not execute bash lib/rewrite-guard.test.sh, the plugin test suites, or scripts/affected-tests.sh --run myself — Bash tool invocations in this environment required interactive approval that wasn't available. The PR body's test/lint results are author-claimed and unverified by me; the analysis above is based on static reading of the lib, its test file, and all six hook diffs against origin/main.

Branch: claude/architecture-improve-5cle8j

[claude]

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

---

Security review — PR #3452

• Gather context (PR diff, changed files, custom instructions)
• Invoke /review:security-review skill
• Post findings to this comment

Scope: reviewed the full diff (git diff origin/main...HEAD, 33 files) at head 830b52a — the new lib/rewrite-guard.sh module and its per-plugin copies, the six adopting hooks (go-format, ruff-format, biome-format, bash-format, powershell-format, eol-normalizer), the new rewrite-guard-sync CI job, and scripts/sync-rewrite-guard.sh.

No security issues found.

Notes from the review (informational, not findings):

• The new hook::rewrite_guard_begin/hook::rewrite_take_disclosure snapshot lifecycle uses mktemp (unpredictable name, default 0600 perms) the same way every hand-rolled copy it replaces did — no new TOCTOU or information-disclosure exposure, and the EXIT-trap-based release actually closes the orphaned-temp-file leak the PR describes (go-format: _go_before snapshot temp file leaks on the syntax-error and tool-break arms #3405).
• powershell-format.sh's security-sensitive CustomRulePath trust-gate (content-addressed approval signature, fail-closed on unverifiable/unpinnable rule modules) is untouched by this PR — diffed it directly against origin/main to confirm only the disclosure plumbing around it changed, not the gate logic itself, matching the PR description ("generalized, behavior unchanged").
• The new rewrite-guard-sync CI job in .github/workflows/ci.yml follows the existing hook-utils-sync job's pattern: pinned action SHA, persist-credentials: false, and github.base_ref passed through an env: var rather than interpolated into the run: script — no template-injection surface.
• All rewrite-disclosure messages that carry a basename "$FILE" are passed through hook::emit_channels/hook::json_escape (pre-existing, unchanged by this PR) rather than string-concatenated into anything executed, so no injection path from an attacker-controlled filename.
• No new command execution, network calls, credential handling, or permission changes anywhere in the diff.

[github-actions]

Last security-reviewed head: 3e6e8f0139f03bcd3fa863bb0a1a48b944887cd5. 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]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 830b52a3a2

ℹ️ 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".

[github-actions]

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

[claude]

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

---

Security review complete

• Gather context (PR diff, changed files)
• Check skip gate (open PR, HEAD c101805 matches; no security-relevant surface found after review)
• Review vendored lib/rewrite-guard.sh and its six per-plugin copies for trust-boundary / injection issues
• Review .github/workflows/ci.yml changes for Actions security issues
• Review scripts/sync-rewrite-guard.sh and registry changes

No security issues found.

This PR centralizes a content-mutation disclosure protocol (snapshot → compare → disclose → release) that six formatter hooks previously hand-rolled, into one vendored lib (lib/rewrite-guard.sh, copied into each carrying plugin's hooks/rewrite-guard.sh), and fixes two correctness bugs (temp-file leaks on certain exit arms, and double-JSON-document emission on runs that both rewrote a file and reported findings). I checked it against the categories in scope for this lane:

• Trust boundaries / injection: No new untrusted input is introduced. $FILE is the same tool-provided path the hand-rolled predecessors already snapshotted and formatted; this PR only moves the snapshot/compare/cleanup lifecycle into a shared function, it doesn't change what's trusted or how the path is used. mktemp is used correctly (atomic, exclusive creation — no symlink-race window) in lib/rewrite-guard.sh#L61.
• Snapshot/cleanup correctness: Verified the EXIT trap set in hook::rewrite_guard_begin (lib/rewrite-guard.sh#L64) isn't clobbered by a competing trap in any of the six adopting hooks (none of them set their own EXIT trap), so the release is structural as claimed and doesn't reopen the fix(powershell-format): release the _ps_before snapshot and disclose a rewrite on the tool-break arm #3401/go-format: _go_before snapshot temp file leaks on the syntax-error and tool-break arms #3405 leak class.
• CI workflow (ci.yml): The new rewrite-guard-sync job uses the same pinned actions/checkout SHA already used elsewhere in the workflow, persist-credentials: false, standard pull_request trigger (not pull_request_target/workflow_run), and runs no untrusted code from the PR beyond the repo's own shell scripts. No permission widening or secret exposure.
• scripts/sync-rewrite-guard.sh / registry: Plain build-time consistency checks (diff copies against source, verify version bumps) — no network calls, no untrusted input, mirrors the pre-existing sync-hook-utils.sh pattern.

Traced the logic for all six hooks' exit arms (go, ruff, biome, bash, powershell, eol-normalizer) and confirmed each emit_skipped/exit path either takes-and-composes the disclosure or relies on the guard's trap for release — no arm reintroduces a leaked snapshot or a stray second JSON document.

This is an internal dev-tooling hygiene refactor with no change to what's trusted, no new attack surface, and no security-relevant behavior change — findings are all pre-existing correctness/architecture ones already called out in the PR description, not new to this review.

[claude]

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

---

Claude finished the code review

• Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md conventions)
• Understand the request (invoke /review:code-review per repo instructions)
• Review lib/rewrite-guard.sh + lib/rewrite-guard.test.sh (canonical protocol)
• Review the six vendored hooks/rewrite-guard.sh copies + adoption sites
• Review scripts/sync-rewrite-guard.sh, registry, CI job wiring
• Post findings

Summary

This is a clean, well-scoped extraction. I verified independently (not just restating the PR body):

• All six plugins/*/hooks/rewrite-guard.sh copies are byte-identical to lib/rewrite-guard.sh (diff -q, confirmed for bash/biome/eol-normalizer/go/powershell/ruff-format).
• The rewrite-guard-sync CI job (.github/workflows/ci.yml lines 534–552) mirrors the existing hook-utils-sync job exactly and is wired into ci-status's needs: list (line 1609), so a drifted copy or unbumped carrying plugin actually fails the gate.
• scripts/sync-rewrite-guard.sh follows the shared sync-cluster.sh pattern byte-for-byte (only the names differ from sync-hook-utils.sh), and scripts/affected-tests.sh discovers it generically via scripts/sync-*.sh + --print-manifest, so a lib/rewrite-guard.sh change correctly fans out to every carrying plugin's suite without any hardcoded mapping.
• All six plugin manifests got a patch version bump (plugin.json).
• Traced the two defect classes the PR claims to fix through the actual diffs:
  • go-format: _go_before snapshot temp file leaks on the syntax-error and tool-break arms #3405 (leaked snapshot on error arms): go-format's syntax-error and tool-break arms (plugins/go-format/hooks/go-format.sh:278, :296) now call hook::rewrite_take_disclosure before hook::emit_channels, and the guard's EXIT trap (lib/rewrite-guard.sh:64) is a structural backstop for any arm that doesn't call take at all. Confirmed no other hook in this set sets a competing trap ... EXIT that the guard's trap would silently clobber.
  • ruff/biome/bash-format: two JSON documents on stdout when one run both rewrites and reports findings #3406 (two JSON documents on rewrite+findings): ruff/biome/bash-format all now take the disclosure once and route it through the single hook::emit_channels call for that arm (e.g. ruff-format.sh:270-271, biome-format.sh:228 + reused across the findings/ignored/tool-break arms below it, bash-format.sh:240 + single emission point at :301/:303). hook::emit_channels (lib/hook-utils.sh:80) composes both channels into one object, and telemetry (hook::emit_telemetry) writes to a separate sink, not stdout, so there's no second document.
• New regression tests are non-vacuous: go-format's snapshot-hygiene cases run under an isolated TMPDIR and assert both "zero leftover files" and "exactly one JSON document" per arm; bash-format's rewrite+findings case asserts one document carrying both SC2154 and the reformat message.
• eol-normalizer's reuse of the taken message as EFFECTIVE_ACTION's changed/unchanged signal (eol-normalizer.sh:118-123) is sound: normalize_eol_file only ever prints skip when it left the file untouched, so the empty-message-on-skip path can't diverge from the guard's own byte comparison.

No blocking findings. I couldn't execute the shell test suites directly in this sandbox (script execution required approval that wasn't available here), so the specific pass counts in the PR's Verification section (e.g. "13/13", "54/54") are author-claimed and not independently re-run by me — the static trace above is what I verified myself.

Branch: claude/architecture-improve-5cle8j

[claude]

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

---

Security review — PR #3452

• Gather context (PR diff, changed files, custom instructions)
• Invoke /review:security-review skill and follow its instructions exactly
• Post findings to this comment

Skip gate: PR is open. Per the auto-comment above, the last security review covered head c101805; this run's HEAD is f1774d2, so only the commit(s) since c101805 are in scope: f1774d2 — "fix(rewrite-guard): chain a caller's existing EXIT trap instead of replacing it."

Diff reviewed: git diff c101805..f1774d2 — touches only lib/rewrite-guard.sh (+ its 6 synced plugin copies) and lib/rewrite-guard.test.sh. This is the fix for the Codex P2 finding on the prior review round (begin's trap ... EXIT was clobbering a caller's pre-existing EXIT trap instead of chaining it).

No security issues found.

What changed and why it doesn't open new surface:

• hook::rewrite_guard_begin now captures any pre-existing EXIT trap via trap -p EXIT, strips the trap -- '...' EXIT framing, and stores the inner command string in _HOOK_REWRITE_PREV_EXIT_TRAP. The new hook::_rewrite_guard_on_exit handler releases the snapshot first, then evals that captured string so the caller's original cleanup still runs. See lib/rewrite-guard.sh#L79-L96.
• Both eval calls (eval "prev=$prev" and eval "$_HOOK_REWRITE_PREV_EXIT_TRAP") operate on a string that originates from the shell's own trap -p EXIT output — i.e., code the hook script itself previously installed via its own trap ... EXIT call earlier in the same process. Nothing attacker- or tool-input-controlled (file paths, tool output, JSON payloads) flows into this string, so this isn't a new injection surface; it's re-installing trusted, already-in-process trap code, which is exactly what "chain instead of replace" requires.
• Guarded against self-chaining: if [[ "$prev" != "hook::_rewrite_guard_on_exit" ... ]] prevents a second hook::rewrite_guard_begin call from wrapping the handler around itself (which would otherwise recurse or double-release).
• No changes to trust boundaries, input handling, CI workflow permissions, or any of the six hooks' formatter-invocation logic — this commit is scoped entirely to the shared lib's trap bookkeeping and its test coverage (lib/rewrite-guard.test.sh, new "caller's EXIT trap is chained, not replaced" cases).

This confirms the conclusion of the three prior review rounds on this PR: no security-relevant surface, now including the trap-chaining fix.

[claude]

Claude finished @kyle-sexton's task in 2m 30s —— View job

---

Code review — /review:code-review

• Gather context (diff, changed files, CLAUDE.md/AGENTS.md conventions)
• Invoke /review:code-review and follow its instructions
• Post inline findings (if any) via inline comments
• Final summary update

This repository carries .github/workflows/claude-security-review.yml, so per REVIEW.md this pass stays scoped to correctness/design/maintainability and omits security findings.

Re-review scope: the last review on this thread covered head 830b52a. Two commits landed since — c101805 (adds a shellcheck disable=SC2016 directive to a test fixture, mechanical) and f1774d2 (fixes the Codex P2 finding on this PR: hook::rewrite_guard_begin's EXIT trap was replacing a caller's pre-existing EXIT trap instead of chaining it, wholesale-clobbering any cleanup the caller had armed). I focused this pass on the substantive change, the trap-chaining logic added to lib/rewrite-guard.sh.

Findings: none.

• lib/rewrite-guard.sh#L61-L101 — hook::_rewrite_guard_on_exit now captures and re-evals the caller's pre-existing trap -p EXIT output before installing its own handler, then chains to it on exit. Traced the trap -p EXIT → strip trap -- / EXIT → eval "prev=$prev" round-trip: bash's trap -p output is always valid, re-evaluable shell syntax (single-quoted with '\''-escaped embedded quotes), so this reconstructs the original command faithfully, including commands containing embedded quotes or the literal substring EXIT.
• Self-chaining is correctly guarded: a second hook::rewrite_guard_begin call recognizes its own installed handler (hook::_rewrite_guard_on_exit) and skips re-capturing it, so the original caller-trap chain doesn't get lost or looped. None of the six adopting hooks call begin more than once (verified via grep -c 'hook::rewrite_guard_begin' across all six *.sh hooks — each is exactly 1), so that path is only exercised by the lib's own test suite, which now locks the contract directly (lib/rewrite-guard.test.sh "second begin does not chain the guard's handler to itself").
• The documented limitation ($? inside the chained handler reflects the release's exit status, not the original process's) is accurate and inert for every current caller — confirmed none of the six hooks arms its own EXIT trap that reads $?.
• Diff'd all six plugins/*/hooks/rewrite-guard.sh copies against lib/rewrite-guard.sh — byte-identical, consistent with the sync-gate contract.
• No plugin.json/CHANGELOG.md changes accompany this fix commit, which is correct: the version was already bumped once relative to origin/main in the PR's initial commit, and sync-rewrite-guard.sh --check-bump compares against that base ref, not commit-by-commit within the PR — an unshipped, pre-merge fix to already-bumped code doesn't need its own bump.

Not independently verified: I could not execute bash lib/rewrite-guard.test.sh or any other command in this environment — Bash tool invocations required interactive approval that wasn't available here, consistent with the earlier review passes on this PR. The analysis above is a static trace of the trap-capture/chain logic and its test coverage, not a live test run.

Branch: claude/architecture-improve-5cle8j

[claude]

Claude finished @kyle-sexton's task in 36s —— View job

---

Security review — PR #3452

• Gather context (PR diff, changed files, custom instructions)
• Invoke /review:security-review and follow its instructions exactly
• Post findings to this comment

Skip gate: the last security review on this thread covered head f1774d21960f868c540d3f4cf12fffed91cfb934. The only commit since then is 3e6e8f0 — "fix(rewrite-guard): record the exec bit on lib/rewrite-guard.test.sh" — which is a pure file-mode change (100644 → 100755) with zero content diff (git diff f1774d2 3e6e8f0 shows 0 insertions(+), 0 deletions(-), mode-only). This has no security-relevant surface: it doesn't change what code runs, what's trusted, or how any input is handled.

Per the skip gate in /review:security-review, stopping early: no new review performed, no findings to post. The prior review's conclusion (no security issues found across the vendored lib/rewrite-guard.sh protocol, its six per-plugin copies, the rewrite-guard-sync CI job, and the EXIT-trap-chaining fix) still applies at current HEAD 3e6e8f0139f03bcd3fa863bb0a1a48b944887cd5.
· Branch: claude/architecture-improve-5cle8j

[claude]

Claude finished @kyle-sexton's task in 47s —— View job

---

Code review — /review:code-review

• Gather context (diff, changed files, CLAUDE.md/AGENTS.md conventions)
• Invoke /review:code-review and follow its instructions
• Post inline findings (if any) via inline comments
• Final summary update

Re-review scope: the last code-review pass on this thread covered head f1774d2. One commit landed since — 3e6e8f0, "fix(rewrite-guard): record the exec bit on lib/rewrite-guard.test.sh". git diff f1774d2..3e6e8f0 shows exactly one file with a mode change (100644 → 100755) and zero content lines added or removed:

lib/rewrite-guard.test.sh | 0
 1 file changed, 0 insertions(+), 0 deletions(-)

Per the skip gate in /review:code-review ("change is trivial/automated with no meaningful review surface"), this doesn't warrant a fresh full-PR pass — it's a mechanical fix to make the test file directly executable (bash lib/rewrite-guard.test.sh invokes it via the shebang/exec bit rather than needing bash prefixed explicitly), matching the "13/13" suite this PR's tests exercise elsewhere.

Findings: none, consistent with the four prior review rounds on this PR (code + security, each re-scoped to the commits since the previous pass). No inline comments were needed for this diff.

Branch: claude/architecture-improve-5cle8j