fix(bash-format): apply-ignore fallback, [*] opt-in, Windows paths

[kyle-sexton]

Summary

• Keep --apply-ignore on the shfmt formatting path via a capability probe (no strip-ignore fallback). Defect 1 landed earlier in fix(bash-format): probe --apply-ignore instead of falling back on any failure #1823; retained here.
• Stop treating bare [*] EditorConfig sections as shell opt-in; require an explicit shell glob ([*.sh], [*.bash], [*.{sh,bash}], [**/*.sh]).
• Stabilize Windows/MSYS tool paths (cygpath -lm), re-check file existence before shfmt/ShellCheck, and never surface GHC openBinaryFile as a finding.

Closes #1817

Related

• fix(bash-format): probe --apply-ignore instead of falling back on any failure #1823 (merged) — defect 1 only (--apply-ignore probe); this PR finishes defects 2 and 3 and closes the issue.
• Issue triage comment on bash-format: fallback drops --apply-ignore, bare [*] .editorconfig section treated as shell opt-in, openBinaryFile failures on Windows paths #1817 — defect 2 decision (no bare [*] opt-in); defect 3 diagnosed as missing-file/openBinaryFile with Windows path hardening.

Test plan

• bash plugins/bash-format/hooks/bash-format.test.sh — PASS=48 FAIL=0
• Bare [*] (with indent props and eol-only) leaves shell files untouched
• Explicit [*.sh] / brace-list / path-prefixed globs still format
• Failed --apply-ignore run does not re-format without the flag
• openBinaryFile stub output is not surfaced; Win32 path form runs cleanly

[claude]

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

---

Security Review — PR #1867 (fix/bash-format-apply-ignore-windows)

• Gather context (fetch PR diff, read changed files)
• Understand the request (security review scope)
• Review plugins/bash-format/hooks/bash-format.sh for injection / path / trust-boundary issues
• Review plugins/bash-format/hooks/bash-format.test.sh for issues
• Review README/CHANGELOG/SKILL.md for contract or config changes
• Post findings

No security issues found. This PR touches plugins/bash-format/hooks/bash-format.sh (the PostToolUse hook), its test file, and docs. Scope reviewed:

• New TOOL_FILE/cygpath path-normalization block (bash-format.sh#L157-L169) — cygpath -lm -- "$FILE" uses -- to stop option parsing, and every downstream use (_fmt_target, _lint_target) is double-quoted in both [[ -f ... ]] tests and the shfmt/shellcheck invocations (L207-L220, L235-L248). No command-injection or word-splitting surface introduced. (Note: this is actually more careful than the pre-existing, unmodified cygpath -lm "$FILE" call for FILE_REL at L74-75, which omits -- — that line isn't part of this diff so it's out of scope here, but worth a follow-up.)
• section_applies_to_shell / shell_editorconfig_opt_in changes (removing bare [*] as an opt-in) — pure logic tightening, no new input-handling surface; .editorconfig is still only read via <"$cfg" with regex matching, no eval/sourcing of its contents.
• openBinaryFile retry/suppression logic (L239-L248) — SUGGESTION / low confidence: grep -v 'openBinaryFile' strips any ShellCheck output line containing that substring. Since ShellCheck's gcc-format output is file:line:col: severity: message [SC####] with no source-line echo, a legitimate finding colliding with that substring is very unlikely, but this is a blanket substring filter rather than anchored to the known GHC error prefix, so it's theoretically capable of swallowing a real finding rather than just the intended openBinaryFile: does not exist noise. Not exploitable (the hook is advisory-only and never blocks an edit either way — see header comment L5-6), just worth noting since it slightly widens what gets silently dropped from a security-relevant lint tool's output.
• Race-window existence re-checks ([[ -f "$TOOL_FILE" || -f "$FILE" ]] before mutating/linting) — availability/correctness fix for deleted scratch files, not a TOCTOU security issue (both shfmt -w and shellcheck already operated on $FILE directly before this PR; the re-check only skips work, it doesn't add a new write path).
• Test file changes — test-only, not shipped to consumers, no injection surface (stub tool paths are fixed, fully-quoted).
• README/CHANGELOG/SKILL.md — documentation only, no contract/behavioral claims that diverge from the code.

No pull_request_target/workflow_run usage, no GitHub Actions workflow changes, no secrets/token handling, and no unpinned dependencies in this diff — out of scope for this PR anyway since no .github/workflows/* files changed.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2aa00ed29d

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

---

Code Review — PR #1867 (fix/bash-format-apply-ignore-windows)

• Gather context (fetch PR diff, read changed files)
• Review bash-format.sh changes for correctness (apply-ignore, [*] opt-in, Windows path handling)
• Review bash-format.test.sh for coverage of new behavior
• Review README/CHANGELOG/SKILL.md for consistency with code changes
• Post findings

Scope: correctness, error handling, test coverage, maintainability (security is covered by the separate claude-security-review lane, already posted with no issues).

Findings

1. Manifest version not bumped — blocks the release this CHANGELOG entry describes.
CHANGELOG.md adds a ## [0.6.11] entry, but plugins/bash-format/.claude-plugin/plugin.json is still "version": "0.6.10". Per CLAUDE.md's "Versioned" rule, the manifest version is the cache/update key consumers key off of — without the bump, installed plugins won't pick up the apply-ignore/[*]/Windows-path fixes on next marketplace refresh. (Codex flagged the same issue in a review comment; confirmed correct.) Fix this →

2. Format pass has no path-retry fallback that the lint pass now has, for the same failure mode.
bash-format.sh#L207-L220 runs shfmt -w "$_fmt_target" (where _fmt_target may be the cygpath-normalized TOOL_FILE) and discards stderr unconditionally with no retry against the original $FILE spelling. The lint pass added exactly that retry for ShellCheck at L243-L245 ("some hosts accept only one of the two forms"). If that's true for ShellCheck it's plausibly true for shfmt too — on an affected host, formatting could now silently no-op on TOOL_FILE with zero notice (shfmt errors go to /dev/null, and there's no capability-probe-style message for "wrote nothing"). This mirrors the pre-existing behavior (errors were already suppressed before this PR), so it's not a regression, but it is an inconsistency the PR introduces between the two passes while fixing the identical underlying problem in only one of them. Worth a follow-up rather than blocking this PR.

3. Test coverage gap: the new ShellCheck path-retry branch itself isn't directly exercised.
The new tests (bash-format.test.sh#L424-L477) cover "openBinaryFile from a stub is never surfaced" and "a real Windows path form lints cleanly," which are the right end-to-end checks, but neither forces _lint_target != $FILE and the first attempt to fail while the retry succeeds (i.e., a stub that fails specifically on the cygpath-normalized path and succeeds on the original). That's the one new branch (L243) with no direct coverage. Not blocking — the two existing tests do cover the surrounding logic and the observable behavior — but worth adding if there's a cheap way to fake a cygpath mismatch.

Non-issues verified

• section_applies_to_shell/shell_editorconfig_opt_in regex changes correctly drop the bare [*] catch-all while still matching [*.sh], [*.bash], brace lists, and path-prefixed globs — logic and doc comments agree, and README/SKILL.md were updated consistently with the code.
• The --apply-ignore capability-probe fix (probe via --version instead of inferring from a failed -w run) correctly stops the old bug where a transient -w failure silently discarded a 3.8+ shfmt's ignore = true opt-out — this is the core fix from bash-format: fallback drops --apply-ignore, bare [*] .editorconfig section treated as shell opt-in, openBinaryFile failures on Windows paths #1817 and looks right.
• Race-window re-checks ([[ -f "$TOOL_FILE" || -f "$FILE" ]]) before both the format and lint passes are a reasonable, low-risk hardening — they only skip work on a deleted file, they don't add a new write path.
• CI (ci workflow, which runs bash-format.test.sh) passed on this commit.

[github-actions]

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

[claude]

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

---

Security Review — PR #1867 (fix/bash-format-apply-ignore-windows) — re-run at 3bb7524

• Gather context (git show on both PR commits, since HEAD is now 3bb7524)
• Diff HEAD (3bb7524) against the previously-reviewed commit (2aa00ed) to find anything new
• Re-confirm no security issues in the unchanged bash-format.sh / test logic
• Post findings

No security issues found. This run was triggered by a new commit (3bb7524) on top of the commit already reviewed earlier in this thread. That new commit touches exactly one line:

-  "version": "0.6.10",
+  "version": "0.6.11",

in plugins/bash-format/.claude-plugin/plugin.json — a manifest version bump with no security surface.

All other files in this PR (bash-format.sh, bash-format.test.sh, README.md, CHANGELOG.md, SKILL.md) are unchanged since the earlier security review posted in this thread, which covered:

• The cygpath -lm -- tool-path normalization and its fully-quoted use in _fmt_target/_lint_target.
• The section_applies_to_shell / shell_editorconfig_opt_in tightening (dropping bare [*]) — pure logic change, no new input-handling surface.
• The openBinaryFile suppression (grep -v 'openBinaryFile') — a blanket substring filter, flagged as a low-confidence SUGGESTION (could theoretically swallow a real finding containing that substring), but not exploitable since this hook is advisory-only and never blocks an edit.
• The pre-mutation/pre-lint existence re-checks — availability fix, not a new write path.

That prior analysis still stands; there's nothing new to add for the current HEAD.

[claude]

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

---

Code Review — PR #1867 (fix/bash-format-apply-ignore-windows) — re-run at 3bb7524

• Diff HEAD (3bb7524) against the previously-reviewed commit (2aa00ed)
• Confirm the version-bump finding from the prior review is addressed
• Re-confirm no new issues introduced
• Post findings

Only change since the last review: the manifest version bump.

-  "version": "0.6.10",
+  "version": "0.6.11",

in plugins/bash-format/.claude-plugin/plugin.json. This resolves Finding #1 from the earlier code review (and the matching Codex comment) — the manifest now matches the ## [0.6.11] entry in CHANGELOG.md, so consumers refreshing the marketplace will pick up the apply-ignore/[*]/Windows-path fixes.

No other files changed (bash-format.sh, bash-format.test.sh, README.md, CHANGELOG.md, SKILL.md are unchanged from 2aa00ed), so the rest of the prior review still stands:

• Finding fix: correct plugin-system inaccuracies and harden references #2 (format pass lacks the path-retry fallback the lint pass now has) — still open, non-blocking, suggested as a follow-up.
• Finding feat: markdown-formatter plugin (pilot migration) #3 (new ShellCheck path-retry branch not directly exercised by a dedicated test) — still open, non-blocking, suggested as a follow-up.
• All non-issues previously verified (bare [*] opt-out logic, --apply-ignore capability probe, race-window re-checks, CI passing) are unaffected by this commit.

Nothing new to flag. This PR is in good shape from a code-review standpoint; #2 and #3 are minor, non-blocking suggestions for a follow-up rather than blockers on this PR.

[claude]

Claude encountered an error after 1s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[github-actions]

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/30670550402
• Failure class: rate-limit
• Last SDK result: {"subtype":"success","is_error":true,"num_turns":1,"duration_ms":479,"total_cost_usd":0,"api_error_status":429,"class":"rate-limit"}

Re-run the job to retry the review. A new push does not re-trigger this lane.
An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator (auth).

[claude]

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

---

Security Review — PR #1867 (fix/bash-format-apply-ignore-windows) — re-run at b509361

• Diff HEAD (b509361) against the previously-reviewed commit (3bb7524)
• Re-confirm no security issues in unchanged logic
• Post findings

No security issues found. This run was triggered by a new commit on top of the commit already reviewed earlier in this thread. The new commit (b509361) is a merge of origin/main into this branch, bringing in unrelated commits from other plugins (repo-fleet-hygiene, work-items, knowledge, guardrails) — none of which touch this PR's files.

Confirmed via git diff 3bb7524..HEAD -- plugins/bash-format/: zero changes to any bash-format file since the last review. git diff origin/main..HEAD -- plugins/bash-format/ shows the PR's diff is still exactly the same 6 files / 204 insertions / 67 deletions reported in the PR metadata.

All prior analysis in this thread stands unchanged:

• The cygpath -lm -- tool-path normalization and its fully-quoted use in _fmt_target/_lint_target — no injection or word-splitting surface.
• The section_applies_to_shell / shell_editorconfig_opt_in tightening (dropping bare [*]) — pure logic change, no new input-handling surface.
• The openBinaryFile suppression (grep -v 'openBinaryFile') — blanket substring filter, low-confidence SUGGESTION (could theoretically swallow a real finding containing that substring), not exploitable since this hook is advisory-only and never blocks an edit.
• The pre-mutation/pre-lint existence re-checks — availability fix, not a new write path.
• No .github/workflows/* files changed in this PR — Actions-specific risks (permissions, pull_request_target, unpinned actions) are out of scope here and covered by zizmor's advisory lane.

Branch: fix/bash-format-apply-ignore-windows