Skip to content

ci(scripts): flag a bare & in a ${var//pat/repl} replacement - #2097

Merged
kyle-sexton merged 3 commits into
mainfrom
ci/portability-lint-substitution-ampersand
Aug 9, 2026
Merged

ci(scripts): flag a bare & in a ${var//pat/repl} replacement#2097
kyle-sexton merged 3 commits into
mainfrom
ci/portability-lint-substitution-ampersand

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

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:

soh=$'\x01'; n="cat 1>${soh}2"; n="${n//"$soh"/&}"; printf '%q\n' "$n"
# $'cat 1>\0012'   -- the replacement was a NO-OP
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:

$ git show 32add0fa: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:

$ 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

("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
;
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 (ci: harden shell-portability-lint detection precision (sed -i spellings, portability-scope precision, awk operand edge case) #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

Since bash 5.2 an UNQUOTED `&` in the replacement half of a pattern
substitution expands to the text the pattern just matched — the `sed`
rule, under the `patsub_replacement` option that is on by default.
Before 5.2 the same character was an ordinary literal. macOS ships bash
3.2 and every runner here ships 5.2+, so this lands on the exact
uncovered-platform axis the shell-portability gate exists for, reached
through a bash version rather than through a utility dialect.

It has already shipped a defect in this repo: #2008 fixed a sentinel
restored to itself in plugins/guardrails/hooks/block-hook-bypass.sh,
where the restore was a silent no-op on bash >=5.2 and produced a live
false positive (`echo x >&2` blocked as a file write).

The class is implemented in the script rather than as a token ERE
because it cannot be one: matching runs on views in which every
separator inside a masked run — `&` among them — has been neutralized,
and a `${…}` body is masked whole, so a pattern has nothing to match.
Activation stays data all the same: a token line beginning with `!`
names a script-implemented class, an unrecognized one fails the run
closed, and a class-scoped fixture enables exactly this class the way
an ERE fixture enables a pattern.

Grammar measured against bash 5.3.15, not recalled:

  - the pattern ends at the FIRST unquoted, unescaped `/`, so a `/`
    after it is replacement text (`v=aXbXc; "${v//X/Y/Z}"` -> aY/ZbY/Zc);
  - a quoted or backslash-escaped `/` in the pattern is NOT the
    separator, while a `[...]` bracket expression does not protect one;
  - `\&`, `"&"` and `'&'` are each a literal ampersand and none is
    flagged; `\&` is what the failure message recommends, because the
    quoted spellings carry their own pre-4.3 quote-removal divergence;
  - `${var//pat}` (deletion) has no replacement to flag, and an
    expansion whose operator is not `/` is not a substitution at all.

Known limit, in the under-flag direction: the rule is applied after the
replacement expands, so an `&` arriving through a variable or through
`$(...)` output is a live match reference this static check cannot see
— the same indirection class the gate already declares out of scope.

Proof on real history: the rule flags the pre-fix file at 2b60bf0,
line 439, and nothing else in it. An `--all` sweep of all 418 in-scope
shell files finds ZERO occurrences of the class on current main.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@cursor

cursor Bot commented Aug 9, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

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


Review complete

  • Gather context (PR body, changed files list)
  • Fetch diff via gh pr diff
  • Read scripts/check-shell-portability.sh changes
  • Read scripts/check-shell-portability.test.sh changes
  • Read scripts/shell-portability-tokens.txt changes
  • Check CLAUDE.md / AGENTS.md alignment
  • Post inline findings
  • Post summary review

Summary

This adds a script-implemented detection class (!subst-replacement-ampersand) for an unquoted & in the replacement half of ${var/pat/repl}/${var//pat/repl}, gated behind a new !name-prefixed token-list activation mechanism, plus an extensive test section and documentation.

I manually traced the core parsing (mask_quotes's new ${…} extent tracking via VOPEN/VS/VE, and amp_in_frame/skip_frame/subst_amp_hit) against the grammar claims in the PR description — first-slash-as-separator, quoted/escaped slashes not acting as separators, [...]-bracket subscripts not protecting a separator, nested ${…}/$(...) frames being skipped correctly on both sides, single-quoted constructs never opening a frame at all, and the nested-substitution-with-its-own-& case (e.g. ${v//X/${r/a/&}}) still being caught via the inner frame's own independently-recorded extent. All of this checks out against the stated bash semantics. Note: I was not able to actually execute check-shell-portability.test.sh or check-shell-portability.sh in this sandbox (script execution required approval that wasn't obtainable in this session), so this is a static/manual-trace review, not a dynamic-execution verification — I did not independently run the test suite the PR claims passes.

Finding posted inline (scripts/check-shell-portability.sh L1663-1671): the new amp-class-specific footer text is appended to the failure output for every violation, not just ones from the ampersand class — violations is a generic counter and the new echo block only checks violations > 0. A run that only trips an unrelated mktemp -p or regex-escape (\b) hit will still print misleading guidance about !subst-replacement-ampersand. This is new behavior (the pre-PR footer on origin/main was fully generic), and no test pins the footer content, so it wasn't caught.

Everything else — the fail-closed handling of an unrecognized !name token, the per-physical-line attribution for quote-joined records, the three excuse mechanisms (portability-ok: inline/block, portability-scope:), and the test coverage for the grammar edge cases — looked correct and thorough on inspection. No CLAUDE.md/AGENTS.md concerns: this is an internal CI lint script, not a plugin manifest/marketplace schema/hook contract surface, so the fresh-docs mandate doesn't apply here.

@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

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


Security review

  • Gather context (PR diff, changed files)
  • Review scripts/check-shell-portability.sh for injection/logic/trust-boundary issues
  • Review scripts/check-shell-portability.test.sh
  • Review scripts/shell-portability-tokens.txt
  • Post final findings

No security issues found.

This PR is scoped entirely to scripts/check-shell-portability.sh (the new subst_amp_hit() / amp_in_frame() / skip_frame() functions and their !subst-replacement-ampersand activation path), its test suite, and a comment-only data addition to scripts/shell-portability-tokens.txt. I reviewed the full diff (git show HEAD against the parent commit, since this PR is a single commit) rather than the repo at large.

Notes from the review, none of which rose to a reportable finding:

  • No new execution surface. The new logic is pure AWK string scanning (substr/character-walk over the record text) operating on file contents already read by the existing scan_file() pipeline. Nothing added here calls eval, source, or otherwise executes scanned file content — it only classifies it and prints a diagnostic line.
  • No new shell-injection surface. The pre-existing awk_file="./$awk_file" disambiguation guard (to stop a bare relative path shaped like VAR=value from being parsed by awk as a variable assignment) is unchanged by this diff; this PR doesn't touch argument/path handling at all.
  • Fail-closed behavior is consistent with the rest of the gate. An unrecognized !name line in the token list exits 2 (FATAL=1; exit 2) rather than silently disabling the class — same posture as the existing empty-pattern-set check, and it's covered by a dedicated test (an unknown script-implemented class fails closed).
  • Bounded scanning, no ReDoS/DoS concern. skip_frame/amp_in_frame/subst_amp_hit all walk forward over a bounded index range (i/at monotonically increasing up to a fixed limit/e), so even a maliciously crafted shell file (e.g. in a fork PR) can't drive these into unbounded work — no backtracking regex, no recursion.
  • Test file uses mktemp/mktemp --suffix=.sh for fixture files and passes fixture bodies as literal single-quoted data (printf '%s\n' "$1" >"$f"), never interpolated into a command line — no injection surface there either.
  • No workflow, permission, or secrets changes. The changed files are all under scripts/; no .github/workflows/* files, permissions: blocks, or credential-handling code are touched by this PR, so the dangerous-trigger / permission-widening / supply-chain concerns this lane watches for don't apply here.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8546355c4a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/check-shell-portability.sh Outdated
Comment thread scripts/check-shell-portability.sh Outdated
Comment thread scripts/check-shell-portability.sh Outdated
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown

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

The new paragraph sat in the unconditional failure epilogue, so every
`grep -P` or unsuffixed `sed -i` failure was followed by advice about an
ampersand the developer never wrote. It is now printed only when a
reported hit belongs to !subst-replacement-ampersand, pinned in both
directions by a paired case — present on an & failure, absent from an
unrelated one — the same present/absent pairing the inertness case uses.

Also narrows the `\&` justification to what was actually measured.
BASH_COMPAT is NOT a pre-5.2 oracle for this rule: a bare `&` still
expanded at every level down to 32 on bash 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;
the manual supplies the rest. The earlier wording read as pre-5.2
coverage it never had.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

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


Security review

  • Gather context (PR diff, changed files)
  • Review scripts/check-shell-portability.sh for injection/logic/trust-boundary issues
  • Review scripts/check-shell-portability.test.sh
  • Review scripts/shell-portability-tokens.txt
  • Post final findings

No CRITICAL or IMPORTANT security issues found. One low-severity observation below.

This PR is scoped entirely to scripts/check-shell-portability.sh (the new !subst-replacement-ampersand class: skip_frame(), amp_in_frame(), subst_amp_hit(), and the !name-token activation path), its test suite, and a comment-only data addition to scripts/shell-portability-tokens.txt. No workflow files, permissions, or secret-handling code are touched, so the dangerous-trigger / permission-widening / supply-chain concerns this lane watches for don't apply, and zizmor's advisory lane covers that surface anyway.

  • No new execution surface. The added AWK functions are pure string/offset scanning (substr, character-by-character walks) over text already read by the existing scan_file() pipeline. Nothing here calls eval, source, or otherwise executes the scanned shell file's content — it only classifies text and prints a diagnostic line.
  • No injection surface. The bash-side addition (amp_violation tracking via a case "$v" in *"!subst-replacement-ampersand"*) match) is a literal substring match against text this script itself generated (PORTABILITY: file:line: ...), not attacker-influenced shell metacharacters reaching a command line. The pre-existing awk_file="./$awk_file" disambiguation guard is untouched.
  • Fail-closed activation. An unrecognized !name line in the token list exits 2 (FATAL=1; exit 2) rather than silently disabling the class — consistent with the gate's existing "no silent skip" posture, and covered by a dedicated test.
  • Bounded scanning, minor algorithmic-complexity note (SUGGESTION, low confidence): skip_frame()/amp_in_frame()/subst_amp_hit() (L1191, L1280, L1370) each walk forward over a bounded index range with no recursion (no stack-exhaustion risk), so this isn't a classic ReDoS. However, subst_amp_hit() calls amp_in_frame() once per recorded ${…} extent (NV of them), and each call re-scans its own span; a line containing deeply/adjacently nested ${…} frames (e.g. a long run of ${${${…}}}) makes the summed work across frames roughly quadratic in line length rather than linear. Since this scanner runs in CI over files that are part of a PR's diff, a crafted shell file with a pathologically long line of nested expansions could measurably slow this check. Impact is limited (wasted CI time, not code execution or data exposure), and this is a private repo where PR authorship is already gated by repository access, so I'd rate this a suggestion rather than a blocking finding — flagging it since it's the kind of thing static analysis wouldn't surface, not because it's readily exploitable here.

Everything else — the fail-closed handling of an unrecognized !name, quote/backslash handling inside skip_frame, and the amp_violation footer scoping fix in the second commit (faee078) — checked out as sound with no injection, authZ, or trust-boundary concerns.
· branch

@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

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


Review complete

  • Gather context (PR diff, changed files)
  • Read scripts/check-shell-portability.sh diff (both commits: 8546355 and faee078)
  • Read scripts/check-shell-portability.test.sh diff
  • Read scripts/shell-portability-tokens.txt diff
  • Check CLAUDE.md / AGENTS.md alignment
  • Cross-check prior review threads (Codex, earlier Claude runs) against HEAD faee078
  • Post inline findings
  • Post summary review

Summary

This is the second commit on this PR (faee078). I diffed it against the first commit (8546355, already reviewed) to see exactly what changed, rather than re-reviewing the whole thing from scratch.

The previously-flagged footer bug is fixed. My earlier review on this PR flagged that the !subst-replacement-ampersand-specific remediation paragraph was appended to every violation regardless of which class fired (violations > 0 was the only gate). faee078 adds a dedicated amp_violation counter, set only when a reported hit's text contains !subst-replacement-ampersand (check-shell-portability.sh L411-414), and gates the paragraph on it (L1681-1694). Two new tests pin this in both directions (an amp hit prints the paragraph; an unrelated grep -P hit does not) — I confirmed this by reading the diff directly, not from the PR's own description. Well handled.

Two Codex findings from the prior commit are unaddressed — I traced both directly against the current amp_in_frame()/skip_frame() code (unchanged by faee078) and posted inline comments with the specifics:

  • L1284${#//2/&} may parse in real bash as a substitution on the special parameter $#, not a length expansion (the code unconditionally treats a leading # as length). I could not run bash in this sandbox to confirm the exact parse, so I've flagged it as plausible-but-unverified rather than confirmed — worth checking against real bash before dismissing, since it contradicts the PR's own stated invariant that ${#var} "can never be a substitution."
  • L1328 (and its twin at ~L1354) — process substitution <(...)/>(...) isn't recognized as a nested frame, so an && inside one (e.g. ${v//X/<(a && b)}) gets scanned as ordinary replacement text and false-flagged. This one I'm confident in independent of bash-version subtleties — it's a direct read of which frame-opener shapes the AWK code checks for (backtick, $(, ${) versus which exist in bash (<(/>( too). Neither case has test coverage in check-shell-portability.test.sh.

Both are in the over-flag direction (false positive), the opposite of the "Known limits" section's stated invariant that the class never produces a false pass — so if confirmed, either merits a fix before this ships as a hard-error gate, since there'd be no escape short of a per-site portability-ok: annotation for otherwise-portable code.

No other issues. The rest of the diff — mask_quotes()'s new VS/VE extent tracking, the !name activation/fail-closed mechanism, the quote-joined per-physical-line attribution, the three excuse mechanisms, and the shipped-token-list wiring — reads correctly against the stated grammar and is covered by the (otherwise very thorough) new test section. No CLAUDE.md/AGENTS.md concerns: this is an internal CI lint script, not a plugin manifest/marketplace-schema/hook-contract surface the fresh-docs mandate scopes to. Security review is out of scope for this lane per REVIEW.md's split (already covered by the separate security-review run on this PR, which found nothing).

Comment thread scripts/check-shell-portability.sh Outdated
Comment thread scripts/check-shell-portability.sh Outdated
Two review findings, both reproduced against real bash 5.3.15 before
being accepted.

OVER-flag: a process substitution in either half was walked into rather
than skipped, so `${v//X/<(cmd1 && cmd2)}` was reported. Its body is a
command list — measured, `v=aXb; "${v//X/<(a && b)}"` yields
`a/dev/fd/63b` with patsub_replacement BOTH on and off, so there is zero
version divergence and the hard error red-lined portable code. `<(` and
`>(` now open a frame; skip_frame already handled the shape, so the fix
is the opener test, factored into opens_frame() and shared by both
halves.

UNDER-flag: `#` straight after `${` was taken to be the length operator
unconditionally. It is the length operator only while what follows could
start a parameter name; a `/` cannot, so `${#//2/&}` is a substitution
on the positional-argument COUNT. Measured with two positional
parameters: `2` with patsub_replacement on, `&` with it off — the exact
divergence this class exists for, reported clean. This was a false PASS
on a literal ${var//pat/&}, which the limits section claimed could not
happen; the claim is true again now rather than being relaxed.
`${#}`, `${##}`, `${#v}` and `${#arr[@]}` keep a non-`/` successor and
stay length expansions.

Also rewrites a comment garbled by an earlier edit, and puts the
report_hit/next_hit doc block back over the function it describes.

Ten new cases, including both halves of the frame skip proving a real
hit AFTER a skipped process substitution is still found.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

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


Security review complete

  • Gather context (PR diff at HEAD 3b86f03, prior review threads)
  • Review latest commit (3b86f03) against the two previously-flagged Codex/Claude findings ($# special parameter, process-substitution frames)
  • Review full diff for injection/logic/trust-boundary issues
  • Post final findings

No security issues found.

This is a re-review triggered by the third commit, 3b86f03 ("fix(scripts): close both directions the & class got wrong"), which addresses the two findings from the prior review round on this PR (Codex + my own trace): the process-substitution false-positive and the ${#...} special-parameter false-negative. Both are correctness findings about the lint gate's detection accuracy (over-flag / under-flag on this specific bash-version class), not security findings — this script doesn't enforce a security boundary, it's a CI text scanner that prints diagnostics about other files' portability.

I re-traced the fix itself for any new security-relevant surface:

  • opens_frame() (L1203-1210) and skip_frame() (L1211-1243) now also recognize <(/>( as frame openers. The added branch reuses the exact same bounded, quote-aware, depth-tracked walk already used for $(...)/${...} — no new loop shape, no recursion, no unbounded backtracking. It's pure AWK string scanning over text the script already read via scan_file(); nothing here executes, sources, or shells out to the scanned content.
  • amp_in_frame()'s new #-as-special-parameter branch (L1300 area, the if (c == "#") { if (substr(l, i + 1, 1) != "/") return 0; ... } block) is a fixed number of bounded index increments — no new loop, no way to make the scan run away.
  • amp_violation case-match (L1704) is an unchanged pattern from the prior commit (faee078), already reviewed — a literal substring case match against text this script itself generated, not attacker-influenced.

No workflow files, permissions, or secret-handling code are touched by this commit (unchanged from prior rounds — still scoped to scripts/check-shell-portability.sh, its test file, and a comment-only token-list addition). The previously-reported low-severity algorithmic-complexity SUGGESTION (quadratic frame re-scan on pathologically nested ${…}) is unaffected by this commit — same complexity class as before, not re-reported here per the standing guidance to avoid duplicate findings across rounds.
· branch

@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 5m 7s —— View job


Review complete

  • Gather context (gh pr diff for the actual PR diff, changed files)
  • Check whether the two open Codex/Claude findings from prior rounds ($# special parameter, process-substitution frames) were addressed by the latest commit (3b86f03)
  • Trace the new logic (opens_frame, updated amp_in_frame) against bash's documented grammar
  • Review new test coverage for the fixes
  • Post inline findings (none survived verification — see note)
  • Post summary review

Summary

This PR has already been through several review rounds on the earlier commits (8546355, faee078). The latest commit, 3b86f03 ("close both directions the & class got wrong"), is a direct, targeted fix for the two findings that were still open at the last round — one from Codex, duplicated by an earlier Claude pass:

1. ${#//2/&} / ${#/2/&}$# as a special parameter, not the length operator. amp_in_frame() used to bail unconditionally on any # right after ${. The fix (check-shell-portability.sh L1310-1317) only treats # as the length operator when the next character can't start a parameter name; when it's immediately followed by /, it now falls through into the substitution walk with named=0. I traced this by hand against ${##} (length-of-$#, stays clean since the successor is another #, non-/), ${#v}/${#arr[@]} (unaffected), and ${#//2/&}/${#/2/&} (now correctly flagged) — the logic is internally consistent with the bash disambiguation rule the PR cites. I could not execute bash in this sandbox to independently confirm the runtime parse (execution was blocked here, same limitation earlier review rounds on this PR hit), so treat the underlying bash-semantics claim as author-traced-and-plausible, not independently run — but the AWK-side logic change does exactly what the finding asked for, and it's now pinned by 5 new test cases (amp_fires/amp_clean at L1219-1224 of the test file).

2. <(...)/>(...) process substitution not recognized as a nested frame. Fixed by extracting a new opens_frame() helper (L1201-1207) that also matches <(/>(, used at both call sites that previously inlined the backtick/$(/${ check. I traced skip_frame()'s paren-depth counting for a <(...) opener and confirmed it correctly balances nested parens the same way it already did for $(...) (the shared counting logic works for any single-char-prefix-plus-( opener, including incidentally handling $((...)) via the same accidental double-count that predates this PR). Six new test cases cover &&/& inside <(/>( in both the pattern and replacement halves, plus two cases confirming a real hit after a skipped process-substitution frame is still caught (frame-skip resumes scanning rather than swallowing the rest of the record).

Both fixes are precisely scoped to the two open findings, correctly reasoned against the bash grammar as I can verify it statically, and each has dedicated test coverage in both directions (hits and non-hits). No new issues introduced by this commit that I could find on inline trace.

One thing I checked and ruled out: a local git diff origin/main..HEAD on my end initially showed an unrelated-looking change to the sed -Ei class's comment in scripts/shell-portability-tokens.txt (dropping a documented false-negative caveat about nested process substitutions). Cross-checking against the actual gh pr diff for this PR shows that hunk is not part of this PR's diff — my local origin/main ref is stale relative to GitHub's actual base, which produced a false comparison. Retracted; no finding there.

No CLAUDE.md/AGENTS.md concerns — this is an internal CI lint script (scripts/), not a plugin manifest/marketplace-schema/hook-contract surface, so the fresh-docs mandate doesn't scope to it. Security review is out of scope for this lane (already covered twice by the dedicated security-review runs on this PR, both clean).
· branch

@kyle-sexton
kyle-sexton merged commit 341ab3a into main Aug 9, 2026
35 checks passed
@kyle-sexton
kyle-sexton deleted the ci/portability-lint-substitution-ampersand branch August 9, 2026 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant