Skip to content

perf(scripts): make the shell-portability scan near linear, 979s to 34s on a full sweep - #3488

Merged
kyle-sexton merged 5 commits into
mainfrom
wip/portability-perf-unverified
Aug 29, 2026
Merged

perf(scripts): make the shell-portability scan near linear, 979s to 34s on a full sweep#3488
kyle-sexton merged 5 commits into
mainfrom
wip/portability-perf-unverified

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

No linked issue

Summary

scripts/check-shell-portability.sh had superlinear per-file cost, and its own header documented that as structural and permanent: worst file ~123s, --all exceeding a 600s timeout, four attempts once spent mislabeling the timeout as flakiness. The consequence was that CI runs changed-file mode only, so a fleet regression in an untouched file goes unproven, and the shell-portability-lint lane (which runs the self-test) was a top-three CI cost at roughly 135 runs/day.

Root cause, measured rather than guessed: the awk record loop called mask_quotes() on the ENTIRE accumulated logical line once per physical line. This script's own awk program is a single quote-joined record of about 1,580 lines, so the quote walk restarted once per line over a growing 80 KB string, and mawk's m = m c append is itself quadratic. The two multiplied. The driver was never file length but the length of the longest logical record: a flat 25,600-line file was already linear before this change and still is.

Fix

mask_quotes() becomes resumable. Every branch reads at most two characters of lookahead, so the walk commits state and mask up to len - 2 and resumes there, holding and restoring the full walk state (frame stack, ARDEPTH/VOPEN keyed by depth, NV, and a sticky inline-comment flag so a comment keeps masking text joined on later). The mask is materialized once per record instead of once per physical line.

The commit bound accounts for the record loop's own truncation: when a dangling backslash is stripped, the bound is computed from the effective length via climit = len - 2 - (substr(l, len, 1) == BS). Without that term, a decision committed at len - 2 would rest on a character the loop then deletes, and the only two-character-lookahead branch ($(( versus $() would mis-decide, so a guard ladder written as

stat -c %s $(\
(1 | 2)) || stat -f %z f

