fix(skill-quality): stop check 21 passing silently under mawk (0.17.1)

[kyle-sexton]

Closes #3005

Summary

Check 21 (fresh-eyes declaration conformance) silently passed every file it was given on any machine
whose awk is mawk — the Ubuntu/Debian default. Its embedded awk program used ERE interval
expressions, and mawk aborts the program before it emits a single record, so malformed
fresh-eyes-exempt directives that should FAIL were reported as a clean run. Silent-pass is the worst
shape a gate can fail in, and the plugin advertises running "against any repo", so this made that
claim false for a large share of consumers.

Fix

The issue attributed the break to mawk not implementing ERE intervals. That turned out to be the wrong
diagnosis for the container in question, and the correction narrowed the fix. awk there is mawk
1.3.4 20240123, which does implement intervals. Isolating each construct:

Pattern	Result
^ {0,3}> ? — interval + literal	compiles
^ {0,3}[abc] — interval + bracket class	compiles
^ {0,3}[0-9]{1,9} — interval + interval	compiles
^ *([-*+]) — star + group	compiles
^ ? ? ?([-*+]) — optionals + group	compiles
^ {0,3}([-*+])[ \t]+ — interval + group	panics

The trigger is specific: an ERE interval immediately followed by a parenthesized group panics mawk
1.3.4's regex compiler with REcompile() - panic: values still on machine stack. The panic is fatal,
which is the silent-pass mechanism. Sweeping every .sh file in the repo for that shape finds exactly
two sites, both in this scanner — the list-marker strip and fence detection — which is why check 23,
written interval-free in #2963, was unaffected.

The scanner is now interval-free throughout, not just at the two panicking sites. The two
surviving {0,3} sites compiled fine under 1.3.4, but mawk 1.3.3 implements no intervals at all
and matches the braces as literal text, degrading exactly the same way — a second silent failure mode
for the same regexes on an older but still widely deployed mawk. The rewrite preserves the exact
CommonMark bounds already enforced: the three-space indent cap becomes three optional spaces, the
ordered-marker digit cap one digit plus eight optional ones. Both bounds were verified to still reject
out-of-range input (4 spaces does not match; a 10-digit marker is not stripped).

The two alternatives the issue floated at triage were declined deliberately. A loud gawk required
exit would make the "runs against any repo" claim false on stock Debian/Ubuntu rather than fixing it,
and command -v gawk || awk leaves the silent-pass path live wherever gawk is absent.

The portability guard

A source-level guard assertion fails the suite if an interval returns to any awk-consumed regex. It is
deliberately implementation-aware rather than black-box: gawk compiles intervals happily, so a
reintroduced interval would pass the entire suite on the CI runner and break only for consumers. No
behavioral assertion can observe this class of break on gawk.

Two properties of the guard came out of the review cycle (see the resolved threads):

• All three interval forms are rejected, {n} included, not just the comma forms. mawk panics on
  a{3}(b) exactly as it does on a{0,3}(b) — verified directly — so a comma-only pattern would have
  left the same silent-failure class open.
• It is scoped to what awk actually compiles, not to the whole file. Bash is not mawk, and
  check-skill.sh legitimately uses an interval in a [[ =~ ]] regex to date a frontmatter synced:
  value; an unscoped broadening would have failed the suite on correct code. The two surfaces that do
  reach awk are covered: slash-delimited regex literals inside the embedded programs, and
  FRESH_EYES_JUDGE_RE, which is a POSIX ERE awk compiles via -v but which never appears between
  slashes and would be missed by a literal-only or line-range scan.

Verification

Run in the affected environment (awk → /usr/bin/mawk, mawk 1.3.4 20240123):

• Premise reproduced on origin/main @ d9f15d4f first — check-skill.test.sh failed 21
  assertions, all Check 21, emitting the REcompile() panic.
• check-skill.test.sh: 96 → 118 passing, 0 failures, 0 panics. Diffing the baseline and
  post-fix ok sets shows exactly +21 newly passing from the scanner fix, zero assertions that
  regressed from passing to failing, plus the new guard assertion.
• Guard proven to fail, not just to pass. Reintroducing the original bug drove the suite to 22
  failures — the 21 restored panics plus the guard naming the file and line. After the review
  hardening, each of {n}, {n,} and {n,m} was reintroduced in turn and the guard caught every
  one, while the clean tree still passes with the Bash date regex untouched. Tree restored after each
  probe.
