ci: honor -- end-of-options in shell-portability-lint via a word layer

[kyle-sexton]

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

• ci(shell-portability): the scanner cannot see command position or quoting state #1551 — the word-layer decision this PR implements stage 1 of (stages 2-4 remain open there;
  decision recorded at
  ci(shell-portability): the scanner cannot see command position or quoting state #1551 (comment))
• ci: enable staged shell-portability-lint classes (date -d, stat -c) #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.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C

[claude]

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

---

Review: #1834 — honor -- end-of-options via a word layer

• Gather context (diff, CLAUDE.md/REVIEW.md guidelines)
• Review scripts/check-shell-portability.sh changes
• Review scripts/check-shell-portability.test.sh changes
• Post review feedback

Scope note: this repo has a claude-security-review workflow, so per REVIEW.md this lane omits security findings — none applied here regardless (no external input reaches these functions; both sides parse trusted shell text already scoped by the base gate).

---

🔴 Important — fallback_proven() misreads a bare redirect after -f as a present format argument (fail-open on the trusted guard side)

scripts/check-shell-portability.sh L854–L862:

j = oe + 1
while (substr(m, j, 1) ~ /[ \t]/) j++
c = substr(m, j, 1)
if (c == "" || substr(q, j, 1) == "#" || index(";&|)" NL BT, c) > 0) return 0
return 1

This is the "does -f have a following format argument" check. It skips leading spaces/tabs after the -f word, then treats any remaining non-terminator character as proof an argument exists. It does not skip over a redirection the way the earlier pre-command-word loop does (L826–L831), and the terminator set ;&|) NL BT doesn't include <, >, or a digit (IO_NUMBER).

So for a line like:

stat -c "%s" "$g" || stat -f 2>/dev/null

c lands on 2 (or > for stat -f >/dev/null) right after -f, which is not in the reject set — the function returns 1 ("proven") even though BSD stat -f requires a format argument (per the synopsis this same function documents at L798–L800) and this invocation has none, only a redirect. That's exactly the fail-open class this PR is built to close: the fallback ladder is accepted as guarded, suppressing the real GNU-only stat -c report, for a BSD call that doesn't actually run correctly.

The added tests don't cover this shape — the "unprovable fallback" table (L1780–L1791 in the diff) tests bare stat -f and stat -f # comment, but not stat -f followed only by a redirect.

Suggested fix: after oe, skip a redirection (operator + optional space + target word) the same way the pre-loop at L826–L831 does before re-checking for a terminator/EOL, so a bare redirect with nothing else following still rejects.

Fix this →

---

Other observations (no action needed)

• The word-layer primitives (is_wordbreak, word_start/word_end, unquote_word, dashdash_between) are careful and internally consistent: structural decisions consistently read the mask (m), word content consistently reads the matched view (qline/cline), offsets stay aligned because both are length-preserving — matches the established convention from the ci: enable staged shell-portability-lint classes (date -d, stat -c) #1544 character-mask layer.
• dashdash_demoted's scoping (anchored to [ce+1, os-1] inside the matched extent) correctly keeps a -- in an outer/sibling command from leaking into a nested hit's evaluation, and correctly requires the trailing matched word to itself be option-shaped before considering demotion.
• Test coverage for the reporting side is thorough — ambiguous shapes (expansions, nested frames, redirection targets, undecoded ANSI-C, regex-escape operands) are each pinned as "must still report," matching the stated over-flag posture.
• Docstrings throughout are dense but match this file's pre-existing house style (heavy WHY-focused rationale tied to specific historical fail-opens); consistent with the rest of the file, not a new deviation.
• Scope check: this change touches CI script logic only (no plugin manifest, marketplace schema, hook contract, or documented harness behavior), so the fresh-docs mandate in CLAUDE.md doesn't apply here.

[claude]

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

---

Security review — no security issues found

