fix(ci): correct two shell-portability-lint false results - #1519
Merged
Conversation
#1511 added scripts/check-shell-portability.sh and merged before a second round of automated review on the branch's later pushes could land two confirmed fixes -- both verified empirically before landing here, not assumed: - The gate's sed -i auto-guard for a space-separated empty-suffix argument ("-i" followed by '' or "" as a SEPARATE argument) was WRONG: verified against a real GNU sed 4.9, `sed -i '' 's/foo/bar/' file` exits 2, because GNU consumes the space-separated empty string as sed's SCRIPT argument, shifting the real script and target file to be read as filenames. That idiom is BSD-only, not portable on this repo's own GNU/Linux CI, so the guard is removed -- it correctly stays flagged. The genuinely dual-compatible spelling (an ATTACHED nonempty suffix, e.g. `sed -i.bak ... && rm -f the backup`) was already, correctly, never matched. - The regex-escape family (\b \< \> \s \S \w \W) matched unconditionally, flagging portable, non-regex uses like `printf '\b'` (a real backspace byte, valid on GNU and BSD alike, confirmed via `printf '\b' | xxd`) purely for containing the same two characters. Each token now requires a co-located grep or sed invocation on the same line. Also lands two precision fixes from the same review round that were correct and already tested, but likewise never made it into the merge: - The readlink -f / realpath guard now requires an actual `||` fallback relationship, not mere line co-location (`realpath "$1"; readlink -f "$1"` -- semicolon-separated, no real fallback -- still flags). - `sort --version-sort` (GNU's documented long-form alias for -V) is now a separate literal token alongside the existing short-flag pattern. 38/38 self-tests passing (4 new regression tests for these fixes), verified against the current main baseline: `scripts/check-shell-portability.sh origin/main` run directly against this branch's own diff, shellcheck, typos, actionlint, and the sibling check-skill-portability.test.sh all clean. Refs #1491
|
Claude encountered an error after 1s —— 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. |
|
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: 5b8fb0a3ee
ℹ️ 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".
This was referenced Jul 26, 2026
… still flag Requiring a co-located grep/sed invocation on the escape's own line traded the gate's documented over-flag direction for precision and bought a real false negative: a pattern is very often assigned on one line and consumed several lines later, which is exactly the near-miss shape the class exists to catch. Verified against this repo's own corpus, not assumed -- plugins/claude-config/skills/audit-instructions/scripts/instruction-scan.sh assigns \b-bearing EREs to I6_ERE and RATIONALE_ERE on lines 66 and 68 and passes them to grep -niE on lines 83-85. With the context requirement the gate reports that file clean; bare, it flags both assignment lines. The printf '\b' false positive the requirement was meant to remove is the mirror-image cost of staying fail-closed, and portability-ok: <reason> is the one-line per-site escape the gate already ships for it. Narrowing the class precisely means knowing which operand of which command is a regex -- real shell parsing, not a token edit -- and stays tracked in #1517. Tests: the printf assertion is inverted (fires, and is excused by an annotation) and a build-then-consume regression test is added for the reverted shape. 40/40.
|
Claude encountered an error after 1s —— 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. |
kyle-sexton
added a commit
that referenced
this pull request
Jul 26, 2026
Closes #1513 ## Summary Fixes the three deferred detection-precision findings tracked in #1513 (a follow-up to #1491 / #1511's `check-shell-portability.sh` gate): - **`sed -i` additional GNU spellings.** Adds a `--in-place` (long-form) token, and a combined `-Ei` short-flag-cluster token (mirroring the existing `grep -P` / `sort -V` / `echo -e` combined-cluster treatment). Both are anchored to a `sed` COMMAND token — start of line or a non-identifier character, whitespace after — so neither fires on an identifier that merely contains those three letters (`used="$(grep -Ei ...)"`) and `--in-place` does not flag a script defining or forwarding an unrelated in-place option of its own. `i` must be the cluster's LAST letter: GNU's syntax is `-i[SUFFIX]`, so `-iE` is an attached backup suffix, not a second flag (verified against GNU sed 4.9 — `sed -iE 's/a/b/' f.txt` writes the backup `f.txtE`), which makes it the same dual-compatible shape as `-i.bak` and correctly never flagged. The attached no-space EMPTY-suffix ambiguity (`-i''`) stays explicitly deferred, per the issue, since it needs real sed-dialect research rather than a pattern tweak — `-Ei''` is likewise left unflagged for the same reason. - **`portability-scope:` whole-file exemption precision.** Both `check-shell-portability.sh` and `check-skill-portability.sh` (shared fix, as the issue calls for) anchored the exemption check to a genuine comment-line declaration (`^[[:space:]]*#[[:space:]]*portability-scope:`, plus the HTML-comment-opener spelling for the skill gate) instead of an unanchored substring search. This was not just a theoretical gap: both scripts' own header prose *documenting* the mechanism (`` a whole-file `portability-scope: <reason>` declaration `` ...) already contained the literal string and was silently self-exempting the script from its own gate — verified before and after the fix that both scripts stay genuinely clean on their own merits, not by accident of the bug. - **`awk` operand disambiguation.** `check-shell-portability.sh`'s `scan_file()` passed the scanned file positionally to `awk`; a changed file whose relative path is shaped like an `identifier=value` assignment (e.g. a top-level `FOO=bar.sh`) is silently consumed by awk as a variable assignment instead of opened as a file, dropping it from the scan with no error. Verified empirically against a real awk (`awk '{print}' "FOO=bar.sh"` exits 0 with no output; prefixing with `./` makes it read the file correctly). Fixed by prefixing an unrooted file operand with `./` before it reaches awk. ## Test plan - [x] `bash scripts/check-shell-portability.test.sh` — 56/56 passing (16 new: `-Ei` combined-cluster detection incl. command-token anchoring and no-double-fire checks, the `-iE` / `-Ei.bak` attached-suffix negatives, `--in-place` bare and `=SUFFIX` forms plus the unrelated-option-arm negative, the `portability-scope:` mention-vs-declaration precision cases, and the `identifier=value`-shaped-filename awk regression). - [x] `bash scripts/check-skill-portability.test.sh` — 19/19 passing (2 new: the shared `portability-scope:` precision fix, for both the mention-vs-declaration case and the `#`-comment-style declaration alongside the existing HTML-comment style). - [x] `scripts/check-shell-portability.sh origin/main` run directly against this branch's own diff — clean (4 files, no unexcused constructs). - [x] `scripts/check-skill-portability.sh origin/main` — clean (no skill files in scope). - [x] `shellcheck --rcfile=.shellcheckrc` on all four changed shell scripts — clean. - [x] `typos --config _typos.toml` on all five changed files — clean. ## Related - #1491, #1511 — added the `check-shell-portability.sh` gate this PR hardens. - #1517 — a sibling, concurrently-dispatched follow-up on the same gate; verified its five findings (escape-class command-context, backslash-continuation, realpath fallback control-flow, the `sed -i ''` auto-guard debate, `sort --version-sort`) are disjoint from this PR's three items — no overlap. - #1519 — removed the `sed -i ''`/`sed -i ""` auto-guard entirely and reworked the `readlink -f` guard. It has since landed on `main` and is merged into this branch; the one textual conflict (the `sed -i` token's comment block) was resolved by composing both sides — #1519's verified rationale for why the space-separated empty suffix IS flagged, plus this PR's `-i''` deferral note. - #1532 — filed during this PR's implementation: `check-skill-portability.sh` has the textually identical `awk` operand pattern this PR fixes in `check-shell-portability.sh`, but fixing it there was out of this issue's stated scope (item 3 named only the shell gate). Follow-up tracks closing that gap. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Jul 26, 2026
) *This was generated by AI during work-loop execution.* ## Summary - #1517 is a bundle of five round-2 detection-precision findings on `scripts/check-shell-portability.sh` / `scripts/shell-portability-tokens.txt` (the gate added by #1491 / #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 #1517 itself followed #1511 and #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." - This PR does item 5: GNU `sort`'s `-V` (natural/version sort) class already matched the short flag, including inside a combined cluster (`-Vr`), but not its two documented long-form spellings — `-V, --version-sort` and `--sort=WORD` where `WORD` includes `version` (verified against man7.org's `sort(1)` page before encoding, not assumed). 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 — neither string collides with anything else a shell script would plausibly contain). - Corpus-checked before landing: no existing `.sh` file in this repo uses either long form today, so this isn't retroactively red-lining anything already merged. - **#1519 overlap, and what to do about it.** #1519 (`fix(ci): correct two shell-portability-lint false results`) is still open and unmerged. It already carries fixes for #1517's items 1, 3, 4, and half of item 5 (`--version-sort` alone, not `--sort=version`). This PR was cut from current `main`, which does not yet have #1519's changes, so it adds both `sort` long forms independently rather than assuming #1519 lands first. **If both PRs merge**, `shell-portability-tokens.txt` ends up with a duplicated `--version-sort` line — harmless to the gate's pass/fail outcome, but it would make the scanner emit two `PORTABILITY:` lines and double-count `violations` for what is really one hit. 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 - [x] `bash scripts/check-shell-portability.test.sh` — 38/38 passing (3 new regression tests: `sort --version-sort` and `sort --sort=version` long-form detection via an isolated single-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 own working tree. - [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` on both changed scripts — clean. - [x] `typos --config _typos.toml` on the changed files — clean. - [x] `grep`-swept every tracked `*.sh` file for `--version-sort` / `--sort=version` — no existing site outside this PR's own new test fixtures, so nothing else needed a `portability-ok:` annotation. - [x] Verified `--sort=version` and `--version-sort` against GNU coreutils `sort(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). --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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>
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 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
ci: add shell-portability-lint gate for GNU-only constructs, refs ci: no gate covers shell portability — a GNU-only \b nearly shipped a fail-open security predicate #1491) merged(
8949b577) before a second round of automated Codex review on that branch's later pushes couldland two confirmed correctness fixes — the review comments landed on the PR only ~1 minute before
an independent merge-lane session merged it as gate-proven-green, so this follow-up carries the
fixes forward. Both were verified empirically against real tools before landing here, not assumed.
sed -i ''/sed -i ""auto-guard was wrong and is removed. It was added believing aspace-separated empty-suffix argument was "the portable BSD-safe idiom" for
sed -i. Verifiedagainst a real GNU sed 4.9:
sed -i '' 's/foo/bar/' fileexits 2, because GNU consumes thespace-separated empty string as sed's SCRIPT argument (not
-i's suffix), shifting the realscript and target file to be read as filenames. That idiom is BSD-only — it breaks on this repo's
own GNU/Linux CI — so it correctly stays flagged now. The genuinely dual-compatible spelling (an
ATTACHED nonempty suffix,
sed -i.bak '...' file && rm -f file.bak) was already, correctly, nevermatched by the token (no separating whitespace).
\b \< \> \s \S \w \W) stays BARE — a co-located-grep/sedrequirement was tried here and reverted in review. The motivation was real (the bare token flags
portable, non-regex uses like
printf '\b', a genuine backspace byte on GNU and BSD alike), butthe requirement bought a worse defect than it removed: a pattern is very often assigned on one line
and consumed several lines later, so requiring the command on the escape's own line silently
un-catches exactly the near-miss shape the class exists for. Verified against this repo's corpus,
not assumed —
plugins/claude-config/.../audit-instructions/scripts/instruction-scan.shassigns\b-bearing EREs toI6_ERE/RATIONALE_ERE(lines 66, 68) and passes them togrep -niE(lines83-85); with the requirement the gate reports that file clean, bare it flags both assignment lines.
The
printf '\b'false positive is the mirror-image cost of the token file's documented over-flagdirection, and the per-site
portability-ok: <reason>annotation is the one-line escape alreadyshipped for it. Narrowing the class precisely needs real shell parsing, not a token edit — tracked
in ci: shell-portability-lint detection precision, round 2 (escape-class scoping, line continuations, realpath control-flow guard, sed -i scope, sort long forms) #1517.
but likewise never made it into the merge:
readlink -f/realpathguard now requires an actual||fallback relationship, not mereline co-location (
realpath "$1"; readlink -f "$1"— semicolon-separated, no real fallback —still flags).
sort --version-sort(GNU's documented long-form alias for-V) is now a separate literaltoken alongside the existing short-flag pattern.
Test plan
bash scripts/check-shell-portability.test.sh— 40/40 passing (6 new regression tests: a\bpattern built in a variable and consumed later still fires,printf '\b'fires and isexcused by a
portability-ok:annotation, thesed -i ''/-i ""now-correctly-flagged cases,the
||-required readlink guard,sort --version-sort), run against the currentmainbaseline (this branch was cut fresh from
mainafter feat(ci): add shell-portability-lint gate for GNU-only constructs #1511 merged, not carried over from theclosed PR's stale branch).
scripts/check-shell-portability.sh --paths .../instruction-scan.shexits 1 (flags lines 66and 68) — the false negative the reverted requirement introduced is gone.
scripts/check-shell-portability.sh origin/mainrun directly against this branch's own diff —clean.
shellcheck --rcfile=.shellcheckrcon both changed scripts — clean.typos --config _typos.toml— clean.actionlint .github/workflows/ci.yml— clean (workflow itself untouched by this PR).bash scripts/check-skill-portability.test.sh(sibling gate) — still passing, no cross-gateregression.
sed -i ''exit code on GNU sed 4.9, and
printf '\b'byte output viaxxd.Related
No related issue: #1491 (the original item) and #1511 (the PR this fixes) are both already closed —
there is no open issue for this PR to close. Refs #1491 and #1511 for context only. This corrects a
defect in #1511 found by automated review after that PR had already merged.
#1517 tracks the same review round and stays OPEN: this PR lands its items 3 (
readlink/realpath||fallback), 4 (sed -i ''scope) and 5 (sort --version-sort), while item 1 (escape-classscoping) is re-confirmed here as needing real shell parsing rather than a token edit, and item 2
(backslash line-continuation normalization) is untouched.