feat: derive the ADRs a repo already obeys, and make the skill linter a gate that actually runs - #555
Merged
Merged
Conversation
…one before proposing it
Taking over a repo, the architecture decisions are in the code, not in docs/adr.
`kit adr derive` recovers them: an absent import edge with a populated reverse is a
decision someone made and everyone has obeyed since. Measured on kit itself, no
subsystem directory has ever imported the command layer (utils -> commands, 112
reverse edges across 32 files in scope) and it is in none of the five ADRs.
Two failure modes are checked before a candidate is ever shown, because a derived
rule that breaks CI on acceptance is worse than no rule, and one that can never go
red is worse still:
- static discrimination: the emitted regex is tested against the specifier shape a
real violation would use, so a rule that cannot fail is not proposed;
- dynamic verification: each draft is rendered, parsed with the real parser, armed
in memory and run through the same evaluator `adr check` uses, over the real repo
with tests included. Anything that fires is dropped AND reported, never silently
absent. This is not theoretical -- a bucket containing a nested directory named
after another bucket makes `../commands/` resolve inside the bucket, which the
graph cannot see and the evaluator can.
Drafts are emitted `status: proposed`, and evaluateAdr ignores every non-accepted
ADR, so a derived file gates nothing until a human flips the status. kit proposes
with evidence; it never decides that a habit was a decision.
The CLI half lives one module deeper than commands/adr.ts on purpose: flags are
derived by walking a handler's imports one level, and review/baseline/standards embed
adrCheck -- parsing argv in adr.ts would have put --root/--min-support/--emit in their
allowlists, flags they would accept and silently ignore.
Limits, stated rather than discovered later: TS/JS + Python relative imports only,
top-level buckets under one source root, files directly in the root excluded (they
reach a sibling as ./x, not ../x), and a snapshot rather than a history -- "never
occurs" says the rule holds now, not that anyone intended it.
…'s scope, wire it into review and CI Same failure class as the ADR gate in #542, found the same way: `kit skill test --gate` has always exited 1 on a failing skill, and nothing ever ran it. Not CI, not `kit review`, not verify-suite.sh. Measured, kit's ONLY shipped SKILL.md failed kit's own linter for as long as the linter had existed -- no `allowed-tools`, so the skill implicitly claimed EVERY tool -- and no pipeline said a word. A tool that lints skills while shipping one that fails its own lint is not a gate, it is advice. Three parts, none of which works without the others: 1. skills/triage/SKILL.md declares `allowed-tools: Bash` and says why in a Scope section. The skill's whole action is one subprocess call to scripts/triage.py: it reads no files through the agent, fetches nothing, writes nothing. This was an omission rather than a considered choice -- the only commit that ever touched the file was an unrelated release wizard, and no ADR or shared-memory entry mentions it. The module surface is now pinned in .kit-skill.snapshot.json, so silently widening the declared privileges is drift and fails. 2. src/skill-run.ts is the embeddable gate: it discovers every SKILL.md under skills/ and .claude/skills/, runs the four deterministic checks against each, and folds them into one result. Per-skill rows are named `<skill>: <check>` so a red row points at a file and a rule. A repo with no skills SKIPS honestly (didNotRun stays false -- nothing was prevented from running); a skipped check WARNS rather than passing, because an unproven check is not a clean one. It judges module discipline only: whether a skill's output is any good is a model judgement and stays outside kit (ADR-0001). 3. `kit review` gains a fifth stage, and ci.yml runs it as a hard failure. The CI step invokes `review --stages skill` rather than a hard-coded path, so a second skill cannot be added un-gated. ci-adr-gate.test.ts now pins this invocation the way it pins the ADR gate's -- deleting the step, or adding continue-on-error, fails the suite. Proved by mutation rather than by inspection: dropping `allowed-tools` and widening it both turn `kit review` red with the stage named; deleting the CI step and neutering it with continue-on-error both fail the pin; three mutations of the gate itself (drop sibling comparison, render a skip as a pass, render "no skills" as a pass) each break exactly the one test that should notice. While writing the pin I shipped a regex whose leading \b could never match an alternative starting with `-`, so it passed while matching nothing -- caught only by deleting the CI step and watching the test stay green. That is the same defect class this whole change is about, one level up. Every statement of review's stage list is updated with it (README, CLAUDE.md, AGENTS.md, COMMANDS.md, MCP guide, MCP server, CLI help), since CLAUDE.md's four-stage line is what made the gap invisible in the first place.
…n that survived Running a real mutation score over adr-derive.ts (8 mutations) instead of only the two I already suspected: five were killed, three survived unnoticed. One of the survivors is a defect worth closing — dropping the `^` from the derived rule's regex broke no test, and an unanchored `(?:\.\./)+commands/` matches mid-specifier, so a package path like `@scope/pkg/../commands/x` would be reported as a layering violation. The anchor was already correct; nothing proved it stayed correct. One assertion added to the existing non-match test rather than a new test: the requirement is "this rule does not match things that are not sibling imports", which that test already owns. The other two survivors are recorded rather than patched. `ruleWouldFire`'s second internal assertion is redundant with its first. The `from === to` guard in the edge-count loop is provably unobservable — the candidate loop skips the same case — so removing it changes nothing, which makes it dead defensiveness rather than a coverage gap. Neither is worth a test that locks behaviour nobody depends on.
|
✅ Docker image built successfully
|
CI caught what the module split left behind: `kit check --category tests --enforce-tests` reported `src/commands/adr-derive.ts` as 1 new untested file. The tests for `deriveAdrs` were still sitting in `commands/adr.test.ts`, where the code lived before it moved one module deeper to keep its flags out of review/baseline/ standards. The code migrated; the tests did not. A move, not an addition — same three tests, same assertions, now in the file whose name the coverage check derives from the source. Locally the check goes from "1 new untested file(s) (83 total)" to "82 pre-existing (baseline-frozen)", exit 0.
|
✅ Docker image built successfully
|
|
✅ Docker image built successfully
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Two gates that existed but never ran, plus a new command for the repos you inherit.
kit adr deriverecovers the architecture decisions a repo is already obeying — an absent import edge with a populated reverse is a decision someone made and everyone has obeyed since. Measured on kit itself: no subsystem directory has ever imported the command layer (utils → commands, 112 reverse edges across 32 files in scope), and it is in none of the five ADRs.Then the same failure class as the ADR gate in #542, found the same way:
kit skill test --gatehas always exited 1 on a failing skill and nothing ever ran it — not CI, notkit review, notverify-suite.sh. Measured, kit's only shippedSKILL.mdfailed kit's own linter for as long as the linter had existed (noallowed-tools, so the skill implicitly claimed every tool) and no pipeline said a word.Type of Change
Changes Made
kit adr derive— architecture recoverysrc/adr-derive.ts— pure derivation, draft rendering, static discrimination.src/commands/adr-derive.ts— argv, the repo-backed verification pass, output. Deliberately one module deeper thancommands/adr.ts: flags are derived by walking a handler's imports one level, andreview/baseline/standardsembedadrCheck, so parsing argv there put--root/--min-support/--emitin their allowlists — flags they would accept and silently ignore. kit's own flag scanner caught it.status: proposed, andevaluateAdrignores every non-accepted ADR, so a derived file gates nothing until a human flips the status. kit proposes with evidence; it never decides that a habit was a decision.The skill linter becomes a gate
skills/triage/SKILL.mddeclaresallowed-tools: Bashwith a## Scopesection saying why — the skill's whole action is one subprocess call toscripts/triage.py. This was an omission, not a considered choice: the only commit that ever touched the file was an unrelated release wizard, and no ADR or shared-memory entry mentions it. The module surface is pinned in.kit-skill.snapshot.json.src/skill-run.ts— the embeddable gate over everySKILL.mdunderskills/and.claude/skills/. A repo with no skills skips honestly (didNotRunstays false); a skipped check warns rather than passing.kit reviewgains a fifth stage;ci.ymlrunsreview --stages skillas a hard failure, so a second skill cannot be added un-gated.ci-adr-gate.test.tspins the invocation the way it pins the ADR gate's.One defect closed on the way
A mutation score over
adr-derive.ts(8 mutations, 5 killed, 3 survived) found that dropping the^from a derived rule's regex broke no test. Unanchored,(?:\.\./)+commands/matches mid-specifier, so a package path like@scope/pkg/../commands/xwould be reported as a layering violation. One assertion added to the test that already owns that requirement — not a new test.Testing
Proved by mutation rather than by inspection. Every claim below is a command that was run:
acceptedinstead ofproposedutils → commandsrejected, cited tosrc/utils/deep/z.ts:1allowed-toolsfrom the shipped skillkit reviewexit 1, "failed — skill"continue-on-errorEnd-to-end proof that a derived draft is inert until armed:
Test Coverage
review.test.ts/mcp-server.test.tswent from four stages to five)npm test) — 4714 tests, 0 fail, 3 skipped, all three skips loud (0500 does not deny writes … running as root?×2,semgrep not installed)Manual Testing
node dist/cli.js adr derive— 9 verified candidates, 0 rejectednode dist/cli.js adr derive --emit utils/commands— draft parses, lists asproposednode dist/cli.js review --stages adr,skill,design— passesnode dist/cli.js skill test skills/triage/SKILL.md --gate— exit 0Breaking Changes
None.
ReviewStageNamegains"skill", which widens an existing union rather than changing one; a scoped--stagesrun is unaffected.Checklist
npm test)eslintclean on every new filePerformance Impact
commands/adr.tsloads the derive module and the repo map through a dynamic import, soadr check— which runs in the pre-commit hook — never pays for either.Reviewer Notes
Nothing here accepts an ADR on kit's behalf. The nine derived candidates are proposals; accepting them is yours to decide, and the design would be undermined by the tool's author quietly accepting its own output. To arm one:
Known limits, stated rather than left to be discovered: TS/JS + Python relative imports only; top-level buckets under one source root; files directly in the root excluded (they reach a sibling as
./x, not../x); and a snapshot rather than a history — "never occurs" says the rule holds now, not that anyone intended it.Two mutation survivors are recorded but not patched:
ruleWouldFire's second internal assertion is redundant with its first, and thefrom === toguard in the edge-count loop is provably unobservable (the candidate loop skips the same case), which makes it dead defensiveness rather than a coverage gap. Neither is worth a test that locks behaviour nobody depends on.🤖 Generated with Claude Code