Addresses review on #603.
The comma claim was wrong. The code comment said a leading `,` parses cleanly;
PyYAML rejects it with a ParserError and psych with a SyntaxError. The same
comment was wrong about `]` and `}`. I had asserted all three without measuring,
and a test pinned the comma claim, so the mistake was defended rather than
caught.
Now flagged, each verified against PyYAML and psych: `,` `*` and a one-line
`|`/`>` header. A bare `|` with indented lines under it is a real block scalar
and stays accepted; `|foo` is not one.
Still not flagged, because each parses: `&anchor`, `#`, `:`/`-` without a
following space, and `[`/`{`, which open valid flow collections. `]` and `}` are
rejected by both parsers but left out for now - no frontmatter has ever started
with them, and widening past what was asked risks false positives I have not
thought through.
Docstring no longer says "the three shapes".
docs/advanced-per-agent-configuration.md says vendor and runtime fields
such as model, tools, max_turns and context must not appear at the top
level of a SKILL.md: hosts do not promise to ignore unknown keys and a
strict client can reject the file. Nothing enforced it; a skill with
model: at the top level passed CI.
Add SPEC_FRONTMATTER_KEYS (name, description, license, compatibility,
metadata, allowed-tools) and an error for any other column-zero key,
naming the key and pointing at the guide. Nested lines under metadata and
list items are children of a spec key and are left alone. Export
topLevelFrontmatterKeys for direct tests. validate-commands.js shares only
frontmatterYamlErrors, so command frontmatter is unaffected.
All 25 skills use only name and description, so the rule fires on nothing
today and follows the #571 pattern of errors for rules nothing violates.
The self-exemption test now expects two errors, since exempt: is also not
a spec key.
Two follow-ups from the review on #600, both already on main before it:
- The diff walk treated any line starting with `---` or `+++` as a file
header. Inside a hunk those lines are content. An added line that starts
with `++` (`++n; // eslint-disable-line no-plusplus`) shows as `+++n; ...`
and was dropped, so the suppression passed. An added line that starts with
`++ ` was taken as a header, so the lines after it were filed under a
made-up path and a removed assertion further down the same test file went
unreported. A removed line starting with `--` never reached the removed
list either. Headers are now read only between a file's `diff` line and
its first `@@`.
- Run from a subfolder, `git ls-files --others` listed only that
subfolder's untracked files, relative to it, while `git diff` covers the
whole tree from the top. An untracked suppression elsewhere in the
repository was missed, and one inside the subfolder was reported under a
different name from the tracked files. The guard now changes to
`git rev-parse --show-toplevel` first, and exits 2 outside a work tree.
The test gains six cases: five fail on main, and the sixth (outside a work
tree) passes on both and covers the new exit-2 path; 28 pass. The test now
also runs in the macOS/Windows matrix.
validate-artifact-paths.js exists to stop the #93 class of drift, where a
producer moves SPEC.md or tasks/plan.md and a consumer keeps reading the
old path. It guarded only the .claude/commands copies plus two skills and
two docs. The same commands ship twice more — .gemini/commands/*.toml and
the root commands/*.toml — each with its own prompt body that names the
same artifacts, and validate-commands.js deliberately compares only
descriptions, so a drift on either TOML surface merged green while the
Claude Code copy stayed canonical. docs/copilot-setup.md was also unguarded
although its prompt-file aliases spell the artifact paths out for users.
Add the six TOML producers and consumers and the Copilot doc to
GUARDED_FILES, grouped by surface. Rule, allowlist and regex are unchanged.
Four new tests cover a Gemini producer drifting alone, a root TOML consumer
drifting alone, all three surfaces agreeing, and the Copilot doc. All seven
newly guarded files already use canonical paths: 14 files checked, 0 errors.
The negated-trigger check removed only the first "Do not use when …" clause
before testing whether a positive trigger remained, because the replace used the
non-global DESCRIPTION_TRIGGER_NEGATE. A description carrying two or more negated
clauses and no positive trigger left a second "use when" substring behind, which
DESCRIPTION_TRIGGER then matched, so the skill passed validation with no trigger
at all.
One negated clause was correctly rejected; two were accepted. Adds a global form
of the pattern for the strip and keeps the non-global one for .test(), since a
/g regex carries lastIndex across test() calls.
Follow-up from the review on #600 (@abhisheksharma2411): a number that
gains or loses its direction words (">= 80%" becoming "80%", or the
reverse) is paired within its direction and so reads as removed. That is
kept on purpose, since the rule changed shape and a person should look once,
but the output now says so, as it already does for a renamed rule, so the
finding is not read as a literal deletion.
One case added for a minimum losing its direction words: 22 pass, and the
new case fails without the message.
Review on #600 (@abhisheksharma2411) found three cases in the threshold
comparison:
- Deleting one threshold from a rule that keeps others was silent: pairing
was by position, and a threshold with no counterpart after the edit was
skipped. It is now reported as threshold-removed.
- A number inserted earlier in a rule shifted the pairing, so "per ADR 7,
>= 80%" compared 80 against 7 and reported a loosened minimum. Numbers are
now paired within their direction (minimum with minimum, maximum with
maximum, undirected with undirected); an added number tightens and stays
silent.
- A renamed rule is still reported as rule-removed, since the key is the
label and blocking is the safe default, and the output now says the label
may have changed.
The review's cases are in the test, plus a control that adding a threshold
is silent: 21 pass, and the three review cases fail on the previous commit.
The reference guard in constraint-driven-development promised to include
untracked files, catch deleted test files and weakened thresholds, and stay
silent when constraints tighten. Run unchanged, it did none of those for
ordinary local git changes:
- `git diff --no-index /dev/null <file>` exits 1 whenever the sides differ,
and the shared git() wrapper turned every nonzero exit into null, so every
untracked file diffed as empty and no added-line rule could see it.
- The filename came from the `+++` header only, so a deletion
(`+++ /dev/null`) sliced to `ev/null` and matched nothing; there was no
deletion rule, and staging did not help.
- A number going down was treated as weaker, which is right for a minimum
(coverage) and inverted for a maximum (size, latency, error budget).
- A removed floor bullet was never reported, although the comment said it was.
The wrapper now keeps stdout on the expected exit 1 and every other failure
exits 2 rather than reading as clean; both diff headers name the file and a
deleted test file is its own finding; each threshold carries a direction read
from the words around it (>=, at least, must not fall; <=, at most, under,
must not grow), a number with no readable direction is reported when it
changes; a removed floor bullet or table row is reported, except an Exceptions
row, whose removal tightens. The three language patterns are unchanged.
scripts/floor-guard-reference-test.js extracts the fenced block and runs it
with real git against fourteen fixtures plus three edge cases; eight of them
fail on the previous reference and all pass now. The workflow runs it beside
the other script tests.
The frontmatter YAML checks catch an unquoted `: `, a tab indent and an
unterminated quote, but not a plain scalar that *begins* with a character YAML
reserves. A real parser rejects the whole file on those, which is the failure
the check exists to prevent.
The backtick is the one that bites this repo. Descriptions routinely name other
skills, and
description: `other-skill` designs things. Use when …
is a natural way to start one. It passes the validator today and throws
`YAMLException: bad indentation of a mapping entry` in any consumer that parses
the frontmatter.
The rejected set was measured against a real parser rather than read off the
spec, every candidate indicator tried both with and without a following space,
and only the ones invalid in *both* forms with no legitimate single-line use
are rejected:
always invalid: ` @ %
invalid before a space: - ? & (`:` was already covered)
Deliberately still accepted, because each parses cleanly and flagging it would
be a false positive on valid frontmatter: `:` or `-` when *not* followed by a
space (`:platform`, `-hyphenated`), a leading `,`, `]`, `}` or `#`, and `[`/`{`,
which open valid flow collections. `|` and `>` are left alone because they
legitimately open a multi-line block scalar. Those exclusions are pinned by a
test of their own, so the rule cannot be widened into false positives later —
both widening mutations are caught by exactly that test.
Cross-checked the finished rule against the parser over 38 values spanning every
indicator in both forms: **0 false positives, 11 remaining misses**, all of them
the deliberate exclusions above.
Seven mutations caught: removing either rule, dropping the backtick or the at
sign from the set, widening either rule (to anywhere in the value, or to ignore
the required space), and letting quoted values fall through to the new checks.
The repo's own 25 skills still pass with 0 errors and 0 warnings.
docs/skill-anatomy.md states three structural rules that nothing checks:
the 500-line context budget for SKILL.md, no empty subdirectories, and
lowercase-hyphen-separated supporting filenames. skill-lint.js calls
itself the single source of truth for that document, so the rules belong
there rather than in a sixth validator script.
The line-count rule is a warning: an over-budget skill is an editorial
call, and failing CI on it would block unrelated work. Layout rules are
errors — nothing on main violates them, so they cannot break the build.
Also wires scripts/lib/skill-lint-test.js into CI. It has never run
there; every other *-test.js in scripts/ has its own step.
`descriptionFromMd` in validate-commands.js splits each frontmatter line on its
first colon — the same forgiving read the skill linter used before the strict
rules landed. So a command whose frontmatter is not valid YAML passes every
check here, and Claude Code parses that frontmatter when the command is loaded.
Same class of gap as the SKILL.md one, on the other set of files: the #494
thread verified all 25 SKILL.md files *and* the command files by hand. This
makes the second half a check too, sharing the rules rather than restating them.
All nine current commands already pass — confirmed independently against ruby's
psych before writing this — so it is a guard, not a repair. That is the same
shape the skills half had, and the same reason: the manual verification was
real, it just could not stay true on its own.
Scoped to `.claude/commands/*.md`. The `.gemini/` and `commands/` directories
are TOML and already go through a real TOML parser, so a malformed one surfaces
as a missing description in the existing check. A test pins that the YAML rules
are NOT applied to them, since a TOML description legitimately carries a colon
and running these rules over it would fail nearly every command.
The test sandbox copies the validator into a temp tree, so it now copies
scripts/lib/skill-lint.js beside it. Without that every existing test fails with
a module-not-found, which is how I found it.
Five new tests. Mutation-tested 2/2: skipping the check, and reporting its
errors without counting them, each fail two. Every validator exits 0 and all six
script suites pass.
Refs #494
`parseFrontmatter` splits each line on its first colon. That is enough to read
`name` and `description`, and it is forgiving by design — but it means a
SKILL.md whose frontmatter is not valid YAML passes every check in the linter.
The hosts that consume these skills do not share that forgiveness. Per Cursor
staff in #494, attaching a skill to a message parses its frontmatter as YAML,
and a parse failure fails the request with a generic server error and takes the
rest of the chat's context with it. So the repo could ship frontmatter that
breaks a host while CI stayed green.
Three shapes pass the current linter and are rejected by a strict parser
(confirmed against ruby's psych, each isolated by mutating only the
frontmatter of a real SKILL.md that otherwise lints clean):
description: Use this: when X colon-space: YAML reads a nested mapping
\tkey: value tab indentation is invalid YAML whitespace
description: "unterminated an unclosed quote
All three now fail with the line number and the fix. Deliberate boundaries:
a colon with no trailing space is untouched, so `https://example.com/a:b`
still passes, and a duplicate key is NOT reported because `safe_load` accepts
one — the rule tracks the parser, not taste.
This was verified once by hand against all 25 skills in the #494 thread. This
makes it a check instead of a memory.
Narrow on purpose: the repo has no package.json and therefore no YAML parser to
depend on, so this is a small set of rules aimed at the classes actually
observed to break a host, not a YAML implementation. No current frontmatter
uses block scalars or multi-line values and the check says so.
All 25 skills still pass with 0 errors, every validator exits 0, and the six
script suites are green (35 pass in skill-lint-test). Mutation-tested 4/4:
removing the call, widening the colon rule to any colon, and dropping either
the tab or the unterminated-quote rule each fail the new tests.
Refs #494
A file under skills/<name>/references/ sits one directory deeper than SKILL.md, so a link to a shared checklist needs ../../../references/ from there. The validator only read SKILL.md, which left that off-by-a-level mistake unguarded in the one place it can now happen (hardening-patterns.md on main, optimization-patterns.md in #580).
The rule is unchanged: only references/*.md links, fenced blocks exempt, paths that do not exist yet ignored. The file set grows to include the markdown files directly inside each skill's references/ directory, and every link resolves from the file that contains it. The failure hint names the three-level prefix when the violation is in a reference file.
Extract the two fs.rmSync calls into clearGradingSlot(base), export it,
and call it from runBehavioral. Rewrite both slot-clearing tests to call
clearGradingSlot instead of inlining fs.rmSync, so each test exercises
the production cleanup path and fails if the call is removed.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move stale-file cleanup from persistGradingOutcome (which only ran on
rejected grading) to the top of each eval iteration, before the executor
is invoked. Both ${base}.grading.json and ${base}.grading.raw.txt are
removed with fs.rmSync(p, { force: true }), which ignores a missing file
but still throws on a permissions error.
This closes three gaps at the source:
- The bare catch around unlinkSync swallowed EACCES alongside ENOENT.
- An executor or grader crash skipped persistGradingOutcome entirely,
leaving the previous run's .grading.json intact.
- A successful grading left a stale .grading.raw.txt from a prior
rejected run beside the new .grading.json.
persistGradingOutcome now only writes, never deletes.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Each .grading.json now carries a run object with executor_model,
grader_model, and timestamp so results can be attributed to a
specific model version and point in time.
The executor model is read from the init event of the stream-json
output that the executor already produces (type "system",
subtype "init", top-level model field). The grader model is recorded
as "unknown": the grader is a bare `claude -p` whose output
parseGrading extracts by regex, so switching it to stream-json would
break parsing, and adding --model is a behaviour change that does
not belong in a recording fix.
RESULTS_DIR is only ever created and never cleared, and the output
path is derived from skill name and eval id alone. When a run's
grading is rejected (parseGrading returns null), the runner writes
.grading.raw.txt but leaves any .grading.json from a previous
successful run in place. A collector reading that directory gets the
old score beside the raw output of the run that actually happened.
Extract persistGradingOutcome so the write-and-cleanup is a testable
unit, and unlink the stale .grading.json before writing the raw file.
Reuse the skill linter's fence parser so inline backticks cannot hide broken references and trailing text cannot close a code fence. Cover both failures with LF and CRLF regression cases, preserving diagnostic line numbers.
Two hardening changes to parseGrading:
- Once a result's id is validated, overwrite its text with the declared
expectation at that index. The id is the binding, so a grader that
paraphrases an expectation no longer leaks its own wording into the
saved grading report.
- Stop rejecting a grading whose summary.pass_rate disagrees with the
counters. pass_rate is derived, not an independent observation, so a
rounded or mis-divided value is now recomputed from the validated
results instead of discarding the run. The integer counters
(passed/failed/total) remain exact-match checks.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
parseGrading accepted any result set whose length matched the expected count, without verifying that each result corresponded to a declared expectation. A grader could return duplicates, omit an expectation, or include results for undeclared expectations and still pass validation.
parseGrading now takes the full expectations array, requires each result to carry an integer id in 1..n matching the numbered expectations in the prompt, rejects duplicates and out-of-range ids, and derives pass_rate from the validated counters. The grader prompt requests the id field and the call site in runBehavioral is updated.
Five tests added: reordered-but-complete results, duplicate ids, out-of-range ids, omitted expectations, untrusted pass_rate.
stripFencedCodeBlocks matched fences with a single whole-document regex that
only recognised a column-zero backtick fence closed by a run of exactly the
same length. Three CommonMark-legal forms slipped through and their contents
were linted as prose: tilde fences, fences indented one to three spaces, and
closing fences longer than their opener. This is live on main today —
spec-driven-development's list-nested examples use indented fences, so the
linter has been reading their contents as skill text.
Replace the regex with a line scanner that tracks the opener's marker and
length, closes only on a same-marker run at least as long, treats an
unterminated fence as running to end of file (so sections after it fail
loud), and follows the CommonMark rule that a backtick fence's info string
may not contain backticks. Stripped lines become empty lines so positions
are preserved.
Add unit coverage for each form plus CRLF input, and run
scripts/lib/skill-lint-test.js in CI — it was the only test file the
workflow did not execute.
Revives the approach from #444 by @ujjwalks, which was approved and then
closed after a rebase conflict.
Closes#437
`validate-reference-links.js` scans every line of a SKILL.md, so a
`references/…md` path shown inside a fenced code block is treated as a link
to resolve. The rule this validator enforces therefore cannot be documented:
its own failure message tells authors to write `../../references/<file>.md`
rather than `references/<file>.md`, and a skill showing that contrast as an
example fails the check that is explaining it.
Reproduced on main by appending a fenced markdown example to
using-agent-skills/SKILL.md — exit 1, with the guidance text printed
underneath the very link it had just rejected.
Lines inside a fence are now skipped. The scan is line-by-line rather than
one regex over the document so the three CommonMark fence forms all hold:
tilde fences, up to three spaces of indent, and a closing fence longer than
its opener — and a ``` line inside a ~~~ block does not close it.
Line numbers are preserved by collecting fenced line indices and skipping
them during the scan, rather than stripping the text first, so the L<n> in
an error still points at the real line.
Deliberately local rather than shared with scripts/lib/skill-lint.js: that
module's stripFencedCodeBlocks is the single regex with the gaps described
in #437, and #444 is already rewriting it. Forking a rewrite in flight
costs more than the duplication; once #444 lands the two should collapse
into one helper under scripts/lib/.
Three tests added. All three fail against the current validator; the third
pins that a genuinely broken link outside a fence is still reported, so the
exemption cannot silently swallow real breakage.
`dirName in SECTION_EXEMPT_SKILLS` walks the prototype chain, and
`constructor` satisfies KEBAB_CASE (the `*` quantifier permits a single
word). A skill directory named `constructor` therefore resolved to
Object.prototype.constructor and was silently exempt from every
required-section check — `exempt: true`, zero errors, no sections. The
bypass guard missed it for the same reason, since the resolved value is
truthy.
Both lookups now use Object.hasOwn, which is what the comment above the
allowlist already promises: "Exemptions live HERE, not in skill
frontmatter, so contributors cannot bypass the validator by editing their
own skill file."
Adds scripts/lib/skill-lint-test.js — skill-lint.js was the only
validator without a test file. Confirmed the prototype-chain test goes red
against main; the other six pass either way, so existing behaviour is
unchanged.
Scope kept to the exemption lookup: #428 already rewrites frontmatter
parsing and the cross-reference patterns, so this stays clear of both.
The path fix has no regression guard: nothing in CI resolves references/
links, so all 18 were broken while CI stayed green. validate-artifact-
paths.js is scoped to spec/plan/todo artifacts and says in its own header
that it is not a general markdown path linter.
Add validate-reference-links.js, which resolves every `references/*.md`
link in skills/*/SKILL.md against that skill's own directory. This accepts
both conventions in CLAUDE.md: shared checklists reached via
../../references/, and a skill's own colocated references/ directory.
Scope stays narrow on purpose. Skills legitimately name paths that do not
exist yet -- tasks/todo.md, PERF.md, docs/ideas/[idea-name].md -- and a
general markdown linter would fail the build on them. A test pins that.
Proven against the pre-fix tree: 18 error(s), exit 1, matching the 18
links fixed in the previous commit. After the fix: 0 error(s), exit 0.
7 unit tests cover the regression itself, colocated references/,
markdown-link syntax, a renamed target, multiple violations in one skill,
and the non-reference paths that must be ignored. Wired into the
validate-skills job, alongside the other skill-content checks.
Known limitation: fenced code blocks are not stripped, so a SKILL.md that
documents the anti-pattern inside a fence would be flagged. Nothing does
today. Sharing stripFencedCodeBlocks looks right once #444 lands.
Refs #468
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The /spec and /plan commands write spec, plan, and todo artifacts to paths that /build and the spec/plan skills read back. When a producer moves an artifact without updating the consumers, the pipeline breaks and nothing in CI catches it: the command-parity check only compares descriptions, not paths. PR #93 hit exactly this, pointing /spec and /plan at docs/features/[name]/ while /build still required SPEC.md and tasks/plan.md.
validate-artifact-paths.js enforces one canonical set of artifact paths across every file in the pipeline (the spec/plan/build commands, the spec-driven-development and planning-and-task-breakdown skills, and the getting-started and adoption guides). Any spec/plan/todo artifact path outside the allowlist fails CI, so changing the convention has to touch the allowlist and every guarded file in the same change.
Scope is deliberately narrow: only spec/plan/todo artifacts, only the pipeline files; it is not a general markdown path linter. Wired into the validate-commands CI job alongside its test.
materializeWorkspace() creates directories in os.tmpdir() but
runBehavioral() never cleans them up on success or failure. If
execFileSync throws (timeout, signal, OOM), the workspace persists
in /tmp indefinitely with fixture data. The test file
run-evals-test.js uses try/finally cleanup (line 234), proving
the pattern was known but omitted in production code.
Fix: Wrap eval execution in try/finally with best-effort
fs.rmSync cleanup.
The skillName argument from --behavioral has zero validation and is
used directly in path.join() for reading .json files, reading
SKILL.md, and WRITING .grading.json results. A value like
'../../etc/passwd' traverses outside the project directory for
both reads and writes.
Fix: Validate skillName against /^[a-z0-9]+(-[a-z0-9]+)*$/ before
any filesystem operations. This matches the existing KEBAB_CASE
validation in skill-lint.js.
Joan Leon