perf(skill-quality): measure the listing budget in one awk pass, byte-identical and ~100x faster

[kyle-sexton]

Summary

bash plugins/skill-quality/scripts/check-listing-budget.sh plugins/*/skills — verbatim what
#2023's procedure and .github/recurring-schedule.json's listing-budget-watch row tell an
operator to run each cycle — took 289s on Windows (Git Bash) and, run in the foreground by an
agent, was killed at 180s with exit 143 and zero output. On the one machine where that quarterly
routine is actually driven, a report-only drift watch silently produced nothing.

The cause is process-spawn cost, not the machine. The per-file loop spent at least eleven forked
subshells and five external process execs (4× awk, 1× tr) on every one of the repo's ~200
SKILL.md files — on the order of 2,000 spawns. Process creation costs roughly two orders of
magnitude more on Windows than on Linux, which is why CI (ubuntu-24.04) never surfaced it.

The measurement is now one awk pass. Same command, same tree: 232s → 2.2s, output
byte-identical.

The byte-identical claim, and how it was actually checked

Diffing the two reports is not sufficient — two files with offsetting extraction errors produce a
matching aggregate, a matching entry count, and a matching top-10, so the report diff comes back
clean while the parser is broken. Both implementations were therefore instrumented to dump their
CONTRIB_FILE (the per-file entry_len ⇥ skill ⇥ root rows) and those were diffed first:

$ diff baseline.contrib new.contrib && echo "IDENTICAL TRIPLES ($(wc -l < new.contrib) rows)"
IDENTICAL TRIPLES (144 rows)

$ diff baseline.out new.out && echo "IDENTICAL REPORT"
IDENTICAL REPORT

144/144 per-file rows identical, aggregate 94,468 identical, top-10 ordering identical. Emission
order into the contribution file is preserved by construction (roots in argv order, glob order per
root), so sort -t $'\t' -k1,1nr's tie-breaking cannot shift.

A port, not a rewrite

The awk program reimplements, behaviour for behaviour, the four helpers the loop shelled out to:
skill_frontmatter::extract; ::field — including block-scalar unfolding for | and > and the
quote-aware trailing-comment strip with its doubled-single-quote case; ::strip_quotes (ONE outer
layer, double or single, never both, and never a lone quote character); and
normalize_bool / trim_ws, whose deliberate refusal to fold YAML 1.1's yes/on aliases is
carried across with its reasoning.

Two behaviours the old pipeline got for free from command substitution are reproduced explicitly
and commented as such, because they were load-bearing rather than incidental:

• fm="$(…extract…)" stripped trailing newlines from the extracted frontmatter, so a trailing
  blank line could never append a separator inside a block scalar — and an all-blank block counted
  as no frontmatter at all.
• "$(…field…)" stripped trailing newlines from each field's value. A folded (>) scalar joins
  with spaces, which command substitution does not strip, so only newlines are removed.

The file list reaches awk on stdin, not as operands: awk reads an operand containing = as a
variable assignment, so a path with = in it would be silently swallowed instead of read. The root
travels alongside each path rather than being re-derived from it, because the report prints the root
string the caller passed and a derivation would have to reproduce its exact spelling to stay
byte-identical.

skill-frontmatter.sh is no longer sourced here — its helpers are the per-call execs that caused
this. The library file itself is unchanged and check-skill.sh remains its consumer.

No flag, no option, and no output format changed. skill-quality 0.15.2 → 0.15.3 (patch —
byte-identical output is the reason).

The locale trap, handled rather than hoped past

The pre-port script measured with bash ${#var}; the port measures with awk length(). Both are
locale-dependent and they can disagree — mawk ignores the locale and counts bytes while gawk does
not. A fixture asserting a hardcoded character count would encode one environment's answer and
fail elsewhere for the wrong reason, so case 18 asserts an equivalence: the script's reported
aggregate for a multibyte description must equal ${#UTF8_DESC} computed in the same shell. A
divergence is then reported as exactly what it is. On this machine both say 5
(GNU Awk 5.4.0, LANG=en_US.UTF-8).

Test plan

Five new cases; the full suite:

$ bash plugins/skill-quality/scripts/check-listing-budget.test.sh
...
ok   - a trailing YAML comment is excluded from the measured description
ok   - a plain scalar drops its trailing comment but keeps a non-comment #
ok   - a # inside a block scalar is content and is still measured
ok   - a literal block scalar joins with newlines and drops the trailing blank
ok   - a doubled single quote is content; the trailing comment is still cut
ok   - multibyte description measures the same as the shell's own ${#var} (5 chars)
ok   - 200-file corpus measured in 1s (bound: 30s)
all assertions passed

Byte-identity over the real tree, both implementations run over the same worktree:

pre-port  (merge base) : rc=0 elapsed_ms=232534   144 skills, aggregate 94468
single-pass            : rc=0 elapsed_ms=2180     144 skills, aggregate 94468
report diff            : empty
per-file triple diff   : empty (144 rows)

The wall-clock bound discriminates — measured, not assumed. Same 200-file fixture corpus the test
builds, both implementations:

PRE-PORT   : 194s over 200 files
SINGLE-PASS:   1s over 200 files
bound in the test: 30s
--- identical output over the fixture corpus? ---
YES - byte-identical

The 30s bound is deliberately very loose against a ~1s target: this runs in required CI, and a tight
timing assertion is a flaky gate — worse than the defect it guards. It fails only on a return to
per-file process spawning, which is two orders of magnitude away.

Lint:

$ shellcheck -x plugins/skill-quality/scripts/check-listing-budget.sh \
    plugins/skill-quality/scripts/check-listing-budget.test.sh
(no output)

$ bash scripts/check-shell-portability.sh $(git merge-base origin/main HEAD)
No unexcused GNU-only constructs in 2 shell file(s).

This change reads the same files the script already read and writes only its own temp files — no new
external read or write, no new grant, no hook. No security review note applies.

Related

• Closes perf(skill-quality): check-listing-budget.sh takes 289s over plugins/*/skills on Windows, so #2023's own cycle command cannot complete in the foreground #2216
• [Maintenance] Listing-budget watch: report the shared aggregate and drift #2023 / .github/recurring-schedule.json listing-budget-watch — the routine whose own command
  this makes runnable. Neither carries a "run detached" note that this would make stale (checked).
