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.
`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.
Two heuristic bugs in the section and trigger checks, surfaced in
addyosmani#387 and triaged by @addyosmani and @nucliweb:
Section check: content.includes('## Overview') matches headings inside
fenced code blocks and sub-headings (### Overview) because substring
matching can't distinguish prose from examples. Replace with a
line-anchored regex match against content that has had fenced code
blocks stripped, so only real top-level headings satisfy the check.
Trigger check: 'Do not use when testing' satisfied the DESCRIPTION_TRIGGER
regex because it contains 'use … when'. Add a negation pattern and reject
descriptions where the only trigger match is negated.
Verified: all 24 skills pass (output unchanged — no current skill has
either bug pattern). Crafted edge cases confirm the fixes catch both
issues. Eval suite green, rank-1 rate unchanged at 86%.
Co-authored-by: Cursor <cursoragent@cursor.com>
Per review: the policy collections (REQUIRED_SECTIONS, SECTION_EXEMPT_SKILLS,
SKILL_REF_PATTERNS, regexes) were exported by reference, so a consumer could
mutate shared state and change lint results process-wide. Export only the
linting functions; keep the policy collections private. No behavior change
(validator output byte-identical).
Split the skill validation rules out of the validate-skills.js CLI into a
shared, importable scripts/lib/skill-lint.js: a pure lintSkillContent() with
no I/O plus a thin lintSkill() file wrapper. validate-skills.js becomes a thin
CLI over the lib. No behavior change to the validator's output or exit codes;
this only makes the rules unit-testable for a follow-up test battery.
Two latent Tier-3 bugs caught in review before the path was ever exercised:
- The grader prompt embeds the full stream-json trace (up to megabytes) and
was passed as an argv entry, which would fail with E2BIG on any real run.
It now goes to `claude -p` over stdin; the executor prompt moves to stdin
for the same reason.
- The executor ran headless with no permission mode, so file edits and
command runs could be denied, forcing the narrate-instead-of-perform
failure mode that trace grading exists to catch. It now runs with
--permission-mode acceptEdits and a pre-approved tool list
(Read,Glob,Grep,Edit,Write,Bash), documented in the README.
Per review from @federicobartoli and @nucliweb on #342:
Tier 3 (behavioral):
- Grade the execution trace, not the final output: executor runs with
--output-format stream-json --verbose so the grader judges tool calls
and file edits rather than the model's self-reporting.
- Run each eval in a throwaway workspace; files[] fixtures materialize
from evals/fixtures/ so evals can operate on real code.
- Node-level timeouts on executor and grader calls; grader output parsed
and shape-validated before writing (raw saved on failure); the trace is
fenced as untrusted data in the grader prompt.
- All 24 behavioral evals flagged trust_level: "provisional" until they
gain fixtures; the runner surfaces this and exits nonzero on failed
expectations.
Tier 2 (deterministic):
- Negative triggers accept an "owner" skill that must outrank this one,
turning them into pairwise routing tests that cannot pass vacuously;
37 of 48 negatives now declare owners (the rest are tracked in #351).
- Warn when a case file is below the documented minimums (3 positive /
2 negative / 1 behavioral); promotion to error tracked in #352.
- Stemmer: cluster trailing y/i ("simplify"/"simplifies").
Baseline holds: 120 checks, 0 errors, 85% trigger rank-1 rate.
There was no way to measure whether skills trigger correctly, stay
distinct, or change agent behavior. This adds evals, aligned with what
the community has converged on, with a deterministic CI tier on top:
- evals/cases/<skill>.json for all 24 skills. The evals[] block uses
Anthropic skill-creator's evals.json schema verbatim (id, prompt,
expected_output, expectations[]) so its runner, benchmarks, and eval
viewer work against our files unmodified. A trigger block (this
repo's extension) adds positive/negative routing prompts per skill.
- scripts/run-evals.js, zero-dependency runner:
Tier 2 (CI): trigger evals via stemmed TF-IDF ranking over skill
descriptions (positive prompts must rank top-k, negative prompts
must not rank first), catalog collision detection between skill
descriptions, schema and coverage checks.
Tier 3 (opt-in): --behavioral <skill> executes each eval through
headless claude -p and grades the transcript against expectations[]
(superpowers-style); --dry-run previews without spending tokens.
- CI: run the deterministic tier in the validate-skills job.
- Docs: evals/README.md defines the framework and prior art;
CONTRIBUTING requires an eval file for new skills (warning-level in
the runner until in-flight skill PRs clear); CLAUDE.md pointers.
Current baseline: 120 checks pass, 85% trigger rank-1 rate across 72
positive prompts, zero catalog collisions.
The validator claims to check skills "against the rules in
docs/skill-anatomy.md", but two rules that doc marks as Required were
never enforced:
- Directory names must be lowercase-hyphen-separated (Naming Conventions).
Previously only `name === dirName` was checked, so `My_Skill/` passed.
- Descriptions must say *when* to use the skill, not just what it does
(Required vs Recommended). Formalized in #167 but the validator was
never updated to match.
Adds both as blocking checks. All 24 existing skills pass, so this is a
non-breaking guardrail. Closes the remaining gap in #233 (frontmatter,
name-match, and section checks already shipped in the original validator).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mavericksea-ai