ci: enable staged shell-portability-lint classes (date -d, stat -c) - #1544
Merged
Conversation
Closes #1510 Enables two of the three STAGED shell-portability-lint classes from #1491: `date -d` and `stat -c`. `mktemp -p` stays staged — its corpus (~24 files, ~56 call sites, all test scaffolding across ~13 plugins) is a real migration effort out of scope here, tracked as a follow-up: #1528. - Extended is_guarded() with a same-line stat -c / stat -f guard, mirroring the existing readlink/realpath shape. - Fixed a precision bug found while activating both new patterns: the original STAGED regexes matched "date"/"stat" as bare substrings, so `[[ -d "$candidate" ]]` (via "can-DATE") and `git -c alias.x=status -c ...` (via "STATus") false-positived. Both patterns now require whitespace immediately after the command name. - Ran `--all` against the full corpus 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 (spanning claude-ops, context-guard, kindle-dedrm, work-items), a genuine fix for one previously-unguarded gap (skill-quality's vendor-sync-age check had no BSD date fallback at all), and Windows-only-script annotations for kindle-dedrm's two stat -c sites (no BSD fallback needed — those scripts already require Git Bash + PowerShell + LOCALAPPDATA). - Along the way, touching skill-quality/scripts/check-skill.sh and context-guard's context-zone.test.sh surfaced pre-existing violations of already-active gate classes (GNU-only \S/\b regex escapes, an unsuffixed sed -i whose real portability comes from a perl fallback) that CI's changed-file-scoped gate would have newly caught once these files were touched — fixed/annotated those too so the PR's own diff stays clean against the gate. Every touched plugin's version is bumped with a matching CHANGELOG entry: claude-ops, context-guard, kindle-dedrm, skill-quality, work-items. *This was generated by AI during work-loop execution.* Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Closes #1510 Enables two of the three STAGED shell-portability-lint classes from #1491: `date -d` and `stat -c`. `mktemp -p` stays staged — its corpus (~24 files, ~56 call sites, all test scaffolding across ~13 plugins) is a real migration effort out of scope here, tracked as a follow-up: #1528. - Extended is_guarded() with a same-line stat -c / stat -f guard, mirroring the existing readlink/realpath shape. - Fixed a precision bug found while activating both new patterns: the original STAGED regexes matched "date"/"stat" as bare substrings, so `[[ -d "$candidate" ]]` (via "can-DATE") and `git -c alias.x=status -c ...` (via "STATus") false-positived. Both patterns now require whitespace immediately after the command name. - Ran `--all` against the full corpus 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 (spanning claude-ops, context-guard, kindle-dedrm, work-items), a genuine fix for one previously-unguarded gap (skill-quality's vendor-sync-age check had no BSD date fallback at all), and Windows-only-script annotations for kindle-dedrm's two stat -c sites (no BSD fallback needed — those scripts already require Git Bash + PowerShell + LOCALAPPDATA). - Along the way, touching skill-quality/scripts/check-skill.sh and context-guard's context-zone.test.sh surfaced pre-existing violations of already-active gate classes (GNU-only \S/\b regex escapes, an unsuffixed sed -i whose real portability comes from a perl fallback) that CI's changed-file-scoped gate would have newly caught once these files were touched — fixed/annotated those too so the PR's own diff stays clean against the gate. Every touched plugin's version is bumped with a matching CHANGELOG entry: claude-ops, context-guard, kindle-dedrm, skill-quality, work-items. *This was generated by AI during work-loop execution.* Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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>
…w a non-force push
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![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: 19ab388b04
ℹ️ 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".
6 tasks
Both tokens spanned the gap with `[^\n]*`, but a physical line is not a command. The gap walked past `;`/`&`/`|` and attributed a LATER command's flag to `date`/`stat`: date +%s; test -d "$dir" -> reported, though date never received -d stat "$file"; tool -c x -> reported, though stat never received -c Verified end to end through the real gate: a file containing exactly those two lines is FLAGGED under the previous tokens and PASSES under these. Excluding `;&|()` from the gap keeps every true positive matched — `date -d @0`, `x=$(date -d @0)`, `date --utc -d @0`, `stat -c %s "$f"` — and the co-located BSD-fallback auto-guard still fires, since `stat -c ... || stat -f ...` matches on the first segment while the guard inspects the whole line. Suite: PASS=79 FAIL=0.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc54287a77
ℹ️ 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".
The same-line auto-guard accepted any `stat -f` / `readlink` text to the right of a `||`, so a failure branch that merely PRINTS the portable form suppressed the hit: `stat -c '%s' "$f" || echo 'stat -f is unavailable'` passed the gate with no portable command anywhere on the line. The counterpart must now sit at command position after the `||` — whitespace, an optional `name=` assignment, and an optional `$(` — which is exactly the shape the two real corpus ladders use (remove-path.sh dev_of() and the 15 hook-utils.sh copies), and is not the shape a quoted diagnostic has. The `date -d` and `stat -c` tokens also had no LEFT boundary, so any word ending in the command name took its flag: `validate -d config`, `update -d /tmp` and `mystat -c foo` were all reported. `/` stays outside the excluded class so `/usr/bin/date -d @0` still matches. Repo-wide `--all` sweep is unchanged at 61 hits, and byte-identical to the sweep under origin/main's token list with the corpus held constant.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ac419aed9
ℹ️ 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".
Three gaps in the newly enabled classes. The auto-guard reached across a command separator to find its fallback, so `stat -c '%s' "$f"; true || stat -f '%z' "$f"` was excused even though the GNU-only call runs unconditionally and the `||` belongs to a later command. The run between the GNU call and the `||` now stops at `;` and `|`; `)` and `&` stay admissible because a real ladder closes a `$( )` and may redirect with `2>&1`. `date -d` matched only the space-separated spelling, but `-d` takes a mandatory argument and GNU accepts it attached, with `--date` as the documented long form — `date --help` prints `-d, --date=STRING`, and `date -u -d@0` and `date -u --date=@0` print the same instant. Likewise `stat --help` prints `-c --format=FORMAT` and `--printf=FORMAT`, so the class was enforceable under one of three spellings. The wider `date` tail matches the skip message in morning-brief.test.sh, which names both dialects in a diagnostic string without invoking either; annotated at the site, exactly as the sibling probe on line 21 already is. Repo-wide `--all` sweep stays at 61 hits, byte-identical to the sweep under origin/main's token list with the corpus held constant.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4955c7ab28
ℹ️ 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".
Three more gaps in the newly enabled classes. A backgrounding `&` is a command separator the guard still crossed, so `stat -c '%s' "$f" & true || stat -f '%z' "$f"` was excused even though the `||` binds to `true` and the BSD form never runs. The segment now rejects a lone `&` while keeping `>&`, `<&` and `&&`, because a real ladder may redirect with `2>&1` and `a && b || c` does fall back to c when a fails. `-d` accepted only a quote, `$`, `@` or a digit as an attached value, but GNU documents a mandatory long-option argument as mandatory for the short option too, and `date -u -dtomorrow +%Y` succeeds. The value is now unrestricted; what keeps a longer flag from being read as `-d` is the run before it, which may not end in `-`. That is what stops `date --debug` — a real GNU flag — from matching on the `-d` inside it. `stat` searched for a literal `-c` and so missed `stat -Lc%s "$file"`, which succeeds on GNU stat (`stat --help` documents `-L, --dereference`). The cluster branch is now `-[A-Za-z]*c`, with the same not-ending-in-`-` run so `stat --dereference` is not read as a cluster. Repo-wide `--all` sweep stays at 61 hits, byte-identical to the sweep under origin/main's token list with the corpus held constant.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39540c9e4d
ℹ️ 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".
…redirects Three more findings, two of them false positives. The run before the flag now has to end in WHITESPACE rather than merely not in `-`, so the hyphen BEGINS an argument. That drops two false positives — `stat foo-c` and `stat "$file-c"` pass no option at all and were reported — and it subsumes the previous long-flag guard, since `date --debug` and `stat --dereference` still cannot reach their inner letter. `d` may sit at the argument-taking end of a cluster: GNU lists `-u, --utc` beside `-d, --date=STRING` and `date -ud tomorrow +%Y` succeeds, so the branch is `-[A-Za-z]*d`, mirroring what `stat` already does for `-Lc`. The auto-guard required each option to follow its command immediately, so `size=$(stat -c "%s" "$f") || size=$(stat 2>/dev/null -f "%z" "$f")` was rejected. A redirection does not change the argv the command receives, so that is the same GNU-first/BSD-fallback ladder and no longer needs a hand-written exemption; the option must still begin its own argument. Repo-wide `--all` sweep stays at 61 hits, byte-identical to the sweep under origin/main's token list with the corpus held constant.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23d3cd500a
ℹ️ 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".
Version restacks only: main's released claude-ops 0.23.0 and work-items 0.30.0 win whole, and this branch's annotation-only entries move to 0.23.1 and 0.30.1 on top of them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TkLLDcB8Ht2tm3j688F5ZP
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b926fa252
ℹ️ 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 comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
Merging with admin override per operator directive (2026-07-29 full queue drain). Rationale on record: the sole failing required check, security-review / security-review, fails with its own declared infrastructure class ( |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Bash ANSI-C (`$'…'`) and locale (`$"…"`) quoting splice a word exactly as an ordinary quote pair or a backslash does, so `d$'a'te -d tomorrow`, `date -$'d' tomorrow`, `stat -$'c' %s "$f"` and `date $"--date"=tomorrow` all reach the GNU utility with the GNU-only option while reading clean. A quote-run element is now `(\$?['"]|\)` rather than a bracket class of quote characters, applied in the same one place the command word, the short-option cluster and the long option are all spelled — so the two utilities and all three positions gained it together. A BARE `$` is deliberately excluded. `$config` is a variable expansion, not quote removal, so admitting one would flag ordinary parameterized code: `validate -d $config` stays clean and `d$a$t$e` is not a spelling of `date`. Both are pinned as negatives alongside the five positive spellings. The fallback guard shares the element, so a spliced GNU call still finds a spliced BSD fallback instead of every such ladder newly reporting. 211/211, and `--all` still reports the same 19 hits origin/main's own scanner reports over the same tree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TkLLDcB8Ht2tm3j688F5ZP
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 801a9debb8
ℹ️ 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".
Moving the whole-file `portability-scope:` decision into awk fixed the heredoc case but left a wider one: the check ran against the raw record without asking what the lines before it had left open. Inside a multiline quoted value, a multiline substitution, or any other open construct, a physical line spelling `# portability-scope:` is DATA, yet it granted whole-file scope and suppressed every buffered hit — so a value like x='foo # portability-scope: bogus bar' exempted an entire file it never declared anything about. The marker now counts only on a line that also OPENS its own record. `pending` is the state carried in from earlier lines, so it is false exactly when no quote, expansion, substitution, or continuation is open — the one context where a leading `#` starts a comment. A heredoc body was already excluded by the branch above, for the same reason. A genuine declaration is unaffected: it still grants scope from either side of a hit, indented or not, and still has to START with the token so a doc-block sentence or a string-literal mention exempts nothing. 215/215, and `--all` still reports the same 19 hits origin/main's own scanner reports over the same tree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TkLLDcB8Ht2tm3j688F5ZP
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Claude encountered an error after 9s —— View job I'll analyze this and get back to you. |
|
Claude finished @kyle-sexton's task in 7m 21s —— View job Security Review
No exploitable security vulnerabilities introduced. This PR is net-positive from a security standpoint — it fixes three real bypass scenarios in a CI gate whose failure mode is "report clean when violations exist." Findings below. Fixed: three pre-existing fail-opens in the scanner gate1. Before this PR, Fixed by moving the recognition into the awk program (line 1013), where 2. Empty token-set silent bypass via If Fixed by two complementary changes: the 3. Commented-out BSD fallback satisfied Before quote-join and comment masking, a line like Fixed by the inline-comment masking in Known accepted fail-open (documented in code, pre-existing)Trusted-side The guard's BSD-fallback side accepts This was introduced in an earlier revision, not this PR, and is explicitly documented at lines 656-660 and tracked in #1562. The PR body also names it in the table of unresolved findings. No change needed here unless #1562 is prioritized. Plugin script changes — no new vulnerabilitiesThe |
kyle-sexton
added a commit
that referenced
this pull request
Jul 31, 2026
#1834) Fixes #1562. Refs #1551 (stage 1 of the decision recorded at #1551 (comment) — made under owner-delegated session authority; #1551 stays open for stages 2–4). ## Summary First slice of the #1551 word layer, built additively on the #1544 character mask, consumed by `--` (end-of-options) handling in both trust directions: - **Word primitives** inside the scanner's awk pass: word delimitation read from the quote mask (a quoted space never splits a word, a masked separator never splits a command) and per-word POSIX quote removal (2.6.7; ANSI-C/locale `$`-quote forms recognized as openers, content not decoded). No re-lexing — tokens still match exactly as before. - **Reporting side (`dashdash_demoted`)**: a matched option is discarded when a word that *statically unquotes to exactly `--`* sits as a whole argv word between the hit's command word and its matched option, inside the matched extent. Matching resumes past a discarded occurrence, so a later real invocation on the same record is still evaluated. Everything ambiguous keeps reporting: expansions (`$marker`), nested frames on either view (`stat "$(printf '%s' x --)" -c`), redirection targets (`stat > -- -c`), undecoded ANSI-C escapes, and any extent that crosses a command separator. - **Guard side (`fallback_proven`)**: the stat fallback ladder is now rejected unless its `-f` is proven to be an option BSD getopt actually parses (verified against FreeBSD/macOS stat(1)): no `--` word before it — closing the fail-open that was live on `main` (`stat -c '%s' "$f" || stat -- -f` read as guarded) — no operand word between the fallback's `stat` and its option (BSD getopt stops at the first operand), no argument-taking cluster letter ahead of `f` (`-tf` hands `f` to `-t` as its timefmt value), and a format argument present (attached or following). Every rejection is an over-flag with the one-line `portability-ok:` escape; every acceptance of those shapes was a fail-open. Both directions read a trusted input, so both are built to err toward over-flag — the posture the token list documents. ## Why now, and why this shape #1562's five recorded failure shapes (from the three withdrawn `--` attempts in #1544) all trace to three missing capabilities: per-invocation scoping, resumed matching, quoted-word recognition. Resumed matching already landed in #1544; the other two are exactly the word layer. The decision comment on #1551 records the full rationale, the staged plan (command-position axis and the reporting-side short-option table deferred with triggers), and the rejected alternatives (big-bang re-lexing rewrite; narrow guard patch; won't-fix). An independent different-vendor review (Codex CLI, advisory) of the design contributed the redirection-target, wrong-frame-marker, and `-tf`/operand/missing-argument adversarial shapes; all are pinned as tests. ## Tests - The suite test that pinned the withdrawal as stated behaviour now pins the honored behaviour; the resume and option-before-marker pins are unchanged and still pass. - New: 8 marker spellings demote (incl. quoted/backslash/ANSI-C and inside a quoted substitution); 9 ambiguous shapes never suppress; regex-escape operands are never demoted; 7 unprovable fallbacks rejected on the trusted side; provable spellings (`-Lf`, attached `-f%z`) plus every pre-existing genuine-ladder pin stay guarded. - Full suite green; `shellcheck` clean on the scanner. - Corpus: `--all` over the tracked tree produces an identical hit set under the `origin/main` scanner and this branch's scanner (same tree, both scanners). ## Related - #1551 — the word-layer decision this PR implements stage 1 of (stages 2-4 remain open there; decision recorded at #1551 (comment)) - #1544 — the character mask this layer builds on, and where the feature was previously withdrawn ## Residuals (recorded in scanner comments) A marker held in a variable (suppression side) and an ANSI-C escape-spelled marker (both sides) are not recognized — variable indirection is out of the gate's scope throughout (#1513); both residuals err toward over-flag. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Aug 9, 2026
No linked issue
## Summary
Adds one class to the shell-portability gate: an **unquoted `&` in the
replacement
half of `${var/pat/repl}` / `${var//pat/repl}`**.
Since bash 5.2 that `&` expands to the text the pattern just matched —
the `sed`
rule — under the `patsub_replacement` shell option, which is **on by
default**.
Before 5.2 the same character was an ordinary literal. The construct is
accepted
on both sides and silently means something different on each, with no
error
either way.
This is not a new axis for the gate. Its stated exposure is macOS — the
one
platform no runner here covers — and macOS ships bash 3.2 while every
runner in
this repo ships 5.2 or later. Same shape as the existing `mktemp -p`
class, whose
token comment already describes a silent precedence divergence rather
than an
absent flag, and it is a **hard error** for the same reason that one is
(there is
no warning channel; a hit is exit 1, escapable per site with
`portability-ok: <reason>`).
## Repro
Verified on bash 5.3.15:
```bash
soh=$'\x01'; n="cat 1>${soh}2"; n="${n//"$soh"/&}"; printf '%q\n' "$n"
# $'cat 1>\0012' -- the replacement was a NO-OP
```
```bash
v=aXb
shopt -u patsub_replacement; printf '%q\n' "${v//X/&}" # a\&b (pre-5.2 behaviour)
shopt -s patsub_replacement; printf '%q\n' "${v//X/&}" # aXb (5.2+ default)
```
## Historical-detection proof
The class shipped a real defect in this repo, fixed by #2008. Run the
new rule
against the pre-fix file:
```console
$ git show 32add0f:plugins/guardrails/hooks/block-hook-bypass.sh > /tmp/prefix.sh
$ scripts/check-shell-portability.sh --paths /tmp/prefix.sh
PORTABILITY: /tmp/prefix.sh:425: !subst-replacement-ampersand -> normalized="${normalized//"$soh"/&}"
```
`32add0fa` is the pre-fix parent of #2008's fix on `main`, so this
reproduces for
anyone. One hit, at the exact line #2008 fixed, and nothing else in that
file. That
no-op restore produced a live guardrails false positive (`echo x >&2`
blocked as
a file write) on bash >=5.2 only.
## Repo sweep
`--all` audit over every in-scope shell file, with **only** this class
active:
```console
$ SHELL_PORTABILITY_TOKENS=<amp-only list> scripts/check-shell-portability.sh --all
No unexcused GNU-only constructs in 418 shell file(s).
```
**ZERO occurrences of the class on current `main`** (434 tracked `*.sh`,
418 in
scope after the gate's existing `vendor/` and cross-plugin-sync
exclusions — no
new exclusion was added). Nothing to fix; no live bug found.
The full `--all` run with the shipped token list reports 14 hits, all
from the
pre-existing regex-escape classes (`\b`/`\s`/`\w`/`\S`) in four files
this change
does not touch. Confirmed pre-existing by scanning those same four files
with
`origin/main`'s unmodified gate and token list in a throwaway tree —
byte-identical
output. `--all` is an audit mode; CI gates changed files only.
The stale pre-fix copy under
`.claude/worktrees/agent-ac8ee00680b8101e3/` that a
sweep would legitimately hit **does not exist in this worktree** (it is
untracked
in another session's worktree), so nothing was excluded for it, and
nothing needed
to be: CI runs changed-file mode, where an untracked nested checkout can
never
appear in a `git diff`.
## Why it is script-implemented, not a token ERE
The gate keeps *what* is detected in
`scripts/shell-portability-tokens.txt`. This
class cannot live there as a pattern: matching runs on the
`qline`/`cline` views,
and `neutralize()` replaces every `SEPS` character — `&` among them —
inside a
masked run, while a `${…}` body is masked in its entirety. That is
exactly right
for every other class (a `;` in an expansion body is data, not an
operator) and
leaves this one nothing to match on.
Activation stays data anyway: a token line beginning with `!` names a
class the
script implements in code, it runs only while that line is active, an
**unrecognized `!name` fails the run closed**, and a class-scoped unit
fixture
enables exactly this class the way `one_token_list` does for an ERE. The
extent of
each `${…}` comes from `mask_quotes()` — the one existing authority on
quote and
frame structure — rather than from a second tracker written beside it.
## Grammar, measured not recalled
Every expectation below was probed against bash 5.3.15 before it was
encoded.
- The pattern ends at the **FIRST** unquoted, unescaped `/`, not the
last:
`v=aXbXc; "${v//X/Y/Z}"` yields `aY/ZbY/Zc`, so the pattern is `X` and
the
replacement is `Y/Z`. (The task brief said *last*; that is measurably
wrong, and
a last-slash reading would miss the `&` in `${v//X/b&/c}` — there is a
test for
exactly that line.)
- A quoted or backslash-escaped `/` in the pattern is **not** the
separator
(`s=a/b; "${s//"/"/-}"` and `"${s//\//-}"` both yield `a-b`), while a
`[...]`
bracket expression does **not** protect one (`"${p//[/]/-}"` leaves
`a/b`
untouched).
- `\&`, `"&"` and `'&'` are each a literal ampersand — the manual's
"Quoting any
part of string inhibits replacement in the expansion of the quoted
portion" —
and **none of them is flagged**. The failure message steers to `\&`, and
it is
worth separating measurement from inference there: `BASH_COMPAT` is
**not** a
pre-5.2 oracle for this rule — a bare `&` still expanded at every level
down to
32 on 5.3.15, so the option is not compat-gated. What the ladder does
establish
is that the backslash before an `&` is removed even under the pre-4.3
quote-removal regime (tested at 32/42/44/50/51 and the default); the
manual
supplies the rest ("the backslash is removed in order to permit a
literal
'&'"). The quoted spellings are the ones with a version quirk of their
own
(compat42: "The replacement string in double-quoted pattern substitution
does
not undergo quote removal, as it does in versions after bash-4.2"),
which
leaves the quote characters in the output on the older regime — a reason
to
prefer `\&`, not a reason to flag them.
- `${var//pat}` (deletion) and an empty replacement have nothing to
flag; an
expansion whose operator is not `/` (`${v:-a/b/&}`, `${v#*/}`,
`${v%/*}`,
`${v:0:1}`, `${#v}`) is not a substitution at all.
- `&` outside any substitution — `a && b`, `cmd &`, `2>&1`, `echo "a &
b"` — is
never flagged.
Sources: GNU Bash Reference Manual, [Shell Parameter
Expansion](https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html)
("Any unquoted instances of '&' in string are replaced with the matching
portion
of pattern"; "Backslash escapes '&' in string; the backslash is removed
in order
to permit a literal '&' in the replacement string") and [Shell
Compatibility
Mode](https://www.gnu.org/software/bash/manual/html_node/Shell-Compatibility-Mode.html);
bash [NEWS](https://tiswww.case.edu/php/chet/bash/NEWS) records
`patsub_replacement` as new in bash-5.2.
## Known limits (stated, not overclaimed)
1. An `&` that **arrives by expansion** is undetectable statically. The
rule is
applied after the replacement expands, so an `&` held in a variable, or
in the
output of a `$(...)` inside the replacement, is a live match reference.
Verified:
for `v=aXb`, a replacement of `$(printf 'p&q')` yields `apXqb`. Same
indirection
class the gate already declares out of scope (#1513).
2. A literal `&` inside a nested `$(...)`/backquote in the replacement
is skipped,
because there it is ordinary command syntax (`&&`, backgrounding). Its
runtime
*output* is limit 1.
3. A substitution assembled from fragments before use — the same limit
the ERE
classes carry.
4. A parameter spelling the walk does not recognise (a name that is not
an
identifier, a digit run, or one of `@ * ? $ ! -`) is skipped rather than
guessed.
All four are in the **under-flag** direction; none produces a false pass
on a
literal `${var//pat/&}`.
## Review round (both findings real, both reproduced before accepting)
Review caught that the limits list was incomplete **in both
directions**. Both
are now **fixed**, not documented away, so the sentence above is true
again.
**Over-flag — process substitution.** `<(`/`>(` was not treated as a
nested
frame, so `${v//X/<(cmd1 && cmd2)}` was reported. Measured on 5.3.15:
```
patsub_replacement on : a/dev/fd/63b
patsub_replacement off: a/dev/fd/63b
```
Identical — zero version divergence, so a hard error was red-lining
portable
code. `<(`/`>(` now open a frame (`skip_frame` already handled the
shape; the
opener test is factored into `opens_frame()` and shared by both halves).
**Under-flag — `$#` as the special parameter.** `#` after `${` was taken
to be
the length operator unconditionally. It is the length operator only
while what
follows could start a parameter name; `/` cannot, so `${#//2/&}` is a
substitution on the positional-argument count. Measured with two
positional
parameters:
```
$ bash -c 'set -- 1 2; printf "%s\n" "${#//2/&}"' → 2
$ bash -c 'set -- 1 2; shopt -u patsub_replacement; printf "%s\n" "${#//2/&}"' → &
```
A genuine false pass on the literal shape. Fixed; `${#}`, `${##}`,
`${#v}` and
`${#arr[@]}` keep a non-`/` successor and stay length expansions.
Ten cases added, including both halves of the frame skip proving a real
hit
*after* a skipped process substitution is still found.
## Tests
New section in `scripts/check-shell-portability.test.sh` covering: the
#2008 defect
shape (asserted with the exact `PORTABILITY: file:line:` prefix, so a
silently
inert rule cannot pass), the first-slash grammar,
escaped/quoted/nested-expansion
forms mixed with a bare one, quoted and escaped slashes in the pattern,
array and
positional and anchored parameter spellings, the correct `\&` form, both
quoted
forms, the deletion and empty-replacement forms, non-substitution
operators, `&`
outside any substitution, all three excuse mechanisms (same-line
`portability-ok:`, the comment block above, whole-file
`portability-scope:`),
per-physical-line attribution inside a quote-joined record, class
inertness when
the `!name` line is absent, a directive-only token list not tripping the
fail-closed empty-pattern check, an unrecognised `!name` failing closed,
and the
class remediation paragraph appearing on an `&` failure while staying
off an
unrelated one.
`shell-portability-lint` on CI: **PASS=312 FAIL=0** (268 before this
change), and
the changed-file gate reports the two touched shell files clean. The job
is
`ubuntu-24.04` only, so this class — like every other class in this gate
— has no
Windows CI coverage.
## Related
- #2008 — the merged fix whose defect this rule makes non-recurrable;
the sweep
proof runs against its pre-fix parent `2b60bf0b`.
- #1491 — the issue this gate was built for.
- #1513 — the variable-indirection limit this class inherits.
- #1544 — the quote-aware masking layer whose `${…}` frame tracking this
class reuses.
---------
Co-authored-by: Claude Opus 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
Enables the two shell-portability-lint classes #1510 staged for this PR —
date -dandstat -c. (The issue's third class,mktemp -p, went active separately in #1543 while thisbranch was open, so the token file's STAGED section is now empty.)
date/statas baresubstrings, so
[[ -d "$candidate" ]](via "can-DATE") andgit -c alias.x=status -c ...(via "STATus") false-positived. Both now require whitespace immediately after the command
name.
is_guarded()with a same-linestat -c/stat -fguard requiring an actual||fallback relationship, matching the rigor fix(ci): correct two shell-portability-lint false results #1519/fix(ci): harden shell-portability-lint detection precision #1534 established for thereadlink/realpathguard.scripts/check-shell-portability.sh --allper the issue's step 4 and resolved everyreal hit from the two newly-active classes:
portability-ok:annotations on already-correct dual-dialect date/stat call sites inclaude-ops,context-guard,kindle-dedrm,work-items(most span a line break or anif/else block, so the same-line auto-guard cannot recognize them even after extension);
skill-quality's vendor-sync-age check had noBSD
datefallback at all and silently no-op'd on macOS;kindle-dedrm's twostat -csites.skill-quality/scripts/check-skill.sh(GNU-only\S/\bescapes in its owngrep -qEpatterns) were fixed so the PR's own diff stays clean.
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:
stat ${x:-$((1 | 2))} -c %sread clean$((is no longer consumed as$(plus a stray(x=$(stat -c …) y=$(true) || stat -f …read as a guarded ladderstatus_swallowed()now establishes that the matched frame is the status-determining frame of its command, rather than excluding one neighbour shape at a timed"a"te -d …/st"a"t -c …read cleanecho "run date -d tomorrow") is reportedOn 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 becaught. 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 abovecollapse_subs()).portability-ok:is the one-line escape. This is the same decision alreadytaken 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:
(
"CustomRule`Path") opened a frame that, once joining was active, swallowed the 57 linesafter 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.
#opening a joined physical line did not start a comment, so a commented-out|| stat -fcould excuse a hit above it. A newline now joinsWORDSTART.The security-review lane then found a third, in the gate's own plumbing: a relative
SHELL_PORTABILITY_TOKENSpath shaped likeidentifier=valueis parsed by awk as a variableassignment 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 thescanned 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:
does, so the quote-run class is now
['"\]in every place the command word, the short-optioncluster and the long option are spelled — closing
da\te -d,date -\d,date "--date",date --"date"=andstat --"format"=together.&>/&>>join the separator class after thecommand name, since bash runs
date&>/dev/null -d tomorrowwith the GNU-only option.inside a
$( )frame, so it now bounds the guard's segment gap and the lookback both guardsshare. That lookback became a backward scan rather than a greedy
.*[;|&)]match, becausewhether
.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 …newlinetrue) || stat -f …had read as a guarded ladder.closing paren popped the substitution — the same unbalanced-frame failure the arithmetic branch
fixed, one spelling over. A
)with no frame open remains acasepattern terminator.Also in that round: a spaced redirection operand (
|| 2> /dev/null stat -f …) is no longer rejectedas a non-ladder, and the whole-file
portability-scope:declaration moved out of a grep pre-passinto 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 -canddate $"--date"=all reach theGNU 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$configis a variable expansion rather than quote removal:validate -d $configstays clean andd$a$t$eis not a spelling ofdate, 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 grantedwhole-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 beingdata. 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 -prationale comment asserted BSD/macOS mktemp "has no-p". It does — FreeBSD 14.2and Apple both document
-p tmpdir, --tmpdir[=tmpdir]. The real hazard is precedence, and itdiverges silently: GNU treats
-pas authoritative and overridesTMPDIR, while BSD/macOSconsults it only as a fallback when
TMPDIRis unset, so the same command writes to differentdirectories 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
bash scripts/check-shell-portability.test.sh— 215/215 passing, including newregression 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).
scripts/check-shell-portability.sh origin/main(this PR's own diff, 15 shell files inscope) — clean.
scripts/check-shell-portability.sh --all— 19 hits, the same hitsorigin/main's ownscanner 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
datewhose-dsits on thenext continued line reportable at all — main reads that shape clean.
morning-brief.test.sh,claude-observability.test.sh,context-zone.test.sh,statusline-tee.test.sh,lease.test.sh,check-skill.test.sh.shellcheck --rcfile=.shellcheckrcon every changed.shfile — clean.scripts/validate-plugins.sh— all manifests + catalog validate.scripts/check-changelog-parity.sh --check-bump origin/main— every version-bumped pluginhas a matching CHANGELOG entry.
Related
mktemp -p, the issue's third class, independently of this PR.mktemp -pmigration; closed.--end-of-options without reopening the fail-open paths #1562 —--end-of-options handling, which shares the word-level tokenization thecommand-position over-flag documented above would also need.
check-shell-portability.sh/shell-portability-tokens.txtfiles. Merged withorigin/mainagain after feat(context-guard): zone-crossing hooks, token bands, and the workflow continuation router #1603 / feat(source-control): gate PR bodies against pr-issue-linkage at authoring time #1751 / chore(knowledge): retire fable-5 guide doc-queue entry after slice completion #1752 landed;
context-zone.test.shtakes main's side whole, sincemain replaced the unsuffixed
sed -ithis branch had annotated with a genuinely portable form.