feat(skill-quality): check 23 — completion-criteria signal (advisory heuristic)

[kyle-sexton]

Closes #2963

Summary

Adds the audit-side half of the course lane 7 completion-criteria adoption: skill-quality:check gains Check 23 — an advisory (WARN-only) heuristic flagging numbered procedures of three or more steps whose text carries no completion-signal token, the premature-completion shape where a step is markable done at the first plausible output. skill-quality 0.15.13 → 0.16.0. Sequencing honored: lands after #2962 (docs-hygiene:write-for-agents 0.17.0), whose doctrine the check's SKILL.md gotcha entry now points authors at via the Skill tool.

Fix

• scripts/check-skill.sh: Check 23, modeled on Check 18's advisory-heuristic shape — fence-aware awk scan grouping consecutive ordered-list steps (blank-tolerant, continuation-line aware), firing only when a ≥3-step block contains none of a deliberately broad completion-signal token set; detects the absence of any done-condition, never grades one. Written interval-free so it runs identically under gawk and mawk.
• scripts/check-skill.test.sh: three cases — signal-free procedure warns; done-condition procedure silent; fenced illustrative list ignored.
• skills/check/SKILL.md: check count twenty-two → twenty-three in the description and body; new gotcha bullet documenting the heuristic's limits and pointing at /docs-hygiene:write-for-agents; the skill's own listing-budget procedure gains the done-condition the new check asked of it (it was Check 23's first real catch).
• plugin.json 0.16.0 + description; CHANGELOG.md entry; docs/CATALOG.md regenerated for the description change.

Verification

• check-skill.test.sh: the three new cases pass; the suite's 21 pre-existing failures in this container are an environment artifact (mawk lacks ERE intervals, breaking Check 21's scanner container-side only) — reproduced on unmodified origin/main and filed as skill-quality: check-skill.sh fresh-eyes scanner breaks under mawk — ERE interval expressions unsupported #3005; CI runs gawk where the suite is green.
• shellcheck and shfmt -d clean on the modified script.
• Self-run: check-skill.sh check PASS (and Check 23's warn on the skill's own procedure was fixed rather than suppressed).
• Full local gate: validate-plugins.sh end-to-end pass (catalog + cheat sheet + identity-prerequisites in sync), markdownlint-cli2 0 issues, check-changelog-parity.sh --check and --check-bump origin/main both exit 0.

Related

• Refs docs-hygiene: implement write-for-agents — authoring-time skill for agent-consumed markdown #2962 (write-side doctrine this points at; merged as 96a97750)
• Refs skill-quality: check-skill.sh fresh-eyes scanner breaks under mawk — ERE interval expressions unsupported #3005 (pre-existing mawk incompatibility in Check 21, found during this build)
• Design provenance: docs/specs/write-for-agents-brief.md acceptance criterion 5; course lane 7 (playbooks/docs-hygiene: aihero course lane 7: authoring-time steering doctrine (writing-for-agents re-decision) #2909)

🤖 Generated with Claude Code

https://claude.ai/code/session_01QbfCrj3X9FfGL7VRZYmrn4

---

Generated by Claude Code

[claude]

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

---

Security review complete ✅

• Gather context (PR diff, changed files)
• Run /review:security-review per custom instructions
• Post findings

No security issues found.

Reviewed the full diff (git diff origin/main...HEAD) against the CI security-review criteria (logic/trust-boundary flaws, injection surfaces, credential handling, GitHub Actions permission/trigger risk, supply-chain pins). This PR touches only:

• plugins/skill-quality/scripts/check-skill.sh — new Check 23, a read-only awk scan over the local SKILL.md file being audited (fence-aware line grouping, checked against a fixed literal token regex). No eval, no external/network input, no shell interpolation of scanned content, no change to trust boundaries.
• plugins/skill-quality/scripts/check-skill.test.sh — new test fixtures/assertions exercising the above.
• plugins/skill-quality/skills/check/SKILL.md, plugin.json, CHANGELOG.md, docs/CATALOG.md — documentation/metadata/version-bump only.

No workflow files, permissions, dependency pins, or credential-handling paths are touched. This falls under the skip gate's "no security-relevant surface" condition, so no findings to report.

[github-actions]

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

[claude]

Claude finished @kyle-sexton's task in 5m 13s —— View job

---

Code review complete

• Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md)
• Run /review:code-review criteria against the diff
• Review check-skill.sh Check 23 implementation
• Review check-skill.test.sh new test cases
• Review SKILL.md doc changes
• Review plugin.json / CHANGELOG.md / docs/CATALOG.md
• Post findings

