fix(skill-quality): narrow check 21 parsing contract (#1493)

[kyle-sexton]

Fixes #1493

Summary

Narrows check 21’s stated parsing contract: inline code spans, backslash escapes, and cross-line span carries are no longer modeled in the awk scanner. Lines with backtick runs or a backslash-escaped < now decline directive hard verdicts (safe direction: missed nudge, not false FAIL). Carries multi-line HTML comment state until --> so delegation wording split across comment lines no longer satisfies Form 1.

Bumps skill-quality to 0.15.6.

Test plan

• bash plugins/skill-quality/scripts/check-skill.test.sh
• New fixture fe-comment-multiline covers the multi-line comment defect
• Updated span/escape fixtures reflect narrowed ambiguity behavior

Related

• skill-quality: narrow what check 21 claims to parse (follow-up to the #1096 parsing contract) #1493

  

[claude]

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

---

Security review — skip gate hit

• Gather context (PR diff, changed files)
• Apply skip gate (open PR / security-relevant surface / no prior review)
• Review diff for security issues
• Post findings

Per /review:security-review skip gate: this PR has no security-relevant surface. The change narrows the awk-based text-scanning heuristic in plugins/skill-quality/scripts/check-skill.sh (check 21's inline-code-span/backslash-escape/HTML-comment handling), plus matching test fixtures, a changelog entry, the spec doc, and a version bump. It touches only pattern-matching logic operating on markdown files within this repo — no authentication, authorization, secrets/credential handling, command construction from external input, or GitHub Actions permissions/workflow changes are involved. awk -v arguments (P, JR) and file paths passed to the scanner are unchanged by this diff and are not attacker-controlled.

No security findings to report.

[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.

[github-actions]

Last security-reviewed head: 8b1195d9afb16589cf51b6d097fe3f67b4d355ac. 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 46s —— View job

---

Security review complete

Task list

• Gather context (PR diff, changed files)
• Invoke /review:security-review per custom instructions
• Post findings (or clean bill of health)

Findings

No security issues found.

This PR narrows the parsing contract of skill-quality check 21 (an awk-based scanner embedded in scripts/check-skill.sh that lints skill markdown for fresh-eyes delegation directives), plus a version bump, changelog entry, test fixtures, and spec doc update. The change:

• Removes inline-code-span pairing / backslash-escape resolution logic and instead declines hard verdicts on any line containing a backtick run or a backslash-escaped < (safe direction: fewer false FAILs, not new false negatives that matter — the judgment detector still runs).
• Adds multi-line HTML comment state carry so delegation wording split across comment lines no longer falsely satisfies Form 1.

Scope assessment:

• No authentication, authorization, secrets, credentials, or network I/O touched.
• No GitHub Actions workflow files changed (no permissions:, pull_request_target, or trigger changes).
• No eval, dynamic source, or command construction from file content in the touched script — confirmed via targeted search of check-skill.sh.
• The scanner only reads awk over local skill markdown files as a dev-tooling linter; there's no trust boundary being crossed (author's own repo content, not untrusted external input processed at elevated privilege).
• plugin.json and CHANGELOG.md changes are metadata/doc-only.

Given the skip-gate criteria in /review:security-review (no security-relevant surface after reading the diff), this PR clears with no findings to report.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 228501fe6f

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

---

Code review complete

• Gather context (PR diff, CLAUDE.md, changed files)
• Review check-skill.sh parsing contract changes
• Review test suite / fixture changes
• Review docs/reference changes
• Post findings

Summary

This narrows check 21's awk scanner as described: it drops inline code-span pairing and backslash-escape reconstruction, declining directive hard verdicts (and Form 1/judgment detection) on lines with a backtick run or a backslash-escaped <, and it now carries HTML comment state (cm_in_comment) across lines until --> so delegation wording split across a multi-line comment can no longer satisfy Form 1.

I traced the awk program (plugins/skill-quality/scripts/check-skill.sh:950-1161) by hand against the new/changed fixtures (fe-span-double, fe-span-crossline, fe-escaped-span, fe-span-literal-backslash, fe-comment-multiline) and the logic checks out:

• fe_bt_ambig/fe_esc_ambig correctly gate directive damb[] and an explicit next that skips that line's own Form 1/judgment scan.
• The new cm_in_comment carry correctly blanks delegation wording on a comment's interior line before it reaches the word/JR match, while the directive classifier still runs on the raw line first (so a directive itself is unaffected by being "inside" a still-open comment) — matches the stated "directive IS a comment" ordering.
• CHANGELOG.md and the 0.15.6 version bump in plugin.json accurately describe the change.

One finding, left as an inline comment on the reference doc:

• Doc/code mismatch in the parsing-contract narrative — plugins/skill-quality/skills/check/reference/fresh-eyes-declarations.md#L148-L150. The reworded sentence reads as if indented-code lines are also exempted from the line's own Form 1/judgment detector run, alongside backtick/escape lines. In the actual scanner, only fe_bt_ambig/fe_esc_ambig gate the next that skips that check (scripts/check-skill.sh:1119); fe_icode only feeds damb[] (a directive's hard verdict + its ability to satisfy a nearby judgment line), not whether the indented-code line itself gets scanned for judgment wording. This is inconsistent with the more precise "Not attempted" bullet a few lines above it in the same file, which correctly scopes the detector-skip to backtick/escape lines only. Since code comments elsewhere point to this doc as the authoritative parsing contract ("that doc is the claim this code implements"), worth tightening so it doesn't mislead a future reader about indented-code behavior. This is a documentation-precision nit, not a behavioral bug — I did not find any incorrect runtime behavior in this PR.

No other issues found. Test coverage for the narrowed contract looks solid, including the new multi-line-comment fixture.
· branch

[claude]

Claude finished @cursor[bot]'s task in 1m 10s —— View job

---

Security review complete

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

Skip gate hit: no security-relevant surface.

The diff at HEAD e8457ba (vs. origin/main) touches only:

• plugins/skill-quality/scripts/check-skill.sh — narrows the awk-based check 21 scanner: drops inline-code-span/backslash-escape reconstruction (ambiguous lines now decline directive hard verdicts) and adds multi-line HTML-comment state carry. This also resolves the correctness issue Codex flagged (comment state starting even on a structurally-ambiguous backtick line) via the fe_bt_ambig/fe_esc_ambig guard now gating the carry at check-skill.sh#L1129.
• check-skill.test.sh — fixture/assertion updates matching the narrowed contract, plus a new fe-comment-multiline fixture.
• fresh-eyes-declarations.md — spec doc updated to match.
• plugin.json / CHANGELOG.md — version bump + changelog entry.

This is a dev-tooling markdown linter operating only on local skill files within this repo:

• No authentication, authorization, secrets, credentials, or network I/O touched.
• No GitHub Actions workflow files changed.
• No eval, dynamic source, or command construction from scanned file content — awk only ever assigns matched substrings to local script variables and prints diagnostics; nothing scanned is passed to a shell.
• awk -v arguments (P, JR) and file paths are unchanged by this diff and are not attacker-controlled.
• No trust boundary is crossed (author's own repo content, not untrusted external input processed at elevated privilege).

Consistent with the two prior security reviews already posted on this PR. No security 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 @cursor[bot]'s task in 37s —— View job

---

Security review complete

• Gather context (diff since last reviewed head, changed files)
• Apply skip gate
• Review diff for security issues
• Post findings

Skip gate hit: no security-relevant surface.

Per the relevance-gate note above, the last reviewed head was e8457ba. The only commit since then is 3c0e365 ("fix(skill-quality): annotate awk backslash-lt index for portability lint"), and its entire diff is:

-      fe_esc_ambig = (index(line, "\\<") > 0)
+      fe_esc_ambig = (index(line, "\\<") > 0) # portability-ok: index() for literal backslash+less-than, not a GNU grep word boundary

A single trailing comment added to an existing line in plugins/skill-quality/scripts/check-skill.sh — no logic, control flow, or behavior change. No new files, no workflow changes, no permissions/trust-boundary surface touched.

This is consistent with the prior security reviews already posted on this PR (which covered the substantive parsing-contract narrowing in commit 228501f/e8457ba) — no security findings to report.

Branch: cursor/fix-1493-skill-quality-check21-2ae1

[claude]

Claude finished @cursor[bot]'s task in 4m 27s —— View job

---

Code review complete

• Gather context (PR diff, CLAUDE.md, changed files)
• Apply skip gate — no early exit; changed surface is reviewable
• Review check-skill.sh parsing contract changes
• Review test suite / fixture changes
• Review docs/reference changes
• Post findings

Summary

Reviewed the full diff (5559176^..3c0e365, all 3 commits on this branch) for plugins/skill-quality/scripts/check-skill.sh, its test suite, the reference doc, CHANGELOG.md, and plugin.json.

I hand-traced the awk scanner (check-skill.sh:950-1161) against the stated contract:

• Span/escape removal. sp_open/sp_keep/fe_span_close and all cross-line span-carry state are fully removed — no leftover dead variables. fe_bt_ambig/fe_esc_ambig correctly gate both damb[] (a directive's own hard verdict + its ability to satisfy a nearby judgment step) and the explicit next at check-skill.sh:1123 that skips that line's own Form 1/judgment scan. Matches the updated fixtures (fe-span-double, fe-span-crossline, fe-escaped-span, fe-span-literal-backslash, fe-span-stray).
• Multi-line comment carry. cm_in_comment correctly carries HTML-comment state across lines until --> (check-skill.sh:1104-1113), and directive classification still runs on the raw line before comment stripping, so a directive itself isn't blinded by being inside a still-open comment.
• Codex's P2 finding ("do not carry comment state from ambiguous backtick lines") is resolved as of e8457ba: the carry-start is now gated with if (!fe_bt_ambig && !fe_esc_ambig) cm_in_comment = 1 (check-skill.sh:1116). I traced this against the exact adversarial shape Codex described — a line with a documented backtick-quoted opener and a real unterminated <!-- on the same line — and confirmed fe_bt_ambig is computed from the full raw line before stripping, so it correctly suppresses the carry and a following judgment line (e.g. "Self-review your own work.") is not incorrectly swallowed. This generalizes beyond the new fe-comment-multiline fixture, which only exercises the non-ambiguous carry path.
• CHANGELOG.md and the 0.15.6 bump in plugin.json accurately describe the change.

Outstanding from a prior review pass on this PR, still unresolved: the doc-accuracy nit at fresh-eyes-declarations.md#L148-L150 was already flagged in an inline review comment on this PR and has not changed since (git diff 5559176..3c0e365 touches nothing in this file after the first commit). The sentence "Today this fires on the indented-code case above and on any line that carries a backtick run or a backslash-escaped <" still reads as if indented-code lines are also skipped by the line's own Form 1/judgment detector run. They are not: only fe_bt_ambig/fe_esc_ambig gate the next at check-skill.sh:1123 that skips that scan; fe_icode only feeds damb[] (affecting a directive's hard verdict and whether it can satisfy a nearby judgment step), not whether the judgment line itself is scanned. This is a documentation-precision issue, not a behavioral bug — not re-filing a duplicate inline thread since one already exists on this exact line, but flagging that it's still open.

No other issues found. Test coverage (check-skill.test.sh, +51/-23, including the new fe-comment-multiline fixture) looks sound by inspection.

Note: this lane's tool grant doesn't permit running arbitrary shell scripts (only Read/Glob/Grep/specific gh pr commands plus a few read-only git operations were available), so I could not execute check-skill.test.sh myself — verification above is by manual trace of the awk program against the diff and fixtures, not a test run.
· branch

[claude]

Claude finished @cursor[bot]'s task in 35s —— View job

---

Security review complete

• Gather context (PR diff, changed files, HEAD SHA)
• Invoke /review:security-review per custom instructions
• Review diff for security issues
• Post findings

Skip gate hit: no security-relevant surface.

Per the relevance-gate note on this PR, the last reviewed head was 3c0e365. The only commit since then is 8b1195d ("docs(skill-quality): tighten check 21 ambiguity scope in reference"), and its entire diff is a 4-line prose edit to fresh-eyes-declarations.md:

-directive satisfy a nearby judgment step — the same lack of confidence cuts both ways, so a literal
-exemption inside an indented example cannot silence the warning that step deserves. The judgment
-detector itself continues to run on unambiguous lines. Today this fires on the indented-code case
-above and on any line that carries a backtick run or a backslash-escaped `<`.
+directive satisfy a nearby judgment step — the same lack of confidence cuts both ways. On an
+indented-code line, `fe_icode` feeds that directive-side suppression only; the judgment detector
+still runs on the line's own prose. On a line with a backtick run or a backslash-escaped `<`, the
+scanner declines the line's own Form 1 and judgment detectors via an explicit skip.

This is a documentation-only wording change — it resolves the doc-accuracy nit raised in earlier review passes on this PR (indented-code lines vs. backtick/escape lines' effect on the judgment detector). No code, workflow, or configuration changes.

• No .sh, .json, or .yml/.yaml files touched by this commit.
• No authentication, authorization, secrets, credentials, or network I/O touched.
• No GitHub Actions workflow files changed.
• No trust boundary crossed — a Markdown reference doc, not executable content.

Consistent with the four prior security reviews already posted on this PR (all skip-gated for the same reason: this PR only narrows a local awk-based markdown linter with no security-relevant surface). No security findings to report.
· branch

[claude]

Claude finished @cursor[bot]'s task in 4m 57s —— View job

---

Code review complete

• Gather context (PR diff, CLAUDE.md, changed files)
• Review check-skill.sh parsing contract changes
• Review test suite / fixture changes
• Review docs/reference changes
• Post findings

Summary

This PR is already at a mature, heavily-reviewed state (2 security-review passes and 3 prior code-review passes on this thread, plus a Codex P2 finding that was fixed at e8457ba). I independently re-traced the awk scanner in plugins/skill-quality/scripts/check-skill.sh:950-1163 against the diff and the updated fixtures, focused on what changed since the last reviewed head (3c0e365) and whether the previously-flagged doc nit was actually resolved.

Confirmed fixed: the doc-accuracy nit raised in an earlier review pass on this PR (indented-code lines incorrectly implied to be skipped by the judgment detector) is resolved in the current head. fresh-eyes-declarations.md:147-150 now correctly states that fe_icode only feeds the directive-side damb[] suppression, while the judgment detector still runs on an indented-code line's own prose — matching check-skill.sh:1085 (damb[dir_n] = fe_line_ambig) vs. the next at check-skill.sh:1123, which is gated only by fe_bt_ambig || fe_esc_ambig.

Also verified independently (all check out):

• The full removal of cross-line span-carry state (sp_open, fe_span_close, sp_keep, etc.) is clean — no leftover references anywhere in the script.
• fe_bt_ambig/fe_esc_ambig correctly gate both the directive damb[] suppression and the explicit per-line next that skips Form 1/judgment scanning.
• Codex's P2 finding (comment-state carry starting even from a structurally-ambiguous backtick line) stays fixed: the carry-start at check-skill.sh:1116 is gated with if (!fe_bt_ambig && !fe_esc_ambig).
• Directive classification still runs on the raw line before comment-stripping, so a directive itself isn't blinded by being inside a still-open multi-line comment.
• The new fe-comment-multiline fixture's scenario traces correctly through the cm_in_comment carry logic, including the interior line being fully blanked before it reaches the judgment regex.
• CHANGELOG.md and the 0.15.6 bump in plugin.json accurately describe the change.

New finding (posted inline)

• Stale "Not attempted" bullet in the reference doc — fresh-eyes-declarations.md#L125-L127. This bullet ("Paragraph-interrupting block constructs... a pending cross-line span carry expires at a blank line or a fence...") describes the cross-line inline-code-span-carry mechanism this PR deletes. Since fe_bt_ambig/fe_esc_ambig are now computed per-line with no cross-line state, there's no carry left to expire, and the bullet is now describing behavior the scanner no longer has. This wasn't touched by the diff (it sits as unchanged context right next to the hunk that rewrote the neighboring bullets), so it was missed in prior passes. Documentation-precision issue only — no behavioral bug. A near-identical stale reference exists in a test comment at check-skill.test.sh:1731 ("Tildes, so the cross-line span carry cannot mask the body..."), flagged as lower-priority in the same inline comment.

No other issues found. Test coverage for the narrowed contract (including the new multi-line-comment fixture) looks sound.

[claude]

Stale "Not attempted" bullet — this PR removes the cross-line inline-code-span carry entirely (sp_open/fe_span_close and friends are deleted from scripts/check-skill.sh), but this bullet still describes that removed mechanism: "A pending cross-line span carry expires at a blank line or a fence, but not at an ATX heading, thematic break, or table that also interrupts the paragraph in CommonMark."

There is no more span carry to expire — fe_bt_ambig/fe_esc_ambig are now computed per-line with index(), with no cross-line state and therefore no paragraph-interruption interaction at all. This bullet was left over from the pre-PR contract and should be deleted (or reworded to describe an actual remaining gap) rather than describing behavior the scanner no longer has.

Same staleness echoes in a dev comment at scripts/check-skill.test.sh:1731 ("Tildes, so the cross-line span carry cannot mask the body incidentally...") — lower priority since it's just test-file prose, but worth a pass while touching the doc.

[Fix this →](https://claude.ai/code?q=In%20plugins%2Fskill-quality%2Fskills%2Fcheck%2Freference%2Ffresh-eyes-declarations.md%20on%20branch%20cursor%2Ffix-1493-skill-quality-check21-2ae1%2C%20remove%20or%20reword%20the%20%22Paragraph-interrupting%20block%20constructs%22%20bullet%20at%20lines%20125-127.%20It%20describes%20a%20%22pending%20cross-line%20span%20carry%22%20that%20no%20longer%20exists%20%E2%80%94%20this%20PR%20deleted%20the%20cross-line%20inline-code-span-carry%20mechanism%20(sp_open%2Ffe_span_close)%20from%20scripts%2Fcheck-skill.sh%2C%20replacing%20it%20with%20a%20per-line%20fe_bt_ambig%2Ffe_esc_ambig%20check%20that%20has%20no%20cross-line%20state.%20Delete%20the%20bullet%20since%20the%20limitation%20it%20describes%20no%20longer%20applies.%20Also%20update%20the%20stale%20%22cross-line%20span%20carry%22%20comment%20at%20scripts%2Fcheck-skill.test.sh%3A1731.&repo=melodic-software/claude-code-plugins