feat(ruff-format): disclose Ruff rewrites on the user channel

[kyle-sexton]

No linked issue

Partially addresses #1596

Related

Refs #1596

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9a16fa18f5

ℹ️ 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 1m 39s —— View job

---

Security Review

• Gather context (diff, changed files)
• Review plugins/ruff-format/hooks/ruff-format.sh for security issues
• Review plugins/ruff-format/.claude-plugin/plugin.json and CHANGELOG.md
• Post findings

Scope: ruff-format.sh (+10 lines), plugin manifest version bump, and CHANGELOG entry.

No security issues found.

The added logic:

1. mktemps a scratch file and cps $FILE into it before the Ruff check --fix / format passes.
2. cmps it against the (possibly rewritten) $FILE afterward.
3. Emits a systemMessage naming $(basename "$FILE") via hook::emit_system_message if content changed, then rm -fs the temp copy.

Checked specifically for:

• Command/argument injection — $FILE and the temp path are always double-quoted in cp/cmp/rm; no unquoted expansion or eval.
• Path traversal / symlink risk — no new untrusted input; $FILE is the same value already passed to ruff check/ruff format on the pre-existing code path, gated to *.py/*.pyi earlier in the script.
• Message/JSON injection into the disclosed systemMessage — the basename is passed through hook::emit_channels → hook::json_escape, which escapes backslash, quote, and control characters before embedding, consistent with existing ctx_append usage elsewhere in the file.
• Temp-file handling — mktemp uses an unpredictable name (no symlink-race/predictable-path issue), the copy is cleaned up with rm -f in the same block, and on most platforms mktemp creates the file mode 0600 (owner-only), so the transient copy isn't more exposed than the source file already is on disk.
• Trust boundary — $FILE originates from the same hook-input parsing (hook::read_file_path) already trusted by the unchanged Ruff-invocation code above; these lines don't introduce a new external input source.

One functional (non-security) issue was already flagged by the automated Codex review on this PR — a mixed-result run (mutation + a residual finding) can emit two concatenated JSON documents on stdout instead of one, since this block's hook::emit_system_message call and the later hook::ctx_flush call are independent. That's a correctness/output-composition bug (could cause the hook's structured output to be dropped/misparsed by the consumer), not a vulnerability — no injection, privilege, or data-exposure vector — so it's out of scope for this security-only pass, but worth fixing per Codex's suggestion before merge.

plugin.json (version bump) and CHANGELOG.md changes are documentation/metadata only — no security relevance.

[claude]

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

---

Security Review

• Gather context (diff, changed files)
• Review plugins/ruff-format/hooks/ruff-format.sh
• Review manifest/changelog
• Post findings

Scope: git diff origin/main...HEAD — ruff-format.sh (+10 lines), plugin manifest version bump, and CHANGELOG entry. No GitHub Actions workflow files are part of this diff.

No security issues found.

Analysis of the added block (lines 230–241):

1. mktemps a scratch file and cps $FILE into it before the Ruff check --fix / format passes run.
2. cmp -ss it against $FILE afterward.
3. If they differ, emits a systemMessage naming $(basename "$FILE") via hook::emit_system_message, then rm -fs the temp copy either way.

Specifically checked:

• Command/argument injection — $FILE, $_ruff_before, and $(basename "$FILE") are consistently double-quoted across cp/cmp/rm/the message string; no unquoted expansion, eval, or subshell word-splitting.
• Message/JSON injection into the disclosed systemMessage — the composed string flows through hook::emit_system_message → hook::emit_channels → hook::json_escape, which escapes \, ", and line-structure control bytes (in the correct order — backslashes before quotes) and strips residual C0 bytes. A filename containing quotes, backslashes, or newlines cannot break out of the hand-built JSON document.
• Path traversal / trust boundary — $FILE is not new untrusted input; it's the same value already resolved by hook::read_file_path and passed to ruff check/ruff format on the pre-existing code path, gated to *.py/*.pyi earlier in the script. This block only reads/copies it, it doesn't widen what's trusted or executed.
• Temp-file handling — mktemp produces an unpredictable path (no symlink-race or predictable-path issue), the copy is cleaned up with rm -f in the same block on both the changed and unchanged branches, and mktemp defaults to mode 0600 — the transient copy is no more exposed than $FILE already is on disk.
• Information disclosure — only the basename is disclosed on the user channel; no file content, diff, or path outside the basename is emitted.

plugin.json (version bump) and CHANGELOG.md are metadata/documentation only — no security relevance.

Note (non-security, not re-reported here): the existing Codex review and the prior Claude run both flagged that a mixed-result run (mutation + a residual finding) emits two concatenated JSON documents on stdout (this block's hook::emit_system_message call is independent from the later hook::ctx_flush), which is a correctness/output-composition bug, not a vulnerability — no injection, privilege, or data-exposure vector. Out of scope for this security-only pass.

[claude]

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

---

Code Review

• Gather context (diff, CLAUDE.md, REVIEW.md)
• Review plugins/ruff-format/hooks/ruff-format.sh changes
• Review plugin.json / CHANGELOG.md
• Post findings

Scope: plugins/ruff-format/hooks/ruff-format.sh (+10 lines, mutation-disclosure systemMessage), the plugin.json version bump, and the CHANGELOG.md entry. Security aspects of this diff are already covered by the security lane's review on this PR — this pass focuses on correctness, tests, and maintainability per REVIEW.md's code-review-lane scope.

🔴 Important — inline comment posted

Posted at ruff-format.sh:238: the new hook::emit_system_message call prints its own complete JSON document immediately after the fix/format pass, but the pre-existing hook::ctx_flush calls further down (residual-findings path, tool-break path) can also print a document in the same run. A file that gets reformatted and still has an unfixable finding (e.g. F821, or the F401 this hook deliberately makes unfixable) — a common, not edge-case, outcome — will emit two concatenated JSON documents on stdout, violating the single-document contract hook::emit_channels documents at hook-utils.sh:78-80 and that sibling plugins (bash-format.sh, markdown-format.sh, typos-format.sh) all honor via a single accumulate-then-emit call. This was already flagged by the automated Codex review (P1); I'm confirming it as a genuine correctness bug rather than a style nit, since it can corrupt or drop the hook's structured output for the consumer.

🟡 Suggestion — test coverage gap for the new feature

ruff-format.test.sh was not updated by this PR, and no existing case exercises the new systemMessage disclosure:

• Case 3 ("bad formatting, no lint error") is the one scenario that should trigger the new mutation notice, but it only asserts on the reformatted file content — it never inspects $OUT for a systemMessage, so the new feature added by this PR has zero assertion coverage.
• No case combines a formatting change with a residual, unfixable finding in the same file — precisely the scenario that triggers the double-JSON-document bug above. Adding that case (and asserting $OUT parses as exactly one JSON document containing both systemMessage and additionalContext) would have caught it before merge and should be added alongside the fix.

Everything else in this PR — the version bump and CHANGELOG.md entry — is documentation/metadata and looks consistent with the change.

[claude]

🔴 Important — mixed-result runs emit two JSON documents on stdout

hook::emit_system_message (via hook::emit_channels) prints a complete JSON document here, immediately after the fix/format pass. But when the file is also mutated and the verify pass below still has residual findings, hook::ctx_flush at line 267 (or the tool-break path at line 291) prints a second JSON document on the same stdout stream.

hook::emit_channels's own doc comment (hook-utils.sh:78-80) states the invariant this breaks: "CC parses the hook's whole stdout as a single JSON doc, so a run that has both lint findings and a pending skip notice must compose them here rather than print twice." This is exactly that case — mutation notice + advisory findings — and it's a common one: Ruff frequently reformats a file while still leaving an unfixable diagnostic (e.g. F821, or the F401 this hook deliberately makes unfixable).

Sibling plugins follow the single-emission-point convention to avoid this: bash-format.sh accumulates into NOTICE/CTX and calls hook::emit_channels exactly once at the end (bash-format.sh:284-289), and markdown-format.sh/typos-format.sh do the same (hook::emit_channels PostToolUse "$CTX" "$SYSMSG"). This PR's hook::emit_system_message call should instead stash the mutation notice in a variable (e.g. $SYSMSG) and let it flow into the existing hook::ctx_flush/exit-0 paths below so only one JSON document is ever printed per run.

This was already flagged by the automated Codex review on this PR (P1) — confirming it here since it's a genuine correctness bug, not just a style nit: a mixed-result run can corrupt/drop the hook's structured output for the consumer.

Fix this →

[github-actions]

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