feat(go-format): disclose goimports 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.

[claude]

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

---

Security Review — go-format PR #2473

Reviewed both changed files (plugins/go-format/hooks/go-format.sh, plugins/go-format/CHANGELOG.md) against origin/main...HEAD. The CHANGELOG change is documentation-only; the substantive change is a ~10-line addition in go-format.sh that snapshots the file before goimports -w, then diffs it after and emits a systemMessage if the content changed.

No CRITICAL or IMPORTANT security issues found.

Specifically checked and ruled out:

• JSON/message injection via filename: the new hook::emit_system_message "go-format: reformatted $(basename "$FILE") via goimports..." call (go-format.sh#L249) routes through hook::emit_channels → hook::json_escape, which escapes \, ", and control characters before building the JSON payload — a filename containing quotes or backslashes can't break out of the systemMessage string.
• Command/path injection via $FILE: $FILE is consistently double-quoted in the new cp "$FILE" "$_go_before" and cmp -s "$_go_before" "$FILE" calls (go-format.sh#L227-L230, #L246-L252); no unquoted expansion or eval introduced.
• Temp-file creation: _go_before=$(mktemp ...) uses no user-influenced template, so there's no predictable-path/symlink-race concern — mktemp creates the file atomically before the cp writes into it.
• Trust boundary: the new logic only reads/copies a file the hook already has full read/write access to (the same $FILE goimports -w mutates); no new data crosses a privilege or trust boundary.

SUGGESTION (low severity, low confidence — hygiene, borderline security-relevant): the pre-format snapshot $_go_before is only rm -f'd inside the RC -eq 0 success branch (go-format.sh#L246-L252). If goimports exits non-zero (syntax error, RC=2, or the generic-failure branch), the temp copy of the file's pre-edit source is left behind in the temp directory indefinitely rather than being cleaned up on all exit paths. Impact is minimal (same-user-owned, mktemp-restricted-permission file, content the user already owns), but it's an unbounded-accumulation/leftover-source-on-disk edge case worth a trap/cleanup-on-all-paths fix.
Fix this →

No authorization/access-control, secrets/credential-handling, or GitHub Actions workflow changes are present in this diff (no .yml/.yaml files touched), so those categories don't apply here.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3fbea8c6da

ℹ️ 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 14s —— View job

---

Security Review — go-format PR #2473 (re-run at 5bd5254)

Scope: git diff / gh pr diff against origin/main for the 3 changed files. The only change since the prior successful review pass (at 3fbea8c) is commit 5bd5254, which bumps plugins/go-format/.claude-plugin/plugin.json version 0.3.7 → 0.3.8 to match the CHANGELOG — no logic change. The substantive diff is still the ~10-line addition in plugins/go-format/hooks/go-format.sh that snapshots a .go file before goimports -w, diffs it after, and emits a systemMessage on the user channel when content changed.

No CRITICAL or IMPORTANT security issues found.

Re-verified against the current head (5bd5254):

• JSON/message injection via filename: hook::emit_system_message "go-format: reformatted $(basename "$FILE") via goimports..." routes through hook::emit_channels → hook::json_escape, which escapes \, ", and JSON-forbidden control bytes before the string is embedded in the hand-built JSON document — a filename with quotes/backslashes/newlines cannot break out of the systemMessage string or forge sibling JSON keys.
• Command/path injection via $FILE: $FILE stays double-quoted throughout the new code — cp "$FILE" "$_go_before" and cmp -s "$_go_before" "$FILE" — no unquoted expansion, eval, or shell metacharacter interpretation introduced.
• Temp-file creation: _go_before=$(mktemp 2>/dev/null) uses the default unpredictable template, created atomically by mktemp itself before cp writes into it — no predictable-path or symlink-race window.
• Trust boundary: the new code only reads/copies a file the hook already has full read/write access to (the same $FILE that goimports -w mutates); no new data crosses a privilege or trust boundary, and nothing here is attacker-controlled beyond the file path/contents the hook was already trusted with.

SUGGESTION (low severity, low confidence — hygiene, not a security boundary): the pre-format snapshot $_go_before is only rm -f'd inside the RC -eq 0 success branch (go-format.sh#L246-L252). On a non-zero goimports exit (syntax error at RC=2, or the generic-failure branch at L281-L293) the temp copy is never removed and leaks into the temp directory indefinitely. Same-user-owned, mktemp-permissioned, content the user already owns — not an information-disclosure or access-control issue — but worth a trap 'rm -f "$_go_before"' EXIT right after creation so cleanup covers every exit path. (Also flagged independently by Codex on this PR.)
Fix this →

No authorization/access-control, secrets/credential-handling, or GitHub Actions workflow changes are present in this diff (no .yml/.yaml files touched), so those categories don't apply.

Note on prior comments: two automated review runs on this PR reported infrastructure failures (no execution file produced) — those were not review verdicts, and this comment supersedes them with a completed pass at the current head.

[claude]

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

---

Security Review — go-format PR #2473 (at 9c368e3)

Scope: git diff origin/main...HEAD for the 3 changed files. Diff vs. the previous review pass (at 5bd5254) is a no-op: 9c368e3 re-bumps plugins/go-format/.claude-plugin/plugin.json version 0.3.7 → 0.3.8 (same version target, cosmetic commit) — no logic change. The substantive change remains the ~10-line addition in plugins/go-format/hooks/go-format.sh that snapshots a .go file before goimports -w, diffs it after, and emits a systemMessage on the user channel when content changed.

No CRITICAL or IMPORTANT security issues found.

Verified against the current head:

• JSON/message injection via filename: hook::emit_system_message "go-format: reformatted $(basename "$FILE") via goimports..." routes through hook::emit_channels → hook::json_escape, which escapes \, ", and control bytes before the string is embedded in the hand-built JSON document — a filename with quotes/backslashes/newlines cannot break out of the systemMessage string or forge sibling JSON keys.
• Command/path injection via $FILE: $FILE stays double-quoted throughout the new code — cp "$FILE" "$_go_before" and cmp -s "$_go_before" "$FILE" — no unquoted expansion, eval, or shell metacharacter interpretation introduced.
• Temp-file creation: _go_before=$(mktemp 2>/dev/null) uses the default unpredictable template, created atomically by mktemp before cp writes into it — no predictable-path or symlink-race window.
• Trust boundary: the new code only reads/copies a file the hook already has full read/write access to (the same $FILE that goimports -w mutates); no new data crosses a privilege or trust boundary, and nothing here is attacker-controlled beyond the file path/contents the hook was already trusted with.

SUGGESTION (low severity, low confidence — hygiene, not a security boundary): the pre-format snapshot $_go_before is only rm -f'd inside the RC -eq 0 success branch (go-format.sh#L246-L252). On a non-zero goimports exit (syntax error at RC=2, or the generic-failure branch at L281-L293) the temp copy is never removed and leaks into the temp directory indefinitely. This is still unaddressed as of 9c368e3. Same-user-owned, mktemp-permissioned, content the user already owns — not an information-disclosure or access-control issue — but worth a trap 'rm -f "$_go_before"' EXIT right after creation so cleanup covers every exit path. (Also flagged by Codex on this PR.)
Fix this →

No authorization/access-control, secrets/credential-handling, or GitHub Actions workflow changes are present in this diff (no .yml/.yaml files touched), so those categories don't apply.

Note: this is a re-run at the current head; two prior passes on this PR (at 3fbea8c and 5bd5254) reached the same conclusion, and the one open suggestion (temp-file cleanup on failure paths) remains unaddressed.

[claude]

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

---

Security Review — go-format PR #2473 (at 2c06f15)

Scope: git diff origin/main...HEAD for the 3 changed files. Diff vs. the last reviewed head (9c368e3) is a no-op for logic: 2c06f15 re-bumps plugins/go-format/.claude-plugin/plugin.json version to 0.3.8 (manifest/CHANGELOG sync only). The substantive change remains the ~10-line addition in plugins/go-format/hooks/go-format.sh that snapshots a .go file before goimports -w, diffs it after, and emits a systemMessage on the user channel when content changed.

• Gather context (diff, changed files, CLAUDE.md)
• Review go-format.sh for injection / trust-boundary issues
• Verify hook::json_escape / hook::emit_channels handling of the new systemMessage
• Re-check temp-file cleanup path raised in prior passes
• Post findings

No CRITICAL or IMPORTANT security issues found.

Verified against the current head (2c06f15):

• JSON/message injection via filename: hook::emit_system_message "go-format: reformatted $(basename "$FILE") via goimports..." routes through hook::emit_channels → hook::json_escape, which escapes \, ", and control bytes before the string is embedded in the hand-built JSON document — a filename with quotes/backslashes/newlines cannot break out of the systemMessage string or forge sibling JSON keys.
• Command/path injection via $FILE: $FILE stays double-quoted throughout the new code — cp "$FILE" "$_go_before" and cmp -s "$_go_before" "$FILE" — no unquoted expansion, eval, or shell metacharacter interpretation introduced.
• Temp-file creation: _go_before=$(mktemp 2>/dev/null) uses the default unpredictable template, created atomically by mktemp before cp writes into it — no predictable-path or symlink-race window.
• Trust boundary: the new code only reads/copies a file the hook already has full read/write access to (the same $FILE that goimports -w mutates); no new data crosses a privilege or trust boundary.

SUGGESTION (low severity, low confidence — hygiene, not a security boundary): the pre-format snapshot $_go_before is only rm -f'd inside the RC -eq 0 success branch (go-format.sh#L246-L252). On a non-zero goimports exit (syntax error at RC=2, or the generic-failure branch at L281-L293) the temp copy is never removed and leaks into the temp directory indefinitely. Same-user-owned, mktemp-permissioned, content the user already owns — not an information-disclosure or access-control issue — but still worth a trap 'rm -f "$_go_before"' EXIT right after creation so cleanup covers every exit path. This has been flagged on every prior pass on this PR (at 3fbea8c, 5bd5254, 9c368e3) and independently by Codex, and remains unaddressed as of 2c06f15.
Fix this →

No authorization/access-control, secrets/credential-handling, or GitHub Actions workflow changes are present in this diff (no .yml/.yaml files touched), so those categories don't apply.

Note: this is a re-run at the current head; four prior passes on this PR reached the same conclusion, and the one open suggestion (temp-file cleanup on failure paths) remains unaddressed.

[claude]

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

---

Code Review — go-format PR #2473

Tasks

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

---

Scope: gh pr diff 2473 against origin/main — 3 files (plugin.json version bump, CHANGELOG.md entry, and the ~10-line addition to go-format.sh that snapshots a .go file before goimports -w, diffs it after, and emits a systemMessage when content changed). Per this repo's REVIEW.md lane split, this pass covers correctness/design/error-handling/tests/docs; security findings (already covered extensively by the security lane and Codex on this PR) are left to that lane.

Posted as an inline comment:

• go-format.sh#L227-L251 — the $_go_before pre-format snapshot is only cleaned up in the RC -eq 0 success branch; the syntax-error (RC -eq 2) and generic tool-break branches leave the temp copy behind. Comparing against the sibling ruff-format hook (plugins/ruff-format/hooks/ruff-format.sh:230-241), that hook doesn't have this gap because its fix/format passes are both || true with no failure branch to skip past — go-format.sh does have a real failure exit for goimports, so cleanup needs to run on every path, e.g. a trap 'rm -f "$_go_before"' EXIT right after the snapshot is created. This is a resource-cleanup/error-handling gap, not a security boundary, so it's in scope here even though the security lane and Codex already flagged it.

Other things checked, no issues found:

• Test coverage: plugins/go-format/hooks/go-format.test.sh has no assertion for the new systemMessage disclosure (e.g. asserting .systemMessage contains "reformatted" after an edit that goimports actually rewrites). Checked whether this is an established gap rather than one specific to this PR: the analogous ruff-format disclosure message (ruff-format.sh:238, added earlier for the same Adopt the content-mutation disclosure clause across the remaining autofix hooks #1596 effort) also has no dedicated test in ruff-format.test.sh. Since the omission matches existing precedent in this codebase rather than being unique to this change, I'm not flagging it as a blocking gap — noting it here for visibility in case the maintainers want to backfill coverage for the whole #1596 disclosure feature set at once.
• CHANGELOG.md / plugin.json: version bump (0.3.7 → 0.3.8) and changelog entry are consistent with the repo's Keep-a-Changelog format and existing entries in this file.
• Message wording/placement: hook::emit_system_message "go-format: reformatted $(basename "$FILE") via goimports (imports and layout only)." follows the same call pattern and phrasing convention (<plugin>: <verb> <basename> via <tool>) as the sibling ruff-format disclosure message.
• Guard logic: the _go_before="" / mktemp failure path correctly degrades to skipping the disclosure ([[ -n "$_go_before" ]] guards both the cmp and the rm) rather than erroring.

[claude]

Suggestion — error-handling: $_go_before snapshot leaks on the non-success paths.

The snapshot is only removed inside the RC -eq 0 branch (rm -f "$_go_before" at line 251). On the two other exits from this hook — the syntax-error branch (RC -eq 2) and the generic tool-break branch below it — $_go_before is left behind in the temp directory. Since every *.go edit that goimports can't parse cleanly will repeat this on every save, the leak is not a one-off.

For comparison, the sibling ruff-format hook doesn't have this gap because its cleanup runs unconditionally after the fix/format passes (both of which are || true, so there's no failure branch to skip past):

if [[ -n "$_ruff_before" ]]; then
  if ! cmp -s "$_ruff_before" "$FILE" 2>/dev/null; then
    hook::emit_system_message "..."
  fi
  rm -f "$_ruff_before"
fi

Here in go-format.sh, goimports has a real failure mode with its own exit path, so the rm -f needs to run on all of them — e.g. trap 'rm -f "$_go_before"' EXIT right after the snapshot is created (covers RC -eq 2, the generic-failure branch, and any future early exit), rather than duplicating the rm -f in each branch.

(Independently flagged by Codex and the security-review lane on this PR as a hygiene issue; reporting here as well since unbounded temp-file accumulation on an error path is squarely an error-handling/correctness concern for this lane.)

[github-actions]

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