would be reported as a violation even though POSIX removes the backslash-newline during tokenization and the shell sees $((.

Four secondary hot spots go with it: blanks() grows a cached pad by doubling, after_last_boundary() returns the last field of a split(), two exact cheap pre-filters short-circuit, and the stat ladder matches inside a pipe-bounded window. No pattern was dropped, no check weakened, no exclusion added.

Verification

Byte-identical to main's implementation over the whole corpus, in both modes, old and new run side by side:

mode files exit stdout md5 stderr md5 old new
--all 1,538 0 both 2680d2e1fabc879a0861f2a85a3f538b empty 983s 34s
--paths 1,684 1 both empty 612e869d4d1a0b5559dd195ae4ea9f74 1136s 37s

cmp clean on all four pairs. The --paths list is deliberately wider than --all's, including vendor/ and evals/ paths the gate normally excludes.

Per-file: this file 68.7s → 1.6s (43x), audit-fleet.sh 175.1s → 0.32s, lib/hook-utils.test.sh 168.3s → 0.22s. The self-test drops 466s → 14s, which is where the CI saving lands.

Independently verified. A fresh-context adversarial pass ran 88 hand-built boundary cases, 35 targeted cases, 5,000 differential fuzz cases and an instrumented oracle carrying the old non-resumable walk as a reference, asserting mask, NV, extent and dangling-state equality on every logical record across the corpus. That pass is what found the climit defect above, which no output diff could surface because the triggering shape appears nowhere in the corpus. It also audited every piece of walk state for commit/restore completeness, confirmed the len - 2 bound is exactly tight (len - 1 provably too far), verified the pipe-bounded window by construction, and confirmed both pre-filters are exact supersets.

Suite grew 336 → 345 assertions, adding cases that exercise the resume boundary as a boundary, which nothing did before: the backslash-continuation $(( shape, the sticky comment across a join, an expansion closed before the commit point, and arithmetic depth at two levels. Each is mutation-proven, and each of those mutations passes the previous 336 untouched. One negative result is recorded honestly in the history: HELD_VO could not be made observable, since every read of VOPEN[d] is preceded in the same walk by the ${ that writes it, so holding it is defensive rather than behavioral.

The header's COST section is rewritten to match reality, including that the residue tracks record length rather than hit density (a zero-hit single record still goes 0.11s to 6.58s from 1,600 to 25,600 lines, while varying hit density at fixed length moves it about 5%), that linearity holds only while the longest logical record is bounded, and per-change ablation numbers with each ablation confirmed output-identical first.

Related

  • Branch history is honest about its own path: the first commit parked this as not merge ready when the defect was open, and a later commit corrects that record after the fix was proven.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XGLX1xYgy27JiRqLjoiH8T


Generated by Claude Code

claude added 5 commits August 29, 2026 00:32
…lure

`claude plugin install --scope user -y` already leaves the plugin enabled, so
the refresh chain's tail `plugin enable` exits 1 with "already enabled at user
scope" on the healthy path. The three steps were joined with `&&`, so every
successful refresh scored as a failure: the startup line read "65 failed" and
"54 failed" for a registry that was verifiably installed, enabled and at HEAD.
The failure count is the bootstrap's only health signal, and dozens of false
alarms per session start buried it.

The three subcommand exit statuses are now advisory. Verification instead reads
the end state once per run, over every plugin `enabledPlugins` turns on, and
counts a plugin failed only when it is absent at user scope, present there
without `enabled: true`, or its own directory under `plugins/` changed between
the recorded `gitCommitSha` and HEAD. Cases where the snapshot cannot be
determined at all (no resolvable HEAD, no registry, absent or null recorded
sha, a commit this clone lacks) fail closed with a named reason rather than
reading as healthy. An unreadable `plugin list --json` fails the batch in one
line instead of one warning per plugin.

Verified live on this machine: 72 enabled, 0 newly installed, 0 refreshed,
0 failed, where the previous code reported every refresh as failed.

Adds `.claude/hooks/cloud-bootstrap-plugins.test.sh`, 32 assertions driven by a
stub CLI over synthetic registries, discovered by the existing
`find plugins .claude/hooks` roots with no ci.yml change. It pins the five
end states that previously reported "0 failed", the extraction anchors it
depends on, and the block's exit status, since a nonzero exit there would abort
the whole bootstrap under `set -e`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XGLX1xYgy27JiRqLjoiH8T
…d basename match

R3 and R4 matched a changed file's basename as an unanchored substring of other
files, so a file with no suite of its own could still select many suites and
exit 0. That is not the safe over-selection the header describes, it is
fail-open: the "a changed file that maps to zero suites is an error" contract
silently did not apply. Measured on this tree, a new uncovered hook body
(`utils.sh` under a plugin's hooks/) selected 131 suites at exit 0; it is now
reported UNMAPPED at exit 1. The same hazard is what let `babysit_lease.py`
report five suites, none of them its own, purely because its name is a
substring of `manage_babysit_lease.py`.

A file now names another only when the basename appears bounded on both sides
by a character outside [A-Za-z0-9_.-]. `/` is deliberately outside that class,
so path-qualified mentions, prose mentions and comment mentions all still
count, and a trailing run of `.` is treated as sentence punctuation. A basename
the rule cannot spell falls back to the old substring test rather than to zero
coverage. Matching stays a basename rule because the cross-plugin copy fan-out
depends on it.

Exposing the false coverage revealed a real gap it had been masking: several
Python files whose only suite is `<dir>/tests/test_<stem>.py` were never
reachable by a name match, because those suites say `import <module>` and never
spell the filename. R2 gained that path arm, and four files that were UNMAPPED
now map to the suites that genuinely test them.

Swept over every tracked file: 534 files select fewer suites (15.8% fewer
selected-suite slots), 4 UNMAPPED files became mapped, nothing became unmapped.
25 files dropped to an empty selection, every one a markdown context file whose
basename had been landing inside a longer one, and every one already covered by
a class in scripts/affected-tests-no-suite.txt, so they report as no-suite at
exit 0.

Independently verified: all 190 dropped basename/enclosing-token pairs were
read, only two name the real file (both keep their suites through other bounded
mentions), and a direct-coverage invariant over all 3455 tracked files found no
file missing a suite that genuinely names it. Selection also got faster, since
the frontier is smaller.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XGLX1xYgy27JiRqLjoiH8T
Preserved work in progress, deliberately not proposed for merge. A verifier
found a correctness regression in this state, and the fix was not finished.

What it does: makes check-shell-portability's per-file scan near linear.
mask_quotes() re-walked the entire accumulated logical line once per physical
line, and this script's own awk program is one giant quote-joined record, so
the walk restarted ~1,500 times over a growing 80 KB string; mawk's string
append is itself quadratic, so the two multiplied into worse than quartic. The
walk is made resumable, plus a cached blanks() pad, a split-based
after_last_boundary(), exact cheap pre-filters, a pipe-bounded ladder window
and chunked buffers.

Independently measured: --all 979.4s to 32.0s, this file 68.69s to 1.57s
(43.8x), audit-fleet.sh 175.05s to 0.32s, its own suite 466s to 14s, with
byte-identical stdout, stderr and exit status over the whole corpus in both
--all and --paths modes, and 5,000 differential fuzz cases clean.

KNOWN DEFECT, why this is not merge ready. climit is computed from the record
length including a trailing backslash, but the record loop then strips that
backslash, so a decision committed at len-2 rests on a character that no longer
exists. Only the `$((` vs `$(` branch reads two characters of lookahead, so a
guard ladder written as

    stat -c %s $(\
    (1 | 2)) || stat -f %z f

is clean at HEAD and reports a violation here. POSIX removes a backslash
newline during tokenization, so HEAD matches the shell and this does not. The
divergence is fail closed, no fail open was found, and the shape appears
nowhere in the current corpus, which is why every output diff passed.

Fix shape: compute climit from the effective length, e.g.
`climit = len - 2 - (substr(l, len, 1) == BS)`, or clamp MQ_POS and truncate
MQ_MASK inside the DANGLING_BS branch. Also needed before merge: suite cases
that exercise the resume boundary (9 of 11 mutations to this code pass the
existing 336 assertions untouched) and four header corrections, chiefly that
the residual cost tracks record length rather than hit density.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XGLX1xYgy27JiRqLjoiH8T
Clears the KNOWN DEFECT the previous commit recorded. That commit's message
said the `$((`-across-a-continuation defect was still live in its own tree; it
was not. The one-line fix was already in the file it shipped, undocumented,
unproven and unpinned by any test, so the message was stale about its own
content. This commit supplies the three things that were genuinely missing:
the proof, the suite cases, and a header that stops misstating the cost model.

WHAT THE FIX IS, and why it is the right one. mask_quotes() commits a decision
once it is at column <= len - 2, on the grounds that no branch reads more than
two characters of lookahead. That reasoning holds only for a record that GROWS.
The record loop also SHRINKS it: when a trailing backslash turns out to be a
continuation it deletes column len. Computing climit from the length INCLUDING
that backslash let a decision committed at len - 2 rest on a character the
record no longer had. The single branch with two-character reach is the `$((`
vs `$(` test, so `$` `(` `\` in the last three columns is where it bit: the
arithmetic test failed on the backslash, the shorter `$(` was committed, and
the `(` arriving on the next physical line could no longer promote it. The fix
is to compute the commit point from the EFFECTIVE length,

    climit = len - 2 - (substr(l, len, 1) == BS)

rather than to clamp MQ_POS or truncate MQ_MASK inside the DANGLING_BS branch.
It is one expression at the one place the bound is derived, it needs no second
site to stay in step with the first, and one deletion is all it has to allow
for: for the record loop to strip column len the walk must have reached that
backslash with nothing consuming it, so column len - 1 was not an unconsumed
backslash either, and the shortened record cannot end in one.

PROOF, against origin/main's implementation as the reference.

  - The reproducer

        #!/bin/sh
        stat -c %s $(\
        (1 | 2)) || stat -f %z f

    now matches main exactly: stdout "No unexcused GNU-only constructs in 1
    shell file(s).", empty stderr, exit 0 on both. The control without the
    continuation matches the same way. Reverting climit to `len - 2` puts the
    reproducer back to exit 1 with a PORTABILITY report while leaving the
    control clean, which is what makes the expression load-bearing rather than
    incidental.
  - Whole-corpus equivalence, 1,684 tracked files (every tracked *.sh plus
    every tracked plugins/*/skills/**.md and plugins/*/reference/**.md), both
    implementations, both modes, stdout, stderr and exit status compared:
    identical in `--all` and in an explicit `--paths` list of all 1,684.

WHAT NOW PINS IT. Nine assertions, in a new suite section about the commit
boundary itself; the suite goes 336 to 345, still FAIL=0. Each was proven by
breaking one specific piece of the held state, watching exactly its own case go
red, then restoring the file and confirming the md5 matched byte for byte:

    climit = len - 2                the `$((` split by a continuation
    HELD_CMT = 0                    the inline comment sticky across a join
    HELD_NV = 0                     the ${...} extent reported at line 2, not 1
    HELD_AR[d] = 0                  both arithmetic-depth cases, at two stack
                                    depths (top level, and nested in a `$( )`)

Each of those four mutations survives the previous 336 assertions untouched.
HELD_ST already had cover (a quoted newline hiding an option). HELD_VO is the
one held field no fixture could distinguish, and not for want of trying: every
read of VOPEN[d] is preceded, in the same walk, by the `${` that writes it, so
holding it is defensive rather than observable. It is left in place and said so
here rather than dropped on the strength of one search.

HEADER CORRECTIONS, all four measured on 2026-08-29 rather than recalled.

  - The residue tracks record LENGTH, not hit density. One quote-joined record
    with no hits at all: 1,600 lines 0.11s, 6,400 lines 0.35s, 25,600 lines
    6.58s. The same record held at 6,400 lines with a hit every eighth line
    0.38s and a hit on every line 0.34s, which is noise.
  - "Cost is linear in file length" holds only while the longest logical record
    stays bounded. A file that is one enormous record is still superlinear in
    that record's length, with a much smaller constant.
  - The "three things drove it" list was wrong about which of the supporting
    changes carry weight. Reverting one at a time and re-scanning this file,
    each ablation confirmed output-identical first: blanks() 2.30x,
    after_last_boundary() 1.69x, the chunked buffer 0.99x, the cheap
    pre-filters 0.96x. The two the list omitted are the two that pay.
  - "a single 1,400-line quote-joined record" was stale in both places it
    appeared. The awk program is ~1,580 lines.

Gates: shellcheck --rcfile .shellcheckrc clean, shfmt -d clean, and the gate
run on its own two files clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XGLX1xYgy27JiRqLjoiH8T
…unverified

# Conflicts:
#	scripts/affected-tests.sh
#	scripts/affected-tests.test.sh
@kyle-sexton
kyle-sexton enabled auto-merge (squash) August 29, 2026 04:58
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-08-29T05:05:59.734141Z f759d20 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

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


Code review complete

  • Gather context (PR body, diff, changed files)
  • Understand request (CI code-review lane via /review:code-review)
  • Review scripts/check-shell-portability.sh diff for correctness/maintainability
  • Review scripts/check-shell-portability.test.sh diff for correctness/coverage
  • Post inline findings (none met the bar)
  • Post summary

Scope: CI code-review lane — correctness/maintainability, high-signal bar only (linter/formatter-catchable issues, pre-existing lines, and style nits excluded).

What I checked

This diff replaces the non-resumable mask_quotes() quote/expansion walk with a resumable one (commit/hold/resume state across physical lines) plus four secondary hot-spot rewrites (blanks() doubling pad, after_last_boundary() via split(), chunked buffers in neutralize()/collapse_subs(), and a pipe-bounded match window in is_guarded()'s stat ladder). I traced the trickiest parts by hand against scripts/check-shell-portability.sh:

  • Commit boundary (climit, mq_hold/mq_resume) at check-shell-portability.sh#L645-L831: verified the len - 2 lookahead bound is tight for every branch (the $(( test is the deepest lookahead), that the backslash-adjustment (len - 2 - (substr(l,len,1)==BS)) is at worst conservative (never unsafe) when the trailing backslash turns out not to be a real continuation, and that held state (ARDEPTH, VOPEN, NV, MQ_CMT) is always reinitialized on frame-open, so depth-indexed arrays holding stale values from a different frame type at the same depth can't be misread.
  • Stat-ladder pipe window at check-shell-portability.sh#L1419-L1437: confirmed the new bar/past truncation is equivalent to the old whole-record match(), since every pattern component except the || itself (SEG, PRE, NAME, the quote runs) already excludes a bare |, so a legitimate match can never contain more than the two || characters — truncating at the next | after the first || can't cut a real match short.
  • after_last_boundary() split-based rewrite: verified field semantics match the old backward char-scan, including the empty-string and trailing-boundary edge cases (split()'s empty trailing field mirrors substr(h, i+1) when h ends in a boundary char).
  • blanks() global PAD: no naming collision with other globals in the program; monotonically grows, never returns stale/wrong-length content.

No correctness discrepancies found. The added tests in check-shell-portability.test.sh (the $(( split-by-continuation case, the sticky-inline-comment-across-a-join case, and the two-depth ${…} extent / arithmetic-depth cases) are well-targeted at exactly the boundary conditions the resumable walk introduces, each with a same-shape single-line control case.

Not independently verified

Per this lane's tooling, Bash access here is restricted to gh pr * subcommands — I could not execute scripts/check-shell-portability.test.sh or the awk program directly to reproduce the PR's timing/byte-identical claims. Those (the 336→345 assertion count, the --all/--paths byte-identical runs, the per-ablation timing figures in the rewritten header) are author-claimed and unverified by me; my review is based on static reading of the diff plus the reasoning above.

Findings

None meet this lane's high-signal bar. No inline comments posted.

@claude

claude Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

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


Security review — no findings

  • Gather context (PR diff, changed files)
  • Apply skip gate
  • Review changed files for security-relevant issues
  • Post findings

Skip gate

Did not early-exit — reviewed the diff in full before concluding.

Scope

Both changed files reviewed against origin/main:

Findings

None. This diff is a self-contained algorithmic refactor of internal AWK string-processing helpers (mask_quotes's resumable walk plus mq_hold/mq_resume/mq_reset/mask_full, chunked-buffer builds in neutralize/collapse_subs, a doubling pad in blanks(), a split()-based after_last_boundary(), and two cheap pre-filters in is_negated/status_swallowed), plus matching test fixtures. Checked specifically for:

  • Injection surfaces — no eval, system(), popen, or command substitution was touched or added; the changed functions only manipulate in-memory strings already read from disk, never shell out.
  • Trust-boundary changes — no new external input path; the script still reads only file content it already had access to (--all / --paths), and */vendor/*/*/evals/* exclusion logic is untouched.
  • New unsafe temp-file / test-fixture handling — the added test block (check-shell-portability.test.sh lines 3244+) uses the same mktemp --suffix=.sh pattern already used throughout the file; no predictable paths or unsafe permissions introduced.
  • Availability/DoS regression — the PR's own header documents that cost is now linear in file length but still superlinear in the length of a single longest logical record (an adversarial "one giant record" file). This isn't a new exposure: that worst case existed before this PR (and was far more expensive then — this change strictly reduces it), and CI already excludes vendor//evals/ and runs only changed-file mode, so it doesn't introduce a new attacker-controlled DoS surface.

No CRITICAL, IMPORTANT, or SUGGESTION findings.

@github-actions

Copy link
Copy Markdown

Last security-reviewed head: f759d20c6ea10732061a966d9fac099202fe7004. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

@github-actions

Copy link
Copy Markdown

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

@kyle-sexton
kyle-sexton merged commit 0472f07 into main Aug 29, 2026
60 checks passed
@kyle-sexton
kyle-sexton deleted the wip/portability-perf-unverified branch August 29, 2026 05:07
kyle-sexton added a commit that referenced this pull request Aug 30, 2026
…ed) (#3492)

No linked issue

## Summary

Full-repository `/code-tidying:batch-simplify repo` sweep: 1,154 code
files in 68 dependency-ordered groups across 12 waves, each group read
end to end by a dedicated simplifier agent and every non-empty diff
adversarially checked by a fresh-context refutation verifier. Only
behavior-preserving changes land; deferred findings are being worked in
this same branch rather than filed as issues. This PR grows by one
commit per wave; this body's Verification section is updated as waves
land.

## Fix

Wave 1 (root config, `lib/` SSOT, `scripts/`; 98 files read, 5 changed):

- `scripts/affected-tests.sh`: `lang_family()` converted from
command-substitution return to a `LANG_FAMILY` global (one fork saved
per frontier path and per reverse-lookup hit, per the header's Windows
spawn budget); single-use `invoke_print_manifest` wrapper inlined.
Selection proven byte-identical old-vs-new across ten seed scenarios.
- `scripts/check-fleet-finding-test-coverage.sh`: baseline temp file
replaced by an ordered in-memory array (same order/duplicate semantics,
one fewer mktemp + trap arg).
- `scripts/check-orphaned-fixtures.sh`: nested two-`find` walk collapsed
to one `find -print0 | sort -z` pipeline; live-tree discovery
byte-identical (363 entries). Only delta: a synthetic `evals/fixtures`
nested inside another fixtures tree was double-reported by the old code
and is reported once now.
- `scripts/check-plugin-catalog-enablement.sh`: header env-override list
gains the missing `PLUGIN_CATALOG_ENABLEMENT_BOOTSTRAP` entry
(comment-only).
- `lib/hook-utils.test.sh`: seven stale `hook-utils.sh:<line>` comment
references refreshed (stale since #3463; comments only).

Waves 2–12 and a deferred-work pass follow on this branch.

## Verification

- shellcheck (repo `.shellcheckrc`) clean on every touched file.
- `scripts/affected-tests.sh --run` over all five changed files: 124
selected shell suites pass; the seven delegated Python suites run
directly, all pass except
`plugins/disk-hygiene/lib/test_hook_telemetry.py`, whose
`test_absolute_sink_used_as_is` races a fire-and-forget subprocess and
fails at HEAD too on fast machines (untouched by this wave; robustness
fix queued for the disk-hygiene wave in this branch).
- All four `lib/` sync clusters verified drift-free (`scripts/sync-*.sh
--check`).
- Per-group fresh-context refutation verifiers: NOT-REFUTED for the
`affected-tests.sh` and `lib/hook-utils.test.sh` diffs (differential
runs byte-identical); the orphaned-fixtures nested-double-report delta
above was surfaced by its verifier and accepted as strictly more
correct.

## Related

Refs #3486 (prior repo-wide simplify sweep this run re-verifies), #3463
(source of the stale test-comment line numbers), #3488 (recent
shell-portability perf work in the same scripts area).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01W61wikkiEK5StWgg9rfEQW

---
_Generated by [Claude
Code](https://claude.ai/code/session_01W61wikkiEK5StWgg9rfEQW)_

---------

Co-authored-by: Claude <noreply@anthropic.com>
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.

2 participants