• Gather context (PR diff, changed files)
• Read scripts/check-shell-portability.sh diff in full
• Read scripts/check-shell-portability.test.sh diff
• Analyze awk word-layer logic for injection / bypass risk
• Analyze dashdash_demoted / fallback_proven logic for logic flaws
• Check for command injection, unsafe eval, or untrusted input handling
• Post findings as review

Scope reviewed: the two changed files only (scripts/check-shell-portability.sh, scripts/check-shell-portability.test.sh), diffed against the pre-PR tip (HEAD~1, confirmed identical shortstat to the reported 376/-41).

What changed: new awk functions (is_wordbreak, word_start, word_end, unquote_word, dashdash_between, dashdash_demoted, fallback_proven) plus wiring in is_guarded and the match loop, all operating on strings already read into awk — no new shell-outs, no eval/system(), no new file or path handling.

Findings, by the categories in scope:

• Injection (command/path/template): none. The new code is pure in-process awk string scanning (character-index walks, substr, unquote_word) over text already captured in awk variables. No system(), backticks, eval, or shell re-invocation was introduced, and the existing awk '...' "$TOKENS" "$awk_file" invocation (unchanged by this diff) already quotes both operands.
• Untrusted-input handling / trust-boundary reasoning: this is exactly what the new code is engineered around, and it gets it right for the threat model that matters here — both new consumers (dashdash_demoted on the reporting side, fallback_proven on the guard side) read a scanned source file, i.e. untrusted/adversarial-shaped content from the gate's own point of view. Consistent with the PR's stated posture, every branch that's ambiguous (expansions, nested substitution frames, redirection targets, undecoded escape spellings) falls back to the safer direction for its side: the reporting side keeps reporting (over-flag) rather than silently suppressing, and the guard side rejects (over-flag) rather than trusting an unproven fallback. I didn't find a path where an adversarial shell snippet flips either side into the fail-open direction the PR is explicitly closing off.
• Denial of service / resource exhaustion: the new functions are linear single-pass character walks with bounded nested loops (parenthesis-depth and backtick-skip loops are both capped at the to extent), not backtracking regex — no ReDoS-shaped construct was added. The one regex touched in is_guarded (STATNAME/QRUN/SEG/CMDPOS composition) is unchanged from main; the diff only swaps ~ for match() to recover RLENGTH, same pattern, same cost.
• Secrets/tokens/credentials, GitHub Actions permissions, supply chain: not applicable — no workflow, permissions, or dependency files are touched by this PR; only the scanner script and its test file changed.

No CRITICAL, IMPORTANT, or SUGGESTION-level security findings to report for this diff.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e9b6500495

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

[github-actions]

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

[claude]

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

---

Review: #1834 — honor -- end-of-options via a word layer (re-review after synchronize)

• Gather context (diff vs origin/main, CLAUDE.md/REVIEW.md guidelines)
• Check whether prior findings (Claude fail-open finding, Codex P1/P2) were addressed by commit 3083701
• Review scripts/check-shell-portability.sh changes
• Review scripts/check-shell-portability.test.sh changes
• Post review feedback

Scope note: claude-security-review runs as a separate lane on this repo, so per REVIEW.md this lane omits security findings.

Sandbox note: git fetch, bash scripts/check-shell-portability.test.sh, and shellcheck were all blocked ("this command requires approval") in this run's environment, so the suite and shellcheck could not be executed directly — this review is based on static reading of the diff and hand-tracing the awk logic against the added test cases. If you want the suite actually executed as part of this lane, --allowedTools needs to permit Bash(bash scripts/check-shell-portability.test.sh) / Bash(shellcheck *).

---

✅ Both external review findings from the previous round are fixed

