feat(skill-quality): fresh-eyes delegation doctrine + conformance gate (check 21) - #1096
Conversation
…e (check 21) Phase 1 of program #304 per docs/topics/fresh-eyes-checkpoint-audit/PLAN.md: PLUGIN-PHILOSOPHY gains the Delegation mechanics section (dispatch ladder, named-agent bar, model tiers with dated mapping, declared patterns; all claims cited to official pages verified 2026-07-22), and check-skill.sh gains check 21 enforcing the declared-pattern contract (FAIL on malformed/reason-less fresh-eyes-exempt directives; WARN-only curated judgment-language heuristic; fence- and span-aware; CommonMark fence matching; per-file proximity). The mechanical spec ships inside the plugin for third-party authors (skills/check/reference/fresh-eyes-declarations.md); registry row added. Also fixes pre-existing stale check-count references (README still said eighteen/seventeen after checks 19-20 shipped). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a7c6ab2e1
ℹ️ 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".
nd/nw/nj read as typo candidates (nd -> and); dir_n/word_n/judge_n say what they count. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude encountered an error after 3s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46c02c9601
ℹ️ 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".
|
🔒 babysit-prs lane claiming this PR for on-branch fix work this cycle (amendment-round: 16, safe tier). Will fix clear branch-owned findings and push; will not resolve threads or merge (safe tier). |
Three review findings, each with a regression fixture: - Fence closers follow CommonMark fully: an info-string line (```yaml) inside an open fence is content, never a closer. - Inline code spans pair backtick runs of exactly equal length, so multi-backtick spans hide their content instead of being split at the first run. - Delegation wording counts only when the line also names the worker or dispatch (agent|worker|advisor|reviewer|verif|dispatch|delegat) — bare "in a fresh context" prose assigns the judgment to no one. Spec pages and the design contract amended to match (dated note). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7454583bd4
ℹ️ 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".
Two review findings, each with a regression fixture: - Fence openers follow CommonMark indentation: at most three leading spaces; a four-space-indented backtick run is an indented code block, so a malformed directive after it is still scanned and still blocks. - Worker token verif narrowed to verifier: the stem matched bare "verification" prose that dispatches no worker, so a same-context review line was misread as fresh-context delegation. Spec page and the design contract amended to match (dated note). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84bb65834e
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7d1f4479b
ℹ️ 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 encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
kyle-sexton
added
the
needs-human
|
Surfaced by an independent review of #1274 (a "Fresh-eyes checkpoints" conflates two different mechanisms that share the word "fork." This PR's hunk at
That is true for one of two mechanisms:
So a This has already produced the error three times downstream, and the source has never been fixed:
Three independent downstream corrections, zero at the source. #1274 removes this doc's citation from the offending PLAN.md text rather than fixing the doctrine, which leaves the next author who consults this section free to reproduce the same mistake — the fourth site. Suggested fix, since this PR is already in these lines: name the mechanism in the sentence, mirroring the wording One caveat worth carrying into whatever wording lands: #1258 reports Agent-tool forks empirically not inheriting the conversation in a real session, against the docs. If that holds, the sentence is wrong in the other direction too. Attributing the claim to the docs rather than asserting it as observed behavior would survive either outcome. This was generated by AI during a session on the fact-check capability. |
No related issue: the rename was decided in-session; the follow-on
metadata work it unblocks is tracked under the issues in ## Related.
## Summary
Renames the `re-anchor` plugin to `discipline`. `re-anchor` is a bare
verb sitting among 60 noun-named siblings, its `re-` prefix presumes a
prior anchored state the conversation-start case does not have, and it
collides with the cognitive-bias term of art. `discipline` is the word
the plugin already used for itself 189 times ("a drift corrector for
research discipline") — the authors' own revealed vocabulary, not a
coinage, and legible to a reader who has never seen the plugin.
The verb "re-anchor" is deliberately KEPT (210 occurrences,
`context/re-anchor-audit-correct.md`, and 3 live trigger phrases). The
three defects above concern its use as a CONTAINER name; as the verb for
step 1 of the loop it is accurate. Changing it would touch eval
expectations and trigger keywords that `skill-quality:check` gates
against HEAD, so it belongs in a separate reviewable pass if wanted at
all.
Mechanically, 45 files:
- Directory moved with `git mv` so history is preserved
- `sweep-all-disciplines` skill renamed to `sweep-all`
- `plugin.json` name + version 0.9.0; marketplace entry name/source plus
a `renames` map entry so installed consumers migrate instead of hitting
`plugin-not-found`
- All `/re-anchor:*` invocations to `/discipline:*`;
`metadata.re-anchor-batch` to `discipline-batch`
- Cross-plugin references in `docs-hygiene`, `planning`, `review`,
`session-flow`, `source-control`, `work-items`
- CHANGELOG 0.9.0 entry carrying the full rationale and every rejected
candidate, plus a header pointer noting entries below 0.9.0 shipped
under the former name
Deliberately NOT rewritten: CHANGELOG history and `docs/topics/` — they
record releases and decisions that genuinely happened under the former
name.
## Test plan
- `claude plugin validate .` — passes; the `renames` chain terminates
- `markdownlint-cli2` over all 26 changed markdown files — 0 errors
- `git grep -i 're-anchor'` — 0 stale `/re-anchor:` invocations and 0
stale `plugins/re-anchor` paths outside CHANGELOG history and
`docs/topics/`; every remaining hit is the verb, verified file by file
- Repo-wide dead-link scan across 952 tracked markdown files — 0
introduced by this rename (9 pre-existing dead links remain in
`code-tidying`, `context7`, `playwright` vendor, and `docs-hygiene` doc
examples; untouched, not ours)
- Open-PR conflict scan — no open PR touches `plugins/re-anchor/` or
`plugins/discipline/`. Trivial single-file overlaps only: #1263
(`marketplace.json`, `README.md`), #1266
(`docs/conventions/loop-lane/README.md`), #1096 and #1033 (`README.md`)
- Rebased onto `origin/main` at `d39ea667`; clean
## Related
- Refs #1271 — skill-metadata description tightening; named this rename
as its blocker, unblocked once this merges
- Refs #1272 — filed from this work: rename detection in the fleet-sync
path so a `renames` map entry stops being required
- Refs #1258 — concerns the skill this PR renames from
`sweep-all-disciplines` to `sweep-all`
- Refs #1269 — its scope prefix becomes `discipline(do-your-research)`
after this merges
- Refs #1283 — filed from this PR's review: the `rename-references`
pattern library missed the two syntactic shapes that produced the six
stale references fixed in `930c97a4`
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01GSXnCLnmzk8y4cKv2y1z9f
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two review findings, both silent passes and both consequences of the container-prefix strip and the escape pre-pass added earlier in this branch. Each was reproduced before the fix and is covered by a fixture. The fence flag is global, so an unclosed container-nested opener never ended: after "> ```markdown" the fence stayed active once the lines left the blockquote, swallowing every following top-level line so a malformed directive in later prose reported PASS. A nested fence now ends with its container — a blockquote when the "> " prefix stops, a list item on a dedent below the opener's content column. Escapes and code spans now resolve in ONE left-to-right pass, because CommonMark couples them: escapes are processed only OUTSIDE a span, and a span's content is literal. The previous global escape pre-pass blanked the closing backtick of `foo\` — a run CommonMark still closes, since the backslash there is content — leaving the span open and masking whatever followed. Two independent passes cannot express the coupling: an escape-blind span pass misreads \` as a delimiter, and an escape-first pass destroys a legitimate closer. Corpus sweep of the extracted scanner over every tracked markdown file still reports 167 records, byte-identical to the pre-change baseline.
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d90c48a38f
ℹ️ 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".
Two review findings, each reproduced before the fix and covered by a fixture. Compare blockquote DEPTH, not mere marker presence, when deciding whether a container-nested fence is still inside its container. A fence opened at "> > " lives in the inner quote, so a later depth-one line has left that quote and ends the fence — the presence test kept it open and swallowed the directive. Strip HTML comments before the delegation-wording detector. Form 1 is visible prose by contract, but a hidden "<!-- dispatch this to a fresh-context agent -->" was satisfying it — precisely the parallel source of truth that form exists to rule out. Comments come off only after directives are classified, since a directive is itself a comment, and an unterminated opener drops the rest of its own line and no further, so one stray marker cannot blind the detectors for the whole file. Also fixes a real markdown error the corpus sweep caught in this branch's own design note: an unpaired single-backtick span carried its opener forward and mis-paired with a later span, exposing that line's literal directive example. The scanner was right; the prose was wrong. Corpus sweep over every tracked markdown file is back to 167 records, byte-identical to the pre-change baseline.
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34d8ffe7cd
ℹ️ 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".
…2's joiner (#1450) *This was generated by AI during work-loop execution.* ## Summary - **Adds `check-listing-budget.sh` + a `listing-budget` action** — the shared/aggregate skill-listing character budget (`skillListingBudgetFraction`, default 1% of the model's context window) had no check at all. `check-skill.sh` check 2 only ever guarded the **per-skill** entry cap (`skillListingMaxDescChars`, 1536 chars) — a different, narrower limit. The new script pools one or more skills roots into a single aggregate estimate against a documented, overridable default (8000 chars — the harness's own `SLASH_COMMAND_TOOL_CHAR_BUDGET` fallback), reports the biggest contributors on overflow, and is always advisory (exit 0): the live budget depends on a model's context window and a consumer's own settings, neither of which a static check can observe. Wired into the `skill-quality-gate` CI job as a report-only step pooling every plugin's `skills/` root into one marketplace-wide aggregate (currently ~109,000 chars against the 8,000-char default floor). - **Fixes check 2's joiner (#1404 item 2).** The harness assembles a listing entry as `description` + `" - "` + `when_to_use` — a literal 3-char joiner — that check 2's sum omitted, under-counting by 3 whenever `when_to_use` is populated. Not currently binding at present description lengths in this repo, but wrong in exactly the direction the listing-budget work is about. - **Settles the issue's stated 4x discrepancy (#1404, "one discrepancy worth resolving").** Fetched current docs (`code.claude.com/docs/en/settings`, live HTML) confirm `SLASH_COMMAND_TOOL_CHAR_BUDGET`'s documented 8000-char fallback is exactly `contextTokens(200000) x ~4 chars/token x skillListingBudgetFraction(0.01)` — the `contextTokens x 4 x fraction` derivation from the shipped binary, not the bare `tokens x fraction` reading. `skillListingBudgetFraction` (default `0.01`) and `skillListingMaxDescChars` (default `1536`, matching this repo's existing `DESC_CHAR_CAP`) were also confirmed directly from the live settings page. - **Item 4's caveat (never hardcode `skillListingBudgetFraction`'s default as a resolved live value)** is folded into the new script's design: every tunable (budget chars, context tokens, fraction, chars/token, per-entry cap) is environment-overridable, and the default budget is anchored on the one documented number (8000) rather than reconstructed from assumed inputs. ### Deferred: item 3 (check 12 amendment) — split into #1451 Item 3 — amending check 12 to accept a populated, single-quoted `when_to_use` as satisfying the trigger-spec requirement instead of requiring a literal `Use when:` prefix — is **not** in this PR. The issue itself sequences it behind #1096 ("already editing that file and claims check 21"), and I verified #1096 is a real, currently open, active PR (branch `feat/fresh-eyes-delegation-doctrine-gate`, 708 additions) that does edit `check-skill.sh` (appending check 21). Per that sequencing note, item 3 is split out into a dedicated follow-up, **#1451**, scoped and ready to pick up once #1096 merges or is otherwise resolved. This PR closes #1404 (items 1, 2, and 4 are complete; item 3's tracking now lives in #1451, not in a reopened #1404). ## Test plan - [x] New `check-listing-budget.test.sh` — 8 assertions (help, missing-root error, empty root, under-budget OK, forced-tiny-budget WARN, joiner counted, per-entry cap applied before summing, multi-root pooling). All pass. - [x] `check-skill.test.sh` — added assertion 29 (boundary case: `desc(1500)+wtu(34)=1534` passes check 2 without the joiner fix, fails at `1537/1536` with it). Full suite: 29/29 pass. - [x] `scripts/check-changed-skills.test.sh` (CI orchestrator self-test): 8/8 pass. - [x] `scripts/check-skill-portability.test.sh`: 17/17 pass; `check-skill-portability.sh origin/main` reports no unexcused coupling tokens in the changed skill file. - [x] Dogfooded `check-skill.sh` against the edited `skill-quality:check` skill itself: PASS, 0 errors, 0 warnings, all 7 base-ref trigger phrases preserved (description rewrite kept every existing single-quoted trigger verbatim and only added new ones). - [x] `check-changed-skills.sh origin/main` (the actual CI gate step): PASS. - [x] `shellcheck --rcfile=.shellcheckrc` clean on both new scripts (fixed one SC2016 literal-backtick note and one SC2034 unused-variable warning during development). - [x] `actionlint .github/workflows/ci.yml`: clean. - [x] `markdownlint-cli2` clean on the three edited Markdown files. - [x] Manually ran `check-listing-budget.sh plugins/*/skills` against this repo: 183 skills, 61 roots, aggregate 108,975 chars vs the 8,000-char default budget (~13.6x over) — reproduces the overflow the issue's own measurement found, with concrete top contributors. ## Related Closes #1404 (items 1, 2, and 4 from that issue; item 3 is split into #1451, not resolved here). Refs #1451 (new follow-up: item 3, check 12 amendment — sequenced behind #1096, ready to pick up once it merges or is otherwise resolved). Refs #1096 (open PR already editing `check-skill.sh`; #1451 sequences behind it). Refs #1271 (the original listing-budget issue #1404 was extracted from). --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…mbiguity Six review rounds each found a distinct CommonMark edge case in this hand-written awk structure pass, and several were defects in the previous round's own fix. The unbounded surface, not any individual case, was the root cause: the check had no stated parsing contract, so every construct CommonMark permits was implicitly in scope. Add the contract. It enumerates the constructs the scanner models, names the ones it does not attempt (indented code blocks, mixed container stacks, paragraph-interrupting headings, multi-line HTML comments, reference definitions and link syntax), and states what happens when structure cannot be resolved. A real markdown parser was rejected deliberately: the check must run with nothing but a POSIX shell and awk. Make the under-claim posture real in code rather than asserted in prose. Where the structure pass cannot resolve a line, it withholds DIRECTIVE_MALFORMED, DIRECTIVE_NOREASON, and the stale WARN; the judgment path still runs. The asymmetry is the argument: those verdicts FAIL the skill, so a false positive blocks a legitimate author on a parser artifact, whereas a missed judgment WARN costs one nudge. This fires today on the four-space-indented case, which was a documented false-POSITIVE FAIL and is now declined — not deciding dissolves the gap that could not be decided. The posture is specific to check 21; a gate whose verdict is a security decision stays fail-closed. Also corrects fixture 37t, which was non-discriminating: its blank line ended the blockquote on its own, so it passed without the depth comparison it was meant to prove. The shape now has no blank line, and fails without the fix. Scanner comments now point at the contract instead of restating CommonMark, and carry a note that the awk program is a single-quoted shell string — an apostrophe in a comment there breaks the script, which it briefly did. Corpus sweep over every tracked markdown file: 167 records before, 167 after, byte-identical. Suppression changed no real skill's verdict.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0aec2b1d3c
ℹ️ 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".
Main shipped skill-quality 0.11.0 (#1450, the shared listing-budget reporter) while this branch also claimed 0.11.0. Both sides are composed across six conflicts; nothing is blanket-picked. - .claude-plugin/plugin.json: recomputed to 0.12.0 above main's release. The description takes main's shape — the listing-budget reporter, and check 2 renamed to the per-skill listing-entry cap — plus this branch's twenty-one count and fresh-eyes item. A shared version would have silenced changelog-parity entirely: its bump gate short-circuits when head equals base. - CHANGELOG.md: this branch's section becomes 0.12.0 and main's 0.11.0 section is restored verbatim beneath it. - skills/check/SKILL.md: main's frontmatter, which carries the listing-budget action and its two new trigger phrases, with the check count and fresh-eyes item re-applied. Preserving those phrases is what keeps check 3 green. - README.md and plugins/skill-quality/README.md: main's rewrites naming the listing-budget reporter, with this branch's counts restored; the more detailed of the two injection-portability bullets is kept over main's terser duplicate. - scripts/check-skill.test.sh: purely additive on both sides — this branch's check-21 fixtures and main's joiner-boundary test. Git placed the shared trailing `fi` outside the conflict region, so it is restored explicitly. Main's check-skill.sh changes (check 2's 3-char joiner, header comments) merged without conflict and do not touch check 21. Verified post-merge: 81 assertions pass — 80 before the merge plus main's one added test, which is the arithmetic confirming the test-file splice lost nothing. Main's check-listing-budget.test.sh passes. check 2 now measures 1014/1536 for the longer composed description under its new joiner arithmetic, and all 7 base-ref trigger phrases are preserved. Comparing this branch's scanner before and after the ambiguity suppression over the SAME merged tree gives 166 records either way, byte-identical, so suppression still changes nothing on real content.
|
Claude finished @kyle-sexton's task in 5m 30s —— View job PR Review — feat(skill-quality): fresh-eyes delegation doctrine + conformance gate (check 21)
Status update since the last review (commit 81288a9)The previous Claude review identified two open findings. Five additional Codex-review rounds (commits 3acf21d → 0aec2b1) each fixed a new CommonMark edge case and added the parsed contract to Finding 1 — Pre-merge: fork conflation STILL OPEN
|
| Mechanism | Inherits history? | Satisfies fresh-eyes? |
|---|---|---|
Agent tool subagent_type: "fork" |
Yes — documented in sub-agents | No |
Skill frontmatter context: fork |
No — starts fresh per Skills docs | Yes |
The current text tells an author that context: fork "carries the same bias forward," which is incorrect. Three downstream corrections already happened because of this sentence (docs-hygiene/#1053, re-anchor/sweep-all-disciplines, docs/topics/plugin-audit-port/ being corrected in #1274/#1268). The PR rewrites these exact lines; it is the right place to fix this.
Minimal fix (mirrors the wording the two downstream sites already landed on):
-A fresh-context (non-fork) subagent — generic or named — removes it: it starts in its own fresh
-context window, blind to the reasoning under review. A fork does not: it inherits the parent
-session's full conversation history, so it carries the same bias forward
-([subagents](https://code.claude.com/docs/en/sub-agents), verified 2026-07-22).
+A fresh-context subagent — generic or named — removes it: it starts in its own fresh context
+window, blind to the reasoning under review. The Agent tool's `subagent_type: "fork"` does not,
+per [sub-agents](https://code.claude.com/docs/en/sub-agents) (verified 2026-07-22): it inherits
+the parent session's full conversation history and carries the bias forward. A skill's own
+`context: fork` frontmatter is a separate mechanism — per the [Skills docs](https://code.claude.com/docs/en/skills)
+it starts fresh and does satisfy the requirement.Note: #1258 reports empirical evidence that Agent-tool forks don't inherit conversation in a real session. Attributing the claim to the documented spec rather than asserting it as observed behavior ("per [sub-agents docs]") keeps the sentence durable against either outcome.
Finding 2 — Resolved: cross-line span carry was implemented
The prior review marked the cross-line span carry as "deferred" with a structural limitation. That fix did land — in commit f3839c2 — before the last review could verify it. The sp_open variable now persists across awk records:
if (sp_open) {
sp_at = fe_span_close(line, sp_open)
if (!sp_at) next # whole line is span content
line = substr(line, sp_at + sp_open)
sp_open = 0
}The reference doc's "Modeled" section (fresh-eyes-declarations.md:103) correctly reflects this. The remaining limitations (headings interrupting paragraph, paragraph-interrupting block constructs) are now recorded in the "Not attempted" section with the reasoning. This finding is closed.
Finding 3 — New Codex (0aec2b1): ambiguous directive in proximity match INCONSISTENCY
for (k = 1; k <= dir_n; k++) if (dt[k] == "valid" && d[k] >= j[i] - P && d[k] <= j[i] + P) hasd = 1Compare with the stale-WARN check at line 1054:
if (dt[i] != "valid" || damb[i]) continue # damb is excluded from staleThe damb flag (set when a directive sits on a four-space-indented line — structurally ambiguous between indented code and list continuation) is excluded from hard FAILs and from the stale WARN, but not from the proximity loop. The reference doc's Ambiguity section justifies the exclusions with "it is not confident the directive is live markdown." By that same reasoning, an ambiguous directive should not suppress the WARN for a nearby judgment hit — yet it does.
The practical impact is WARN-tier only: a judgment step near an indented directive gets HIT_DIRECTIVE (pass) instead of HIT_NONE (WARN). The fix is a one-word addition:
for (k = 1; k <= dir_n; k++) if (dt[k] == "valid" && !damb[k] && d[k] >= j[i] - P && d[k] <= j[i] + P) hasd = 1The asymmetry with the stale check suggests this is an oversight rather than an intentional design decision. However, the reference doc's phrase "The judgment path continues to run" could be read as intentional (ambiguous directives still participate in proximity matching). The doc is silent on this specific question. This should be fixed or explicitly documented; either is acceptable.
Finding 4 — New Codex (0aec2b1): frontmatter fence tracking LOW RISK
The awk program has no frontmatter awareness. SKILL.md files do have YAML frontmatter (--- delimiters), but the --- delimiter is not matched by /^ {0,3}(```+|~~~+)/ and never resets fe_fence. If a frontmatter description contained a backtick-fence example in a YAML block scalar, the scanner would enter fe_fence inside the frontmatter and stay there.
Practical impact: near-zero. All current SKILL.md frontmatter descriptions are single-line strings in double quotes (verified by scanning the repo). None use YAML block scalars with fence examples. The Codex finding is technically valid but describes a scenario that doesn't occur in the fleet today.
This should be added to the "Not attempted" section of fresh-eyes-declarations.md if the pattern ever appears in practice, or authors can be told (in that same doc) to avoid code examples in YAML frontmatter descriptions. No code change required now.
Fixes verified from prior rounds (all confirmed present)
All seven precision improvements from rounds 1–3 (confirmed in the prior review) remain present. Rounds 4–7 landed between the prior review and this one:
| Commit | Fix |
|---|---|
3acf21d |
Container-nested fences recognized; blockquote/list marker prefixes stripped |
64dfa54 |
Container strip gated on opener carrying a prefix (prevents cross-contamination) |
d90c48a |
Nested fence expires with its container (depth tracking) |
34d8ffe |
HTML comments stripped before Form 1 detector; fixture for multi-line comment body |
0aec2b1 |
Parsing contract stated in reference doc; ambiguity suppression for hard FAILs and stale WARN |
f3839c2 |
Cross-line span carry implemented via persistent sp_open |
Bottom line
One pre-merge CORRECTNESS finding remains open: Finding 1 (fork conflation) in docs/PLUGIN-PHILOSOPHY.md lines 446–450. This PR rewrites these exact lines; the fix is a targeted prose change. Three downstream sites were already corrected because of this sentence — fixing it here closes the source.
Finding 3 (ambiguous directive proximity) is a WARN-tier inconsistency worth a one-word fix. Finding 4 (frontmatter) is theoretical with zero current instances. Finding 2 is resolved.
| Branch
|
Claude finished @kyle-sexton's task in 4m 43s —— View job Security Review
ScopeReviewed the full diff at HEAD This review covers commits not seen by the prior security review at FindingsNo CRITICAL or IMPORTANT security vulnerabilities found. Detailed assessment of the surfaces examined: awk program — code execution primitives All commits in this range add or refine the inline awk program embedded in awk output → shell case dispatch All awk HTML comment stripping (new in The `` stripper added at lines 1017–1028 is pure awk string manipulation ( Container-depth tracking (new in Introduces Escape handling (new in The backslash-escape pre-processing replaces Cross-line span carry (new in Adds Test harness isolation (existing, verified) The test harness exports No workflow changes, no credential exposure No SUGGESTION (low confidence, carried from prior review) ' "$fe_file")The Bottom line: No security issues in the changes since the prior review. The awk precision fixes (rounds 4–7 as the PR describes them) are structurally sound and introduce no new attack surface. The SUGGESTION on the missing |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 387f1f6069
ℹ️ 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".
…ntmatter Three review findings. None is an unbounded CommonMark fidelity case, so the contract's stopping rule does not cover them — each is triaged as inside the stated claim, and each was reproduced before the fix. The directive name needed a terminator. A prefix-only match read an ordinary comment about a longer identifier — "<!-- fresh-eyes-exemption is explained here -->" — as a directive and FAILed the skill on prose. That is a false-positive hard FAIL, precisely the failure the under-claim posture exists to prevent, and directive syntax is squarely inside the contract. Ambiguity now cuts both ways. The suppression added in the previous commit withheld the hard verdicts and the stale WARN from a structurally ambiguous directive, but still let it satisfy a nearby judgment step, so a literal exemption inside an indented example silenced the warning that step deserved. The earlier choice traded coherence for a smaller corpus diff; that was the weaker argument. YAML frontmatter is no longer parsed as markdown. A block-scalar description carrying an example fence opened a fence the closing --- never ended, which suppressed the entire body: judgment language and any malformed directive went unexamined and the file passed silently. Frontmatter is skipped and every structural carry resets at its terminator. Corpus effect, measured over every tracked markdown file with the same tree on both sides: 166 records to 162. All four dropped records sit inside frontmatter (do-your-research, discovery/research, playbooks/boris/vendor, review/quality-gate) and every one is a judgment hit on a `description` field — listing metadata, not a procedural step. Three were advisory WARNs and one a note, so no skill's pass/fail verdict changes; the gate loses four spurious hits.
|
Claude finished @kyle-sexton's task in 3m 43s —— View job PR Review — feat(skill-quality): fresh-eyes delegation doctrine + conformance gate (check 21)
Three fixes in
|
| Mechanism | Inherits history? | Satisfies fresh-eyes? |
|---|---|---|
Agent tool subagent_type: "fork" |
Yes — per sub-agents docs | No |
Skill frontmatter context: fork |
No — starts fresh per Skills docs | Yes |
The current wording tells an author that context: fork "carries the same bias forward," which is incorrect. As kyle-sexton's comment documents, this sentence has already produced three downstream corrections: docs-hygiene/#1053, re-anchor/sweep-all-disciplines, and docs/topics/plugin-audit-port/ (being corrected in #1274/#1268). The sentence "A fork does not" is unqualified — the author of any new skill who consults this doc faces the same misreading.
This PR rewrites the exact lines at @@ -386,10 +387,11 @@; it is the natural home for the fix. Minimal surgical change that mirrors what the two corrected downstream sites already say:
-A fresh-context (non-fork) subagent — generic or named — removes it: it starts in its own fresh
-context window, blind to the reasoning under review. A fork does not: it inherits the parent
-session's full conversation history, so it carries the same bias forward
-([subagents](https://code.claude.com/docs/en/sub-agents), verified 2026-07-22).
+A fresh-context subagent — generic or named — removes it: it starts in its own fresh context
+window, blind to the reasoning under review. The Agent tool's `subagent_type: "fork"` does not,
+per [sub-agents](https://code.claude.com/docs/en/sub-agents) (verified 2026-07-22): it inherits
+the parent session's full conversation history and carries the bias forward. A skill's own
+`context: fork` frontmatter is a separate mechanism — per the [Skills docs](https://code.claude.com/docs/en/skills)
+it starts fresh and does satisfy the requirement.One additional note worth carrying in: #1258 reports empirical evidence that Agent-tool forks don't inherit conversation in practice, against the docs. Attributing the claim to the documented specification ("per [sub-agents docs]") rather than asserting it as observed behavior makes the sentence durable either way — consistent with this repo's fresh-docs mandate.
All prior findings — status summary
| Round | Finding | Status at HEAD |
|---|---|---|
| R1 | Info-string line must not close a fence | ✓ Fixed (7454583) |
| R1 | Multi-backtick spans need exact-length pairing | ✓ Fixed (7454583) |
| R1 | Bare "fresh context" without a worker must WARN | ✓ Fixed (7454583) |
| R2 | Opener indentation capped at 3 spaces | ✓ Fixed (84bb658) |
| R2 | verif narrowed to verifier |
✓ Fixed (84bb658) |
| R3 | Backtick fence with backtick in info string is prose | ✓ Fixed (4d6ac5a) |
| R3 | Worker tokens matched as whole words | ✓ Fixed (4d6ac5a) |
| R4 | Container-nested fence openers recognized | ✓ Fixed (3acf21d + 64dfa54) |
| R5 | Nested fence ends with its container | ✓ Fixed (d90c48a) |
| R6 | HTML comments stripped before Form 1 detector | ✓ Fixed (34d8ffe) |
| R7 | Cross-line span carry implemented | ✓ Fixed (f3839c2) |
| R8 | Parsing contract stated; ambiguity posture declared | ✓ Fixed (0aec2b1) |
| Deferred | Mixed container stacks | Documented in "Not attempted" |
| Deferred | Paragraph-interrupting block constructs | Documented in "Not attempted" |
| Deferred | Multi-line HTML comments | Documented in "Not attempted" |
| R9 | Directive-name boundary | ✓ Fixed (d7a1c79) |
| R9 | Ambiguous directive does not satisfy proximity | ✓ Fixed (d7a1c79) |
| R9 | YAML frontmatter skipped | ✓ Fixed (d7a1c79) |
| ALL ROUNDS | Fork conflation in PLUGIN-PHILOSOPHY.md | Still open |
Bottom line: The scanner is in good shape. Nine rounds of correctness work have each been addressed or documented. The single remaining pre-merge issue is the fork conflation in docs/PLUGIN-PHILOSOPHY.md — a targeted prose fix that this PR already rewrites the surrounding lines for.
| Branch
|
Claude finished @kyle-sexton's task in 4m 12s —— View job Security Review — feat(skill-quality): fresh-eyes delegation doctrine + conformance gate (check 21)
ScopeTwo prior security reviews covered this PR through commits Changed in FindingsNo CRITICAL or IMPORTANT security vulnerabilities found in the new commit. Detailed assessment of the three changes: 1. YAML frontmatter skip ( NR == 1 && /^---[ \t]*$/ { fe_fm = 1; next }
fe_fm {
if (/^---[ \t]*$/) {
fe_fm = 0
fe_fence = 0; fe_open_pre = 0; sp_open = 0; fe_icode = 0; fe_blank = 1
}
next
}An unclosed frontmatter (no closing Mitigated by check 1: The reset at the closing 2. Directive name terminator ( Changed from 3. Ambiguous proximity fix ( Added Test harness (fixtures 37w–37y) The three new test cases use heredoc-style static strings in Carried from prior reviewsSUGGESTION (low confidence) — ' "$fe_file")The The safer form: Bottom lineCommit | Branch | |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7a1c79813
ℹ️ 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".
| "name": "skill-quality", | ||
| "version": "0.11.0", | ||
| "description": "Skill-authoring QA tooling: a static contract checker that runs twenty deterministic checks over a Claude Code skill (frontmatter, per-skill listing-entry cap, trigger-keyword preservation, line caps, broken internal refs, markdownlint, gotchas surface, evals presence, precompute opportunity, injection shell-declaration), a shared skill-listing budget reporter across a set of skills, and a bundled evals.json schema for validation. Runs against any repo's skills directory via the convention-resolution ladder — no baked layout.", | ||
| "version": "0.12.0", |
There was a problem hiding this comment.
The manifest now contains two version members, making the advertised plugin release parser-dependent: last-wins readers such as jq see 0.12.0, first-wins readers can retain 0.11.0, and duplicate-rejecting JSON consumers can reject the manifest entirely. This can prevent or misidentify the plugin update, so remove the old member rather than adding a second one.
Useful? React with 👍 / 👎.
| The default worker is a **generic fresh-context subagent carrying rich inline instructions** — the | ||
| task, the artifact, the criteria, and the output shape all travel in the dispatch prompt. A subagent |
There was a problem hiding this comment.
This new dispatch ladder makes a generic fresh-context subagent the default, but the normative rule at docs/PLUGIN-PHILOSOPHY.md:452-454 still glosses the required target as “a named subagent.” An author following that rule can conclude that the generic fallback is nonconformant and create a named agent unnecessarily, contrary to the named-agent bar at lines 518-521; update the normative wording to allow both generic and named workers.
Useful? React with 👍 / 👎.
#1494) ## Summary `plugins/skill-quality/.claude-plugin/plugin.json` carried **two** `version` members on `main`: ```text 4: "version": "0.11.0", 5: "version": "0.12.0", ``` The advertised release was therefore parser-dependent — last-wins readers (`jq`, Python `json.load`) resolve `0.12.0`, first-wins readers retain `0.11.0`, and duplicate-rejecting consumers can reject the manifest outright. Any of those can misidentify or block a plugin update. **Why `0.12.0` is correct.** `main` carried `0.11.0` before #1096, released in #1450, and this plugin's CHANGELOG has `## [0.12.0]` above main's `## [0.11.0]`. The `0.11.0` member is the stale one, and it is what this PR deletes. **Scope.** One file, one deletion. Nothing else in the manifest and nothing else in the repo. **How it happened.** Introduced while resolving a `main` merge in #1096. Both sides of that conflict carried `"version": "0.11.0"` — main had released 0.11.0 in #1450 and the branch also claimed 0.11.0 — so git treated the version line as **common context outside** the conflict region and placed only the differing `description` inside it. A scripted replacement of the conflict region then emitted a fresh `version` + `description` pair above the surviving common-context one. ## Test plan A duplicate-key-aware parse is the only check that observes this, and it is clean after the change: ```console $ python -c "... json.load(..., object_pairs_hook=<duplicate detector>) ..." duplicate keys: NONE version: 0.12.0 ``` Also confirmed: - `git diff --stat` is exactly `1 file changed, 1 deletion(-)`. - The CHANGELOG's top section is still `## [0.12.0]`, so the manifest and the release notes agree. - `changelog-parity-gate` and `plugin-gate` pass in CI on this branch. Worth flagging for reviewers: `json.load`, `jq`, and JSON Schema validation all resolve duplicate keys last-wins, so every one of them reports `0.12.0` whether or not the duplicate is present. That is why this defect passed a fully green 27-context suite on #1096, and it is why the fix is verified with a duplicate-key hook rather than a plain parse. ## Related - #1096 — the PR whose merge-conflict resolution introduced the duplicate member. - #1450 — released skill-quality `0.11.0` on `main`, the collision that produced the conflict shape. - #1493 — follow-up covering the gate gap: no CI check currently detects a duplicate key in a plugin manifest. Closes #1492
…1506) Closes #1498 ## Summary *This was generated by AI during work-loop execution.* No gate in CI could detect a duplicate key in a plugin/marketplace manifest. JSON Schema validates the already-parsed document (structurally blind to a repeated member), and every consumer downstream resolves it last-wins with no signal — the exact shape that shipped `plugins/skill-quality/.claude-plugin/plugin.json` with two `"version"` members through a fully green 27-context suite (#1492). ## Fix Added `scripts/check-manifest-duplicate-keys.py`: reads each `plugins/*/.claude-plugin/plugin.json` and `.claude-plugin/marketplace.json` via `json.loads(text, object_pairs_hook=...)`, intercepting the raw key/value pairs of every JSON object literal (at any nesting depth) *before* the standard de-duplicating collapse into a dict. A duplicate is recorded (not raised) so the parse still completes last-wins — identical to how `json.load`/`JSON.parse`/`jq` resolve it — and every distinct duplicate key in a file is reported in one pass. Wired into the `hygiene` job in `.github/workflows/ci.yml`, directly adjacent to the existing "Validate plugin manifests" step (per the issue's own routing rationale — same file scope, same tier as "does it parse"/"does it match the schema"), gated the same way the four `check-jsonschema` steps are (skipped only on a provably docs-only diff), and its outcome is added as one line to `scripts/aggregate-hygiene-results.sh`'s existing `CHECK_RESULTS` block so a duplicate key fails the job exactly like any other hygiene check. ## Verification - `bash scripts/check-manifest-duplicate-keys.test.sh` — 17 unit/CLI tests, all passing (nested-object duplicates, sibling-object non-duplicates, multiple distinct duplicate keys in one file, malformed JSON *not* double-reported, default-discovery glob covering both `plugin.json` and `marketplace.json`, missing-file handling, exit codes). - `test_catches_the_1492_shaped_duplicate_version_key` reproduces the actual #1492 defect shape (two `"version"` members) as an inline fixture and asserts the gate fails on it — the fixture proving the gate catches a real duplicate key, per the issue's own ask. - Live empirical cross-check against the two claims in the issue's verification table, run locally against a copy of the real (now-fixed) `plugins/skill-quality/.claude-plugin/plugin.json` with the duplicate `"version"` member re-injected: - `check-jsonschema --schemafile https://json.schemastore.org/claude-code-plugin-manifest.json <file>` → `ok -- validation done`, exit 0 (confirms schema validation is blind to it). - `python3 scripts/check-manifest-duplicate-keys.py <file>` → `DUPLICATE KEY: ... defines 'version' more than once ...`, exit 1 (confirms the new gate catches exactly what schema validation cannot). - `python3 scripts/check-manifest-duplicate-keys.py` (default discovery, no args) against the actual repo tree → `No duplicate JSON object keys found in 62 manifest file(s).`, exit 0 — no false positives on the real manifest set. - `actionlint .github/workflows/ci.yml` — clean. - `shellcheck --rcfile=.shellcheckrc scripts/check-manifest-duplicate-keys.test.sh` — clean. - YAML parse + step-order check of the modified `hygiene` job — new steps land in the intended position, ids wired correctly into `CHECK_RESULTS`. - `typos --config _typos.toml` over the new/changed files — clean. - No `plugins/*` manifest touched, so the per-plugin CHANGELOG-parity gate does not apply to this change. ## Related - #1492 — the shipped duplicate-key instance this gate would have caught. - #1494 — the manifest fix for #1492. - #1096, #1450 — the version-collision conflict shape that produces this defect class. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
Phase 1 of the fresh-eyes program (#304), per
docs/topics/fresh-eyes-checkpoint-audit/PLAN.md:the delegation-mechanics doctrine and its deterministic conformance gate land together, so every
Wave-2 retrofit PR that follows conforms to a merged spec.
docs/PLUGIN-PHILOSOPHY.md— new## Delegation mechanicssection beside the existingfresh-eyes section: dispatch ladder (generic fresh-context subagent with rich inline
instructions = default; named agent behind the earned bar; cross-vendor advisor on top, always
presence-gated with the generic fallback), inline-template conventions (fresh-context wording,
artifact-not-story, degrade-when-absent), named-agent bar, model-tier ladder with a dated
tier-to-model mapping and recheck trigger, tool-cage framing (Bash-holding allowlists are not
read-only), and the two declared patterns with the
skill-qualityplugin as spec owner(convention-registry row added). Every load-bearing claim cites its official page, fetched and
verified 2026-07-22 (six dated citations). Existing fresh-eyes section reconciled: "a named
subagent removes it" → fresh-context (non-fork) subagent, generic or named.
check-skill.shcheck 21 — fresh-eyes declaration conformance — implements the semanticstable in
docs/topics/fresh-eyes-checkpoint-audit/design/design-resolution.mdverbatim:FAIL on malformed/unknown-class/reason-less
fresh-eyes-exemptdirectives; WARN-onlyjudgment-language heuristic (curated POSIX ERE, seeded from the audited skills' phrasing);
stale-directive WARN (advisory); per-file line proximity (
FRESH_EYES_PROXIMITY_LINES, tunableconstant); fence- and inline-code-span-aware detectors (self-reference guard); CRLF-tolerant;
scan surface = SKILL.md + skill-internal spoke dirs,
vendor/andevals/excluded.plugins/skill-quality/skills/check/reference/fresh-eyes-declarations.mdcarries the mechanical contract (grammar, classes, semantics table, curation policy); check-21
messages point at it so third-party authors can read the rule without this repo's doctrine.
undeclared judgment language / stale directive; PASS delegation prose (both spellings), valid
exemption, fenced literals (self-reference guard), no judgment language.
descriptions "twenty" → "twenty-one"; also fixes pre-existing stale counts (plugin README still
said "eighteen"/"seventeen" after checks 19–20 shipped; the checks list gained the missing
injection-portability row).
Plan deltas (re-verified against current main per the plan's own freshness mandate)
docs/MIGRATION-PLAYBOOK.md:984no longer applies: the current"seventeen" hit (line ~1115) is a dated 2026-07-13 decision record describing what the plugin
shipped THEN — historical, true at its date, left intact.
doctrine claims verified against current official pages; no Brief constraint reopened. Notable
current facts cited in the doctrine: frontmatter
modeldefaults toinherit;CLAUDE_CODE_SUBAGENT_MODEL(model-config page) overrides both the per-invocation param andfrontmatter; no per-plugin model seam in
userConfig.Verification
shellcheckclean oncheck-skill.sh;bash plugins/skill-quality/scripts/check-skill.test.shexits 0 — 56 assertions including 10 new check-21 fixtures (fixture demo of the FAIL half: the
malformed-directive fixture exits 1 naming the defect).
npx markdownlint-cli2clean on every changed markdown file.empirically re-verified (consumer-facing spec path in messages; word-boundary guards on the
judgment regex — "upgrade your own"/"underscore its own" no longer hit; CommonMark
fence-matching so nested
~~~/contradictory-declaration branch).
untouched and the suite proves no regression there, so the full-gate baseline diff is vacuous —
the only possible gainers are check-21 emissions. The shipped check-21 scanner (extracted
verbatim at runtime) ran over all 173 skills: 0 FAIL, 56 WARN sites in 24 skills, every
site triaged and dispositioned (
.work/fresh-eyes-checkpoint-audit/baselines/ check21-warn-triage-20260722.md): expected tranche-1/2 gainers ride Phases 2/4; the rest arehand-verified meta-text/quote/narrative hits; zero mechanical false hits, no regex change
warranted. A clean full-gate pre-change baseline at the merge-base is retained alongside.
Related
(retrofits, corrections, tranche-2 filing).
prematurely close the multi-phase program).