fix(powershell-format): release the _ps_before snapshot and disclose a rewrite on the tool-break arm

[kyle-sexton]

Summary

plugins/powershell-format/hooks/powershell-format.sh snapshots the target file to an mktemp
copy (_ps_before) so maybe_disclose_ps_rewrite can name a formatter rewrite on the user
channel. Arms 3 and 5 rm -f it, and the disclosure helper releases it on the arms that call it.
Two arms did neither:

• the trust-gate arm 6) reached emit_skipped without releasing it, leaking one temp file per
  gated run;
• the tool-break catch-all *) exited 0 without releasing it AND without calling
  maybe_disclose_ps_rewrite, so besides the leak a rewrite that had already landed went
  undisclosed.

The second one is the more serious of the two. Invoke-Formatter writes the reformatted file
back at line 631, Invoke-ScriptAnalyzer runs at line 640, and both sit inside the same
try/catch that raises exit 4. So on a tool break the file on disk can already differ from
what the user wrote, and the hook said nothing.

Reproduced against the pre-fix hook with a stub pwsh and an isolated TMPDIR (see Verification
for the harness):

--- gate:  rc=0 during=1 left=1          <- snapshot leaked
--- break: rc=0 during=1 left=1          <- snapshot leaked
    out={... "tool break, not a finding" ...}   <- no "reformatted" disclosure

Fix

• maybe_disclose_ps_rewrite splits into take_ps_rewrite_disclosure (release the snapshot,
  record the text in PS_REWRITE_MESSAGE) and the emitting wrapper. An arm that also carries
  additionalContext can then compose both channels through hook::emit_channels instead of
  printing a second JSON object. Arms 0/3/5/6 keep the wrapper and are unchanged in behavior.
• Arm *): build the tool-break context in a variable, take the disclosure, and emit both as
  ONE document. That releases the snapshot on the changed and unchanged paths alike, so it
  satisfies the disclosure and the cleanup criteria together.
• Arm 1) (findings) gets the same composition. It already printed hook::ctx_flush's document
  followed by the disclosure's, so a run that both reformatted and reported findings emitted two
  objects. That is a pre-existing violation of the same contract, in the same file, and closing
  it is two lines given the split above; leaving it would have left the fix half-applied.
• Arm 6): rm -f "$_ps_before", exactly parallel to arms 3 and 5.

No disclosure call on arm 6, and that is deliberate rather than an omission. The issue raised
it as an open question ("a rewrite disclosure decision may also be warranted"). Every exit 6 in
the pwsh block is raised by the trust gate at lines 269-591; Invoke-Formatter is at line 622. No
rewrite can have landed on that path, so the snapshot is provably identical to the file and only
needs releasing. A comment on the arm records that reasoning so a future reader does not read the
asymmetry as a second bug.

No EXIT trap. The issue floated one as a hardening idea; a trap spanning this hook's many exit 0 arms, emit_skipped, and its subshells is materially more blast radius than the two lines the
acceptance criteria ask for. Left as a follow-up rather than smuggled in here.

Verification

Windows 11, Git Bash, pwsh 7.6.5, PSScriptAnalyzer present.

New cases in powershell-format.test.sh, driven through the suite's existing stub-pwsh
mechanism (make_stub_pwsh), each with its own TMPDIR so "left nothing behind" is a strict
emptiness check, and with HOOK_TELEMETRY_SINK unwired so hook-utils' own envelope mktemp
cannot land in the scratch. They sit ABOVE the real-pwsh prerequisite gate, because they need
neither a real pwsh nor PSScriptAnalyzer and would otherwise be skipped on exactly the
pwsh-less hosts where this regression coverage matters:

• trust-gate arm (exit 6): scratch empty after the run.
• tool-break arm (exit 4 after the stub rewrites the file): scratch empty; stdout is exactly
  ONE JSON document (jq -s length == 1); it carries both the disclosure and the tool-break
  additionalContext.
• tool-break arm with no rewrite: scratch empty, and NO disclosure (no-op paths stay silent).
• Case 4b, arm 1 with a real pwsh: a fixture pairing a lowercase alias with a global var
  produces a rewrite AND a finding; stdout is one document carrying both channels, and a third
  assertion confirms the formatter actually rewrote, so the composition check cannot pass on a
  run with nothing to compose.

The emptiness assertions are not vacuous, and the suite proves it rather than asserting it.
The stub pwsh counts the scratch's entries while it is running, and each arm asserts that count
is at least 1 before asserting the post-run count is 0. Without that live probe, a scratch the
hook's mktemp never used would read as "empty" and the check would pass over a leak somewhere
else.

Pre-fix reproduction used a standalone replica of run_arm against a git stash-ed-out hook (the
full suite is ~12 minutes on this box, so the arms were iterated in isolation first): both arms
reported left=1 and the tool-break arm emitted no disclosure, as quoted above. The numbers in
this section come from the real suite run, not the replica.

Full suite: bash plugins/powershell-format/hooks/powershell-format.test.sh -> PASS=86 FAIL=0
(74 on main; 12 new assertions).

Repo gates, all green: scripts/affected-tests.sh --run (selects this suite),
scripts/check-changelog-parity.sh --check and --check-bump origin/main,
scripts/validate-plugins.sh, scripts/check-shell-portability.sh --paths over both touched
shell files.

One unrelated note: the env invocation in the new helper puts -u HOOK_TELEMETRY_SINK before
every NAME=VALUE, because env stops parsing options at the first operand and a trailing -u FOO is taken as the command to run (exit 127). Caught while building these cases.

Related

Closes #3366

[claude]

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

---

Code review (/review:code-review)

• Gather context (skill, diff, CLAUDE.md/AGENTS.md conventions)
• Review changed files for correctness/maintainability
• Check hook script logic (arm 6 / arm * changes)
• Check test coverage additions
• Post findings

No blocking findings.

Reviewed the 4 changed files against hooks/powershell-format.sh:

• Arm 6) (trust gate, L758-763): rm -f "$_ps_before" added before emit_skipped, parallel to arms 3/5. Confirmed emit_skipped (L122-125) does nothing with the snapshot itself, so the explicit release is required here, and the claim that every exit 6 in the pwsh block precedes Invoke-Formatter (L622) checks out against the trust-gate code at L590-593 — no disclosure is owed on this arm.
• Arm *) (L778-783): maybe_disclose_ps_rewrite() added before exit 0. The helper (L215-222) is a no-op-safe cmp against the snapshot either way, so it correctly handles both the "rewrite landed before the throw" and "no rewrite happened" sub-cases without over-disclosing. Call ordering (after ctx_flush/emit_tel) matches arm 1)'s existing order, as the PR description states.
• Test additions in powershell-format.test.sh reuse existing harness primitives (run_hook_env, make_stub_pwsh, new_repo) correctly, and the "assert ARM_DURING >= 1 before trusting ARM_LEFT == 0" pattern is a legitimate way to keep the leak check from being vacuous. Traced the env -u HOOK_TELEMETRY_SINK ordering fix mentioned in the PR body against run_hook_env's env -u CLAUDE_PROJECT_DIR "$@" bash "$HOOK" — placing -u HOOK_TELEMETRY_SINK first in the passed args is correct, since env stops parsing options at the first non-option operand.
• plugin.json version bump (0.7.24 → 0.7.25) and the new CHANGELOG.md entry are consistent with each other and describe the fix accurately.
• Scope: no security-relevant surface touched (local temp-file lifecycle in a formatter hook); nothing deferred to a security lane.

Verification note: I could not execute powershell-format.test.sh or run shellcheck/shfmt in this sandbox — both git fetch and direct script execution required interactive approval that wasn't grantable here. Review is based on static reading of the diff and surrounding code, not independent test execution. The PR body's own verification output is author-claimed and I have not independently reproduced it.
(branch)

[claude]

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

---

Security review — /review:security-review

• Gather context (PR diff, custom instructions)
• Run skip gate (open PR? security-relevant surface? already reviewed?)
• Review changed files for security issues
• Post findings

No security issues found.

This PR touches only cleanup/disclosure bookkeeping around an already-existing mktemp snapshot mechanism in plugins/powershell-format/hooks/powershell-format.sh:

• Arm 6) (trust-gate) adds rm -f "$_ps_before" at L762, exactly parallel to the existing arms 3)/5) releases (L690, L697).
• Arm *) (tool-break) adds a call to the pre-existing maybe_disclose_ps_rewrite helper at L782, the same helper already invoked on arms 0)/1).

Neither change introduces a new code path, new input handling, or a new trust boundary — both simply invoke existing, already-reviewed logic (rm -f, cmp -s, hook::emit_system_message) on two arms that previously skipped it. $_ps_before is hook-generated via mktemp (unchanged by this diff) and rm -f on an empty/unset value is a safe no-op, so there's no injection or race-condition surface added here. The trust-gate logic that governs CustomRulePath code-loading (the security-sensitive part of this hook) is untouched.

The other three changed files — powershell-format.test.sh (new test cases), CHANGELOG.md, and plugin.json (version bump) — are test-only and metadata changes with no production code path and no security relevance.

Branch: fix/misc1-powershell-format-snapshot

[github-actions]

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

[github-actions]

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fc4980e3f3

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

[claude]

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

---