• Codex P1 (grep -P pattern -- -P losing the real option before --): dashdash_demoted now returns the demoted word's start offset via DD_OS (L784-795), and the caller only blanks [DD_OS, st+len-1] instead of the whole match (L1111-1114), so the resumed match() can still find the earlier real option. Traced by hand: since DD_OS always sits strictly after the command word (ce), at least one word is always blanked, so this can't loop forever. Covered by the new grep -P pattern -- -P / sort -V -- -V / stat -c "%s" -- -c / mktemp -p /tmp -- -p table.
• Codex P2 (stat -t "%Y" -f "%z" misread as an operand): fallback_proven's wantarg tracking (L824-866) now consumes the word after a detached -t/-f cluster as that option's argument rather than treating it as an operand or as the ladder's own -f. Correctly matches BSD getopt semantics (the very next argv word is unconditionally consumed as the argument, whatever it looks like), including the redirection-between--t-and-its-argument case (wantarg survives the redirect-skip block unchanged). Covered by both the "provable fallback" table (stat -t "%Y" -f "%z", -Ht "%Y" -f "%z") and the dedicated stat -t -f "%z" (argument-slot swallows the final -f) test.

---

🔴 Important — still outstanding: fallback_proven misreads a bare redirect after -f as a present format argument

This is the finding from the prior review round; 3083701 fixed the two Codex findings but did not touch this one, and no new test exercises it.

scripts/check-shell-portability.sh L870-878:

j = oe + 1
while (substr(m, j, 1) ~ /[ \t]/) j++
c = substr(m, j, 1)
if (c == "" || substr(q, j, 1) == "#" || index(";&|)" NL BT, c) > 0) return 0
return 1

Tracing stat -c "%s" "$g" || stat -f 2>/dev/null by hand: os..oe is just -f; uw = "-f"; the attached-argument check at L869 is empty since nothing follows f in the same word; j skips the one space after -f and lands on 2 (the fd digit of 2>/dev/null). 2 is not in the terminator set ;&|) NL BT (which still has no <, >, or digit), so the function falls through to return 1 — "proven" — even though BSD stat -f requires a format argument (the synopsis this function documents at L802-804) and this call has none, only a trailing redirect. That suppresses the genuine GNU-only stat -c report for a BSD fallback that doesn't actually run correctly — exactly the fail-open class this PR exists to close, and it fails on the trusted guard side.

The redirect-skip block earlier in the same function (L831-837) already knows how to step over an operator + target word; the missing-argument check after oe needs the same skip before it re-checks for a terminator/EOL. Worth adding a regression case (stat -f 2>/dev/null, stat -f >/dev/null) alongside the existing "unprovable fallback" table (L1799-1814).

Fix this →

---

Other observations (no action needed)

• matchend → mend rename in 3083701 is clean; no leftover references anywhere in the script.
• The DD_OS-based partial blanking is scoped correctly: only the demoted word onward is discarded, and hand-tracing confirms the blanked range always includes the demoted word itself (progress is guaranteed each iteration of the has_unguarded loop, so no infinite-loop risk from the new resume behavior).
• wantarg state correctly survives the redirection-skip block inside fallback_proven (redirects reset the accumulated word w but not wantarg), which is exactly what keeps stat -t 2>/dev/null "%Y" -f "%z"-shaped ladders working.
• Scope check: CI script + test file only — no plugin manifest, marketplace schema, hook contract, or documented harness behavior touched, so the fresh-docs mandate in CLAUDE.md doesn't apply.

[claude]

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

---

Security review — 1 finding carried forward, unresolved

• Gather context (git show on both PR commits — e9b6500 and 3083701; shallow clone blocked gh/git fetch network access in this run, see note below)
• Read scripts/check-shell-portability.sh diff in full (both commits)
• Read scripts/check-shell-portability.test.sh diff
• Analyze the word-layer / dashdash_demoted / fallback_proven logic for trust-boundary flaws
• Post findings

Scope: this repo runs a separate claude-security-review workflow; per REVIEW.md this lane's earlier pass on e9b6500 already covered supply-chain/Actions concerns (none apply — no workflow files touched) and found no new issues introduced by the word-layer primitives themselves. This pass re-examines the guard side (fallback_proven) after the follow-up commit 3083701, since that's exactly the trust-boundary surface this PR is built around (§ "Anything the guard reads in order to SUPPRESS a report is a trusted input").

