fix(ci): detect sort's --version-sort / --sort=version long forms - #1530
Merged
Conversation
shell-portability-lint's sort -V class only matched the short flag (incl. inside a combined cluster). GNU sort also documents two long-form spellings of the same option -- `-V, --version-sort` and `--sort=WORD` where WORD includes `version` (man7.org sort(1), verified) -- that slipped through undetected. Adds both as unambiguous literal ERE tokens, the same shape --perl-regexp already uses alongside grep -P's combined-cluster pattern (no command-context prefix needed since neither string collides with anything else). Item 5 of #1517's round-2 findings; items 1-4 need scanner/scope changes with real design content and stay deliberately deferred per the issue's own "re-opens when" framing (items 1, 3, 4, and half of item 5 already overlap the still-open #1519, which is left to land on its own). Closes #1517 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous commit's two regression tests used an isolated single-token fixture (SHELL_PORTABILITY_TOKENS) to prove the scanner CAN match --version-sort / --sort=version -- that alone doesn't prove the tokens actually shipped in scripts/shell-portability-tokens.txt. Adds one more case against REAL_TOKENS (the real shipped list) with a fixture containing both spellings, asserting a distinct PORTABILITY line for each, mirroring the existing REAL_TOKENS cases further down this file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
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."
Re-running the job, or pushing a new commit, will retry the review. |
|
Warning Automated security 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."
Re-running the job, or pushing a new commit, will retry the review. |
![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: 6a67a5831a
ℹ️ 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".
…lity-lint-round-2 # Conflicts: # scripts/check-shell-portability.test.sh # scripts/shell-portability-tokens.txt
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56898d97aa
ℹ️ 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".
`--sort=WORD` is not an unambiguous GNU-sort literal the way `--version-sort` is: `git tag`, `git for-each-ref` and `git branch` all take a portable `--sort=<key>` whose key set includes `version:refname`, which the bare token matched as a prefix. Any changed script using Git's version-aware ref ordering would have red-lined CI with no accurate exemption to reach for. The token now carries the same `sort`-command prefix the short-flag pattern uses and requires a boundary after the WORD, and it accepts the WORD in every spelling the shell can hand over: attached after `=` or as the next argv element, bare or quoted. Regression cases cover all four flagged spellings plus the Git counter-example that must stay clean, at both the isolated-token and shipped-list levels.
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cfbb87a34
ℹ️ 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".
A whitespace-or-end-of-line boundary let the no-whitespace forms through: `x=$(sort --sort=version)`, `sort --sort=version|head` and `sort --sort=version; echo` are the same GNU-only operation, but the shell terminates the option argument with an operator rather than a space, so the token missed all three. The boundary is now every character that can actually end a shell word — a control operator, a redirection, a subshell close, a quote — alongside whitespace and end of line. A longer word such as Git's `version:refname` key still does not match, because none of its continuation characters terminate. Regression cases cover the command-substitution, pipe, semicolon and redirection forms at both the isolated-token and shipped-list levels.
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd92c740c8
ℹ️ 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".
| # those spellings is the same GNU-only operation. | ||
| sort[^\n]*[[:space:]]-[A-Za-z]*V[A-Za-z]*([[:space:]]|$) | ||
| --version-sort | ||
| sort[^\n]*[[:space:]]--sort(=|[[:space:]]+)['"]?version([[:space:]|&;()<>'"`]|$) |
There was a problem hiding this comment.
When the complete option is quoted, such as sort '--sort=version' "$file" or sort "--sort=version" "$file", the shell still passes the exact --sort=version argument, but this pattern requires --sort immediately after whitespace and the portability scanner returns clean. I checked GNU coreutils 9.4 sort --help, which documents --sort=WORD with version -V, so these invocations still select the GNU-only operation. Fresh evidence after the quoted-WORD fix is that these quotes begin before --sort, outside the newly added optional quote position; accept quoting around the complete option and add regression cases.
Useful? React with 👍 / 👎.
kyle-sexton
added a commit
that referenced
this pull request
Jul 26, 2026
Resolves onto the now-merged #1519/#1534/#1530 (readlink guard now requires an actual || fallback, sed -i empty-suffix guard removed entirely as non-portable). Re-applies the stat -c guard on the new base with matching ||-required rigor, fixes an apostrophe that broke the awk single-quoted block during manual conflict resolution, and adds a regression test proving the stat -c guard requires an actual || relationship (mirroring the existing readlink/realpath test). *This was generated by AI during work-loop execution.* Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Jul 26, 2026
Closed
Merged
7 tasks
kyle-sexton
added a commit
that referenced
this pull request
Jul 26, 2026
…erator-terminated forms (#1548) *This was generated by AI during work-loop execution.* Closes #1545 ## Summary - #1513/#1534 added GNU-only `sed -Ei` (combined extended-regex + in-place short-flag cluster) and `sed --in-place` (long-form) tokens to `scripts/shell-portability-tokens.txt`, but both only accepted a trailing whitespace-or-end-of-line boundary — the same shape #1530's original `sort -V` short-flag token had, and the same false negative #1537/#1546 fixed for `sort -V` / `grep -P` / `echo -e`: an operator-terminated form with no separating whitespace (`x=$(sed -Ei)`, `sed -Ei|cat`, `sed --in-place|cat`, `x=$(sed --in-place)`) evaded detection entirely. **Verified via repro** before any fix: all four forms passed the gate clean on `main`. - Widens both tokens' trailing boundary to a control operator, redirection, or subshell close — `([[:space:]|&;()<>]|$)` (plus the pre-existing `=` alternative for `--in-place=SUFFIX`). - **Deliberately narrower than the sibling `sort -V`/`grep -P`/`echo -e` boundary** (which also includes quote/backtick chars): a quote immediately following `-Ei` with no separating whitespace is exactly the attached-EMPTY-suffix shape (`sed -Ei''`) #1513/#1534 explicitly deferred as ambiguous sed-dialect territory (same as the `-i''` case). Confirmed empirically — copying the sibling tokens' full quote-inclusive boundary verbatim broke the existing `"sed -Ei'' must not be flagged"` regression test, since a bare quote character then satisfied the widened boundary at the `i`+`'` junction. Excluding quotes/backtick from this pair's boundary keeps that deferred case correctly unflagged while still catching every operator-terminated form the issue named (none of which need a quote as the terminator). - **Verified before landing, per the issue's instruction.** Ran `check-shell-portability.sh --all` against the corpus before and after the token edit: the hit sets are byte-for-byte identical (75 pre-existing findings, none `sed`-related), so the widened boundary surfaces no new corpus violations and needs no `portability-ok:` annotations. ## Test plan - [x] `bash scripts/check-shell-portability.test.sh` — 75/75 passing (6 new regression cases: 3 operator-terminated forms each for `sed -Ei` and `sed --in-place` at the isolated-token level, plus one shipped-list assertion proving all 4 forms are detected under the real `shell-portability-tokens.txt`). - [x] Verified the new tests are meaningful: reproduced the bug first (`bash scripts/check-shell-portability.sh --paths t.sh` against a file containing the four operator-terminated forms passed clean on `main`'s shipped list); after the fix, all four are flagged. - [x] Verified the boundary is not over-widened: the full sibling (quote-inclusive) boundary was tried first and demonstrably broke the pre-existing `sed -Ei''` deferred-ambiguous-suffix test (`PASS=74 FAIL=1`); narrowed to exclude quotes/backtick, re-ran — `PASS=75 FAIL=0`. - [x] `scripts/check-shell-portability.sh --all` — before/after hit sets identical (75 findings, unrelated to these tokens); see summary above. - [x] `scripts/check-shell-portability.sh origin/main` run directly against this branch's own diff — clean (no unexcused GNU-only constructs in the 2 changed files). - [x] `shellcheck --rcfile=.shellcheckrc scripts/check-shell-portability.test.sh` — clean. - [x] `typos --config _typos.toml` on both changed files — clean. ## Related - #1537 / #1546 (established the operator-terminated boundary shape this PR mirrors, and named `sed -Ei` / `sed --in-place` as the deferred-out sibling defect this PR fixes) - #1513 / #1534 (introduced the two tokens this PR widens, and the attached-empty-suffix deferral this PR's narrower boundary deliberately preserves) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Jul 26, 2026
…nated forms (#1546) *This was generated by AI during work-loop execution.* Closes #1537 ## Summary - #1530 (which added `--sort=WORD` for `sort -V`'s long form) fixed the whitespace-only-boundary false negative on its own new token, but deliberately left the pre-existing `sort -V` short-flag cluster token untouched — widening it changes the gate's firing envelope over the existing corpus, not just #1530's new token, so it was scoped out. #1537 asked for exactly that widening, plus the same treatment for the sibling `grep -P` / `echo -e` short-flag tokens, which share the identical defect: the boundary accepted only trailing whitespace or end of line, so a flag terminated by a shell control operator with no intervening whitespace evaded detection — `x=$(sort -V)`, `sort -V|head -n1`, `sort -V; echo done` (and the same shapes for `grep -P` / `echo -e`). - Widens all three tokens to the operator-terminated boundary #1530 already established for `--sort=WORD`: `([[:space:]|&;()<>'"`+"`"+`]|$)` — every character that can actually end a shell word (whitespace, a control operator, a redirection, a subshell close, a quote), not only whitespace. - **Verified before landing, per the issue's instruction.** Ran `check-shell-portability.sh --all` against the corpus before and after the token edit: the hit sets are byte-for-byte identical (68 pre-existing findings — the regex-escape family `\b \< \> \s \S \w \W` plus one unrelated `sed -i` site — none of them `sort`/`grep`/`echo`), so the widened boundary surfaces no new corpus violations and needs no `portability-ok:` annotations. - **Found the same defect in two more tokens while auditing the three named siblings — scoped out, not absorbed.** `sed -Ei` and `sed --in-place` (from #1513/#1534) carry the identical whitespace-or-end-of-line boundary and the identical false negative, verified empirically: `x=$(sed -Ei)`, `sed -Ei|cat`, `sed --in-place|cat`, `x=$(sed --in-place)` all pass the shipped list today. #1537 named only `sort -V` / `grep -P` / `echo -e`, so — following the same narrow-scope discipline #1530 itself modeled — filed as #1545 rather than expanding this PR's blast radius. ## Test plan - [x] `bash scripts/check-shell-portability.test.sh` — 78/78 passing (12 new regression cases: 3 operator-terminated forms each for `sort -V`, `grep -P`, `echo -e` at the isolated-token level, plus one shipped-list assertion proving all 6 forms are detected under the real `shell-portability-tokens.txt`, not just the isolated-token mechanism). - [x] Verified the new tests are meaningful: stashed only `shell-portability-tokens.txt` (reverting to the old boundary) while keeping the widened test file — the shipped-list assertion fails as expected (`PASS=77 FAIL=1`); restored and confirmed 78/78 again. (The isolated-token tests hardcode the widened pattern directly via `one_token_list` and so are unaffected by the tokens-file revert — same shape as the existing `--sort=WORD` tests.) - [x] `scripts/check-shell-portability.sh --all` — before/after hit sets identical (68 findings, unrelated to these tokens); see summary above. - [x] `scripts/check-shell-portability.sh origin/main` run directly against this branch's own diff — clean (no unexcused GNU-only constructs in the 1 changed `.sh` file). - [x] `shellcheck --rcfile=.shellcheckrc scripts/check-shell-portability.test.sh` — clean. - [x] `typos --config _typos.toml` on both changed files — clean. ## Related - #1530 (established the operator-terminated boundary shape this PR mirrors onto the sibling short-flag tokens) - #1545 (follow-up: `sed -Ei` / `sed --in-place` carry the identical boundary defect, found while auditing this PR's three named siblings, explicitly scoped out) - Follows #1491, #1511, #1513, #1519, #1534, #1538, #1543, #1544 on the same gate. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Jul 29, 2026
…1544) *This was generated by AI during work-loop execution.* ## Summary Enables the two shell-portability-lint classes #1510 staged for this PR — `date -d` and `stat -c`. (The issue's third class, `mktemp -p`, went active separately in #1543 while this branch was open, so the token file's STAGED section is now empty.) - **Precision fixes to the staged regexes.** The original patterns matched `date`/`stat` as bare substrings, so `[[ -d "$candidate" ]]` (via "can-**DATE**") and `git -c alias.x=status -c ...` (via "**STAT**us") false-positived. Both now require whitespace immediately after the command name. - **Extended `is_guarded()`** with a same-line `stat -c` / `stat -f` guard requiring an actual `||` fallback relationship, matching the rigor #1519/#1534 established for the `readlink`/`realpath` guard. - **Ran `scripts/check-shell-portability.sh --all`** per the issue's step 4 and resolved every real hit from the two newly-active classes: - `portability-ok:` annotations on already-correct dual-dialect date/stat call sites in `claude-ops`, `context-guard`, `kindle-dedrm`, `work-items` (most span a line break or an if/else block, so the same-line auto-guard cannot recognize them even after extension); - a genuine fix for one previously-unguarded gap: `skill-quality`'s vendor-sync-age check had no BSD `date` fallback at all and silently no-op'd on macOS; - Windows-only-script annotations for `kindle-dedrm`'s two `stat -c` sites. - **Pre-existing violations of already-active classes** surfaced by touching `skill-quality/scripts/check-skill.sh` (GNU-only `\S`/`\b` escapes in its own `grep -qE` patterns) were fixed so the PR's own diff stays clean. - Every touched plugin's version is bumped with a matching CHANGELOG entry. ## Scanner correctness work (review rounds) Codex review found defects in the scanner itself across several rounds. Every one is addressed here — all but one fixed, and that one recorded as designed behavior. The first five: | Reported shape | Direction | Resolution | | --- | --- | --- | | `stat ${x:-$((1 \| 2))} -c %s` read clean | fail-open | Fixed — arithmetic expansion is its own mask state with per-frame paren-depth tracking, so `$((` is no longer consumed as `$(` plus a stray `(` | | `x=$(stat -c …) y=$(true) \|\| stat -f …` read as a guarded ladder | fail-open | Fixed — `status_swallowed()` now establishes that the matched frame is the *status-determining* frame of its command, rather than excluding one neighbour shape at a time | | `d"a"te -d …` / `st"a"t -c …` read clean | fail-open | Fixed — command names are spelled letter-by-letter with optional quote runs between them, since quote removal splices the word before the utility sees argv | | A quoted word spanning physical lines hid its option | fail-open | Fixed — records join on an unterminated quote as they already did on a dangling backslash, with every escape attributed to the physical line the hit sits on | | A utility named in a string (`echo "run date -d tomorrow"`) is reported | false positive | **Not fixed — documented.** Recorded in the script header as the gate's largest accepted over-flag | On the last row: matching text the shell would treat as a string literal is the whole mechanism behind the regex-escape classes, where `grep -E "\bword"` lives inside quotes and must still be caught. Requiring command position for the option-based classes alone needs a per-class axis in the token data plus word-level tokenization, and every partial answer trades this false positive for a fail-**open** — the same trade already made and withdrawn for `--` (see the block above `collapse_subs()`). `portability-ok:` is the one-line escape. This is the same decision already taken once in this file, now written down rather than left implicit. Two further defects were found and fixed while closing the quote-join finding, both pre-existing: - **Heredoc bodies leaked quote state.** A stray backquote in a PowerShell settings body (``"CustomRule`Path"``) opened a frame that, once joining was active, swallowed the 57 lines after it. Heredoc bodies are now excluded from joining — they are data, so they can neither continue a command nor leave a quote open — while still being scanned, since this corpus writes real scripts through heredocs. - **A `#` opening a joined physical line did not start a comment**, so a commented-out `|| stat -f` could excuse a hit above it. A newline now joins `WORDSTART`. The security-review lane then found a third, in the gate's own plumbing: a relative `SHELL_PORTABILITY_TOKENS` path shaped like `identifier=value` is parsed by awk as a variable assignment rather than opened, so no class loaded, every file reported clean, and awk still exited 0 — invisible to the scanner-fault check. It now gets the same `./` disambiguation the scanned file already had, and an empty pattern set fails closed however it arose. A further review round then found six more, five of them pre-existing and one a regression from the quote-join above. Rather than answer them one at a time — the pattern that had been producing a fresh variant every round — they were taken as three families and generalized: - **Quote spellings the token classes did not admit.** A backslash quotes exactly as a quote pair does, so the quote-run class is now `['"\]` in every place the command word, the short-option cluster and the long option are spelled — closing `da\te -d`, `date -\d`, `date "--date"`, `date --"date"=` and `stat --"format"=` together. `&>` / `&>>` join the separator class after the command name, since bash runs `date&>/dev/null -d tomorrow` with the GNU-only option. - **Boundaries that predate records containing a newline.** A structural newline ends a command inside a `$( )` frame, so it now bounds the guard's segment gap and the lookback both guards share. That lookback became a backward scan rather than a greedy `.*[;|&)]` match, because whether `.` matches a newline is an awk-implementation difference this gate must not rest on. **This closes the one regression the quote-join introduced**: `x=$(stat -c …` newline `true) || stat -f …` had read as a guarded ladder. - **Frames still not tracked.** A raw subshell inside a command substitution was not pushed, so its closing paren popped the substitution — the same unbalanced-frame failure the arithmetic branch fixed, one spelling over. A `)` with no frame open remains a `case` pattern terminator. Also in that round: a spaced redirection operand (`|| 2> /dev/null stat -f …`) is no longer rejected as a non-ladder, and the whole-file `portability-scope:` declaration moved out of a grep pre-pass into the awk program. A grep sees no shell structure, so it honored the token inside a heredoc **body**, where the line is generated data rather than a declaration the file makes about itself — one such line silently exempted a whole file. A final round found the same quote family reached through Bash ANSI-C (`$'…'`) and locale (`$"…"`) quoting: `d$'a'te -d`, `date -$'d'`, `stat -$'c'`, `st$'a't -c` and `date $"--date"=` all reach the GNU utility while reading clean. A quote-run element is now `(\$?['"]|\\)` — an optional `$` before a quote, or a backslash — defined once and shared by the command word, the short-option cluster, the long option, and the fallback guard. A **bare** `$` is deliberately excluded, since `$config` is a variable expansion rather than quote removal: `validate -d $config` stays clean and `d$a$t$e` is not a spelling of `date`, both pinned as negatives. Moving the scope decision into awk then turned out to have fixed only the heredoc half of its own problem: the check still read the raw record without asking what earlier lines had left open, so a physical line spelling `# portability-scope:` inside a multiline quoted value or substitution granted whole-file scope and suppressed every hit in the file. The marker now counts only on a line that also *opens* its own record — the one context where a leading `#` starts a comment rather than being data. A genuine declaration is unaffected, and the regression cases pin both directions, since the cheap fix here is one that quietly breaks the declaration it exists to protect. ## Token-file premise correction (rode along) The `mktemp -p` rationale comment asserted BSD/macOS mktemp "has no `-p`". It does — FreeBSD 14.2 and Apple both document `-p tmpdir, --tmpdir[=tmpdir]`. The real hazard is **precedence, and it diverges silently**: GNU treats `-p` as authoritative and overrides `TMPDIR`, while BSD/macOS consults it only as a fallback when `TMPDIR` is unset, so the same command writes to different directories per platform with no error either way. The gate's *behavior* was already correct; only its stated reason was wrong. Carried here because this PR owns the token file. The plugin CHANGELOG entries that quoted the old sentence are historical and left alone. ## Test plan - [x] `bash scripts/check-shell-portability.test.sh` — **215/215 passing**, including new regression cases for every shape above (arithmetic-expansion frames, sibling-substitution status ownership, quote-spliced command words on both rungs of a ladder, quoted words spanning lines, per-physical-line attribution and annotation scoping, heredoc-body isolation, and the joined-line comment opener). - [x] `scripts/check-shell-portability.sh origin/main` (this PR's own diff, 15 shell files in scope) — clean. - [x] `scripts/check-shell-portability.sh --all` — **19 hits, the same hits `origin/main`'s own scanner reports over the same tree**, all from unrelated already-active regex-escape classes and none from the two newly-active ones. Every scanner change above was held to that comparison, so no fix introduced a false positive anywhere in the corpus. One hit is attributed to a different line than main reports it: this PR introduces logical-line joining, so a backslash-continued record is now reported at its first physical line, as the script header specifies. That joining is also what makes a `date` whose `-d` sits on the next continued line reportable at all — main reads that shape clean. - [x] Full test suites for every touched script pass: `morning-brief.test.sh`, `claude-observability.test.sh`, `context-zone.test.sh`, `statusline-tee.test.sh`, `lease.test.sh`, `check-skill.test.sh`. - [x] `shellcheck --rcfile=.shellcheckrc` on every changed `.sh` file — clean. - [x] `scripts/validate-plugins.sh` — all manifests + catalog validate. - [x] `scripts/check-changelog-parity.sh --check-bump origin/main` — every version-bumped plugin has a matching CHANGELOG entry. ## Related - Closes #1510. - #1491 — original shell-portability-lint gate. - #1543 — activated `mktemp -p`, the issue's third class, independently of this PR. - #1528 — the deferred `mktemp -p` migration; closed. - #1562 — `--` end-of-options handling, which shares the word-level tokenization the command-position over-flag documented above would also need. - Rebased onto #1519 / #1534 / #1530, which merged mid-session and changed the same `check-shell-portability.sh` / `shell-portability-tokens.txt` files. Merged with `origin/main` again after #1603 / #1751 / #1752 landed; `context-zone.test.sh` takes main's side whole, since main replaced the unsuffixed `sed -i` this branch had annotated with a genuinely portable form. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Aug 6, 2026
kyle-sexton
added a commit
that referenced
this pull request
Aug 9, 2026
…hell-portability token gaps (#2064) 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 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This was generated by AI during work-loop execution.
Summary
scripts/check-shell-portability.sh/scripts/shell-portability-tokens.txt(the gate added byci: no gate covers shell portability — a GNU-only \b nearly shipped a fail-open security predicate #1491 / feat(ci): add shell-portability-lint gate for GNU-only constructs #1511). The issue's own text frames items 1-4 as needing scanner or scope changes with
real design content, each carrying an explicit "Re-opens when: ..." condition — a closed record
of a deliberate deferral, not a live TODO; a future review round that re-raises one of them would
file a new issue, the same way ci: shell-portability-lint detection precision, round 2 (escape-class scoping, line continuations, realpath control-flow guard, sed -i scope, sort long forms) #1517 itself followed feat(ci): add shell-portability-lint gate for GNU-only constructs #1511 and ci: harden shell-portability-lint detection precision (sed -i spellings, portability-scope precision, awk operand edge case) #1513 rather than reopening either.
Item 5 is the one item the issue names as ready now: "the natural first item to pick up ... a
literal token addition with no combined-cluster complexity."
sort's-V(natural/version sort) class already matched the shortflag, including inside a combined cluster (
-Vr), but not its two documented long-formspellings —
-V, --version-sortand--sort=WORDwhereWORDincludesversion(verifiedagainst man7.org's
sort(1)page before encoding, not assumed). Adds both as unambiguous literalERE tokens, the same shape
--perl-regexpalready uses alongsidegrep -P's combined-clusterpattern (no command-context prefix needed — neither string collides with anything else a shell
script would plausibly contain).
.shfile in this repo uses either long form today, sothis isn't retroactively red-lining anything already merged.
fix(ci): correct two shell-portability-lint false results) is still open and unmerged. It already carries fixes for ci: shell-portability-lint detection precision, round 2 (escape-class scoping, line continuations, realpath control-flow guard, sed -i scope, sort long forms) #1517's items 1, 3, 4,and half of item 5 (
--version-sortalone, not--sort=version). This PR was cut from currentmain, which does not yet have fix(ci): correct two shell-portability-lint false results #1519's changes, so it adds bothsortlong forms independentlyrather than assuming fix(ci): correct two shell-portability-lint false results #1519 lands first. If both PRs merge,
shell-portability-tokens.txtendsup with a duplicated
--version-sortline — harmless to the gate's pass/fail outcome, but it wouldmake the scanner emit two
PORTABILITY:lines and double-countviolationsfor what is really onehit. Whoever merges second should drop the duplicate line as part of the routine merge-conflict
resolution (they'll already be looking at that hunk).
Test plan
bash scripts/check-shell-portability.test.sh— 38/38 passing (3 new regression tests:sort --version-sortandsort --sort=versionlong-form detection via an isolatedsingle-token fixture, plus one case proving both forms are active in the SHIPPED token list
— not just the isolated-token matching mechanism — with a single fixture file containing both
spellings and a distinct
PORTABILITY:line asserted for each), run against this branch's ownworking tree.
scripts/check-shell-portability.sh origin/mainrun directly against this branch's owndiff — clean (no unexcused GNU-only constructs in the 2 changed files).
shellcheck --rcfile=.shellcheckrcon both changed scripts — clean.typos --config _typos.tomlon the changed files — clean.grep-swept every tracked*.shfile for--version-sort/--sort=version— no existingsite outside this PR's own new test fixtures, so nothing else needed a
portability-ok:annotation.
--sort=versionand--version-sortagainst GNU coreutilssort(1)(man7.org)before encoding as tokens, rather than assuming from memory.
Related
Closes #1517. Items 1-4 stay documented-but-deferred in the closed issue per its own reopen
conditions above — not carried forward as an open tracker.
Follows #1491, #1511. Sibling items on the same gate, left untouched by this PR's scope: #1513
(distinct sed-spelling / portability-scope / awk-operand findings, still open), #1519 (still open,
overlaps items 1/3/4 and half of item 5 — see the dedupe note above), #1510 (staged-class enable
trigger).