Applies the review suggestions on #580, which carry nucliweb's three points:
the condensed index bullet lost the partial, trigram/full-text and expression
index qualifiers, it no longer said a bad rows= estimate means ANALYZE, and
the caching bullet dropped the staleness window the Red Flags still audit.
Co-authored-by: Joan León <nucliweb@users.noreply.github.com>
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".
Step 2 asks "Would the tests catch a regression if the code changed?"
but gives no way to answer it. Reading the tests answers it badly: a
check tested in only one direction looks covered. Inverting one
condition of the change and running the suite answers it in one run,
and a surviving mutation names the missing test case.
Assisted-by: claude-code:claude-opus-5-5
Agent-Session: https://claude.ai/code/session_01282cSgXhjM3B6sZA1ZRCSF
Agent-Host: 0493f0
Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
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.
docs/advanced-per-agent-configuration.md calls itself the canonical
reference for keeping SKILL.md frontmatter portable, yet nothing in the
repository links to it. Add the frontmatter rule it states to the anatomy
doc's Frontmatter section, and point at it from the README contributing
paragraph and CONTRIBUTING's frontmatter step, so contributors meet the
rule where they read the format.
The template's Status line listed Accepted, Superseded by ADR-XXX and
Deprecated, while the ADR Lifecycle section below it starts at PROPOSED,
and the behavioral eval (since #598) accepts any status from that
lifecycle. An ADR written from the template before the decision is made
had no status to record. Add Proposed to the line.
Raised in the review on #598 (@abhisheksharma2411) and by the maintainer
there.
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.
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
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.
Addy Osmani