Reviewed the diff against origin/main (scope: the 6 changed files). One finding posted inline.

Finding

check-skill.sh L1289-1303 — blank line doesn't close a step block, so two unrelated short numbered lists can merge into one false-positive block. Traced the awk by hand: a blank line is a no-op (never calls close_block()), and when a second list restarts at 1. right after it, steps is already non-zero so it isn't treated as a new block's start — it just keeps incrementing the existing counter. Two independent 2-step lists separated only by a blank line (no heading/prose between them) merge into a tracked 4-step block and fire the WARN even though neither individual list reaches the 3-step threshold. The same gap can also suppress a genuine signal-free 3+-step procedure if an unrelated adjacent list happens to contain a signal word. This isn't in the gotcha's documented limitations (which cover under-reporting only, not this merge case) and isn't exercised by any of the three new test cases — each fixture has a single list. See the inline comment for the full trace and a fix-this link.

Everything else checked out

• check-skill.test.sh: the three new cases (signal-free warns, done-condition silent, fenced list ignored) correctly exercise the intended happy paths; test 23a/23b/23c logic and assertions are sound.
• SKILL.md: check count (twenty-two → twenty-three) updated consistently across SKILL.md, plugin.json, and docs/CATALOG.md; new gotcha bullet accurately scopes the heuristic's limits and correctly points at /docs-hygiene:write-for-agents; the skill's own listing-budget procedure fix (adding a done-condition) is correct and — per hand-trace of the fence-tolerant grouping logic — would indeed have been Check 23's first real catch (fenced shell block between steps 2 and 3 doesn't break the group, and "done-condition" trips the done signal token).
• plugin.json / CHANGELOG.md / docs/CATALOG.md: version bump (0.15.13 → 0.16.0), description text, and changelog entry are consistent with the code change and with the repo's existing changelog format (no dates, matches prior entries).
• Placement of Check 23 in the script (after Check 22, before the Summary block) matches its numbering.
• No security, performance, or test-coverage concerns beyond the finding above.

Verification notes

