Skip to content

refactor(scripts): define the list-file format once, and close a third twin fail-open - #3181

Merged
kyle-sexton merged 1 commit into
mainfrom
claude/work-items-integration-3cf72i
Aug 23, 2026
Merged

refactor(scripts): define the list-file format once, and close a third twin fail-open#3181
kyle-sexton merged 1 commit into
mainfrom
claude/work-items-integration-3cf72i

Conversation

@claude

@claude claude Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Closes #3161

Summary

Route-lane finding 7 of 7 from the /coupling:reduce dry-run over scripts/ (#2914). Extracts the list-file format into scripts/lib/read-list.sh — and, while migrating, found and fixed a live fail-open in check-skill-portability.sh.

Two things differ from the filed finding, both because scoping it turned up more than the ledger recorded. Details below.

Fix

The premise needed correcting: two formats, not one

The finding described "one # comment/blank-stripped line format, five bespoke parsers". There are eight parsers in four spellings across two genuinely different semantics:

Rule Spelling Sites
inline# anywhere starts a comment sed -E 's/#.*//' check-docs-only, check-orphaned-fixtures, check-shell-portability (skill-md baseline)
inline bash ${line%%#*} check-changelog-parity, affected-tests
leading# comments only at line start awk FNR == NR check-shell-portability, check-skill-portability (token lists)
leading (+ CR strip) bash check-hook-userconfig-argv

leading is not a bug. Token-list entries are EREs, and a regex may legitimately contain #; applying the inline rule to a token list truncates such a pattern, and a truncated or emptied pattern is a gate enforcing less than it reports. A single read_active_lines would therefore have been actively wrong. --comments is required with no default — the same call-site-explicit shape #3144 used for --include-deleted.

Measured before changing anything: no active line in any of the fifteen data files carries a non-leading #, so the divergence was latent, not live. Stated plainly rather than oversold — the value here is that the two semantics currently exist only inside their parsers.

check-hook-userconfig-argv.sh turned out to disagree with itself: allowed() grep-matched raw lines with no comment stripping and no trimming (so an entry with trailing whitespace silently never matched), while the stale-entry guard below it skipped leading-# lines and stripped a CR. The allowlist is now read once and both consumers share the result.

A third twin fail-open, found while migrating

check-shell-portability.sh has refused an empty active pattern set since #1513 (np == 0 && ncls == 0 → exit 2). check-skill-portability.sh had no such guard. Measured, with a discriminating control on the same fixture:

                          real tokens    all-comments token list
check-skill-portability   exit 1 ✓       exit 0   <-- gates nothing
check-shell-portability   exit 1 ✓       exit 2 ✓

That is the #1513 shape a third time — after the awk-operand guard (#1513) and the token-list operand guard (#2914 finding 2): a protection added to one twin and never propagated to the other. Resolving the list in the shell puts the guard in one place for both.

Scope

scripts/lib/read-list.sh is sourced-only, returns through a nameref, and prefixes every internal local _rl_ — the correctness requirement #3144 measured, not a naming style. All eight parsers migrated. Both awk loaders now receive a pre-filtered list and keep only their scanner-specific logic (check-shell-portability's !class dispatch).

Deliberately no escape syntax: a \# escape in inline mode was considered and rejected — no current file needs it, adding it would silently change how an existing entry parses, and the leading mode already covers "the # is data". Recorded in the file, per the issue's acceptance criteria.

Verification

The fail-open, before and after — same fixture, discriminating control:

before:  check-skill-portability  real=1  empty-list=0   <-- silent pass
after:   check-skill-portability  real=1  empty-list=2

New suitescripts/lib/read-list.test.sh, 29 assertions, wired into the plugin-gate lane. The two modes are each asserted against the other mode's answer on the same input, so a change that quietly collapses them fails rather than passing:

leading: a trailing-# entry survives intact        a#  ->  a#
inline:  the same entry is TRUNCATED               a#  ->  a

It also asserts the nameref hazard (an out-array named file, mode, line, or out), loud failure on an unreadable file, and — as a liveness check — that the real shipped token list and docs-only allowlist still yield non-empty results under the mode their consumer uses.

Both portability suites gained an empty-token-list regression test, the skill-portability one with a control proving the fixture really does flag under a real list.

Three suites gained the stage_libs fixture helper they now need; three existing ones stage the new lib. That consequence was predicted in #3160 when these items were filed.

647 assertions green:

Suite Result
check-shell-portability.test.sh PASS=335 FAIL=0
check-skill-portability.test.sh PASS=92 FAIL=0
check-changelog-parity.test.sh PASS=82 FAIL=0
affected-tests.test.sh PASS=44 FAIL=0
scripts/lib/read-list.test.sh (new) passed: 29
check-docs-only.test.sh PASS=21 FAIL=0
scripts/lib/changed-files.test.sh passed: 19
check-orphaned-fixtures.test.sh PASS=13 FAIL=0
check-changed-skills.test.sh PASS=13 FAIL=0
check-hook-userconfig-argv.test.sh rc=0

shellcheck --rcfile=.shellcheckrc -x, shfmt -d, actionlint, typos, editorconfig-checker all clean on the staged diff. Every gate modified here was run against its own diff. The exec-bit gate was replicated repo-wide before pushing (0 violations) — it caught me on the previous PR.

No plugin manifest is touched, so no version bump or CHANGELOG entry applies; --check-bump confirms.

Related


Generated by Claude Code

…d twin fail-open

Route-lane finding 7 of 7 from the /coupling:reduce dry-run over scripts/
(#2914, tracked as #3161).

The premise needed correcting first

The finding described "one line format, five bespoke parsers". Scoping it found
EIGHT parsers in FOUR spellings across TWO genuinely different semantics, so a
single read_active_lines would have been actively wrong:

  inline   `sed -E 's/#.*//'` (check-docs-only, check-orphaned-fixtures,
           check-shell-portability's skill-md baseline) and the equivalent bash
           `${line%%#*}` (check-changelog-parity, affected-tests).
           A `#` ANYWHERE starts a comment.
  leading  the awk `FNR == NR` loaders in both portability scanners, plus
           check-hook-userconfig-argv. A `#` comments only at line start.

`leading` is not a bug. Token-list entries are EREs and a regex may legitimately
contain `#`; applying the inline rule to a token list truncates such a pattern,
and a truncated or emptied pattern is a gate enforcing less than it reports. So
`--comments` is required and has no default, the same call-site-explicit shape
#3144 used for `--include-deleted`. Measured before the change: no active line
in any of the fifteen data files carries a non-leading `#`, so the divergence
was LATENT, not live. This makes it stay that way.

check-hook-userconfig-argv.sh turned out to disagree with ITSELF: `allowed()`
grep-matched raw lines with no comment stripping and no trimming, while the
stale-entry guard below it skipped leading-`#` lines and stripped a CR. The
allowlist is now read once and both consumers share the result.

The third twin fail-open, found while migrating and fixed here

check-shell-portability.sh has refused an empty active pattern set since #1513
(`np == 0 && ncls == 0` -> exit 2). check-skill-portability.sh had no such
guard. Measured, with a discriminating control on the same fixture:

  check-skill-portability   real tokens=1 (caught)   all-comments list=0  <-- gates nothing
  check-shell-portability   real tokens=1 (caught)   all-comments list=2

That is the #1513 shape a third time, after the awk-operand guard (#1513) and
the token-list operand guard (#2914 finding 2): a protection added to one twin
and never propagated. Resolving the list in the shell puts the guard in one
place for both. Both suites now assert it, the skill-portability one with a
control proving the fixture really does flag under a real list.

Scope

scripts/lib/read-list.sh, sourced-only, nameref out-array, every internal local
`_rl_`-prefixed for the correctness reason #3144 measured. All eight parsers
migrated. Both awk loaders now receive a pre-filtered list and keep only their
scanner-specific logic (check-shell-portability's `!class` dispatch).

Tests

New scripts/lib/read-list.test.sh, 29 assertions, wired into the plugin-gate
lane. The two modes are each asserted against the OTHER mode's answer on the
same input, so a change that quietly collapses them fails rather than passing.
Three suites gained the stage_libs fixture helper they now need; three existing
ones stage the new lib.

647 assertions green: 335 + 92 + 82 + 44 + 29 + 21 + 19 + 13 + 13. shellcheck
(repo rcfile), shfmt, actionlint, typos, editorconfig clean; every gate touched
here run against its own diff; exec-bit gate replicated repo-wide.

Refs #3161
Refs #2914

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q6Xm8Swd6ELYid3fiDCRq3
@kyle-sexton

Copy link
Copy Markdown
Contributor

🔒 Lane claim (melo-lap-001-merge-20260823T045045Z, autopilot, PR-queue coordinator): checking readiness and merge-gate now. Head pinned; will yield if it moves. No recent foreign activity observed.

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.

scripts/lib: define the comment-stripped list format once in read-list.sh

2 participants