feat(bash-format): disclose shfmt rewrites on the user channel - #2471
Conversation
|
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 finished @kyle-sexton's task in 3m 54s —— View job Security Review
Scope: only the two files changed in this PR ( No injection, auth/access-control, secret-handling, or supply-chain issues found. All new variables ( One logic-flaw finding with a security-relevant consequence (loss of visibility into ShellCheck's security-relevant lint output): IMPORTANT — new code can cause two JSON documents to be printed to stdout in one run, silently dropping the hook's ShellCheck output plugins/bash-format/hooks/bash-format.sh#L236-L241 adds: if [[ -n "$_fmt_before" ]]; then
if ! cmp -s "$_fmt_before" "$_fmt_target" 2>/dev/null; then
hook::emit_system_message "bash-format: reformatted $(basename "$FILE") via shfmt (structural layout only)."
fi
rm -f "$_fmt_before"
fi
That end-of-run call is documented as the file's single emission point ("composed into one JSON document emitted at the end", L289-290), and The new code does neither: it doesn't exit, and it doesn't compose the reformat notice into Why this matters for security review scope (not just correctness): the practical effect is that ShellCheck's findings — which are the mechanism by which this hook surfaces security-relevant shell issues (unquoted expansions/SC2086, unsafe Suggested fix: don't call Per instructions, unpinned-action / supply-chain / trigger / permissions concerns are left to zizmor's static lane — not applicable here anyway, since this PR touches no workflow files. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23ed248392
ℹ️ 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".
d93c220 to
d74f9a5
Compare
|
Claude finished @kyle-sexton's task in 1m 32s —— View job Security Review
Scope: the two files changed in this PR — No injection, authorization/access-control, secret-handling, or supply-chain issues in this diff. The new One logic flaw with a security-relevant consequence: IMPORTANT — the new code can make the hook print two top-level JSON documents to stdout in one run, which silently drops ShellCheck's (security-relevant) findings plugins/bash-format/hooks/bash-format.sh#L236-L241 adds: if [[ -n "$_fmt_before" ]]; then
if ! cmp -s "$_fmt_before" "$_fmt_target" 2>/dev/null; then
hook::emit_system_message "bash-format: reformatted $(basename "$FILE") via shfmt (structural layout only)."
fi
rm -f "$_fmt_before"
fi
That end-of-run call is documented as the file's single emission point ("composed into one JSON document emitted at the end", L289-290), So whenever Why this is security-relevant, not just a correctness bug: ShellCheck's findings are how this hook surfaces security-relevant shell issues (unquoted expansions / SC2086, unsafe Suggested fix: don't call Per instructions, unpinned-action / supply-chain / trigger / permissions concerns are left to zizmor's static lane — not applicable here anyway, since this PR touches no workflow files. |
d74f9a5 to
9f5c718
Compare
|
Claude finished @kyle-sexton's task in 1m 19s —— View job Security Review
Scope: the two files changed in this PR — No injection, authorization/access-control, secret-handling, or supply-chain issues in this diff. The new One logic flaw with a security-relevant consequence, confirmed by reading the full control flow of both files at HEAD: IMPORTANT — the hook can print two top-level JSON documents to stdout in one run, silently dropping ShellCheck's (security-relevant) findings bash-format.sh#L236-L241 adds: if [[ -n "$_fmt_before" ]]; then
if ! cmp -s "$_fmt_before" "$_fmt_target" 2>/dev/null; then
hook::emit_system_message "bash-format: reformatted $(basename "$FILE") via shfmt (structural layout only)."
fi
rm -f "$_fmt_before"
fi
CTX="${CTX%$'\n'}"
if [[ -n "$NOTICE" ]]; then
AGENT_CTX="$CTX"
[[ -n "$AGENT_CTX" ]] && AGENT_CTX+=$'\n'
AGENT_CTX+="$NOTICE"
hook::emit_channels PostToolUse "$AGENT_CTX" "$NOTICE"
else
hook::emit_channels PostToolUse "$CTX" ""
fi
So whenever Why this is security-relevant, not just correctness: ShellCheck's findings are this hook's mechanism for surfacing security-relevant shell mistakes (unquoted expansions / SC2086, unsafe Suggested fix: don't call This matches the finding already raised by the two prior review runs on this PR — re-verified here against the current HEAD ( Per instructions, unpinned-action / supply-chain / trigger / permissions concerns are left to zizmor's static lane — not applicable here anyway, since this PR touches no workflow files. |
9f5c718 to
6070223
Compare
|
Claude finished @kyle-sexton's task in 59s —— View job Security Review
Scope: the two files changed in this PR — No injection, authorization/access-control, secret-handling, or supply-chain issues in this diff. The new One logic flaw with a security-relevant consequence: IMPORTANT — the hook can print two top-level JSON documents to stdout in one run, silently dropping ShellCheck's (security-relevant) findings bash-format.sh#L236-L241 adds: if [[ -n "$_fmt_before" ]]; then
if ! cmp -s "$_fmt_before" "$_fmt_target" 2>/dev/null; then
hook::emit_system_message "bash-format: reformatted $(basename "$FILE") via shfmt (structural layout only)."
fi
rm -f "$_fmt_before"
fi
CTX="${CTX%$'\n'}"
if [[ -n "$NOTICE" ]]; then
AGENT_CTX="$CTX"
[[ -n "$AGENT_CTX" ]] && AGENT_CTX+=$'\n'
AGENT_CTX+="$NOTICE"
hook::emit_channels PostToolUse "$AGENT_CTX" "$NOTICE"
else
hook::emit_channels PostToolUse "$CTX" ""
fi
So whenever Why this is security-relevant, not just correctness: ShellCheck's findings are this hook's mechanism for surfacing security-relevant shell mistakes (unquoted expansions / SC2086, unsafe Suggested fix: don't call This is the same finding raised by the three prior review runs on this PR ( Per instructions, unpinned-action / supply-chain / trigger / permissions concerns are left to zizmor's static lane — not applicable here anyway, since this PR touches no workflow files. |
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Partially addresses #1596 Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
c56786d to
28b02ea
Compare
|
Claude finished @cursor[bot]'s task in 1m 16s —— View job Security Review
Scope: the three files changed in this PR, verified against current HEAD No injection, authorization/access-control, secret-handling, or supply-chain issues in this diff. The One logic flaw with a security-relevant consequence, still present at HEAD IMPORTANT — the hook can print two top-level JSON documents to stdout in one run, silently dropping ShellCheck's (security-relevant) findings if [[ -n "$_fmt_before" ]]; then
if ! cmp -s "$_fmt_before" "$_fmt_target" 2>/dev/null; then
hook::emit_system_message "bash-format: reformatted $(basename "$FILE") via shfmt (structural layout only)."
fi
rm -f "$_fmt_before"
fi
CTX="${CTX%$'\n'}"
if [[ -n "$NOTICE" ]]; then
AGENT_CTX="$CTX"
[[ -n "$AGENT_CTX" ]] && AGENT_CTX+=$'\n'
AGENT_CTX+="$NOTICE"
hook::emit_channels PostToolUse "$AGENT_CTX" "$NOTICE"
else
hook::emit_channels PostToolUse "$CTX" ""
fi
So whenever Why this is security-relevant, not just correctness: ShellCheck's findings are this hook's mechanism for surfacing security-relevant shell mistakes (unquoted expansions / SC2086, unsafe Suggested fix: don't call This is the same finding raised by all four prior review runs on this PR ( Per instructions, unpinned-action / supply-chain / trigger / permissions concerns are left to zizmor's static lane — not applicable here anyway, since this PR touches no workflow files. |
|
Last security-reviewed head: |
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude finished @cursor[bot]'s task in 4m 14s —— View job Code Review
This is the code-review lane (correctness/design/conventions/error-handling/observability/tests/docs). Per Scope: the three changed files, verified against HEAD Two findings posted as inline review comments:
Nothing else in the diff raised correctness, design, or documentation concerns — the |
| [[ -f "$_fmt_target" ]] || _fmt_target="$FILE" | ||
| _fmt_before="" | ||
| if _fmt_before=$(mktemp 2>/dev/null); then | ||
| cp "$_fmt_target" "$_fmt_before" 2>/dev/null || _fmt_before="" |
There was a problem hiding this comment.
🟡 Nit: leftover temp file when cp fails after a successful mktemp
_fmt_before=""
if _fmt_before=$(mktemp 2>/dev/null); then
cp "$_fmt_target" "$_fmt_before" 2>/dev/null || _fmt_before=""
fimktemp creates the temp file on disk and returns its path; the if only guards on mktemp's own exit status. If the subsequent cp then fails (permission hiccup, disk full, or $_fmt_target disappearing in the same race window this file already guards against elsewhere — see the "Re-check existence immediately before mutating" comment a few lines below), _fmt_before is reset to "" so the later rm -f "$_fmt_before" (line 240) never runs. The file mktemp created is orphaned — it's never cleaned up.
Low impact (an empty stray file in the OS temp dir on a narrow failure path), but easy to close: capture the path before clearing the variable, e.g.
_fmt_before=""
if _fmt_before_tmp=$(mktemp 2>/dev/null); then
if cp "$_fmt_target" "$_fmt_before_tmp" 2>/dev/null; then
_fmt_before="$_fmt_before_tmp"
else
rm -f "$_fmt_before_tmp"
fi
fi| if [[ -n "$_fmt_before" ]]; then | ||
| if ! cmp -s "$_fmt_before" "$_fmt_target" 2>/dev/null; then | ||
| hook::emit_system_message "bash-format: reformatted $(basename "$FILE") via shfmt (structural layout only)." | ||
| fi | ||
| rm -f "$_fmt_before" | ||
| fi |
There was a problem hiding this comment.
🔴 Important: new mid-script hook::emit_channels call site has no test coverage
This adds a second call path to hook::emit_channels (via hook::emit_system_message) that fires mid-script, before the file's existing single end-of-run emission at lines 289-299. bash-format.test.sh already has a dedicated test for the "single JSON document" invariant on an analogous combination — the shfmt-absent-notice + ShellCheck-findings case (bash-format.test.sh:569-606, asserting jq -s 'length' == 1 on stdout) — which shows the project treats that invariant as something worth a regression test whenever a new notice source is added.
Nothing in this PR adds the equivalent test for the new path: a run where shfmt actually reformats the file (so this new mid-script emission fires) and ShellCheck also produces findings (or the missing-tool notice is pending) on the same run. That's exactly the combination that would need to hold this invariant, and it currently has no automated check — a regression here (e.g. two top-level JSON objects on stdout instead of one) would ship silently.
Suggested addition: a case modeled on the existing "mix" test at bash-format.test.sh:569-606, but with shfmt present and actually rewriting the fixture file (rather than absent) plus a ShellCheck finding present, asserting jq -s 'length' on stdout is still 1.
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
No linked issue
Partially addresses #1596
Related
Refs #1596