This lane's Bash access is scoped to git diff/git log/gh pr *; I could not execute check-skill.sh, check-skill.test.sh, shellcheck, or shfmt myself, so the PR description's shellcheck/shfmt/test-suite/validate-plugins.sh results are author-claimed and unverified by me — everything above is from reading the diff and hand-tracing the awk logic, not from running it. The PR's own note that CI runs gawk (where the suite is green) versus the container's mawk (pre-existing, tracked separately as #3005) sounds right and is orthogonal to this review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a114c70a7a

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

---

Security review complete ✅

• Gather context (PR diff since last reviewed SHA)
• Run /review:security-review per custom instructions
• Post findings

No security issues found.

Per the skip gate, only the commits since the last-reviewed head (a114c70a) were in scope. The single new commit, 990d0f2, addresses the tilde-fence false positive flagged by the Codex review by broadening the fence-toggle regex in check-skill.sh's awk scanner from ``` to (```|~~~), plus corresponding test cases in check-skill.test.sh.

• This is a read-only, fence-aware scan over the local SKILL.md file being audited — a fixed literal-pattern regex change with no external/network input, no eval, no shell interpolation of scanned content, and no change to trust boundaries.
• check-skill.test.sh changes are test-only fixtures/assertions.
• No workflow files, permissions, dependency pins, or credential-handling paths are touched.

This falls under the skip gate's "no security-relevant surface" condition — no findings to report.

[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 47s —— View job

---

Security review complete ✅

• Gather context (PR diff since last reviewed SHA)
• Run /review:security-review per custom instructions
• Post findings

No security issues found.

Per the skip gate, only the commits since the last-reviewed head (990d0f22) were in scope. The single new commit, 3349494, fixes the block-merging bug flagged in code review (two independent short numbered lists separated only by a blank line could merge into one tracked block, or a loose list could clear a genuine signal-free procedure). The fix tracks each item's own number and closes the current block when a numbered item restarts (n <= last_n) after a blank line, plus two corresponding test cases (23e, 23f) in check-skill.test.sh.

• Still a read-only, fence-aware awk scan over the local SKILL.md file being audited — pure control-flow/state-tracking change (numbering comparison), no external/network input, no eval, no shell interpolation of scanned content, and no change to trust boundaries.
• check-skill.test.sh changes are test-only fixtures/assertions.
• No workflow files, permissions, dependency pins, or credential-handling paths are touched.

This falls under the skip gate's "no security-relevant surface" condition — no findings to report.

[claude]

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

---

Code review complete

• Gather context (PR diff since last review, prior review threads)
• Review new commit 3349494 (blank-line block-merge fix) against the previously flagged finding
• Re-check whether the fix introduces any new gaps
• Post findings

Scope: commits since the last full code-review pass (a114c70) — 990d0f2 (tilde-fence recognition) and 3349494 (blank-line block-merge fix), which is what changed since this head was last reviewed.

Previously flagged finding: confirmed fixed

The blank-line block-merge bug flagged in the prior review (and independently) — two unrelated short numbered lists separated only by a blank line silently merging into one trackable block — is fixed in 3349494. Hand-traced the new logic:

• A numbered item now records its own number (n); if the block is open (steps > 0), the prior line was blank (had_blank), and this item's number doesn't exceed the previous item's (n <= last_n), the block closes before this item starts a new one.
• Traced against both new tests: two adjacent 2-step lists (1,2 / 1,2) correctly split and never reach the 3-step threshold (test 23e); a genuinely loose ascending 3-step list (1 / 2 / 3 with blank lines, same list) correctly stays merged and still warns when signal-free (test 23f).
• The accepted side effect — an all-1.-numbered loose list closes at every item and under-reports — is honestly documented in the code comment at L1282-1288, consistent with the check's advisory posture.

Good fix, well-targeted, tested at the actual mechanism (numbering-restart-after-blank), not just the symptom.

New finding (posted inline)

One remaining gap in the tilde-fence commit itself, posted inline on check-skill.sh#L1294: the fence toggle treats backtick and tilde markers as one shared flag instead of tracking which marker opened the fence (as the file's other, more rigorous fence scanner around L1109-1113 does). A backtick-fenced block containing a literal ~~~ line (or vice versa) flips the mask off mid-block, which can produce a false-positive WARN — plausible in a meta-tooling repo whose own skills document fence syntax. Not covered by 23c/23d, which each exercise a single fence type.

Also checked, no issues

• CHANGELOG.md/plugin.json: version stayed 0.16.0 across both fix-up commits (appropriate — pre-merge fixups to the same unreleased entry, not new releases). Minor accuracy nit, not blocking: the changelog text still says "Three test cases," but the full PR now ships six (23a-23f, covering both fence forms and both merge directions) — worth a quick word-count fix before merge but not worth a separate finding.
• check-skill.test.sh: all four new cases across both commits (23d tilde-fence, 23e adjacent-list split, 23f loose-ascending-still-warns) assert the right thing and match the mechanism, not just the observed symptom.

Verification notes

This lane's Bash access is scoped to git/gh — I could not execute check-skill.sh/check-skill.test.sh myself, so all of the above is from reading the diff and hand-tracing the awk logic against concrete inputs, not from running it.

[claude]

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

---

Code review complete

• Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md)
• Run /review:code-review per custom instructions
• Review latest commit (798956a) against previously flagged findings
• Post findings

Scope per /review:code-review's skip gate: this lane already fully reviewed a114c70 and separately confirmed the two follow-up fixups (990d0f2, 3349494). The only unreviewed commit at HEAD (798956a509baa0da9e5d9cdbda102e95b09205f6) is the newest one, so review focused there.

Previously flagged finding: confirmed fixed

The P3 mixed-fence-marker gap flagged in the prior code-review pass — the shared fence toggle let a literal ~~~ line inside a backtick-fenced block (or vice versa) unmask content mid-block — is fixed in 798956a:

/^[[:space:]]*(```|~~~)/ {
  m = ($0 ~ /^[[:space:]]*```/) ? "b" : "t"
  if (!fence) { fence = 1; fence_ch = m } else if (m == fence_ch) fence = 0
  next
}

