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.
The behavioral eval for documentation-and-adrs graded "The document is
written in timeless language describing current state". SKILL.md never
states that for ADRs: the wording comes from references/definition-of-done.md,
which covers documentation in general, while the skill's own ADR template
carries a Status (Accepted | Superseded by ADR-XXX | Deprecated) and a
Date and defines the lifecycle PROPOSED -> ACCEPTED -> (SUPERSEDED or
DEPRECATED). Grading against a requirement the skill does not state left
the judge to decide, and over the ten runs in #591 it was the one
expectation whose reading varied (3 pass, 2 fail on one machine; 5 pass
on another) while the first two passed in all ten.
Replace it with the requirement the skill does state: the ADR records a
status from that lifecycle and a date. The skill is unchanged, per the
reading in #591.
Fixes#591.
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.
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
Add platform coverage for all six Node test suites, validators, and routing checks while preserving existing Ubuntu check names. Require both matrix jobs before plugin validation and retain failures from each platform.
#579 moved the code patterns verbatim, but three prose sections (supply chain, privacy, LLM) and the cookie bullet were rewritten rather than moved, and some rules ended up in none of SKILL.md, the reference, or the shared checklist.
Restores them inline: the sameSite value and its CSRF reason, why forced audit fixes are dangerous, the audit signatures commands and "not automatic proof of compromise", the retention TTL, auditable consent, the (2025) on the OWASP LLM list, and "the full system prompt" plus "validate every tool argument" in the LLM rule. Red Flags already audits the system-prompt item, so the rule and its audit agree again.
Same 216 lines, +55 words.
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.
SKILL.md loaded ~5.3k tokens on every activation, 30% of it inline
TypeScript examples that an agent only needs when it reaches that code.
docs/skill-anatomy.md already asks for SKILL.md under 500 lines with
reference material in supporting files; this skill was at 524.
Move the OWASP prevention patterns, input-validation and upload code,
SSRF check, destructive-path discussion, dependency-audit decision tree,
rate-limiting and secrets snippets, data-classification table, and LLM
output example verbatim into references/hardening-patterns.md (same
convention as constraint-driven-development/references/floor-guard.md).
SKILL.md keeps the workflow: threat model, three-tier boundaries, and a
condensed "Hardening Controls" section stating each rule with a deep link
to its pattern. Frontmatter, Process, Rationalizations, Red Flags, and
Verification are byte-identical to before.
The skill's own review checklist duplicated references/security-checklist.md;
replace it with a pointer and add its three privacy items to the shared
checklist so nothing is lost.
SKILL.md: 524 -> 216 lines, 4096 -> 2449 words (~-40% tokens per load).
Routing unchanged: run-evals rank-1 stays 100%, no description edits.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The round-trip test added in this PR failed on ubuntu-latest while passing
on macOS: after Read → Stop, a protected block containing a backslash came
back altered (byte 109, line 3 of the fixture).
expand_file rebuilds each line with `${line//$placeholder/$block_content}`.
Bash 5.2 enables patsub_replacement by default, which makes the replacement
side of that expansion special: an unquoted `&` expands to the matched text
and backslashes are consumed. Block content is raw code and routinely
contains `&&` and backslashes, so on any bash 5.2 host every protected block
with either character was corrupted when it was expanded back. Bash 3.2 on
macOS has no such option, which is why local runs never showed it.
Turn the option off at the top of the hook; older bashes reject the shopt,
so the error is ignored there. Extend the byte-fidelity fixture with `&&`
and `&` so the ampersand case is guarded alongside the backslash one.
Wire the two regression suites into the workflow as a test-hooks job
so the coverage actually guards the hooks, per review. Deliberately
excludes session-start-test.sh: it currently asserts the pre-#474
payload shape and fails against the envelope the hook now emits;
PR #510 updates that test and can wire it in when it lands.
mavericksea-ai