• Adjacent, not duplicate: ci: no Windows runner anywhere — green checks carry no signal for platform-sensitive shell changes #1414 (no Windows CI runner — why this was never caught), guardrails: recover the PreToolUse spawn-reduction work from the closed #1385 #1403 (same
  spawn-cost class, different plugin and file)
• Inbox item: 20260810-225907-melodic-software-skill-listing-budget-overflow
  (ledger I4-listing-budget.md § I4-V1; adjudication RECONCILE.md § AD-6)

[cursor]

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 finished @kyle-sexton's task in 2m 0s —— View job

---

Security review complete

• Gather context (PR diff, changed files)
• Review check-listing-budget.sh for injection / unsafe handling
• Review check-listing-budget.test.sh
• Review plugin.json / CHANGELOG.md for permission-widening or supply-chain changes
• Post final findings

No security issues found. This PR is a behavior-preserving performance refactor of a local, report-only CLI script (bash loop → single awk pass). It has no network calls, no secrets/credentials, no GitHub Actions changes, and no permission changes (plugin.json is a version bump only, 0.15.2 → 0.15.3).

What I checked specifically

• Command/argument injection: The rewrite actually removes the injection-relevant surface rather than adding one — the old code shelled out to awk/tr per file (~11 forked subshells + 5 execs per SKILL.md); the new code has zero system()/exec-equivalent calls inside the awk program. File paths are transported as data lines in a tab-separated temp file ($FILE_LIST, created via mktemp) that is passed to awk as a single filename operand — see check-listing-budget.sh#L353. This deliberately avoids the documented risk of a path containing = being swallowed as an awk variable assignment if passed as a command-line operand directly.
• Path handling: getline ln < path (check-listing-budget.sh#L338) opens path via a direct syscall inside awk — no shell interpolation, so no path/command injection even though path is glob-derived from repo content.
• Temp files: Both CONTRIB_FILE and FILE_LIST are created with mktemp/mktemp -d (safe, non-predictable, race-free) and cleaned up via trap ... EXIT. No sensitive data involved.
• Trust boundary: The script only reads files under caller-specified roots inside the repo checkout (same boundary as before the port) and never evaluates file contents as code — frontmatter is parsed as text with bounded, non-backtracking POSIX awk regexes (no ReDoS concern).
• Supply chain / CI / permissions: No workflow files, dependency pins, or plugin.json permission fields touched — only the version string and changelog prose changed.

One non-security robustness note (not reported as a finding per the review scope): the new tab/newline-delimited transport between bash and awk (SKILL_ENTRIES → $FILE_LIST) would misparse a skill directory name that itself contained a literal tab or newline byte, since those are used as the field/record separators. This can't cross a privilege boundary (it requires already controlling a file inside the trusted checkout, same as the pre-port code) and at worst produces a mis-attributed report row in this report-only, exit-0 tool — so it doesn't rise to a security finding, but may be worth a fixture if the authors want to harden it further.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a8335a11f1

ℹ️ 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 6m 18s —— View job

---

Review complete

• Gather context (diff, CLAUDE.md, REVIEW.md)
• Review check-listing-budget.sh awk port for correctness
• Review check-listing-budget.test.sh new test cases
• Review CHANGELOG.md / plugin.json version bump
• Post findings as inline comments + summary

Environment note: repo CI/test execution (the sandbox in this review lane) requires interactive approval for running shell scripts, git init, etc., and none was available here — so I could not execute check-listing-budget.test.sh or the script itself to independently confirm the PR's "byte-identical" / timing claims. Everything below is from manually reading and hand-tracing the diff against the unchanged skill-frontmatter.sh helpers it replaces (line by line, including the block-scalar join, quote-stripping, and comment-stripping paths) — not from running the code. Treat the PR's own Test Plan output as author-claimed and unverified by me.

What I checked

I traced the new single-pass awk program in plugins/skill-quality/scripts/check-listing-budget.sh (lines 130–422) against the four bash/awk helpers it replaces in skill-frontmatter.sh (extract, field, strip_quotes, and the removed normalize_bool/trim_ws), for several representative cases:

• Frontmatter fence detection (must be line 1; second --- ends the block) — matches.
• Trailing-blank-line stripping via the while (FN > 0 && FM[FN] == "") FN-- loop, reproducing what command substitution used to strip for free — matches, including the "all-blank frontmatter counts as none" case.
• Literal (|) vs folded (>) block-scalar joining, including a blank line mid-block before a following key — hand-traced against the original getline-based loop and got identical output for the description in test 16 ("abc\nde", 6 chars).
• The quote-aware trailing-comment strip and its doubled-single-quote handling (test 17: 'it''s ok' → 8 chars after strip_quotes) — matches; note this doesn't collapse '' to a literal ' in either the old or new implementation, so this is pre-existing, unchanged behavior, not a port regression.
• getline ln < path (not bare getline) is used for reading each SKILL.md, correctly avoiding clobbering the outer record's $1/$2 (root/path) — a detail that would have been an easy mistake.
• > out inside the awk program is a single open+append stream for the life of the process (not per-record truncation), and close(out)/close(path) are both present, so CONTRIB_FILE is fully flushed before the shell reads it afterward — correct.

I didn't find a functional/correctness bug in the port itself. Two inline nits, both cosmetic/low severity:

• check-listing-budget.sh:407 — a comment lost its apostrophe ("check 2 own FAIL") working around the single-quoted awk program; reads as a typo.
• check-listing-budget.sh:400 — strip_trailing_nl() around the disable-model-invocation field is redundant given normalize_bool()'s own trim_ws() already strips trailing whitespace/newlines; harmless, just a needless call on every scanned file.

Also confirmed:

• Temp-file lifecycle is correct: both CONTRIB_FILE and the new FILE_LIST are cleaned up via the trap ... EXIT at line 224.
• No output-format/report code was touched (lines 435–459 are untouched by the diff), consistent with the "no flag, no output format changed" claim.
• plugin.json version bump (0.15.2 → 0.15.3) and the CHANGELOG entry are consistent with a patch-level, behavior-preserving perf fix.
• Per REVIEW.md's lane split, this repo has a security lane, so I left security-scoped concerns (there weren't any of note here — no new external input reaches a shell, getline ... < path is awk's own file I/O, not shell-interpreted) to that lane.

Nothing here blocks merging; the two nits above are optional polish.

[github-actions]

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