fix(docs-hygiene): hyphen-exclude Form 1; allow inline comments on manifest declarations - #1435
Conversation
…clarations Two P1 findings raised on #1386 after it was already merged. Same review round, so they never reached main. Form 1 rewrote unrelated slash commands. `\b` treats a hyphen as a word boundary, so `\B/<old>\b` prevented `/confirm` matching in `/confirmation` but NOT in `/confirm-changes` -- renaming `context` matched the unrelated `/context-guard`. Form 1 is Certain and sits on container mode's Certain allowlist, so that went through the default auto-apply path and renamed another command. Slash-command and container names are kebab-case, so this fires constantly rather than rarely. Now uses the consumed `([^\w-]|$)` terminator, the shape Forms 13 and 15 already use for the same reason. Verified against this tree that namespaced invocations are unaffected -- a colon is a valid terminator, so the nine `/docs-hygiene:<skill>` references still match -- while `/context-guard`, `/contextual` and `path/context` do not. Form 1 joins the other consume-the-delimiter forms in the survey's rescan-cursor rule, and its consumed character is not part of the reference. Manifest declarations could not carry an inline comment. `name: <old> # package name` and `name = "<old>" # package name` are ordinary self-documenting manifests, and the end-anchored alternatives rejected the whole line -- while filesystem evidence still selected container mode, so the registration went unmatched and was suppressed as residue while apply mode reported completion. The whitespace rule differs between the two on purpose. YAML starts a comment only when `#` follows whitespace, so `name: <old>#x` is the single scalar `<old>#x` and must NOT match; the YAML alternative therefore requires `(\s+#.*)?`. TOML's value is quoted, so the closing quote already ends the string unambiguously and `\s*(#.*)?` is safe. JSON is excluded entirely, having no comment syntax. All six positive shapes and both negatives verified. Phase 6 reconciliation grep on Form 1's boundary found it restated in four more places -- triage.md's Certain criteria, audit-modes.md's Orphans sweep, apply.md's word-boundary-trap gotcha, and audit.md's consume-the-delimiter form list -- all corrected here. Gates: validate-plugin-contracts (43 setup skills, 2122 files), claude plugin validate, markdownlint-cli2 0 errors over 36 files, changelog parity --check-bump, evals parse at 46 cases with no duplicate ids. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATCcexm8GPTaNntu2yrGMk
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3707dcd3c
ℹ️ 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".
…tion by region Two findings on this PR, both consequences of the two fixes it carries. SKILL.md's slash-token gotcha still prescribed `\B/<old>\b` -- the exact expression Form 1 was corrected away from. SKILL.md is ALWAYS loaded, so a stale executable directive there outranks the corrected pattern in practice: an agent following the gotcha reintroduces the hyphenated-sibling defect while patterns.md claims it fixed. Now states the consumed terminator, why a trailing `\b` is insufficient, and that the consumed character is not part of the reference. The reconciliation grep should have caught it last round. It did run over Form 1's boundary, but its output exceeded the display budget and was truncated to a file, and the narrower follow-up greps used a hand-escaped `\B` that matched nothing. This round's grep matched the literal regex text with no escaping guess and found every site. Accepting inline comments widened the match to span text that is documentation rather than declaration -- and a comment routinely mentions the thing it documents. `name: <old> # <old> before publishing` is one declaration and one piece of prose, but the survey enumerates every `<old>` span inside a match, so it emitted two records, both attributed to Form 14. That form is Certain and exempt from the common-word demotion inside a manifest, so apply mode rewrote the comment text. Enumeration is now bounded by a per-form REFERENCE REGION rather than the whole match, with the regions tabulated in audit.md: the declaration alternatives enumerate within the VALUE only; Form 7 keeps the whole quoted field, because its multiple occurrences are all genuine references; the delimiter-anchored forms enumerate the token span they anchor, excluding the consumed delimiter. Narrowing blanket-wide would have reintroduced the wide- match bug those regions were split to fix. Gates: validate-plugin-contracts (43 setup skills, 2122 files), claude plugin validate, markdownlint-cli2 0 errors over 36 files, changelog parity --check-bump, evals parse at 48 cases with no duplicate ids. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATCcexm8GPTaNntu2yrGMk
|
Claude encountered an error after 3s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
Follow-up to #1386. Two P1 findings were raised on that PR after it had already merged, so they never reached
main.Closes #1439
Form 1 rewrote unrelated slash commands
\btreats a hyphen as a word boundary, so\B/<old>\bprevented/confirmmatching in/confirmationbut not in/confirm-changes. Renamingcontextmatched the unrelated/context-guard.Form 1 is rated Certain and sits on container mode's Certain allowlist, so this went through the default auto-apply path and renamed another command. Slash-command and container names are kebab-case, so it fires constantly rather than rarely.
Now uses the consumed
([^\w-]|$)terminator — the shape Forms 13 and 15 already use for the same reason.Verified against this tree: the nine
/docs-hygiene:<skill>namespaced invocations still match, a colon being a valid terminator, while/context-guard,/contextualandpath/contextdo not.Manifest declarations could not carry an inline comment
name: <old> # package nameandname = "<old>" # package nameare ordinary self-documenting manifests, and the end-anchored declaration alternatives rejected the whole line — while filesystem evidence still selected container mode, so the registration went unmatched and was suppressed as residue while apply mode reported completion.The whitespace rule differs between the two deliberately:
#. YAML starts a comment only after whitespace, soname: <old>#xis the single scalar<old>#xand must not match.All six positive shapes and both negatives verified.
Reconciliation
The Phase 6 reconciliation grep on Form 1's boundary found it restated in four more places —
triage.md's Certain criteria,audit-modes.md's Orphans sweep,apply.md's word-boundary-trap gotcha, andaudit.md's consume-the-delimiter form list — all corrected here.Two eval cases added (45, 46).
docs-hygiene0.9.0 → 0.9.1.Gates
node scripts/validate-plugin-contracts.mjs— 43 setup skills, 2122 filesclaude plugin validate .markdownlint-cli2— 0 errors over 36 filesscripts/check-changelog-parity.sh --check-bump origin/mainevals.jsonparses, 46 cases, no duplicate idsRelated
main🤖 Generated with Claude Code
https://claude.ai/code/session_01ATCcexm8GPTaNntu2yrGMk