• scripts/check-changed-skills.test.sh: PASS=13 FAIL=0 — the other suite that exercises
  check-skill.sh, whose fixtures have broken on a checker change before.
• Behavioral sanity on real skills — the fixed checker now genuinely emits fresh-eyes records
  under mawk (previously none at all), and skill-quality/skills/check, work-items/skills/work
  and source-control/skills/pull-request all still PASS with 0 errors.
• Full local gate, all green: validate-plugins.sh; generate-catalog.mjs --check and
  generate-cheatsheet.mjs --check; check-changelog-parity.sh in all four modes, including
  --check-preserved origin/main (36 headings compared, all preserved) and --check-bump origin/main; check-changed-skills.sh origin/main; both portability lints; markdownlint-cli2 on
  the touched CHANGELOG (0 issues); typos on the touched plugin (clean); shellcheck on both
  touched scripts (clean).
• Version collision re-checked against origin/main immediately before each push — main is on
  skill-quality 0.17.0, so 0.17.1 is uncontested.

These counts are reproducible from a clean checkout on any mawk box with
bash plugins/skill-quality/scripts/check-skill.test.sh.

Related

• Refs skill-quality: add completion-criteria check criterion + pointer to write-for-agents doctrine #2963 — established the interval-free scanner shape in check 23 that this change adopts for
  check 21.
• Refs docs(upstream): AI Hero course journey roadmap — remaining lanes and filed follow-ons, in order #2994 — the AI Hero course journey roadmap this queue item belongs to.
• Observation for a possible follow-up, deliberately not changed here: scripts/check-changelog-parity.sh:195
  and plugins/markdown-format/hooks/markdown-format.test.sh:72 also use ERE intervals in awk. Both
  were verified to compile cleanly under mawk 1.3.4 (they precede literals and classes, not groups),
  so neither is broken today, but both would degrade on mawk 1.3.3. They sit in a different plugin
  and a repo-level script, each needing its own version bump, so folding them in would widen this
  diff past the issue's scope.

---

🤖 Generated with Claude Code

https://claude.ai/code/session_01QbfCrj3X9FfGL7VRZYmrn4

[claude]

Claude finished @kyle-sexton's task in 50s —— View job

---

Security review — no issues found

• Gather context (PR diff, changed files)
• Run skip-gate check
• Review diff for security-relevant surface
• Post findings

Skip gate: This PR has no security-relevant surface. All four changed files (plugin.json, CHANGELOG.md, check-skill.sh, check-skill.test.sh) are: a version bump, a changelog entry, and a rewrite of embedded ERE regexes in check-skill.sh's fresh-eyes scanner ({0,3} → ? ? ?, [0-9]{1,9} → repeated [0-9]?) plus a matching source-level guard assertion in the test suite.

Verified there's no injection surface introduced:

• The awk program is a fixed literal script; the only externalized values (FRESH_EYES_PROXIMITY_LINES, FRESH_EYES_JUDGE_RE) are passed via awk -v, not string-interpolated into the program text, both before and after this change.
• The file being scanned (fe_file) is passed as an awk positional argument, never spliced into the script.
• The regex rewrites are behavior-preserving pattern substitutions (interval quantifiers → equivalent optional-repetition chains) with no change to what data flows where or how it's later used — this is a fault-tolerance/portability fix (mawk panic → silent pass), not a change to trust boundaries, authz, secret handling, or any GitHub Actions workflow.
• The new test assertion (grep -nE guard) only greps the checker's own source file for a forbidden syntax shape; no untrusted input reaches it.

No CRITICAL, IMPORTANT, or SUGGESTION findings.

[github-actions]

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c84cb36917

ℹ️ 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 2m 46s —— View job

---

Code review

• Gather context (PR body, REVIEW.md, workflow files)
• Review diff (plugin.json, CHANGELOG.md, check-skill.sh, check-skill.test.sh)
• Post findings

This repo carries a claude-security-review.yml workflow, so this lane (code-review) is scoped to correctness/maintainability only; security is left to that lane.