Code review (/review:code-review)

• Gather context (PR diff vs branch point b7d4747, prior review threads, hook-utils contract)
• Review powershell-format.sh arm-by-arm (0, 1, 3, 5, 6, *)
• Review powershell-format.test.sh additions
• Verify plugin.json/CHANGELOG consistency
• Post findings

No blocking findings.

This is the second commit (2a5717b) on top of fc4980e, fixing the single-JSON-document violation that chatgpt-codex-connector flagged (P1) on the first commit, plus the test-ordering issue (P2). I re-verified both fixes independently rather than taking the author's replies at face value:

• P1 (compose into one document) — confirmed fixed. take_ps_rewrite_disclosure() (hook.sh:222) now only releases the snapshot and records PS_REWRITE_MESSAGE; the two arms that also carry additionalContext (arm 1) at hook.sh:699-700, arm *) at hook.sh:798-799) build PS_CTX as a plain string and emit both channels through a single hook::emit_channels PostToolUse "$PS_CTX" "$PS_REWRITE_MESSAGE" call. I checked hook::emit_channels (hook-utils.sh:80-90): it composes additionalContext/systemMessage into one {...} object and both fields are conditionally comma-joined correctly. Arms 0/3/5/6 still use the unchanged maybe_disclose_ps_rewrite wrapper, which is safe there since none of those arms print a second stdout object. emit_skipped/emit_tel write to the telemetry sink, not stdout, so they can't collide with this contract.
• P2 (stub-only tests above the prerequisite gate) — confirmed fixed. The three stub-driven arm cases (powershell-format.test.sh new block) sit directly above the rm -f "$STUB_BIN/pwsh" line that used to close out that section, i.e. above the real-pwsh prerequisite gate. Only the new Case 4b (which needs a genuine Invoke-Formatter + Invoke-ScriptAnalyzer run to exercise both channels at once) stays below the gate, correctly, since it's a real behavioral case.
• Arm 6 (trust gate): rm -f "$_ps_before" added at hook.sh:779, parallel to arms 3/5. Traced every exit 6 in the pwsh heredoc back to the trust-gate block (raised before Invoke-Formatter at line ~622), confirming the comment's claim that no disclosure is owed there.
• Test harness correctness: the run_arm helper's "discriminating leak check" (assert ARM_DURING >= 1 before trusting ARM_LEFT == 0) is legitimate — it rules out the vacuous case where the isolated TMPDIR was never touched at all. The jq -s length == 1 assertions in the new tests directly exercise the P1 fix, and Case 4b's final grep -q 'Get-ChildItem' assertion (test.sh) guards against the composition check passing on a run with nothing to compose. The env -u HOOK_TELEMETRY_SINK ordering fix (must precede NAME=VALUE pairs since env stops option parsing at the first operand) is correct per run_hook_env's implementation (test.sh:86-94).
• Metadata: plugin.json version bump (0.7.24 → 0.7.25) and the CHANGELOG entry were both introduced in the first commit and updated together in this one, not double-bumped.

One purely cosmetic nit, not worth blocking on: in CHANGELOG.md, the second and third 0.7.25 bullets (

claude-code-plugins/plugins/powershell-format/CHANGELOG.md

Lines 18 to 23 in 2a5717b

	- **`_ps_before` snapshot leaked on the trust-gate and tool-break arms (#3366).** The hook copies
	the target file to an `mktemp` snapshot so a formatter rewrite can be disclosed. Arms 3 and 5
	release it and the disclosure helper releases it on the arms that call it, but the trust-gate
	arm (`6)`) and the tool-break catch-all (`*)`) returned without either, leaking one temp file
	per gated run. Both now release it.
	- **A formatter rewrite went undisclosed when pwsh broke (#3366).** `Invoke-Formatter` writes the

) aren't separated by a blank line, while every other bullet pair in the file is. No lint rule catches it and it doesn't affect rendering, so leaving as-is is fine.

Verification note: as with the prior review rounds on this PR, I could not execute powershell-format.test.sh, shellcheck, or scripts/affected-tests.sh in this sandbox — those commands require interactive approval that wasn't grantable here. This review is based on static reading of the diff and the surrounding hook-utils.sh contract, not independent test execution.
(branch)

[claude]

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

---

Security review (/review:security-review)

• Gather context (diff since last reviewed head, CLAUDE.md/AGENTS.md conventions)
• Invoke /review:security-review skill and follow its instructions
• Review changed files/commits since last review for security issues
• Post findings

No security issues found.

Per the skill's skip gate, scope for this pass is the delta since the last reviewed head (fc4980e3f367502ae4130a6e34114965ad71b811 → 2a5717b3fde3c42be4c313f2be7a270e5eee8896): the new commit 2a5717b, which addresses the earlier Codex P1 finding (two JSON documents on stdout on arms carrying both additionalContext and a rewrite disclosure).

Reviewed plugins/powershell-format/hooks/powershell-format.sh:

• take_ps_rewrite_disclosure now separates snapshot release/message-recording from emission, and both arm 1) (L680-700) and the tool-break arm *) (L784-799) now build PS_CTX/PS_REWRITE_MESSAGE as plain shell strings and pass them to hook::emit_channels.
• PS_CTX incorporates $(basename "$FILE") and PSScriptAnalyzer output lines — both pre-existing inputs that were already flowing into hook::ctx_append before this change, so no new untrusted-input surface here.
• The composition point that matters, hook::emit_channels (hook-utils.sh:80-91), runs both $ctx and $sysmsg through hook::json_escape before interpolating into the JSON string, so building the message text with plain +=/$'\n' concatenation in the hook (rather than passing pieces straight through hook::ctx_append) doesn't create a JSON-injection path — escaping still happens at the one place stdout is assembled.
• No change to the trust-gate logic (CustomRulePath gating, arms 0/3/5/6), no new subprocess invocations, no new file paths, and no change to what data crosses the hook boundary — this commit only changes how two already-computed, already-sanitized-at-emission strings are packaged onto stdout.
• Test-only and CHANGELOG changes carry no production code path.

Scope note: this pass reviewed only the commit since the last recorded review (per the bot's relevance-gate comment); the two prior human/security reviews above already cover fc4980e in full and I have not re-derived those findings.
· Branch

[claude]

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

---

Code review (/review:code-review)

• Gather context (skill, diff since last reviewed head, CLAUDE.md/AGENTS.md conventions)
• Review changed files for correctness/maintainability
• Post findings

No blocking findings.

Per the skill's skip gate, this pass reviews only the delta since the last reviewed head (2a5717b → 5418b5b): commit 5418b5b, a one-line shellcheck fix (SC2012) touching only powershell-format.test.sh.

• run_arm's live-probe and post-run leak checks swap ls -A "$TMPDIR" | wc -l for find "$TMPDIR" -mindepth 1 | wc -l (and the matching ARM_LEFT check at L245), addressing shellcheck's "use find instead of ls" warning.
• Verified the swap is behavior-preserving here, not just quieter: TMPDIR="$scratch" is passed straight into the hook subprocess, and the only mktemp calls reachable from this codepath (powershell-format.sh:211 for _ps_before, hook-utils.sh:1227 for the telemetry envelope, which the harness already unwires via HOOK_TELEMETRY_SINK) create plain files, never mktemp -d. With no subdirectories ever created under the isolated TMPDIR, find -mindepth 1 and ls -A count the same entries — recursion depth isn't a factor.
• -mindepth 1 correctly excludes the scratch directory itself from the count, so the "at least 1 during the run, 0 after" discriminating-leak-check logic this counts into is unaffected.

The rest of the changes in this PR (arm 6 snapshot release, tool-break disclosure/composition, plugin.json/CHANGELOG.md) were covered in the two prior code-review passes on this PR and are unchanged since.

Verification note: as with prior rounds, I did not execute powershell-format.test.sh or shellcheck in this sandbox (no tool access to run them here); this review is a static read of the diff and the surrounding mktemp call sites.
· branch

[claude]

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

---

Security review (/review:security-review)

• Gather context (diff since last reviewed head, CLAUDE.md/AGENTS.md conventions)
• Invoke /review:security-review skill and follow its instructions
• Review changed files/commits since last review for security issues
• Post findings

Skip gate: no security-relevant surface in the new commit.

Scope for this pass is the delta since the last recorded review (2a5717b3fde3c42be4c313f2be7a270e5eee8896 → 5418b5b7225056a2b2f659250de76b6d2256ad53), which is a single commit: 5418b5b.

That commit changes only powershell-format.test.sh, swapping ls -A "$dir" | wc -l for find "$dir" -mindepth 1 | wc -l in the test harness's scratch-directory probe/count, purely to satisfy shellcheck's SC2012 (info severity) in the hygiene lane. It's a like-for-like counting-mechanism swap inside test-only code:

• No production code path touched (powershell-format.sh is unchanged in this commit).
• No new input, subprocess, or trust-boundary surface — find/ls are both run against a mktemp-created scratch dir the test itself owns, with the same arguments/flow as before.
• No effect on the arm logic, disclosure composition, or trust-gate behavior reviewed in the two prior passes on this PR (fc4980e, 2a5717b).

Per the skip gate, stopping here rather than re-deriving findings the two earlier reviews on fc4980e and 2a5717b already covered.
· Branch