feat(ci): add shell-portability-lint gate for GNU-only constructs

[kyle-sexton]

This was generated by AI during work-loop execution.

Summary

• No CI gate covers shell portability: shellcheck lints syntax/style, and portability-lint
  (ci: portability lint lane — enforce declared agnosticism mechanically (19-issue coupling class + top review-churn source) #531) matches skill-coupling tokens against changed skill files only. A GNU-only construct —
  like the \brequire\b word-boundary escape that nearly shipped a fail-open security predicate in
  markdown-format.sh (fixed on main via fix(security): patch six Claude Security scan findings (F1–F6) #1097) — passes both gates silently on BSD userland
  (macOS system grep/sed/date/stat/mktemp/sort), which no runner in this repo's CI covers
  (a Windows runner's Git Bash still ships GNU grep/sed, so it would not help either).
• Adds scripts/check-shell-portability.sh, a changed-file-scoped gate over **/*.sh mirroring
  check-skill-portability.sh's shape: an external ERE token list
  (scripts/shell-portability-tokens.txt), a same-line auto-guard for a co-located BSD counterpart,
  a per-site portability-ok: <reason> opt-out, and a whole-file portability-scope: <reason>
  declaration (used on the gate's own test file, which necessarily contains the constructs it
  detects as fixture data).
• Wires a new shell-portability-lint job into ci.yml (self-test on every push, diff-gated on
  pull requests) and adds it to the ci-status required-check list.
• Active classes today (zero real corpus impact, or auto-guarded): the regex-escape family
  (\b \< \> \s \S \w \W), grep -P/--perl-regexp, echo -e, sort -V, unsuffixed sed -i, and
  readlink -f (guarded when a realpath attempt sits on the same line — the shape
  lib/hook-utils.sh already uses). All four flag-based classes (grep -P, sort -V, echo -e,
  plus sed -i) match the target letter anywhere in a combined short-option cluster (-Pn, -Vr,
  -ne), not only as the cluster's last letter, and sed -i's portable BSD-safe empty-suffix idiom
  (-i '' / -i "") is auto-guarded rather than flagged.
• Staged (commented, inactive) classes: date -d, stat -c, mktemp -p. A corpus survey during
  this change found real, already-legitimate uses (a cross-statement GNU-then-BSD dialect function in
  morning-brief.sh; ~20 shared test-scaffolding mktemp -p sites with no BSD counterpart) that the
  same-line auto-guard doesn't yet cover — enabling them is tracked in the follow-up below, the same
  staged-rollout posture scripts/skill-portability-tokens.txt already documents for its own classes.

Triage note

#1491's triage marked the token-list-vs-BSD-container design fork as decision-defaulted (token list,
vetoable) and separately delegated "the starter token list's exact membership" to the implementer as
reversible/low-stakes. The ACTIVE/STAGED split above is that delegated, reversible call, made from an
actual corpus survey rather than guesswork — not a second judgment call requiring escalation.

Review response

An automated Codex review left 6 findings. Two risked flagging the CORRECT portable form and were
fixed directly (the combined-short-option-cluster gap on grep -P/sort -V/echo -e, and the
sed -i ''/sed -i "" empty-suffix idiom being wrongly flagged) plus a guard-scoping tightening (the
realpath auto-guard now applies only to the readlink pattern match, not the whole line). The
remaining three lower-severity findings (additional sed -i spellings, portability-scope:
substring-match precision — shared with the sibling gate, not unique to this PR — and an awk
operand edge case on a pathological filename) are deferred to #1513. See the threaded replies on each
finding for the per-finding classification.

Two further review rounds followed and the unresolved-thread count grew 6 to 11 without net decrease —
including one finding that asks to REVERSE the sed -i '' auto-guard added in response to round one.
Per this repo's convergence posture, the fix loop is cut off here: the five new findings are grouped
and deferred to #1517 with per-item re-open triggers, and each thread carries the reasoning. None is a
defect in the shipped behavior — four are false-negative detection gaps (before this gate they all
passed silently), and the one false positive is the token file's own documented over-flag posture,
which ships a per-site portability-ok: opt-out. Absorbing them would re-widen the change and
invalidate the corpus survey the ACTIVE/STAGED split rests on.

Test plan

• bash scripts/check-shell-portability.test.sh — 35/35 passing, including: the literal \b
  token actually fires (verified against the real awk resolved in this environment, gawk 5.4.0 —
  not assumed; this is a distinct, POSIX-fundamental escape from the sibling token list's
  documented \b-as-boundary-anchor pitfall, which this gate does not use), each of
  \< \> \s \S \w \W, grep -P/-riP/-Pn (and that a comment merely naming grep -P does not
  fire), echo -e/-ne, sort -V/-Vr, unsuffixed sed -i vs. sed -i.bak vs. the guarded
  sed -i ''/-i "", readlink -f bare vs. realpath-guarded (and that the guard does not leak
  to an unrelated token on the same line), same-line/comment-block-above/leak-boundary
  portability-ok: annotation behavior, the whole-file portability-scope: declaration,
  fail-closed behavior (malformed token, missing token file, invalid base ref), --all scope
  exclusion, a Git-quoted non-ASCII changed path, and — against the real corpus — that the
  shipped list does not flag markdown-format.sh's known-good reference implementation and that
  the staged classes stay inactive.
• scripts/check-shell-portability.sh origin/main run directly against this PR's own diff — the
  new gate's own source files (2 shell files in scope) pass clean.
• shellcheck --rcfile=.shellcheckrc on both new scripts — clean.
• actionlint .github/workflows/ci.yml — clean.
• bash scripts/check-skill-portability.test.sh (sibling gate) still passes — no cross-gate
  regression.
• Full CI run green, including the new shell-portability-lint job and the required ci-status
  aggregate.

Related

Closes #1491. Follow-ups: #1510 (enabling the staged classes), #1513 (detection-precision findings
from review round 1), #1517 (detection-precision findings from review rounds 2-3).

[claude]

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

---

Code Review

• Gather context and understand the request
• Read and analyze all changed files
• Post review findings

---

Summary

This is a well-structured addition that mirrors the existing check-skill-portability.sh shape closely, with sound fail-closed behavior and good test coverage. One medium-severity correctness issue and a few low-severity items follow.

---

Medium — sed -i '' is flagged, but the comment claims it isn't

File: scripts/shell-portability-tokens.txt, lines 65–68

# `sed -i` with no suffix argument attached — GNU treats the suffix as
# optional; BSD sed requires one immediately after `-i` (even an explicit empty
# string, `-i ''`, which this pattern does not flag — see is_guarded()).
sed[^\n]*-i[[:space:]]+[^[:space:]]

The parenthetical claim is wrong. The ERE sed[^\n]*-i[[:space:]]+[^[:space:]] does match sed -i '' 's/foo/bar/' file: after -i comes a space ([[:space:]]+ matches), then ' from the empty-string argument ([^[:space:]] matches). is_guarded() only checks for realpath; it offers no exemption for sed -i ''.

The test suite covers sed -i (GNU, flagged) and sed -i.bak (not flagged), but not sed -i ''. The missing test is the tell: if the intent were "don't flag sed -i ''," that case would have a test.

There are two defensible fixes:

Option A (simplest, consistent with "over-flag" posture) — correct the comment to state that sed -i '' is flagged, since it is BSD-only and not portable to GNU runners:

-# string, `-i ''`, which this pattern does not flag — see is_guarded()).
+# string, `-i ''`, which this pattern also flags — both forms are
+# platform-specific; the only truly cross-platform in-place edit
+# is to write to a temp file and mv, or use `perl -i -pe`).

Option B — add an is_guarded() branch and a test for the BSD form if the intent is genuinely to exempt sed -i '':

function is_guarded(l) {
  return l ~ /realpath/ || l ~ /sed -i[[:space:]]+''/ 
}

Option A requires no code change to the scanner. Fix this →

---

Low — The gate script itself uses sort -z (GNU extension)

File: scripts/check-shell-portability.sh, line 122

done < <(git diff --name-only --diff-filter=d -z "$base" -- '*.sh' | sort -z -u)

sort -z (NUL-delimited records) is a GNU coreutils extension; BSD sort has no -z. This is not a real defect since the gate only ever runs on ubuntu-24.04 runners, but it's mildly ironic for a script whose job is to detect GNU-only constructs. If the gate script were ever audited by --all against itself (it's excluded because it lacks the portability-scope: declaration... actually it has set -uo pipefail, not the declaration — would it self-flag?), sort -z isn't in the active token list so it would pass silently.

Non-blocking. Worth a note in the header or a # portability-ok: gate runs on ubuntu-24.04 only comment at the site.

---

Low — grep -P with P as a non-trailing flag is not caught

File: scripts/shell-portability-tokens.txt, line 52

grep[^\n]*[[:space:]]-[A-Za-z]*P([[:space:]]|$)

The pattern requires P to be the last letter before a space or end-of-line. grep -Pn pattern or grep -Pin pattern (P not trailing) won't match. The test description acknowledges this implicitly ("P last"), but the token comment doesn't. A one-line addition to the comment would make the limitation explicit for the next person who adds a class.

---

Low — is_guarded() extension burden is implicit

File: scripts/check-shell-portability.sh, line 151

function is_guarded(l) { return l ~ /realpath/ }

The function is named generically but handles exactly one class. When a class is promoted from STAGED, the token file's STAGED section mentions extending is_guarded(), but the function's own comment doesn't say this. A developer enabling stat -c and forgetting to add a stat -f guard marker here would get false positives without a clear pointer. Consider adding one sentence to the function's existing comment.

---

Positive observations

• Fail-closed is correct. The scan_file error propagation (exit 2 on scanner fault rather than treating empty output as clean) is exactly right; this is the hardest part of portability gates to get right.
• NUL-delimited diff path handling. Using -z on git diff and sort, with read -r -d '', avoids the silent-drop bug for non-ASCII filenames. The test at lines 312–337 validates this end-to-end against a real git repo.
• Annotation scope logic is correct. The pending_annot state in the awk pass correctly propagates through consecutive comment lines and resets only on the first subsequent code line, matching the "contiguous block above" contract. The boundary-leak test (lines 225–236) exercises this precisely.
• portability-scope: self-reference. The test file using the mechanism it tests is a clean design; same posture as the sibling gate.
• Staged classes. The corpus survey notes in the STAGED section (morning-brief's cross-statement dialect, the ~20 mktemp sites) are exactly the right information to have there; they prevent a future contributor from enabling a class naively.
• CI wiring is clean. Self-test runs unconditionally; gate is diff-scoped on PRs only; fetch-depth: 0 for base-ref resolution; added to ci-status required aggregate. Mirrors the existing portability-lint pattern faithfully.

[claude]

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

---

Security Review — PR #1511

• Gather PR diff and changed files
• Review scripts/check-shell-portability.sh for injection / unsafe patterns
• Review scripts/check-shell-portability.test.sh for security issues
• Review scripts/shell-portability-tokens.txt for security issues
• Review .github/workflows/ci.yml changes for dangerous GHA patterns
• Compile and post findings

---

Findings

No CRITICAL or IMPORTANT security issues found. One SUGGESTION below.

---

SUGGESTION — Shell expansion of $BASE_REF in run: step (low confidence)

Severity: SUGGESTION
Confidence: Low

.github/workflows/ci.yml line 743:

env:
  BASE_REF: ${{ github.base_ref }}
run: scripts/check-shell-portability.sh "origin/$BASE_REF"

Using an env var to hold ${{ github.base_ref }} correctly prevents GHA template injection (an attacker cannot embed ${{ secrets.TOKEN }} in a branch name and have it evaluated). However, the shell still expands $BASE_REF inside double quotes at runtime. Git ref names can technically include $, backticks, and $(...) sequences, so a branch named e.g. main`id` would cause shell command substitution when the run step executes.

Practical risk is low: github.base_ref is the target branch of the PR, which must already exist in the base repo (melodic-software/claude-code-plugins). Creating such a branch requires write access to the base repo — well beyond any external-contributor attack surface. The same pattern is already in use at the adjacent sibling step on line 717 without incident.

The fully hardened form would pass the ref via stdin or use printf '%s' "$BASE_REF" | xargs ... to avoid any shell expansion, but this is consistent with the existing repo pattern and the threat model doesn't justify the change today. Fix this →

---

What looks good

• GHA trigger is pull_request (not pull_request_target or workflow_run) — the job runs in the PR fork's context and cannot access base-repo secrets. No untrusted-code-with-secrets risk.
• Global permissions: contents: read — minimal. The new job inherits this and adds no elevation.
• Checkout pinned to full SHA (actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0) with persist-credentials: false.
• ${{ github.base_ref }} is passed through an env var before use in the shell command — the correct pattern that prevents GHA template injection.
• Fail-closed error handling throughout check-shell-portability.sh: any scanner fault (malformed token, missing token file, invalid base ref, awk error) exits 2 rather than silently treating the file as clean.
• git rev-parse --verify validates the base ref before it's passed anywhere else in the script.
• NUL-delimited git diff (-z) handles non-ASCII filenames without silent truncation.
• awk patterns are matched via ~ (dynamic regex) — no eval or code-execution path from the token file contents.
• SHELL_PORTABILITY_TOKENS env override only matters if the CI workflow itself is modified to set it — no external actor can influence it.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 46a2a615fe

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

[claude]

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

---

Code Review — PR #1511

• Read scripts/check-shell-portability.sh
• Read scripts/shell-portability-tokens.txt
• Read scripts/check-shell-portability.test.sh
• Review .github/workflows/ci.yml diff
• Verify Codex P2 findings
• Analyze for additional issues
• Post final review

---

Review Findings

Three bugs confirmed; one additional test coverage gap found. Listed most-severe first.

---

Bug 1 — is_guarded() suppresses ANY active-token match when realpath appears on the line

scripts/check-shell-portability.sh line 151

function is_guarded(l) { return l ~ /realpath/ }

The comment above (lines 143–150) says this guard is "scoped to the one active class that needs it today (readlink -f)." The implementation is not scoped — it fires for every active-token match. Any line containing realpath anywhere alongside a GNU-only construct silently passes the gate:

path=$(realpath "$1") || grep -P 'foo' "$f"     # grep -P suppressed — false negative
echo -e "msg"; realpath "$dir"                   # echo -e suppressed — false negative

The fix is to condition the guard on the matched pattern, not just the line:

function is_guarded(l, pat) {
  if (pat ~ /readlink/) return l ~ /realpath/
  return 0
}

And update the call-site: if (is_guarded(line, patterns[i])) continue.

Alternatively, a simpler but still correct tightening without touching the signature:

function is_guarded(l) { return l ~ /readlink/ && l ~ /realpath/ }

(A line genuinely hitting readlink -f that's properly guarded will always contain readlink.)

Fix this →

---

Bug 2 — grep -P (and sort -V) tokens miss combined flag groups where the flag is not last

scripts/shell-portability-tokens.txt line 52 and line 63

grep[^\n]*[[:space:]]-[A-Za-z]*P([[:space:]]|$)   # line 52
sort[^\n]*[[:space:]]-[A-Za-z]*V([[:space:]]|$)   # line 63

The ([[:space:]]|$) tail anchors P/V as the final flag before whitespace or end-of-line. Common legitimate GNU invocations escape undetected:

grep -Pn  '\d+' file       # P followed by n — not matched
grep -Pin '\d+' file       # P followed by in — not matched
sort -Vr  versions.txt     # V followed by r — not matched

The fix is to allow trailing letters after P/V — the flag group still contains the GNU flag regardless of what follows:

grep[^\n]*[[:space:]]-[A-Za-z]*P[A-Za-z]*([[:space:]]|$)
sort[^\n]*[[:space:]]-[A-Za-z]*V[A-Za-z]*([[:space:]]|$)

The existing test at check-shell-portability.test.sh line 120 only covers grep -riP (P last). Adding a grep -Pn case would have caught this before shipping.

Fix this →

---

Bug 3 — sed -i token misses sed --in-place (long-form GNU option)

scripts/shell-portability-tokens.txt line 68

sed[^\n]*-i[[:space:]]+[^[:space:]]

sed --in-place 's/foo/bar/' file is not caught — the pattern only matches -i. The sed --in-place[=SUFFIX] form is documented GNU behaviour and equally incompatible with BSD sed. Adding |--in-place([[:space:]]|=|$) covers the unambiguous long-form spelling. The sed -i'' case (no space, empty suffix directly attached) is arguably portable since BSD sed also accepts it, so leaving that out of scope is reasonable.

Fix this →

---

What looks good

• Fail-closed design throughout. Scanner fault → exit 2, never a silent clean result. git rev-parse --verify validates base ref before use. Missing token file is caught on startup.
• NUL-delimited git diff (-z) correctly handles Git-quoted non-ASCII paths that would be silently dropped by a line-oriented read.
• portability-scope: whole-file exemption is a clean mechanism: the test file necessarily contains every forbidden construct as fixture data, and the declaration is diff-visible.
• Changed-file scoping mirrors the sibling gate exactly — enabling a class never red-lines main; pre-existing uses wait for their file's next touch.
• CI job shape is sound: self-test runs unconditionally on every push (broken detector cannot hide behind a no-PR green), gate step gated to pull_request only. Added to ci-status aggregator correctly.
• GHA security posture (confirmed by security review): pull_request trigger only (no secret access), contents: read permissions, pinned checkout SHA with persist-credentials: false, BASE_REF indirection prevents template injection.
• AWK token matching uses line ~ patterns[i] (dynamic regex), not eval — no code-execution path from the token file.
• STAGED-class inertness is proven against the real corpus in the test suite — staged classes are verified inactive, not merely assumed.

---

Summary: Two token-regex coverage gaps (bugs 2 and 3) let real GNU-only invocations pass the gate silently. Bug 1 is more subtle but has wider blast radius: any realpath-containing line suppresses all other GNU-construct hits on that line, not just readlink -f. All three have straightforward fixes; bugs 1 and 2 also need test additions to hold the fix.

[claude]

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

---

Security Review — PR #1511

• Read PR diff and changed files
• Analyze scripts/check-shell-portability.sh for injection and unsafe patterns
• Analyze scripts/check-shell-portability.test.sh for security issues
• Analyze scripts/shell-portability-tokens.txt for security issues
• Analyze .github/workflows/ci.yml changes for dangerous GHA patterns
• Compile and post findings

---

Findings

One IMPORTANT finding (logic flaw that undermines the gate's core security guarantee), one SUGGESTION on token coverage, and a re-confirmation of the prior review's low-confidence GHA note below.

---

IMPORTANT — is_guarded() applies to every token, not just readlink -f

Severity: IMPORTANT
Confidence: High

scripts/check-shell-portability.sh line 151:

function is_guarded(l) { return l ~ /realpath/ }

The comment at lines 145–150 explicitly scopes this guard to the readlink -f class ("Scoped to the one active class that needs it today"), but the predicate is called unconditionally for every token match (line 176):

if (is_annotated(line) || annotated_above) continue
if (is_guarded(line)) continue   ← applies regardless of which token matched

Any line that contains the word realpath anywhere is silently passed — even when the matched construct is grep -P, echo -e, sort -V, or sed -i. Concrete bypasses, none of which require malicious intent:

# All of these pass the gate today:
grep -P "pattern" "$f" | xargs realpath       # grep -P not flagged
echo -e "Resolving: $(realpath "$dir")"        # echo -e not flagged
sort -V | realpath                             # sort -V not flagged
sed -i 's/x/y/' "$f"; realpath "$f" >/dev/null # sed -i not flagged

The PR's stated motivation is preventing constructs that silently fail-open on BSD (e.g., \brequire\b becoming brequireb, matching nothing). A developer who writes grep -P "pattern" | realpath in a security predicate would get a green gate and a silent fail-open on macOS — the exact scenario the gate exists to prevent.

The fix is to scope the guard to the token class it belongs to by passing the matched pattern into is_guarded() and gating the check on pattern ~ /readlink/. The comment already documents the intent; the implementation diverges from it. Fix this →

---

SUGGESTION — Combined option-group forms of grep -P and sort -V evade detection

Severity: SUGGESTION
Confidence: High

scripts/shell-portability-tokens.txt line 52 and line 63:

grep[^\n]*[[:space:]]-[A-Za-z]*P([[:space:]]|$)
sort[^\n]*[[:space:]]-[A-Za-z]*V([[:space:]]|$)

Both patterns require the flag letter (P, V) to be immediately followed by a space or end-of-line. Combined short options where P/V is not the final character — grep -Pn, grep -nPl, sort -Vr — satisfy the ERE anchoring on [A-Za-z]* before the letter but fail the trailing ([[:space:]]|$) requirement and pass the gate silently. These forms are valid GNU invocations and produce the same BSD-incompatible behavior. Changing the trailing anchor to ([[:space:]A-Za-z]|$) (or equivalently [^-]|$) would catch trailing flags. Fix this →

---

SUGGESTION — $BASE_REF shell expansion in run: block (re-confirmed, low confidence)

Severity: SUGGESTION
Confidence: Low

.github/workflows/ci.yml line 743 (same pattern as every existing diff-gated step in this workflow):

env:
  BASE_REF: ${{ github.base_ref }}
run: scripts/check-shell-portability.sh "origin/$BASE_REF"

Passing github.base_ref through env: is the correct GHA template-injection prevention. Shell still expands $BASE_REF in double quotes at runtime, so a base-branch name containing ` or $(...) would execute under the job's context. Practical risk is negligible — exploiting this requires write access to the base repository to create such a branch. This is consistent with the existing pattern at the adjacent sibling step (line 717) and ~10 other identical shapes throughout this workflow; the threat model doesn't justify diverging here.

---

What looks good

• pull_request trigger (not pull_request_target/workflow_run) — job runs in fork context, no base-repo secrets reachable.
• permissions: contents: read inherited — no elevation; the new job adds no elevated permissions.
• Checkout pinned to full SHA (actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0) with persist-credentials: false.
• git rev-parse --verify validates the base ref before any use in git diff — bad refs exit 2, never silently treated as "nothing to scan."
• NUL-delimited git diff -z — non-ASCII and quoted filenames arrive verbatim; no silent path dropping.
• awk dynamic regex (line ~ patterns[i]) — patterns from the token file are matched, not eval'd; no code-execution path from token file content.
• Fail-closed scanner fault handling — an awk error on a malformed token exits 2 rather than returning empty output that reads as "clean."
• SHELL_PORTABILITY_TOKENS env override is only reachable via the CI workflow file (requires repository write access to change).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5908431f66

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

[claude]

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

---

Code Review — PR #1511 (post-fix pass, commit 695ca2d)

• Gather context and understand the request
• Read all changed files at HEAD (695ca2d)
• Verify prior review findings were addressed
• Check for remaining or new issues
• Post final review findings

---

Summary

Commit 695ca2d ("harden shell-portability-lint match precision per review") correctly addresses all three priority bugs from the prior review passes. The fixes are well-implemented and each has a corresponding new test. Remaining deferred items are documented in the token file and tracked in #1513. No new issues were introduced.

---

Prior bugs — all confirmed fixed

Bug 1 — is_guarded() scoped to readlink pattern, not the whole line

check-shell-portability.sh lines 159–168

The old function is_guarded(l) { return l ~ /realpath/ } suppressed every active-token match on any line containing realpath. The fix adds p (the matched pattern string) as a second argument and conditions the realpath check on p ~ /readlink/:

function is_guarded(l, p,    q1, q2, empty_suffix) {
  if (p ~ /readlink/ && l ~ /realpath/) return 1
  ...
}

The call site on line 193 now passes patterns[i] alongside line. A new test at lines 248–258 confirms a realpath-mentioning line does not suppress a \b hit on the same line. ✅

Bug 2 — grep -P, sort -V, echo -e now catch combined flag groups

shell-portability-tokens.txt lines 53, 61, 66

Trailing [A-Za-z]* added after the target flag letter so grep -Pn, sort -Vr, echo -ne are all caught. New tests for grep -Pn (line 129–135), echo -ne (line 160–165), and sort -Vr (line 178–184). ✅

Bug 3 — sed -i '' / sed -i "" now auto-guarded, not flagged

check-shell-portability.sh lines 161–166

is_guarded now builds an empty_suffix regex from sprintf-constructed quote chars (avoiding literal quote chars in the awk source, a clean solution) and returns 1 when sed -i '' or sed -i "" appears on the matched line. The token file comment was updated from the erroneous "this pattern does not flag" to the correct "auto-guarded by is_guarded()." New tests at lines 209–224. ✅

The p ~ /sed\[/ discriminator correctly identifies the sed-i pattern (which starts with sed[^\n], containing the literal text sed[). This coupling is acceptable: the STAGED comment already directs future contributors to extend is_guarded() when enabling a new class.

---

Low — echo -n -e "text" (space-separated flags) not caught

shell-portability-tokens.txt line 61

The echo pattern looks for the -e flag in the first flag cluster immediately after echo. When the flag groups are space-separated and -e is not first — e.g. echo -n -e "text" — the gate does not fire. The gate does catch echo -ne, echo -en, and echo -e; the space-separated form is unusual in practice and the over-flag posture applies to combined-flag cases.

Non-blocking; worth noting that the token comment ("anywhere in echo's short-option cluster") is accurate for combined clusters but is silent on the separate-cluster case. A parenthetical like "(echo -e, echo -ne; not echo -n -e with space-separated groups)" would be complete, but this is a documentation nit.

---

Positive observations

• Correct awk local-variable convention. The function is_guarded(l, p, q1, q2, empty_suffix) signature correctly uses extra leading spaces before the "local" variables — the standard awk idiom. A maintainer adding a new local must follow the same convention or the variable leaks to global scope.
• sprintf quoting trick is clean. Building the empty_suffix ERE from sprintf("%c", 39) / sprintf("%c", 34) correctly avoids embedding literal quote characters inside an awk heredoc — no escaping arms-race, readable intent.
• Empty-suffix ERE grouping is correct. The concatenated pattern -i[[:space:]]+('')|("")([[:space:]]|$) properly scopes the | alternation within the parentheses, not at the top level — the ( and ) in the string literal surround both alternatives.
• Six new tests with surgical scope. Each test targets exactly the case it names (grep -Pn, echo -ne, sort -Vr, sed -i '', sed -i "", realpath-guard-scoping) using a per-class minimal token file — no coupling to other active classes.
• Deferred items properly documented. The token file's sed-i comment now lists exactly what is NOT yet covered (-Ei, --in-place, attached empty suffix) and cross-references ci: harden shell-portability-lint detection precision (sed -i spellings, portability-scope precision, awk operand edge case) #1513. Future contributors won't accidentally conclude the class is complete.
• CI wiring, GHA security, and fail-closed behavior are unchanged from the prior reviews' positive findings and remain sound.

---

The three confirmed correctness bugs are fixed, the new test cases are correctly scoped, and the remaining deferred items are tracked. Ready to merge subject to the echo -n -e documentation nit if the author wants to address it.

[claude]

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

---

Security Review — PR #1511 (HEAD: 695ca2d)

• Read PR diff and current file state
• Review scripts/check-shell-portability.sh — injection, unsafe patterns, guard logic
• Review scripts/shell-portability-tokens.txt — token coverage / bypass paths
• Review scripts/check-shell-portability.test.sh — test harness security
• Review .github/workflows/ci.yml changes — GHA patterns
• Verify prior IMPORTANT finding (is_guarded() too broad) is fixed
• Compile and post findings

---

Findings

No CRITICAL or IMPORTANT security issues found. The prior IMPORTANT finding from the 5908431 review has been correctly fixed in 695ca2d. One SUGGESTION carries forward with a correction to its prior assessment; one pre-existing SUGGESTION is re-confirmed.

---

Prior IMPORTANT finding: FIXED ✓

scripts/check-shell-portability.sh lines 159–168

The is_guarded() function now takes the matched pattern as a second argument (p) and gates the realpath guard on p ~ /readlink/. A line containing realpath for any unrelated reason no longer blanket-suppresses hits from grep -P, echo -e, sort -V, or sed -i. A second guard for the sed -i '' / sed -i "" BSD-safe idiom is added and correctly scoped to p ~ /sed\[/. The call site at line 193 passes patterns[i]. The fix matches the intent already documented in the comments. The test at lines 248–258 of the test file explicitly exercises the scoping invariant.

---

SUGGESTION — portability-scope: check is a substring match, not anchored to a comment

Severity: SUGGESTION
Confidence: Medium

scripts/check-shell-portability.sh line 138:

if grep -qE 'portability-scope:' -- "$file"; then
    return 0
fi

This exempts an entire file from portability scanning whenever the string portability-scope: appears anywhere in the file — not only in a # portability-scope: comment. A developer can embed the string in a variable, a heredoc, a printf call, or a colon-command:

: "portability-scope: not a real declaration"   # whole file now exempt
grep -Eq '\brequire\b' "$file"                   # never flagged

The bypass is visible in the diff, so it is not invisible to reviewers; but a code reviewer scanning for a GNU-only construct on a touched file might not notice an unrelated : "portability-scope:..." earlier in the file. The companion annotation (portability-ok:) has the same property but its blast radius is per-line rather than whole-file. The prior reviews did not flag this explicitly.

Tightening to require the declaration on a #-comment line would close the gap:

if grep -qE '^[[:space:]]*#.*portability-scope:' -- "$file"; then

This is consistent with how portability-ok: annotations are expected to appear (the prose says "comment at the site"). The test file uses a proper # portability-scope: comment, so tightening the check would not break the self-referential test. Fix this →

---

SUGGESTION — $BASE_REF shell expansion in run: block (re-confirmed, low confidence)

Severity: SUGGESTION
Confidence: Low

.github/workflows/ci.yml line 743:

env:
  BASE_REF: ${{ github.base_ref }}
run: scripts/check-shell-portability.sh "origin/$BASE_REF"

Passing github.base_ref through env: correctly prevents GHA template injection. Shell still expands $BASE_REF in double quotes at runtime; a base branch named with embedded ` or $(...) would execute under the job context. Practical risk is negligible: exploiting this requires write access to the base repository to create such a branch. This is consistent with every other diff-gated step in this workflow (lines 65, 255, 275, 295, 315, 423, 453, 483, 556, 624, 676, 717) — changing the pattern here while leaving the others is probably not the right call. Flag for the whole-workflow cleanup tracked separately, if desired.

---

What looks good

• Prior IMPORTANT fix is correct. is_guarded(l, p) now scopes each guard to its class via the matched-pattern argument; the test at lines 248–258 explicitly verifies the scoping invariant.
• New sed-i empty-suffix guard is sound. The p ~ /sed\[/ heuristic correctly identifies the sed class token (which begins sed[^\n]*), and the regex constructed in awk from sprintf avoids quoting problems with the literal quote characters.
• Token precision improvements are correct. grep -Pn, sort -Vr, echo -ne now all fire; tests confirm all three cases.
• GHA trigger remains pull_request (not pull_request_target/workflow_run) — fork-context only, no base-repo secrets reachable.
• Global permissions: contents: read inherited — the new job adds no elevation.
• Checkout is pinned (actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0) with persist-credentials: false.
• git rev-parse --verify validates the base ref before any diff invocation; even a flag-shaped value (e.g. --upload-pack=x) becomes --upload-pack=x^{commit}, which git treats as a ref name and rejects cleanly.
• Fail-closed behavior preserved — scanner fault exits 2, never reads as clean.
• NUL-delimited git diff -z — non-ASCII and quoted filenames arrive verbatim.
• AWK token matching uses line ~ patterns[i] (dynamic regex), not eval — no code-execution path from the token file.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 695ca2d034

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

[claude]

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

---

Code Review — PR #1511 (HEAD: 49a0dbf)

• Gather context and understand the request
• Read current state of all changed files
• Verify open Codex findings at 695ca2d (3 items)
• Check 49a0dbf fix for false-positive correctness
• Post final review findings

---

Head commit (49a0dbf) — correct and minimal

The commit avoids a typos-gate false positive ("Pn" → "On") by:

• Adding # spellchecker:disable-line to the three test lines that reference grep -Pn as a fixture
• Removing -Pn from the grep -P comment in shell-portability-tokens.txt (it was redundant; -riP and -nP already illustrate P-not-last)

The test still exercises grep -Pn as a concrete case — coverage is maintained, the comment is just tighter. This is the right fix given _typos.toml is synced from upstream and can't be locally patched.

---

Three open Codex findings (raised at 695ca2d, not yet replied to)

Low — \b and sibling escape tokens can false-positive on printf '\b'

scripts/shell-portability-tokens.txt line 42

The token \\b is matched against any non-comment code line via line ~ patterns[i]. The comment above it says the class targets "a grep/sed pattern," but the ERE itself isn't constrained to grep/sed argument positions. A POSIX-portable printf '\b' (backspace) contains the literal \b sequence and would be flagged:

printf 'Erasing last char: \b \r' >&2   # POSIX-portable — falsely flagged

Verdict: CONFIRMED. Severity is low because:

• The gate explicitly documents "Direction is deliberately over-flag, not under-flag"
• A portability-ok: <reason> annotation at the site is the correct resolution
• printf '\b' is rare in this corpus; the known-good reference test doesn't exercise it

No immediate code change needed, but the token file comment could clarify the mitigation ("flag any \b in non-comment code — annotate with portability-ok: if this is not a regex pattern"). Fix comment →

---

Low — Backslash line-continuation creates an undetected gap

scripts/check-shell-portability.sh lines 179–196

The awk pass scans physical lines. A command split with \ continuation evades every token:

grep \
  -P "\d+" "$file"    # neither line matches grep[^\n]*[[:space:]]-...-P

Verdict: CONFIRMED as a design limitation. The script header explicitly documents "This is a grep-level tripwire, not a semantic proof." The PRE-existing check-skill-portability.sh has the same limitation. Low severity because continuation-split flag clusters are unusual in this corpus and would be caught by shellcheck if the split is stylistically problematic. Deferred to #1513 per the PR.

---

Very Low — readlink -f guard checks co-location, not fallback semantics

scripts/check-shell-portability.sh line 160

if (p ~ /readlink/ && l ~ /realpath/) return 1

The guard fires whenever both substrings co-exist on the line — it doesn't require the || fallback control flow the comments describe. A sequential form where readlink -f runs unconditionally would be suppressed:

realpath "$1"; readlink -f "$1"   # guarded, but readlink -f always runs

Verdict: CONFIRMED as a theoretical edge case. Practically, this contrived form doesn't appear in real code — sequential realpath; readlink -f is semantically pointless (the result would always be realpath's output). The only real pattern with both on the same line is realpath ... || readlink -f .... The comment already says "a portable form already attempted on the same line" and the gate's stated scope is "grep-level tripwire, not a semantic proof." Very low severity.

---

What looks good

• Critical fixes from earlier reviews held. is_guarded(l, p) correctly scopes guards per token class; combined short-option clusters (grep -Pn, sort -Vr, echo -ne) are detected; sed -i ''/sed -i "" BSD-safe idiom is properly auto-guarded.
• sed -i '' guard construction is correct. p ~ /sed\[/ correctly identifies the sed token (it starts with sed[), and the sprintf-constructed empty-suffix regex avoids quoting problems cleanly.
• Fail-closed everywhere. Scanner fault → exit 2; missing token file → exit 2; invalid base ref → exit 2 (via git rev-parse --verify). No silent-clean false negatives.
• NUL-delimited git diff -z + read -r -d ''. Non-ASCII paths arrive verbatim; the test at lines 371–395 verifies this end-to-end against a real git repo.
• CI wiring is clean. Self-test unconditional; gate diff-scoped to PRs; fetch-depth: 0; BASE_REF indirection prevents GHA template injection; added to ci-status required aggregate.
• Staged classes inert. date -d, stat -c, mktemp -p are commented with substantive corpus notes (not guesswork) on why they're not yet active, and the test verifies their inertness against the shipping list.
• STAGED section notes are load-bearing. Each entry explains what real corpus sites would need migration (morning-brief's cross-statement dialect, ~20 mktemp sites) — a future contributor enabling a class can't do it naively without reading the blocker.

---

Overall

The PR is in good shape. The three open Codex findings are low/very-low severity design limitations, consistent with the gate's documented "over-flag" posture and its "grep-level tripwire" scope. Two of them are already tracked in #1513; the third (the printf '\b' comment accuracy) is a one-line prose fix if desired. Ready to merge from a correctness and security standpoint.

[claude]

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

---

Security Review — PR #1511 (HEAD: 49a0dbf)

• Gather context and understand the request
• Read PR diff and changed files at HEAD
• Analyze scripts/check-shell-portability.sh for injection / unsafe patterns
• Analyze scripts/shell-portability-tokens.txt for security issues
• Analyze scripts/check-shell-portability.test.sh for security issues
• Analyze .github/workflows/ci.yml changes for dangerous GHA patterns
• Verify prior IMPORTANT findings at 695ca2d are still fixed at 49a0dbf
• Compile and post findings

---

Findings

No CRITICAL or IMPORTANT security issues found. The prior IMPORTANT finding from the 5908431/695ca2d review pass is confirmed fixed and unchanged at HEAD. Two SUGGESTIONS carry forward from prior reviews.

---

Prior IMPORTANT finding: CONFIRMED FIXED ✓

scripts/check-shell-portability.sh lines 159–168

The is_guarded(l, p) fix from 695ca2d is intact. The function takes the matched pattern as a second argument and the realpath guard only fires when p ~ /readlink/; a line containing realpath for any unrelated reason does not blanket-suppress hits from other token classes. The sed -i ''/-i "" guard is likewise scoped to p ~ /sed\[/. HEAD (49a0dbf) touches only test-file spellchecker annotations and a one-word comment edit — no change to the guard logic.

---

SUGGESTION — portability-scope: whole-file exemption is a substring match, not anchored to a comment

Severity: SUGGESTION
Confidence: Medium

scripts/check-shell-portability.sh line 138:

if grep -qE 'portability-scope:' -- "$file"; then
    return 0
fi

This exempts the entire file whenever the string portability-scope: appears anywhere — including inside a variable assignment, a heredoc, a printf format, or a no-op colon command. A developer can silently disable the whole gate for a file without a diff-visible #-comment declaration:

: "portability-scope: no real declaration"   # whole file now exempt
grep -P "pattern" "$file"                    # never flagged

The blast radius is one file per occurrence, not the whole gate, and the bypass is diff-visible. The sibling gate (check-skill-portability.sh) has the same property, and Kyle deferred the fix to #1513 as a shared two-gate repair. Acceptable posture for merge; worth landing together with the sibling fix rather than diverging.

Tightening to a comment-only match would close the gap without breaking the existing self-referential test:

if grep -qE '^[[:space:]]*#.*portability-scope:' -- "$file"; then

---

SUGGESTION — $BASE_REF shell expansion in run: block (re-confirmed, low confidence)

Severity: SUGGESTION
Confidence: Low

.github/workflows/ci.yml line 742–743:

env:
  BASE_REF: ${{ github.base_ref }}
run: scripts/check-shell-portability.sh "origin/$BASE_REF"

Using env: to hold ${{ github.base_ref }} is the correct GHA template-injection prevention — the GHA expression is evaluated before the shell sees it, so no GHA context injection is possible. Shell still word-expands $BASE_REF in double quotes at runtime; a base branch named with embedded ` or $(...) would execute under the job context. Practical risk is negligible: exploiting this requires repository write access to create such a branch name. This pattern is used identically in ~12 other diff-gated steps throughout ci.yml (lines 65, 255, 275, 295, 315, 423, 453, 483, 556, 624, 676, 717); a fix here in isolation would diverge from the established repo convention rather than improve it.

---

What looks good

• GHA trigger is pull_request (not pull_request_target/workflow_run) — the job runs in the fork context and cannot reach base-repo secrets. No untrusted-code-with-secrets risk.
• Global permissions: contents: read inherited — the new job adds no elevation; no write permissions anywhere in scope.
• Checkout pinned to full SHA (actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0) with persist-credentials: false — no credential leakage, no supply-chain drift.
• git rev-parse --verify at line 110 validates the base ref before any git-diff use; even a flag-shaped ref (e.g. --upload-pack=x) becomes --upload-pack=x^{commit}, which git rejects cleanly.
• is_guarded(l, p) is correctly scoped — each class's guard fires only when the matched pattern belongs to that class. Prior IMPORTANT finding fully resolved.
• AWK token matching uses line ~ patterns[i] (dynamic regex, not eval) — no code-execution path from token file contents, even with a SHELL_PORTABILITY_TOKENS override.
• Fail-closed scanner fault handling — a malformed token or awk error exits 2, never reads as "clean."
• NUL-delimited git diff -z with read -r -d '' — non-ASCII and quoted filenames arrive verbatim; no silent path drop.
• SHELL_PORTABILITY_TOKENS env override is only injectable via the CI workflow file itself, which requires repository write access.
• Self-test runs unconditionally on every push — a broken detector cannot hide behind a PR-only green gate.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 49a0dbf927

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

[kyle-sexton]

This was generated by AI during work-loop execution.

This PR merged (8949b57) about a minute before a second round of automated review landed two confirmed correctness findings on the branch's later pushes — including that the sed -i '' / sed -i "" auto-guard added in 695ca2d was itself wrong (verified against a real GNU sed 4.9: that invocation exits 2 on this repo's own GNU/Linux CI, so the "portable" idiom it exempted is actually GNU-incompatible). Fixed in follow-up PR #1519, which also carries forward two smaller precision fixes from the same review round that never made it into this merge (the ||-required readlink guard, sort --version-sort).