Regex-conversion correctness (verified by hand-tracing, not by running the suite — I don't have test-execution access in this lane):

• ^ {0,3}> ? → ^ ? ? ?> ?: equivalent — three independently-optional literal spaces before > covers the same 0–3 range as {0,3}, including the boundary case (a 4-space-indented > correctly fails to match either way).
• [0-9]{1,9} → [0-9][0-9]?[0-9]?[0-9]?[0-9]?[0-9]?[0-9]?[0-9]?[0-9]?: 1 required + 8 optional digits = 1–9 digits, matching the original bound.
• ^ {0,3}(```+|~~~+) → ^ ? ? ?(```+|~~~+): same equivalence as the first case.

One finding posted inline on check-skill.test.sh:2988 (Suggestion, non-blocking): the new portability guard's regex only catches comma-form ERE intervals ({n,m}), so a bare exact-count interval like {9} could reintroduce the same mawk-panic class undetected, while the guard's pass message claims the source is fully "free of ERE interval expressions." Widening the regex naively would false-positive on the unrelated bash-level date regex at check-skill.sh:703, so a real fix needs to scope the check to the awk program's line range — noted for awareness, not required for this PR.

Everything else checked out:

• check-skill.sh:703's own {4}/{2} date regex is bash's [[ =~ ]] engine, not awk, so it's correctly untouched.
• CHANGELOG entry and 0.17.0 → 0.17.1 patch bump are consistent with the fix's scope and the plugin's existing changelog format.
• The PR's own scope note (declining to touch check-changelog-parity.sh and markdown-format.test.sh, which use intervals that compile under mawk 1.3.4 but would degrade under 1.3.3) is a reasonable, explicitly-documented boundary — not something this diff needed to absorb.

No other correctness issues found. Test-run counts and CI results in the PR description are author-reported and were not independently re-executed here.

[claude]

Claude finished @kyle-sexton's task in 52s —— View job

---

Security review — no issues found

• Gather context (PR diff since last-reviewed head)
• Run skip-gate check
• Review diff for security-relevant surface
• Post findings

Skip gate: No security-relevant surface in the commits since the last-reviewed head (c84cb36). This push (8f464e2) only scopes the mawk-portability guard to awk-consumed regexes and adds coverage for exact-count {n} intervals, responding to the Codex review comment on the prior revision, plus the matching CHANGELOG update. check-skill.sh itself is unchanged in this push.

Verified no new risk was introduced:

• Both grep -nE patterns only scan the checker's own source file ($SUT, a fixed local path) for a forbidden syntax shape — no untrusted input reaches them, and no new externalized/interpolated value was added.
• The interval regex (\{[0-9]+(,[0-9]*)?\}) and the two scoping greps are static literals; nothing here touches trust boundaries, secrets, or workflow permissions.

No CRITICAL, IMPORTANT, or SUGGESTION findings.

[github-actions]

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

[claude]

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

---

Code review — /review:code-review

• Gather context (PR diff, changed files)
• Run skip-gate check
• Review diff for correctness/maintainability
• Post findings

Skip gate: PR is open, not draft, and the diff (regex rewrite + new source-level guard) has real review surface — proceeded with a full review.

Scope: correctness and maintainability of the two commits currently on this branch (c84cb36 → 8f464e2), which together rewrite check-skill.sh's fresh-eyes awk program to be interval-free and add a source-level portability guard in check-skill.test.sh. Security is out of scope for this lane (already cleared by the security-review lane on c84cb36).

What I checked

• Manually traced each rewritten regex against its {m,n} original for behavioral equivalence (not just re-stated the PR's own claims):
  • ^ {0,3}> ? → ^ ? ? ?> ? (blockquote depth, check-skill.sh:1039)
  • [0-9]{1,9}[.)] → [0-9][0-9]?×8[.)] (ordered-marker digit cap, check-skill.sh:1093)
  • ^ {0,3}(```+|~~~+) → ^ ? ? ?(```+|~~~+) (fence detection, check-skill.sh:1104)
  • All three: rejecting a 4-space indent / 10-digit marker still fails to match at position 0 the same way the original {0,3}/{1,9} did (POSIX leftmost-longest, no backtracking surprises) — the CommonMark bounds are preserved, not just visually similar.