---

🔴 Important — fallback_proven() still accepts a bare -f followed only by a redirect as a "proven" format argument (fail-open, unfixed)

scripts/check-shell-portability.sh L866–L878 (current HEAD):

if (wantarg) return 0
uw = unquote_word(substr(q, os, oe - os + 1))
if (uw !~ /^-[FHhLlnqrsx]*f/) return 0
if (substr(uw, index(uw, "f") + 1) != "") return 1
j = oe + 1
while (substr(m, j, 1) ~ /[ \t]/) j++
c = substr(m, j, 1)
if (c == "" || substr(q, j, 1) == "#" || index(";&|)" NL BT, c) > 0) return 0
return 1

This was flagged in the earlier review round on e9b6500 (comment at 2026-07-30T21:02Z) as a fail-open: after the -f word, j skips only spaces/tabs, then treats any remaining character as proof a format argument follows. The terminator set ;&|) + newline + backtick doesn't include <, >, or a digit (IO_NUMBER), so a bare redirect right after -f is misread as a format argument.

The follow-up commit 3083701 fixed the two other review findings on this function (the -t/-f detached-argument consumption via the new wantarg tracking, verified correct by tracing both the stat -t "%Y" -f "%z" accept path and the stat -t -f "%z" reject path against the new test cases at L1820–L1844) — but it does not touch this block. Confirmed by git show 3083701: the diff's only change in this region is the added if (wantarg) return 0 guard immediately above line 866; lines 867–878 are untouched from e9b6500.

Concretely, this input is still accepted as a "proven" guarded fallback even though BSD stat -f requires a format argument it never receives:

stat -c "%s" "$g" || stat -f 2>/dev/null

fallback_proven returns 1: oe ends at -f, j lands on 2 (or > for stat -f >/dev/null), which isn't in the reject set, so the function falls through to return 1. The GNU-only stat -c report is suppressed for a BSD call that cannot actually run — exactly the class of fail-open this PR states it's closing (#1562), on the guard side the PR's own doc comment (L902–L906) says must be "at least as strict as the reporting side."

Confidence: high — traced statically against the current file (line numbers above are from the HEAD check-shell-portability.sh as of 3083701); I was not able to execute the scanner in this sandboxed run to get a live repro (script execution required approval unavailable in this session), but the logic path is unambiguous and I confirmed via scripts/check-shell-portability.test.sh that no test case exercises a bare -f followed only by a redirect (every existing stat -f ... 2>/dev/null-style test, e.g. L855, always has a format-string argument like '%d' present before the redirect).

Suggested fix (unchanged from the original finding): after oe, skip a redirection (operator + optional space + target word) the same way the pre-loop at L830–L837 does, looping until no more redirections remain, before re-checking for a terminator/EOL.

Fix this →

---

Other observations (no action needed)

• The DD_OS global set by dashdash_demoted() and consumed in has_unguarded() (L1111–L1114) to blank only the demoted word onward — rather than the whole matched extent — is set immediately before every read and only read on the branch where it was just assigned; no stale-value risk across iterations or files.
• The wantarg detached-argument tracking in fallback_proven() correctly mirrors real BSD getopt semantics: a word right after a bare -t/-f cluster is unconditionally consumed as that option's argument (even if it looks like -- or another flag), matching how getopt actually parses argv — this doesn't introduce a new fail-open.
• No workflow, permissions, secrets, or dependency files are touched by either commit in this PR — only scripts/check-shell-portability.sh and its test file.

Note on tooling in this run: the checkout here is shallow and network-gated git fetch/gh calls required approval that wasn't available in this session, so I worked from local git show/git log on the two PR commits already present in the checkout rather than a live gh pr diff; I also could not execute the scanner directly to get a live repro of the finding above (script execution required approval), so it's backed by static trace rather than a run. If live execution is needed to double-check, a run with broader tool allowances would let me confirm directly.