fix(typos-format): guard bundled config path when plugin root unset - #2504
fix(typos-format): guard bundled config path when plugin root unset#2504kyle-sexton wants to merge 4 commits into
Conversation
Fixes #1257 Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
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 1m 29s —— View job Security Review
Scope: This PR touches only a Changes reviewed:
Conclusion: No security issues found. This is a narrow, well-guarded change (empty/unset-var check + file-existence check before use, values passed through a quoted array) with no injection, authorization, or secret-handling concerns. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bdb6d2ab3
ℹ️ 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 finished @kyle-sexton's task in 6m 34s —— View job Code review
This PR (closing #1257) bundles a
Note: I could not run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
|
Claude finished @kyle-sexton's task in 1m 23s —— View job Code review
Result: nothing to review — this PR's diff against Verified two ways:
No inline comments were posted since there is no diff to anchor them to. If this branch is expected to still carry the |
…PR (#2523) ## Summary `scripts/verify-security-review-evidence.sh` runs under `set -euo pipefail` and called its scope helper bare: ```bash pr_touches_security_paths "$base_ref" local in_scope=$? ``` The helper signals OUT-of-scope by **returning non-zero**. Under `set -e`, a bare call with a non-zero return aborts the shell immediately — so the script died on that line, exit 1, no message, and the `"diff does not touch security-relevant paths — guard not applicable"` branch below was unreachable dead code. Every out-of-scope pull request FAILED the guard instead of being waved through. Observed on [run 31637054526](https://github.com/melodic-software/claude-code-plugins/actions/runs/31637054526) (PR #2512): the diff touches only `docs/conventions/loop-lane/*.md`, `security-review / security-review` correctly **SKIPPED**, and `security-review-evidence` failed beside it with a step log containing nothing but `##[error]Process completed with exit code 1`. The empty log is the signature — `set -e` aborts before any `echo` can run. Reproduced standalone: ``` $ cat setE.sh set -euo pipefail f() { return 1; } main() { f "x"; local rc=$?; echo "REACHED with rc=$rc"; } main $ bash setE.sh; echo "exit=$?" exit=1 # "REACHED" never prints ``` ## Fix — and why not the one-liner The obvious fix is `pr_touches_security_paths "$base_ref" || in_scope=$?`. It clears the red check and introduces something worse, which **ShellCheck flags directly via SC2310** — a check this repo enables on purpose in `.shellcheckrc` ("catches the bash trap where errexit silently stops working inside if/while/&&/||"). The helper runs `python3`, and a crashing interpreter also exits 1. With out-of-scope encoded as "returns 1", a **broken** scope check is indistinguishable from a **negative** one — the guard would skip itself and silently pass a PR it exists to check. Fail-open is the wrong direction for a security guard. So the two channels are separated instead: the helper prints its verdict (`in-scope` / `out-of-scope`) on **stdout** and reserves a non-zero **exit** for a genuine fault. The caller consumes it via command substitution, which keeps `set -e` live for the helper, and treats any unrecognised verdict as a fault rather than a pass. ## Verification Run in this worktree — commands I actually ran, with real output: - `bash scripts/verify-security-review-evidence.sh.test.sh` -> **6 pass, 0 fail**, `All checks passed.` - `shellcheck --rcfile .shellcheckrc scripts/verify-security-review-evidence.sh scripts/verify-security-review-evidence.sh.test.sh` -> **clean**, with SC2310 no longer firing on the guard (it fired on the `||` variant, which is how the fail-open hole was found). New regression cases: - a non-zero-returning helper called bare under `set -e` kills the script — modelled in a **separate `bash -c` process** deliberately. A `( … )` subshell will not do: bash suppresses `set -e` for the whole dynamic extent of a command whose status is being tested, and `$( … )` inside `[[ … ]]` is exactly that context, which made the bug unreproducible in the very harness meant to catch it. My first attempt at this test failed for that reason, not because the fix was wrong. - static guards that fail if either older shape (bare call, or `||`-suppressed call) returns - a static guard that fails if the unrecognised-verdict catch-all is removed **Not verified here:** that the three currently-red PRs (#2512, #2504, #2499) go green. They will only pick this up once their branches carry it — the evidence workflow runs from the PR head. ## Related Third distinct defect found in this one file today, which is itself the finding: it infers another workflow's behaviour by reading that workflow's logs. #2517 (grep matched the lane action's echoed source) only became reachable once the `cursor[bot]` outage was fixed; this one only became reachable once #2517 let execution past the grep. No linked issue — filing was interrupted; the detail is captured here instead. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Superseded on main — typos-format bundled-config guard already landed; empty diff vs main. |
2 participants
Closes #1257
Related
No linked issue — maintenance merge/rebase PR.