• Verified the new guard's scope is correct, not just present: swept every awk invocation in check-skill.sh (lines 702, 1021, 1299) and confirmed none outside the fresh-eyes program contain an interval today, and that the guard's two grep surfaces (slash-delimited regex literals + FRESH_EYES_JUDGE_RE=...) would catch a reintroduced interval in any of them.
• Confirmed the guard correctly leaves check-skill.sh:703's Bash [[ "$SYNCED_VAL" =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}$ ]] alone (neither grep surface matches it — no slashes, not FRESH_EYES_JUDGE_RE), which is the intended awk-only scoping, not a gap.
• Confirmed this commit already addresses the Codex P2 finding on the prior commit ({n} exact-count intervals are now covered — INTERVAL_RE='\{[0-9]+(,[0-9]*)?\}' matches with or without a comma).
• Checked the new guard's own regex for false-positive risk against the comment prose it's embedded next to (which itself contains literal {0,3} / {1,9} text) — the comment lines have only one / each, so the guard's two-slash-bracketing pattern can't match them, and the comment-line exclusion (grep -vE '^[0-9]+:[[:space:]]*#') would filter them either way.
• CHANGELOG entry and version bump are consistent with repo convention (Keep a Changelog, no dates on other entries either) and accurately describe the final (post-guard-fix) state, not a stale description of the first commit.

I was not able to execute awk/bash -n/the test suite myself in this sandbox (Bash access in this lane is restricted to gh pr diff/view/comment/review), so the above is static/manual verification, not a re-run of the author's test results — I'm not vouching for the PR's own "96 → 117 passing" / "22 failures on regression" numbers, only for the regex equivalence and guard-scoping reasoning I traced myself.

Findings

One inline nit posted, no blocking issues:

• P3 (nit) check-skill.test.sh:2993 — the guard's comment says "the frontmatter date check below," but that check lives in a different file (check-skill.sh:703) and earlier in it, not below this comment. Purely a misleading pointer for a future reader; the guard's actual behavior toward that check is correct.

No correctness issues found in the regex rewrites or the new guard's scoping logic.
(branch: claude/pocock-steering-course-00zkvd)

[claude]

Claude finished @kyle-sexton's task in 37s —— View job

---

Security review — no issues found

• Gather context (PR diff since last-reviewed head)
• Run skip-gate check
• Review diff for security-relevant surface
• Post findings

Skip gate: No security-relevant surface in the commits since the last-reviewed head (8f464e2). This push (88d0227) contains a single one-line-of-substance change — a comment-text correction in check-skill.test.sh's mawk-portability guard, replacing a misleading pointer ("the frontmatter date check below") with an accurate description of where that check lives. This is the same nit flagged inline on the prior revision.

Verified no functional change accompanies it:

• The guard's regex (INTERVAL_RE='\{[0-9]+(,[0-9]*)?\}') and its scoping logic are byte-identical to the previously-reviewed revision — only the comment prose above it changed.
• No new externalized input, trust boundary, or workflow permission is touched by a comment edit.

No CRITICAL, IMPORTANT, or SUGGESTION findings.
(branch: claude/pocock-steering-course-00zkvd)

[claude]

Claude finished @kyle-sexton's task in 57s —— View job

---

Code review — /review:code-review

• Gather context (PR diff, changed files, prior review comments)
• Run skip-gate check
• Review diff for correctness/maintainability
• Post findings

Skip gate: Not skipped — reviewing the diff introduced since the last-reviewed head. PR is open, not draft.

Scope: Only 8f464e2 → 88d0227 is new since this lane's last full pass (already covered c84cb36 and 8f464e2). That commit touches a single file, check-skill.test.sh, +3/-2, comment text only — no code, no test logic, no assertion behavior changed.

What changed: The scoping-rationale comment above the mawk-portability guard previously said the Bash [[ =~ ]] date regex it intentionally exempts is "the frontmatter date check below" — misleading, since that check lives in a different file (check-skill.sh) and earlier in it, not below this comment in this file. This was flagged independently by both this lane's own prior pass and a human-authored inline comment at check-skill.test.sh:2993 on the prior revision. This commit fixes it by naming the file and describing what the check does instead of citing a location.

Verified by hand:

• The new wording — "check-skill.sh dates a frontmatter synced: value with one" — accurately describes check-skill.sh:703, [[ "$SYNCED_VAL" =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}$ ]], which does check a vendor-sync frontmatter synced: field via a Bash interval regex.
• Describing by content rather than by line number is also more durable — the prior "below" pointer broke because of file/line drift, and this phrasing doesn't have a line number to rot.
• No functional lines changed; the guard's INTERVAL_RE, its two grep surfaces, and the fail/pass logic are untouched.

No correctness or maintainability issues found in this commit. All findings from earlier passes on this PR were either already fixed by the author (exact-count {n} coverage, awk-scoping) or are the exact nit this commit resolves.
(branch: claude/pocock-steering-course-00zkvd)