Hand-traced against the new test fixture (23g — a ~~~text example nested inside a ```markdown block): opening ``` sets fence=1, fence_ch="b"; the inner ~~~text line matches the fence regex but m="t" != fence_ch="b", so it does not toggle — the block stays masked through the nested tilde-fenced numbered list; the matching closing ``` (m="b") correctly closes it. This mirrors (in simplified, interval-free form, as the commit message states) the matching-marker semantics of the more rigorous fe_open_char/fe_open_len scanner elsewhere in the file (around L1104-1116) without needing gawk-only interval matching for fence length, which is an intentional, reasonable scope trim for an advisory-only heuristic.

All three findings raised across this PR's review history are now addressed in-branch:

1. Tilde-fence recognition (Codex, P2) → fixed in 990d0f2.
2. Blank-line block-merge false positive/negative (code review) → fixed in 3349494.
3. Mixed-marker fence toggle (code review, P3) → fixed in 798956a.

Also checked in this commit

• CHANGELOG.md: the "Three test cases" wording nit from the prior review is corrected to "Seven test cases," and the description now accurately lists both fence forms, matching-marker close semantics, and the list-splitting behavior.
• check-skill.test.sh: new case 23g is well-targeted at the actual mechanism (nested mismatched marker inside a fence), not just the previously-reported symptom; fixture front matter and structure are consistent with the other Check 23 cases.
• No new false-positive/negative surface introduced by the fix — the next after the fence-line branch keeps fence-marker lines themselves out of both the step-scanner and the signal-token scanner in every branch (open, close, mismatched-no-op).
• plugin.json intentionally untouched (stays 0.16.0) — correct, this is a pre-merge fixup to the same unreleased entry, not a new release.

No new findings

Nothing further to flag in 798956a. No inline comments posted since there are no new issues on changed lines.

Verification notes

This lane's Bash access is scoped to git/gh; direct execution of check-skill.sh/check-skill.test.sh was not available to me, so the above is from reading the diff and hand-tracing the awk logic against concrete inputs (including the new 23g fixture), not from running it. The PR description's shellcheck/shfmt/test-suite/validate-plugins.sh results remain author-claimed and unverified by me.

[claude]

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

---

Security review complete ✅

• Gather context (PR diff since last-reviewed head)
• Run /review:security-review per custom instructions
• Post findings

No security issues found.

Per the skip gate, only the commits since the last-reviewed head (3349494) were in scope. The single new commit, 798956a, fixes the mixed-fence-marker gap flagged in prior review (both the inline code-review comment and Codex's P3): the fence toggle now tracks which marker (``` vs ~~~) opened the block and only a matching marker closes it, instead of treating either marker as one shared toggle. A regression test (23g) covers a literal ~~~ line inside a backtick fence staying masked.

• Still a read-only, fence-aware awk scan over the local SKILL.md file being audited — pure control-flow/state-tracking change (tracking the opening marker), no external/network input, no eval, no shell interpolation of scanned content, and no change to trust boundaries.
• check-skill.test.sh change is a test-only fixture/assertion; CHANGELOG.md change is a text-only count correction (three → seven test cases), no functional effect.
• No workflow files, permissions, dependency pins, or credential-handling paths are touched.

This falls under the skip gate's "no security-relevant surface" condition — no findings to report.