Skip to content

fix(scripts): reject Windows-dialect contract roots and close three shell-portability token gaps - #2064

Merged
kyle-sexton merged 8 commits into
mainfrom
fix/ci-scripts-stranded-findings
Aug 9, 2026
Merged

fix(scripts): reject Windows-dialect contract roots and close three shell-portability token gaps#2064
kyle-sexton merged 8 commits into
mainfrom
fix/ci-scripts-stranded-findings

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Discharges four stranded review findings against the repo-level scripts/ gates. Every finding was reproduced through its real consumer with a pre-fix control before the fix, and each control discriminates — it passes on origin/main's artifact and changes verdict on this branch's.

Finding 1 — check-contract-slice-prune.sh accepts Windows-dialect absolute roots

PRRT_kwDOTCGFQM6Tz-jR (#1445), scripts/check-contract-slice-prune.sh:119.

canonicalize_repo_path() judged absoluteness only in the POSIX dialect (/*). A drive-qualified or backslash-rooted contract_dir was therefore read as repo-relative. Git names repo-relative diff paths with / separators and never with a drive qualifier or a raw backslash, so such a root can match no diff path at all — --check-diff reports success over an empty match set, policing nothing. That is precisely the fail-open the gate exists to prevent.

The irony is worth stating plainly: 19d736bf (#1445) both discharged the three sibling threads on this file and introduced this one — same harm class, different route in, inside the very function it added.

Reproduced with three fixtures — C:/outside, C:\outside, \\server\share — all of which passed silently under --check-diff before the fix. The legitimate-root control fails correctly on the same machinery, so the reproduction discriminates rather than merely erroring.

Absoluteness is now judged in both dialects. A backslash anywhere is refused, not only a leading one: it is a separator in the Windows dialect and an escaped character in Git's own output, so no value carrying one is comparable to a diff path. Note c:outside is drive-qualified but relative in Windows semantics — it is refused for the same Git-comparability reason, not because it is absolute.

Finding 2 — the --sort token missed a fully quoted option word

PRRT_kwDOTCGFQM6T1s2n (#1530), scripts/shell-portability-tokens.txt, filed at :98, live at :173.

The pattern demanded whitespace immediately before --sort, so a quote wrapping the whole option word sat outside the newly added optional-quote position. sort '--sort=version' "$file" and sort "--sort=version" "$file" hand GNU sort the identical argument after quote removal, yet the scanner returned clean.

An optional quote is now admitted at both positions. git tag --sort=version:refname stays clean for the reason it already did — the WORD boundary still rejects the longer version:refname.

Findings 3 and 4 — the sed tokens, landed together

Both edit the same live token at :216, so they cannot be split.

Finding 3 (PRRT_kwDOTCGFQM6T1rfG, #1534, filed :117, live at :216 and :232): the unrestricted [[:space:]][^\n]* command gap crossed ;, && and |, so a later command's options armed the sed token. sed -n 'p' "$file"; grep -Ei pattern "$file" was reported even though only grep receives -Ei. The reporter also called the second site: "the --in-place pattern has the same issue" — so the :232 edit is in scope as filed, not creep. Both gaps now stop at a shell command separator, matching what the date -d / stat -c / mktemp -p tokens already do. Quoted separators are neutralized before the token matches, so a ; inside a sed script stays ordinary data.

Finding 4 (PRRT_kwDOTCGFQM6T1rfI, #1534, filed :117, live at :188 and :216): scope is broader than filed. Two tokens read sed -ni clean — one keyed on a literal -i substring, which -ni does not contain; the other on an E earlier in the cluster. GNU sed 4.9's --help documents -i[SUFFIX] alongside the no-argument short options -n, -b, -E, -r, -s, -u, -z, so any cluster built from those letters and ending in i is the same unsuffixed in-place edit. Premise verified live against GNU sed 4.9: sed -ni rewrote a file in place, 3 lines to 1.

The two narrower predecessors are therefore consolidated into one token — -i standing alone and -i ending a cluster are the same option. The argument-taking letters -e, -f, -l are deliberately outside the class: GNU accepts their value attached, so sed -ei passes the script i rather than editing in place.

The removed plain -i (no E) does not double-fire this cluster token test goes with the consolidation — with one token there is nothing to double-fire.

Verification

Every row below ran through the real consumer, scripts/check-shell-portability.sh --paths <fixture>, never by hand-running grep -E against the token file. PRE uses origin/main's token file via SHELL_PORTABILITY_TOKENS; POST uses this branch's. 0 = gate passes, 1 = gate reports the construct.

Fixture PRE (main) POST (branch) Meaning
fully quoted --sort=version (4 spellings) 0 1 finding 2 false negative reproduced, then closed
sed -n 'p' f; grep -Ei p f 1 0 finding 3 false positive reproduced, then closed
sed -n 'p' f && tool --in-place x 1 0 finding 3's --in-place site, same shape
sed -ni '/keep/p' f 0 1 finding 4 false negative reproduced, then closed
sed -i '' 's/foo/bar/' f 1 1 regression guard — the space-separated empty-suffix catch survives consolidation
sed -i.bak / sed -Ei.bak 0 0 regression guard — the dual-compatible escape hatch stays clean

The last two rows exist because this change deletes a token. The empty-suffix idiom looks BSD-safe but is not (GNU consumes the empty string as sed's script argument and exits 2), and the attached-nonempty-suffix form is the one genuinely portable spelling. Neither may shift.

The self-gating trap

shell-portability-lint gates this PR, and these edits change the lint that runs against this PR's own diff. A fixture-only check would not have caught a token that newly flags existing legitimate code, so the whole-repo audit was run under both token files and diffed:

  • scripts/check-shell-portability.sh --all with origin/main's tokens and with this branch's tokens produce a byte-identical hit set.
  • The remaining hits are pre-existing whole-repo debt present identically on main (\b / \w / \s / \S in four test files); --all exits 1 on both. CI's gate is changed-file scoped, so that debt is not this PR's to carry.
  • CI's actual invocation, scripts/check-shell-portability.sh origin/main, is green: No unexcused GNU-only constructs in 3 shell file(s).

Nothing was suppressed and no portability-ok: marker was added.

Changelog parity

Not applicable to this PR, determined by reading the gate rather than assuming. scripts/check-changelog-parity.sh scopes itself to plugins: --check and --check-bump glob plugins/*/.claude-plugin/plugin.json, and --check-order globs plugins/*/CHANGELOG.md docs/conventions/*/CHANGELOG.md. This diff touches only repo-level scripts/, which has no plugin manifest, and the repo has no root CHANGELOG.md.

Worth closing the one loose end explicitly: --check-bump does take origin/$BASE_REF, so a diff-scoped gate could in principle fire on a scripts-only diff. It cannot here — the diff scope is used only to detect manifest version changes, and this PR changes no manifest version.

Scope

Four hunks, four findings, no unmapped changes. The three sibling threads on check-contract-slice-prune.sh (all #1429) are already fixed by 19d736bf (#1445), confirmed an ancestor of main with pre-fix controls at 19d736bf^ reproducing both bugs; this PR deliberately contains no fix for them.

Related

No linked issue

kyle-sexton and others added 5 commits August 7, 2026 08:28
Unverified batch preserved from a session that ended before its findings were
falsified against origin/main. Touches check-contract-slice-prune, the
shell-portability test suite, and the portability token list.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cursor

cursor Bot commented Aug 9, 2026

Copy link
Copy Markdown

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 Bot commented Aug 9, 2026

Copy link
Copy Markdown

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


Security review complete

  • Gather context (PR diff, changed files)
  • Review scripts/check-contract-slice-prune.sh changes
  • Review scripts/check-contract-slice-prune.test.sh changes
  • Review scripts/shell-portability-tokens.txt changes
  • Review scripts/check-shell-portability.test.sh changes
  • Post final findings

No security issues found.

Scope: this PR touches only repo-level scripts/ — no GitHub Actions workflow files, no permission or trigger changes, no dependency/action pins. The four changed files are a path-traversal-adjacent gate fix, a lint-token regex tweak/consolidation, and their accompanying tests.

What I looked at specifically:

  • canonicalize_repo_path() (check-contract-slice-prune.sh#L127) — the new absoluteness check [[ "$raw" == /* || "$raw" == [A-Za-z]:* || "$raw" == *\\* ]] is a strict widening (rejects strictly more inputs than before) and fails closed: add_contract_dir() treats any rejection as exit 2, never as "silently skip." I traced downstream use of contract_dir — it's only ever used for prefix string comparison (slug_of()), never interpolated into a command, eval, or passed to a shell. No path-traversal or injection surface from this value.
  • shell-portability-tokens.txt — these ERE patterns are consumed by check-shell-portability.sh (unchanged in this PR) via awk pattern matching, not grep -e "$token" or eval, so there's no argument-injection surface from a token starting with -, and no shell execution of file content. The engine is a linear-time ERE matcher (awk/GNU grep), not backtracking PCRE, so the added alternation/optional-quote complexity in the --sort and sed cluster tokens doesn't introduce a ReDoS risk.
  • Test files — new fixtures are static heredoc content authored by the test itself (not attacker/PR-diff-controlled input), so no new untrusted-data path.

No pull_request_target/workflow_run patterns, no permissions: changes, no secrets handling, and nothing in scope for zizmor's advisory lane (no workflow files changed at all).

@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 8m 39s —— View job


Review complete

  • Gather context (PR body, diff, CLAUDE.md/REVIEW.md)
  • Review check-contract-slice-prune.sh dialect fix
  • Review check-contract-slice-prune.test.sh new tests
  • Review shell-portability-tokens.txt token changes
  • Review check-shell-portability.test.sh test changes
  • Post findings

Verification note: Bash execution (running the test suites / gates directly) required approval this lane couldn't grant, so I could not independently execute check-shell-portability.test.sh, check-contract-slice-prune.test.sh, or the whole-repo audit the PR body describes. Everything below is from reading the diff, reading the full unchanged context of each file, and manually hand-tracing the ERE patterns against the PR's own fixture list (including the --sort/sed quote- and separator-boundary cases). The PR body's PRE/POST verification table and the "self-gating trap" whole-repo audit are author-claimed and unverified by me.

Findings

1. scripts/check-shell-portability.test.sh:764 — the --in-place unit test still exercises the pre-fix, unnarrowed token, so Finding 3's second site has no regression coverage

The shipped token for sed --in-place (scripts/shell-portability-tokens.txt:256) was narrowed by this PR to stop its command gap at a shell separator:

(^|[^[:alnum:]_])sed([[:space:]][^;&|()`\n]*)?[[:space:]]--in-place([[:space:]|&;()<>]|=|$)

But the dedicated unit test block for --in-place still builds its own local copy of the old, wide-gap pattern:

tok="$(one_token_list '(^|[^[:alnum:]_])sed([[:space:]][^\n]*)?[[:space:]]--in-place([[:space:]|&;()<>]|=|$)')"

(

tok="$(one_token_list '(^|[^[:alnum:]_])sed([[:space:]][^\n]*)?[[:space:]]--in-place([[:space:]|&;()<>]|=|$)')"
)

That local tok never got updated to match the shipped narrowing, so it's now silently out of sync with production. Two consequences:

  • None of the assertions in that block (lines 766–804) actually exercise the fix — they'd pass identically against the old or new pattern, so they can't catch a regression in the separator-scoping.
  • The PR's own verification table claims sed -n 'p' "$file" && tool --in-place x flips from 1 (PRE) to 0 (POST) — this exact fixture (or any command-separator-before---in-place case) does not appear anywhere in this test file. I grepped for it directly and found no match. The sibling REAL_TOKENS-backed "shipped list" block at lines 1544–1565 also doesn't cover it — it only exercises operator-terminated forms (sed --in-place|cat, etc.), not a case where an earlier command's own -i/--in-place option must not arm the token.

Contrast with the cluster (-[bEnrsuz]*i) token's own test block a few lines above (line 550), whose local tok was correctly updated to the new narrowed-gap pattern and does carry the sed -n 'p' "$file"; grep -Ei pattern "$file" / && / | regression cases (lines 166–179 in the diff). The --in-place token is missing the equivalent.

Given how deliberately this PR pins down PRE/POST behavior for every other claim (including guarding against its own self-gating trap), this looks like an oversight rather than an intentional gap — worth adding the same three separator-scoping cases (;, &&, |) against the --in-place token, either by updating the local tok at line 764 to the shipped pattern or by moving that coverage onto $REAL_TOKENS.

Fix this →

What checked out

  • check-contract-slice-prune.sh:129-131 — the dual-dialect absoluteness check (/*, [A-Za-z]:*, *\\*) traced correctly against all five new fixtures (C:/outside, C:\outside, c:outside, \outside, \\host\public), and the failure path correctly reaches the existing escapes the repo root message at line 188. Confirmed lib/parse-concern-value.sh (the value parser feeding this) does no backslash unescaping, so the fixtures reach canonicalize_repo_path byte-for-byte as written — the reproduction is sound.
  • shell-portability-tokens.txt:240 (--sort=WORD quote widening) — hand-traced against all four new fixtures ('--sort=version', "--sort=version", '--sort' version, "--sort" "version"); all match, and git tag --sort=version:refname still doesn't (no second --sort occurrence after whitespace, unrelated to this change).
  • shell-portability-tokens.txt:237 (consolidated sed -i cluster token, [bEnrsuz]*i) — hand-traced every new fixture: -ni/-si/-ri/-zi/-bi/-ui match; -ei/-fi (argument-taking letters) and -ni.bak (attached suffix) correctly don't; -iE/-Ei.bak/-Ei'' correctly stay unflagged (suffix vs. flag distinction preserved); the command-separator-stop (;, &&, |) correctly prevents a later command's options from arming the token, and this is consistent with the pre-existing quote-masking (qline/neutralize(), ci: enable staged shell-portability-lint classes (date -d, stat -c) #1544) already in check-shell-portability.sh, unmodified by this PR, that the sed 'a;b' -Ei "$file" case relies on.
  • Test helper reuse (mk_repo/commit_work/run_diff/git_q, one_token_list/scan_paths/tmpsh) is consistent with existing sibling tests in both files — no structural issues.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 16f87dd208

ℹ️ 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".

Comment thread scripts/shell-portability-tokens.txt Outdated
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown

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

kyle-sexton and others added 2 commits August 9, 2026 02:34
The gap class excludes `(` and `)` alongside the separators, but the
comment claimed only separators. Record the exclusion, the converse
catch it buys, and the process-substitution blind spot it costs — a
tradeoff the date/stat/mktemp tokens already carry.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown

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


Security review complete

  • Gather context (PR diff, changed files)
  • Review scripts/check-contract-slice-prune.sh changes
  • Review scripts/check-contract-slice-prune.test.sh changes
  • Review scripts/shell-portability-tokens.txt changes
  • Review scripts/check-shell-portability.test.sh changes
  • Post final findings

No security issues found.

Scope confirmed via git diff origin/main...HEAD: four repo-level scripts/ files, no GitHub Actions workflow files, no permission/trigger changes, no dependency or action pins touched. The newest commit on this sync (d934a28) is comment-only — it documents the sed command-gap's (/) exclusion and process-substitution blind spot that Codex flagged, without changing any pattern or code path.

What I checked specifically:

  • canonicalize_repo_path() (scripts/check-contract-slice-prune.sh:129-131) — the widened absoluteness check [[ "$raw" == /* || "$raw" == [A-Za-z]:* || "$raw" == *\\* ]] only rejects more than before and fails closed (add_contract_dir() exits 2 on any canonicalize_repo_path failure, never silently skips). contract_dir is used only for string prefix comparison and in a static echo error message (check-contract-slice-prune.sh:188, :192) — never interpolated into eval, a command line, or a subshell. No path-traversal or injection surface from this value, and the new dialect check closes the described fail-open (a Windows-rooted root matching zero diff paths, --check-diff policing nothing) without introducing a new one.
  • shell-portability-tokens.txt — confirmed via scripts/check-shell-portability.sh:1349 that the token file is read as ERE pattern data into an awk program (awk '...' "$TOKENS" "$awk_file"), never as a grep -e/eval argument and never executed as shell. No argument-injection surface from a token starting with -, and awk's ERE engine is not backtracking, so the added alternation/optional-quote complexity in the --sort and sed cluster tokens carries no ReDoS risk. This is a portability lint (advisory over shell style), not an access-control or execution gate — a false negative here (e.g. the process-substitution gap Codex correctly identified) is a missed style finding, not a security vulnerability.
  • Test files — new fixtures in both .test.sh files are static heredoc/string literals authored by the test itself, not attacker- or PR-diff-controlled input. No new untrusted-data path.

No pull_request_target/workflow_run patterns, no permissions: changes, no secrets handling, nothing in scope for zizmor's advisory lane (no workflow files changed at all).

@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown

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


Review complete

  • Gather context (PR body, diff, prior review comments)
  • Review check-contract-slice-prune.sh dialect fix
  • Review check-contract-slice-prune.test.sh new tests
  • Review shell-portability-tokens.txt token changes
  • Review check-shell-portability.test.sh test changes
  • Check Codex's process-substitution finding on --in-place/cluster tokens
  • Post findings

Verification note: Bash execution (running the gate/test suites directly) required approval this lane couldn't grant, so — like the prior review pass on this PR — I could not independently execute check-shell-portability.test.sh, check-contract-slice-prune.test.sh, or the whole-repo audit the PR body describes. Findings below come from reading the diff, the full unchanged context of each file, and hand-tracing the ERE patterns (including diffing the test file's local token copies against the shipped tokens byte-for-byte). The PR body's PRE/POST verification table and the "self-gating trap" audit are author-claimed and unverified by me.

Findings (posted inline)

  1. scripts/check-shell-portability.test.sh:764 — the --in-place unit test block's local tok still builds from the pre-fix, unnarrowed gap pattern ([^\n]*), not the shipped narrowed one ([^;&|()\n]*) at [shell-portability-tokens.txt:265](https://github.com/melodic-software/claude-code-plugins/blob/d934a28e2006533caf4509da36e0530fa563e653/scripts/shell-portability-tokens.txt#L265). This is the same gap a prior review round flagged on this PR — it's still open. Consequence: none of that block's assertions can distinguish old from new separator-scoping behavior, and the PR body's own verification-table claim for sed -n 'p' "$file" && tool --in-place x(finding 3's--in-placesite) has no regression coverage anywhere in the suite — I grepped for that fixture shape and it doesn't appear, including in the$REAL_TOKENS`-backed block at lines 1544–1565.
  2. scripts/shell-portability-tokens.txt:265 (minor) — the process-substitution blind-spot note this PR's last commit added to the cluster (-i) token's comment wasn't mirrored to the --in-place token's comment, even though both got the identical gap narrowing in this PR and Codex's review comment explicitly called out that the same regression affects both. Documentation-completeness nit, not a behavior difference.

What checked out

  • check-contract-slice-prune.sh:127-131 (dual-dialect absoluteness check /*, [A-Za-z]:*, *\\*) — hand-traced against all five fixtures in the new test block (C:/outside, C:\outside, c:outside, \outside, \\host\public); all correctly reach the escapes the repo root exit-2 path. The rationale for rejecting c:outside (drive-qualified but Windows-relative, refused for Git-comparability, not absoluteness) is accurately reflected in both the code and the new comment.
  • shell-portability-tokens.txt:196 (--sort=WORD quote widening to admit a quote around the whole option word, not just its value) — traced against all four new fixtures; git tag --sort=version:refname still correctly stays clean.
  • shell-portability-tokens.txt:246 (consolidated sed -[bEnrsuz]*i cluster token, replacing the two narrower predecessors) — traced the new fixtures (-ni/-si/-ri/-zi/-bi/-ui match; -ei/-fi/-ni.bak correctly don't); the removed "plain -i does not double-fire the -Ei token" test is correctly obsolete now that there's only one token to fire.
  • The separator-stop narrowing (;, &&, |) on both the cluster and --in-place tokens is correctly implemented and does have solid regression coverage for the cluster token (lines 605–636 in the new test diff) — the gap is specifically that the --in-place token's equivalent coverage (finding 1 above) is missing.
  • Test helper reuse (mk_repo/commit_work/run_diff/git_q, one_token_list/scan_paths/tmpsh) is consistent with existing sibling tests in both files.
    · branch fix/ci-scripts-stranded-findings

Comment thread scripts/check-shell-portability.test.sh
Comment thread scripts/shell-portability-tokens.txt Outdated
…call

Narrowing the sed command gap to stop at a bare paren also stopped it at
a process substitution, which is not a command of its own -- the shell
folds it into the word it touches. `sed -f <(gen) -i "$f"` therefore went
undetected where the pre-narrowing gap caught it. Readmit exactly that
shape; a `)` closing a real subshell still ends the gap, so the converse
catch it bought is kept.

The `--in-place` unit block was building its local token from the
pre-narrowing pattern, so none of its assertions could tell old behavior
from new. Point it at the shipped pattern and give both sed tokens
separator- and process-substitution-scoping cases, including two backed
by the real token list so a hand-copied pattern cannot drift unnoticed
again.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown

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


Security review complete

  • Gather context (gh pr diff against origin/main, changed files)
  • Review scripts/check-contract-slice-prune.sh changes
  • Review scripts/check-contract-slice-prune.test.sh changes
  • Review scripts/shell-portability-tokens.txt changes
  • Review scripts/check-shell-portability.test.sh changes
  • Post final findings

No security issues found.

Reviewed the diff at HEAD 5018dbf against origin/main (four repo-level scripts/ files, +299/-65). No GitHub Actions workflow files are touched, no permissions:/trigger changes, no dependency or action pins — nothing in zizmor's advisory lane to defer on since no workflow files changed.

What I checked for logic/trust-boundary issues specifically:

  • canonicalize_repo_path() (check-contract-slice-prune.sh#L127-L131) — the new dialect check [[ "$raw" == /* || "$raw" == [A-Za-z]:* || "$raw" == *\\* ]] is a strict widening of rejection (superset of the prior /*-only check), and every caller fails closed: add_contract_dir() exit 2s on any rejection rather than silently falling back to a default or skipping the check. I traced contract_dir's only downstream uses — prefix string comparison in slug_of() (L233-L246) and a literal echo in the error message — never interpolated into eval, a command line, git show, or a subshell. So there's no injection surface from this value, and the change closes a real fail-open (a Windows-dialect root that matches zero diff paths, letting --check-diff "pass" while policing nothing) without introducing a new bypass. The [A-Za-z]:* / *\\* tests are plain glob pattern matches, not evald or regex-compiled from untrusted input in a way that could be abused.
  • shell-portability-tokens.txt — these ERE patterns are read as data into check-shell-portability.sh's awk invocation (unchanged by this PR), never passed to grep -e, eval, or executed as shell — so a token string starting with - or containing shell metacharacters has no argument-injection or code-execution surface. awk's ERE matcher is linear-time (not backtracking PCRE), so the added alternations/optional-quote positions in the --sort and sed cluster/--in-place tokens carry no ReDoS risk. This is an advisory lint over shell style (GNU-only construct detection), not an access-control or execution gate — its false negatives (e.g. the process-substitution gap Codex flagged and this PR's last commit, 5018dbf, closes) are missed style findings, not security vulnerabilities in themselves.
  • Test files (check-contract-slice-prune.test.sh, check-shell-portability.test.sh) — all new fixtures are static heredoc/string literals authored by the test itself, not attacker- or PR-diff-controlled input, so no new untrusted-data path is introduced.

No secrets handling, no credential/token flow, and no authorization/access-control logic in scope — this PR's blast radius is entirely CI-gate correctness (a path-escape lint and a shell-portability lint), not a security boundary.
· branch fix/ci-scripts-stranded-findings

@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown

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


Code review complete

  • Gather context (PR diff, changed files, prior review threads)
  • Review scripts/check-contract-slice-prune.sh dialect fix
  • Review scripts/check-contract-slice-prune.test.sh new tests
  • Review scripts/shell-portability-tokens.txt token changes
  • Review scripts/check-shell-portability.test.sh test changes
  • Check whether prior review findings (--in-place test coverage gap, doc-parity nit) were addressed by latest commits
  • Post findings

No new correctness, error-handling, or test-coverage issues found. This is my independent read of the diff scoped to the four changed files (git diff origin/main HEAD -- scripts/...), not a restatement of the PR body's own verification claims — those (the PRE/POST table, the whole-repo audit) remain author-claimed and unverified by me since I did not execute the test suites myself.

Two previously-open findings are now closed

Two prior review rounds on this PR (mine and Codex's) flagged that the --in-place unit test block still built its local $tok from the pre-narrowing gap pattern, and that the process-substitution rationale wasn't mirrored into the --in-place token's comment. Commits d934a28 and 5018dbf fix both:

  • check-shell-portability.test.sh:251 — the local tok now matches the shipped --in-place token character-for-character (including the [<>]\([^)]*\) process-substitution readmission), and the block gained the same three separator-scoping cases (;, &&, |) the cluster token already had, at lines 260–275, plus a process-substitution regression case at lines 277–287.
  • shell-portability-tokens.txt:468-473 — the --in-place token's comment now explicitly carries the process-substitution rationale ("Its command gap is the cluster token's, character for character... while still admitting a process substitution").

What I independently traced and confirmed

  • canonicalize_repo_path() (check-contract-slice-prune.sh:129-131) — hand-traced all five fixtures (C:/outside, C:\outside, c:outside, \outside, \\host\public); all correctly return 1 and reach the escapes the repo root exit-2 path. The error message text (is absolute or escapes the repo root) does contain the substring the tests assert on. A pre-existing legitimate relative root (docs/slices, check-contract-slice-prune.test.sh:160-165) still resolves correctly, so the widened check doesn't regress the ordinary case.
  • --sort token (shell-portability-tokens.txt:349) — hand-traced all four new fixtures character-by-character through the widened pattern; all match, and git tag --sort=version:refname still doesn't (the :refname suffix fails the boundary class, unrelated to this change).
  • Consolidated sed -[bEnrsuz]*i cluster token — confirmed no stray references to the two removed predecessor patterns remain anywhere in either file, and the removed "plain -i does not double-fire" test is correctly obsolete now that there's one token to fire.
  • Process-substitution handling — read collapse_subs()/neutralize() in the unchanged check-shell-portability.sh (not part of this diff) to confirm the token comments' claims are accurate: qline (quoted separators neutralized) is built first, cline (subs collapsed, process substitutions kept verbatim) is derived from qline, and both views are matched — this is exactly what the new token comments describe.
  • No stale count assertions — grepped for any fixed-count/wc -l style assertions against the token file that a token consolidation could silently invalidate; found none.

Scope note

Security aspects (the path-traversal-adjacent fix, ReDoS risk in the widened patterns) are out of scope for this lane per REVIEW.md's split — the dedicated security lane already reviewed this PR twice and found nothing.

@kyle-sexton
kyle-sexton merged commit 4dc5a14 into main Aug 9, 2026
33 checks passed
@kyle-sexton
kyle-sexton deleted the fix/ci-scripts-stranded-findings branch August 9, 2026 08:57
kyle-sexton added a commit that referenced this pull request Aug 9, 2026
…dmits (#2085)

A one-comment correction to `scripts/shell-portability-tokens.txt:245`,
which #2064 merged before
this fix landed. Comment-only — no token, no behavior, no test changes.

## What is wrong on main

The sed command-gap comment claims `[<>]\([^)]*\)` "readmits exactly
that shape and nothing else".
It does readmit a process substitution, but `[^)]*` is not recursive, so
a process substitution
whose body itself contains one — `sed -f <(diff <(a) <(b)) -i "$f"` —
stops the gap at the INNER
`)` and goes unreported.

An ERE cannot balance parentheses; catching that shape needs real
parsing, which this grep-level
tripwire does not attempt. So the residual is a false NEGATIVE on a rare
shape, which is the
direction this gate already accepts everywhere else. The defect is the
comment asserting a
completeness the regex does not have — a reader trusting it would not
think to check.

## Provenance

Found by the fresh-context verifier on #2064 while auditing the very fix
that introduced the
sentence. #2064 merged with the overstatement still in place; this
carries the correction forward.

Prose asserting more than the artifact does has been a recurring defect
class on this sweep — it
drew CHALLENGE on `context-guard` twice and appeared again as
`disk-hygiene`'s R1 — so it is worth
correcting rather than leaving as a harmless-looking comment.

## Related

Follow-up to #2064. Discharges no review thread; the four findings #2064
addressed are resolved.

No linked issue

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant