From 97698582a867f2951d19ba7ab2656e1ad42ed063 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 21:46:59 +0000 Subject: [PATCH 1/4] feat(adr): derive the decisions a repo already obeys, and prove each 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. --- contracts/kit.opencli.json | 6 +- docs/COMMANDS.md | 2 +- src/adr-derive.test.ts | 261 +++++++++++++++++++++++++++++++++++++ src/adr-derive.ts | 216 ++++++++++++++++++++++++++++++ src/cli.ts | 2 +- src/commands/adr-derive.ts | 195 +++++++++++++++++++++++++++ src/commands/adr.test.ts | 60 +++++++++ src/commands/adr.ts | 15 ++- src/flag-surface.ts | 2 +- 9 files changed, 754 insertions(+), 5 deletions(-) create mode 100644 src/adr-derive.test.ts create mode 100644 src/adr-derive.ts create mode 100644 src/commands/adr-derive.ts diff --git a/contracts/kit.opencli.json b/contracts/kit.opencli.json index 4ad1bf50..ce4f2afd 100644 --- a/contracts/kit.opencli.json +++ b/contracts/kit.opencli.json @@ -68,13 +68,17 @@ }, "adr": { "kind": "command", - "summary": "Enforce architecture decisions (ADR → gate): 'kit adr check' gates the repo on accepted ADRs' deterministic kit-enforce rules, cited to the ADR; 'kit adr list' shows enforced/documented. Zero-LLM (prose is never interpreted).", + "summary": "Enforce architecture decisions (ADR → gate): 'kit adr check' gates the repo on accepted ADRs' deterministic kit-enforce rules, cited to the ADR; 'kit adr list' shows enforced/documented; 'kit adr derive' proposes the decisions an inherited repo already obeys, each verified against the repo and emitted as status: proposed so it gates nothing until you accept it. Zero-LLM (prose is never interpreted).", "x-kit-accepted-flags": [ + "--emit", "--env", "--help", + "--json", + "--min-support", "--non-interactive", "--read-only", "--readonly", + "--root", "--version" ], "x-kit-args-modeled": true, diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 1c8f55c3..e5270f11 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -98,7 +98,7 @@ port = 3107 | `kit standards [--category general\|specific\|plugins\|platform\|] [--enforce]` | Dev-standards gate: general metrics (complexity/duplication/size via lizard/jscpd/scc) + per-language linters (11 langs) + user plugins (`.kit/standards.d/`) + container (hadolint). Warn by default; `--enforce` fails net-new findings AND setup gaps. | | `kit standards freeze` | Snapshot only the standards dimensions into `.kit-baseline.json`. | | `kit review` | Meta-runner — `check + design + standards + adr` gate for PR. | -| `kit adr [check\|list\|freeze]` | ADR → gate: enforce accepted ADRs' `kit-enforce` rules (`forbid_pattern` / `require_pattern` / `forbid_import`, incl. transitive and cross-package via `follow_packages`), cited to the ADR. `list` shows status; `freeze` baselines existing findings. Zero-LLM (prose is never interpreted). | +| `kit adr [check\|list\|freeze\|derive]` | ADR → gate: enforce accepted ADRs' `kit-enforce` rules (`forbid_pattern` / `require_pattern` / `forbid_import`, incl. transitive and cross-package via `follow_packages`), cited to the ADR. `list` shows status; `freeze` baselines existing findings; `derive` proposes ADRs the code already obeys (absent import edges with a populated reverse), each re-run through the real evaluator before it is shown and emitted as `status: proposed` so it gates nothing until a human accepts it. Zero-LLM (prose is never interpreted). | | `kit baseline [freeze]` | Snapshot current acceptable warnings (incl. standards + ADR) to `.kit-baseline.json`. | | `kit analyze [--write]` | Mine git history + framework markers → draft `CLAUDE.md` / `RULES.md`. | diff --git a/src/adr-derive.test.ts b/src/adr-derive.test.ts new file mode 100644 index 00000000..9731fcec --- /dev/null +++ b/src/adr-derive.test.ts @@ -0,0 +1,261 @@ +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { + bucketOf, + detectRoot, + deriveLayerCandidates, + importRegexFor, + renderCandidateAdr, + renderCandidateToml, + ruleWouldFire, + DEFAULT_MIN_SUPPORT, + type LayerCandidate, +} from "./adr-derive.js"; +import { parseAdr, evaluateAdr, globToRegExp } from "./adr.js"; +import { buildImportGraph, type FileImports, type RepoGraph } from "./repomap/graph.js"; + +/** Build a graph from `{ file: [imported files] }`, the shape the deriver consumes. */ +function graphOf(spec: Record): RepoGraph { + const files: FileImports[] = Object.entries(spec).map(([path, internal]) => ({ + path, + internal, + external: [], + })); + // Referenced-but-not-listed files still need to be nodes, as they are in a real repo. + const known = new Set(Object.keys(spec)); + for (const deps of Object.values(spec)) { + for (const d of deps) if (!known.has(d)) files.push({ path: d, internal: [], external: [] }); + } + return buildImportGraph(files); +} + +/** A repo where `commands` imports `utils` many times and `utils` never reciprocates. */ +function layeredRepo(reverseEdges = DEFAULT_MIN_SUPPORT): Record { + const spec: Record = {}; + for (let i = 0; i < reverseEdges; i++) spec[`src/commands/c${i}.ts`] = [`src/utils/u${i}.ts`]; + spec["src/utils/leaf.ts"] = []; + return spec; +} + +describe("bucketOf", () => { + it("returns the first path segment under the root", () => { + assert.equal(bucketOf("src/utils/colors.ts", "src"), "utils"); + assert.equal(bucketOf("src/utils/deep/nested.ts", "src"), "utils"); + }); + + it("returns null for a file sitting directly in the root", () => { + // Such a file reaches a sibling bucket as `./x`, not `../x` — a different rule + // shape, so it is excluded rather than guessed at. + assert.equal(bucketOf("src/cli.ts", "src"), null); + }); + + it("returns null for a file outside the root", () => { + assert.equal(bucketOf("scripts/build.ts", "src"), null); + assert.equal(bucketOf("srcfoo/x/y.ts", "src"), null); + }); +}); + +describe("detectRoot", () => { + it("finds the first conventional root present", () => { + assert.equal(detectRoot(graphOf({ "src/a/x.ts": [] })), "src"); + assert.equal(detectRoot(graphOf({ "lib/a/x.ts": [] })), "lib"); + }); + + it("returns null when no conventional root exists", () => { + assert.equal(detectRoot(graphOf({ "pkg/a/x.ts": [] })), null); + }); +}); + +describe("importRegexFor", () => { + it("matches a sibling-bucket import at any nesting depth", () => { + const re = new RegExp(importRegexFor("commands")); + assert.ok(re.test("../commands/adr.js")); + assert.ok(re.test("../../commands/adr.js")); + assert.ok(re.test("../../../commands/deep/adr.js")); + }); + + it("does not match a same-named segment that is not the first", () => { + const re = new RegExp(importRegexFor("commands")); + assert.ok(!re.test("../../other/commands/adr.js")); + assert.ok(!re.test("./commands/adr.js")); + assert.ok(!re.test("../commandsx/adr.js")); + }); + + it("escapes regex metacharacters in a bucket name", () => { + const re = new RegExp(importRegexFor("a.b")); + assert.ok(re.test("../a.b/x.js")); + assert.ok(!re.test("../axb/x.js")); + }); +}); + +describe("deriveLayerCandidates", () => { + const opts = { root: "src", minSupport: DEFAULT_MIN_SUPPORT }; + + it("proposes the absent direction of an asymmetric pair", () => { + const out = deriveLayerCandidates(graphOf(layeredRepo()), opts); + const hit = out.find((k) => k.from === "utils" && k.to === "commands"); + assert.ok(hit, "utils → commands should be proposed"); + assert.equal(hit.support, DEFAULT_MIN_SUPPORT); + assert.equal(hit.pathsGlob, "src/utils/**"); + }); + + it("never proposes the direction that actually occurs", () => { + const out = deriveLayerCandidates(graphOf(layeredRepo()), opts); + assert.equal( + out.find((k) => k.from === "commands" && k.to === "utils"), + undefined, + ); + }); + + it("drops a candidate the moment the edge appears — the rule is no longer true", () => { + const spec = layeredRepo(); + spec["src/utils/leaf.ts"] = ["src/commands/c0.ts"]; + const out = deriveLayerCandidates(graphOf(spec), opts); + assert.equal( + out.find((k) => k.from === "utils" && k.to === "commands"), + undefined, + ); + }); + + it("enforces the evidence floor — one lonely edge is noise, not a decision", () => { + const thin = graphOf({ "src/a/x.ts": ["src/b/y.ts"] }); + assert.deepEqual(deriveLayerCandidates(thin, opts), []); + // ...but the same asymmetry IS a candidate once the floor is lowered to match it. + const lowered = deriveLayerCandidates(thin, { root: "src", minSupport: 1 }); + assert.equal(lowered.length, 1); + assert.equal(lowered[0].from, "b"); + assert.equal(lowered[0].to, "a"); + }); + + it("ignores buckets outside the named root", () => { + const spec = layeredRepo(); + spec["lib/other/z.ts"] = ["lib/thing/w.ts"]; + const out = deriveLayerCandidates(graphOf(spec), { root: "src", minSupport: 1 }); + assert.ok(out.every((k) => k.from !== "other" && k.to !== "thing")); + }); + + it("counts files in scope so a rule over an empty scope is visible", () => { + const out = deriveLayerCandidates(graphOf(layeredRepo()), opts); + const hit = out.find((k) => k.from === "utils" && k.to === "commands"); + assert.equal(hit?.filesInScope, DEFAULT_MIN_SUPPORT + 1); // u0..u4 + leaf.ts + }); + + it("is deterministic and ranked by evidence weight", () => { + const spec = layeredRepo(6); + spec["src/api/a0.ts"] = ["src/model/m0.ts"]; + spec["src/api/a1.ts"] = ["src/model/m1.ts"]; + const graph = graphOf(spec); + const a = deriveLayerCandidates(graph, { root: "src", minSupport: 2 }); + const b = deriveLayerCandidates(graph, { root: "src", minSupport: 2 }); + assert.deepEqual(a, b); + assert.ok(a[0].support >= a[a.length - 1].support); + assert.equal(a[0].to, "commands"); // 6 beats 2 + }); +}); + +describe("renderCandidateAdr", () => { + const cand: LayerCandidate = { + from: "utils", + to: "commands", + support: 112, + filesInScope: 32, + importRegex: importRegexFor("commands"), + pathsGlob: "src/utils/**", + message: "src/utils must not import src/commands", + }; + + it("renders a draft that parses into exactly one enforceable rule", () => { + const adr = parseAdr(renderCandidateAdr(cand, "ADR-0042")); + assert.ok(adr); + assert.equal(adr.id, "ADR-0042"); + assert.equal(adr.rules.length, 1); + assert.ok(adr.hasEnforceBlock); + }); + + it("ships DISARMED — a derived draft is proposed, and proposed gates nothing", () => { + const adr = parseAdr(renderCandidateAdr(cand, "ADR-0042")); + assert.equal(adr?.status, "proposed"); + const violating = [{ path: "src/utils/x.ts", content: 'import "../commands/adr.js";' }]; + assert.deepEqual(evaluateAdr(adr!, violating), [], "a proposed ADR must never gate"); + }); + + it("arms on exactly one human edit: proposed → accepted", () => { + const adr = parseAdr(renderCandidateAdr(cand, "ADR-0042"))!; + const violating = [{ path: "src/utils/x.ts", content: 'import "../commands/adr.js";' }]; + const found = evaluateAdr({ ...adr, status: "accepted" }, violating); + assert.equal(found.length, 1); + assert.equal(found[0].rule, "forbid-import"); + assert.equal(found[0].adrId, "ADR-0042"); + assert.equal(found[0].kind, "violation"); + }); + + it("carries the measurement, so a reviewer can judge the evidence not the prose", () => { + const body = renderCandidateAdr(cand, "ADR-0042"); + assert.match(body, /\*\*112\*\*/); + assert.match(body, /\*\*zero\*\* times/); + assert.match(body, /32 file\(s\)/); + }); +}); + +describe("renderCandidateToml", () => { + it("escapes the regex so the emitted TOML round-trips", () => { + const cand: LayerCandidate = { + from: "utils", + to: "commands", + support: 5, + filesInScope: 1, + importRegex: importRegexFor("commands"), + pathsGlob: "src/utils/**", + message: "m", + }; + const adr = parseAdr(renderCandidateAdr(cand, "ADR-0001"))!; + const rule = adr.rules[0]; + assert.equal(rule.type, "forbid-import"); + if (rule.type !== "forbid-import") return; + // The backslashes survived TOML parsing: the rule still matches a real specifier. + assert.ok(new RegExp(rule.import).test("../commands/x.js")); + assert.ok(renderCandidateToml(cand).includes("[[forbid_import]]")); + }); +}); + +describe("the emitted scope actually covers the derived bucket", () => { + it("matches nested files and the bucket's tests, not a sibling bucket", () => { + const re = globToRegExp("src/utils/**"); + assert.ok(re.test("src/utils/colors.ts")); + assert.ok(re.test("src/utils/deep/nested.ts")); + assert.ok(re.test("src/utils/colors.test.ts"), "tests are in scope — the rule must hold there"); + assert.ok(!re.test("src/commands/adr.ts")); + }); +}); + +describe("ruleWouldFire", () => { + it("is true for a well-formed candidate", () => { + assert.ok( + ruleWouldFire({ + from: "utils", + to: "commands", + support: 5, + filesInScope: 1, + importRegex: importRegexFor("commands"), + pathsGlob: "src/utils/**", + message: "m", + }), + ); + }); + + it("is false for a rule that cannot match a violating specifier", () => { + // A green gate that can never go red is the failure mode this guards. + assert.equal( + ruleWouldFire({ + from: "utils", + to: "commands", + support: 5, + filesInScope: 1, + importRegex: "^never-matches-anything$", + pathsGlob: "src/utils/**", + message: "m", + }), + false, + ); + }); +}); diff --git a/src/adr-derive.ts b/src/adr-derive.ts new file mode 100644 index 00000000..ff4bdc4b --- /dev/null +++ b/src/adr-derive.ts @@ -0,0 +1,216 @@ +/** + * `kit adr derive` — recover the architecture decisions a repo is ALREADY obeying. + * + * Taking over an unfamiliar repo, the decisions are in the code, not in `docs/adr`. + * A layering constraint that holds across every file is a decision whether or not + * anyone wrote it down — so it can be MEASURED instead of remembered. + * + * The unit of evidence is an absent edge with a populated reverse: if `utils` never + * imports `commands` while `commands` imports `utils` 112 times, that asymmetry is + * intent, not coincidence. Support (the reverse count) is the evidence weight — a + * one-file directory with one edge is noise, and the `--min-support` floor exists so + * the command proposes decisions rather than emitting a catalogue of accidents. + * + * Two properties this module is built around: + * + * - It PROPOSES, never decides. A derived ADR is rendered with `status: proposed`, + * and `evaluateAdr` ignores every non-accepted ADR — so a draft is inert until a + * human edits the status. Promotion stays a deliberate act (same deny-by-default + * posture as `.kit/shared`). + * - A rule that matches nothing passes trivially. Candidates are therefore derived + * over exactly the file set the emitted `paths` glob matches (tests included), so + * "zero violations today" is true of the rule as written, not of a tidier subset + * the gate would never see. The caller re-runs each draft through the real + * evaluator before printing it; anything that fires is dropped, not shown. + * + * Zero-LLM and offline: an import graph, set arithmetic, and a TOML block. Prose is + * never interpreted, and nothing here decides whether a decision is GOOD — only that + * the codebase currently behaves as if it were made. + */ +import type { RepoGraph } from "./repomap/graph.js"; + +export interface LayerCandidate { + /** Subdirectory that never imports `to` (bucket name, not a path). */ + from: string; + /** Subdirectory it never imports. */ + to: string; + /** Distinct importer→imported file pairs in the REVERSE direction — the evidence weight. */ + support: number; + /** Files the emitted `paths` glob covers, so a rule over an empty scope is visible. */ + filesInScope: number; + /** Source of the `forbid_import` regex. */ + importRegex: string; + /** The `paths` glob the rule is scoped to. */ + pathsGlob: string; + /** Message a violation would carry. */ + message: string; +} + +export interface DeriveOptions { + /** Source root the buckets live under, repo-relative posix (e.g. "src"). */ + root: string; + /** Minimum reverse-edge count before an absent edge is proposed as a decision. */ + minSupport: number; +} + +export const DEFAULT_MIN_SUPPORT = 5; + +/** Source roots probed, in order, when the caller does not name one. */ +export const ROOT_CANDIDATES: readonly string[] = ["src", "lib", "app"]; + +/** + * The bucket a repo-relative file belongs to: its first path segment under `root`. + * Files sitting directly in `root` belong to no bucket (null) — a rule for them would + * need a different specifier shape (`./x` rather than `../x`), so they are left out + * rather than guessed at. + */ +export function bucketOf(id: string, root: string): string | null { + const prefix = `${root}/`; + if (!id.startsWith(prefix)) return null; + const rest = id.slice(prefix.length); + const slash = rest.indexOf("/"); + return slash === -1 ? null : rest.slice(0, slash); +} + +/** + * The specifier regex for "a file under `root//…` imports `root//…`". + * + * Any depth of `../` is allowed because the importer may sit in a nested directory + * (`src/a/deep/x.ts` reaches the sibling bucket as `../../to/y.js`). That breadth can + * over-match when a bucket contains a nested directory named after another bucket + * (`src/a/deep/../commands` is `src/a/commands`, not `src/commands`) — which is + * precisely why every candidate is re-run through the real evaluator before it is + * shown. An over-matching rule reports a violation there and is dropped. + */ +export function importRegexFor(to: string): string { + return `^(?:\\.\\./)+${to.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}/`; +} + +/** Detect the source root from the graph's file ids, or null when none of the candidates exist. */ +export function detectRoot(graph: RepoGraph): string | null { + for (const root of ROOT_CANDIDATES) { + if (graph.nodes.some((n) => n.kind === "file" && n.id.startsWith(`${root}/`))) return root; + } + return null; +} + +/** + * Absent edges with a populated reverse, ranked by evidence weight. + * + * Only subdirectory→subdirectory pairs are considered (see `bucketOf`). Ordering is + * deterministic: support descending, then bucket names, so two runs over the same tree + * produce byte-identical output. + */ +export function deriveLayerCandidates(graph: RepoGraph, opts: DeriveOptions): LayerCandidate[] { + const { root, minSupport } = opts; + + const filesPerBucket = new Map(); + for (const node of graph.nodes) { + if (node.kind !== "file") continue; + const b = bucketOf(node.id, root); + if (b) filesPerBucket.set(b, (filesPerBucket.get(b) ?? 0) + 1); + } + + // Distinct file-pairs per bucket edge. The graph already de-duplicates repeated + // imports between the same two files, so this counts connections, not statements. + const edgeCount = new Map(); + for (const edge of graph.edges) { + const from = bucketOf(edge.from, root); + const to = bucketOf(edge.to, root); + if (!from || !to || from === to) continue; + const key = `${from}\u0000${to}`; + edgeCount.set(key, (edgeCount.get(key) ?? 0) + 1); + } + + const buckets = [...filesPerBucket.keys()].sort(); + const out: LayerCandidate[] = []; + for (const from of buckets) { + for (const to of buckets) { + if (from === to) continue; + if (edgeCount.has(`${from}\u0000${to}`)) continue; // the edge exists — no rule to derive + const support = edgeCount.get(`${to}\u0000${from}`) ?? 0; + if (support < minSupport) continue; + out.push({ + from, + to, + support, + filesInScope: filesPerBucket.get(from) ?? 0, + importRegex: importRegexFor(to), + pathsGlob: `${root}/${from}/**`, + message: `${root}/${from} must not import ${root}/${to}`, + }); + } + } + return out.sort( + (a, b) => + b.support - a.support || + (a.from < b.from ? -1 : a.from > b.from ? 1 : 0) || + (a.to < b.to ? -1 : a.to > b.to ? 1 : 0), + ); +} + +/** The `kit-enforce` block a candidate would carry, ready to paste or gate on. */ +export function renderCandidateToml(cand: LayerCandidate): string { + return [ + "[[forbid_import]]", + `import = ${JSON.stringify(cand.importRegex)}`, + `paths = ${JSON.stringify(cand.pathsGlob)}`, + `message = ${JSON.stringify(cand.message)}`, + ].join("\n"); +} + +/** + * A complete draft ADR for a candidate. + * + * Emitted as `status: proposed` on purpose: `evaluateAdr` returns nothing for a + * non-accepted ADR, so the draft can be committed and reviewed without arming a gate + * nobody has agreed to. Flipping the status to `accepted` is the human's act, and the + * moment it flips, `kit adr check` enforces it. + */ +export function renderCandidateAdr(cand: LayerCandidate, id: string, title?: string): string { + const heading = title ?? `${cand.from} does not import ${cand.to}`; + return `--- +id: ${id} +title: ${heading} +status: proposed +--- + +# ${id}: ${heading} + +## Status + +Proposed — derived from the code, not yet agreed. \`kit adr check\` ignores a +non-accepted ADR, so this file gates nothing until someone sets \`status: accepted\`. + +## Decision + +Code under \`${cand.pathsGlob}\` does not import \`${cand.to}\`. + +## Evidence + +Measured from the import graph, not recalled: + +- \`${cand.to}\` imports \`${cand.from}\` across **${cand.support}** distinct file pairs. +- \`${cand.from}\` imports \`${cand.to}\` **zero** times. +- The rule below was evaluated over the ${cand.filesInScope} file(s) its \`paths\` glob + covers, tests included, and reported no violations. + +The asymmetry is the evidence. Whether it was intended is the reviewer's call: accept +this ADR to make it binding, or delete the file to record that it was only a habit. + +\`\`\`toml kit-enforce +${renderCandidateToml(cand)} +\`\`\` +`; +} + +/** + * A rule must be able to FAIL, or its green is meaningless. This is the static half of + * that proof: the emitted regex is checked against the specifier shape a violation + * would actually use. (The dynamic half — running the rule over the repo — is the + * caller's verification pass.) + */ +export function ruleWouldFire(cand: LayerCandidate): boolean { + const re = new RegExp(cand.importRegex); + return re.test(`../${cand.to}/x.js`) && re.test(`../../${cand.to}/deep/y.js`); +} diff --git a/src/cli.ts b/src/cli.ts index a28ae8e2..be61a170 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -838,7 +838,7 @@ const COMMAND_REGISTRY: Record = { adr: { handler: cmdAdr, stability: "experimental", - help: "Enforce architecture decisions (ADR → gate): 'kit adr check' gates the repo on accepted ADRs' deterministic kit-enforce rules, cited to the ADR; 'kit adr list' shows enforced/documented. Zero-LLM (prose is never interpreted).", + help: "Enforce architecture decisions (ADR → gate): 'kit adr check' gates the repo on accepted ADRs' deterministic kit-enforce rules, cited to the ADR; 'kit adr list' shows enforced/documented; 'kit adr derive' proposes the decisions an inherited repo already obeys, each verified against the repo and emitted as status: proposed so it gates nothing until you accept it. Zero-LLM (prose is never interpreted).", }, insight: { handler: cmdInsight, diff --git a/src/commands/adr-derive.ts b/src/commands/adr-derive.ts new file mode 100644 index 00000000..958775ba --- /dev/null +++ b/src/commands/adr-derive.ts @@ -0,0 +1,195 @@ +/** + * `kit adr derive` — the CLI half of architecture recovery. + * + * Kept OUT of `commands/adr.ts` on purpose. kit derives each command's accepted flags by + * walking the handler's import graph one level deep, and `commands/adr.ts` is imported by + * `review`, `baseline` and `standards` (they embed `adrCheck`). Parsing argv there would + * make `--root` / `--min-support` / `--emit` land in THEIR allowlists too — flags those + * commands would then accept and silently ignore. One module deeper, they belong to + * `kit adr` alone, which is where they are actually read. + * + * See `src/adr-derive.ts` for the derivation itself (pure); this file owns argv, the + * repo-backed verification pass, and rendering. + */ +import { readFileSync as read } from "node:fs"; +import { relative as rel } from "node:path"; +import { c } from "../utils/colors.js"; +import { hasFlag, flagValue } from "../utils/flags.js"; +import { walkSourceFiles } from "../source-walk.js"; +import { parseAdr, evaluateAdr, type Adr } from "../adr.js"; +import { createNodeModulesResolver } from "./adr.js"; +import { + deriveLayerCandidates, + renderCandidateAdr, + renderCandidateToml, + detectRoot, + ruleWouldFire, + DEFAULT_MIN_SUPPORT, + type LayerCandidate, +} from "../adr-derive.js"; +import type { RepoGraph } from "../repomap/graph.js"; + +/** Same extension set the ADR gate walks, so derivation sees exactly what enforcement will. */ +const CODE_EXTS = [".ts", ".tsx", ".js", ".jsx", ".py", ".go", ".rs", ".java", ".rb", ".php"]; + +/** Every code file in the repo, as the pure evaluator wants them. */ +function repoFiles(cwd: string): { path: string; content: string }[] { + return walkSourceFiles(cwd, { exts: CODE_EXTS, includeTests: true }).map((f) => ({ + path: rel(cwd, f), + content: read(f, "utf-8"), + })); +} + +export interface VerifiedCandidate { + candidate: LayerCandidate; + /** The draft ADR markdown, rendered exactly as it would be committed. */ + draft: string; +} + +export interface DeriveOutcome { + root: string; + proposed: VerifiedCandidate[]; + /** Candidates the verification pass rejected — reported, never silently dropped. */ + rejected: { candidate: LayerCandidate; reason: string }[]; +} + +/** + * Derive candidates and PROVE each one before showing it. + * + * Two ways a proposal can be wrong, and both are checked here rather than left for the + * reader: the rendered ADR might not parse (then it is not a draft, it is a text file), + * and the rule might over-match some path shape the graph did not reveal. Verification + * runs the real evaluator over the real repo with the ADR temporarily treated as + * accepted — the same code path `kit adr check` would take once a human accepts it. + */ +export function deriveAdrs( + cwd: string, + graph: RepoGraph, + opts: { root?: string; minSupport?: number } = {}, +): DeriveOutcome | null { + const root = opts.root ?? detectRoot(graph); + if (!root) return null; + + const candidates = deriveLayerCandidates(graph, { + root, + minSupport: opts.minSupport ?? DEFAULT_MIN_SUPPORT, + }); + if (candidates.length === 0) return { root, proposed: [], rejected: [] }; + + const files = repoFiles(cwd); + const packages = createNodeModulesResolver(cwd); + const proposed: VerifiedCandidate[] = []; + const rejected: { candidate: LayerCandidate; reason: string }[] = []; + + for (const candidate of candidates) { + if (!ruleWouldFire(candidate)) { + rejected.push({ candidate, reason: "rule cannot match a violating specifier" }); + continue; + } + const draft = renderCandidateAdr(candidate, "ADR-XXXX"); + const parsed = parseAdr(draft); + if (!parsed || parsed.rules.length !== 1) { + rejected.push({ candidate, reason: "rendered draft does not parse to exactly one rule" }); + continue; + } + // The draft ships `proposed` (inert). Verify it as the gate would see it once accepted. + const armed: Adr = { ...parsed, status: "accepted" }; + const findings = evaluateAdr(armed, files, { packages }); + if (findings.length > 0) { + const f = findings[0]; + rejected.push({ + candidate, + reason: `rule fires today (${findings.length} finding(s), first: ${f.file}:${f.line})`, + }); + continue; + } + proposed.push({ candidate, draft }); + } + return { root, proposed, rejected }; +} + +export async function adrDerive(cwd: string): Promise { + const { buildRepoGraph } = await import("./repomap.js"); + const minSupportRaw = flagValue(process.argv, "--min-support"); + const minSupport = minSupportRaw ? Number(minSupportRaw) : DEFAULT_MIN_SUPPORT; + if (!Number.isFinite(minSupport) || minSupport < 1) { + console.error(`${c.red}--min-support must be a positive integer${c.reset}`); + return false; + } + const rootFlag = flagValue(process.argv, "--root"); + const json = hasFlag(process.argv, "--json"); + + const outcome = deriveAdrs(cwd, buildRepoGraph(cwd), { root: rootFlag, minSupport }); + + if (!outcome) { + const msg = `no source root found (looked for src/, lib/, app/) — pass --root `; + if (json) console.log(JSON.stringify({ proposed: [], rejected: [], error: msg }, null, 2)); + else console.error(`${c.yellow}!${c.reset} ${msg}`); + return false; + } + + if (json) { + console.log( + JSON.stringify( + { + root: outcome.root, + minSupport, + proposed: outcome.proposed.map((p) => ({ ...p.candidate, draft: p.draft })), + rejected: outcome.rejected.map((r) => ({ ...r.candidate, reason: r.reason })), + }, + null, + 2, + ), + ); + return true; + } + + const emit = flagValue(process.argv, "--emit"); + if (emit) { + const [from, to] = emit.split("/"); + const hit = outcome.proposed.find((p) => p.candidate.from === from && p.candidate.to === to); + if (!hit) { + console.error( + `${c.red}no verified candidate '${emit}'${c.reset} — run without --emit to list them`, + ); + return false; + } + process.stdout.write(hit.draft); + return true; + } + + console.log( + `${c.bold}kit adr derive${c.reset} ${c.dim}— decisions this repo already obeys (root: ${outcome.root}, min-support: ${minSupport})${c.reset}\n`, + ); + + if (outcome.proposed.length === 0) { + console.log(`${c.dim}No layering candidate cleared the evidence floor.${c.reset}`); + } + for (const { candidate: k } of outcome.proposed) { + console.log( + ` ${c.green}◆${c.reset} ${c.bold}${k.from}${c.reset} never imports ${c.bold}${k.to}${c.reset}` + + ` ${c.dim}support ${k.support} reverse edge(s) · ${k.filesInScope} file(s) in scope${c.reset}`, + ); + for (const line of renderCandidateToml(k).split("\n")) { + console.log(` ${c.dim}${line}${c.reset}`); + } + } + + if (outcome.rejected.length > 0) { + console.log(`\n ${c.dim}not proposed — the rule did not survive verification:${c.reset}`); + for (const r of outcome.rejected) { + console.log( + ` ${c.yellow}−${c.reset} ${r.candidate.from} → ${r.candidate.to} ${c.dim}${r.reason}${c.reset}`, + ); + } + } + + if (outcome.proposed.length > 0) { + console.log( + `\n${c.dim}These are PROPOSALS measured from the import graph, not decisions. ` + + `Write one to docs/adr with \`kit adr derive --emit / > docs/adr/NNNN-x.md\`; ` + + `it lands as status: proposed and gates nothing until you set it to accepted.${c.reset}`, + ); + } + return true; +} diff --git a/src/commands/adr.test.ts b/src/commands/adr.test.ts index 36464ac8..2a66ce19 100644 --- a/src/commands/adr.test.ts +++ b/src/commands/adr.test.ts @@ -4,6 +4,7 @@ import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { adrFindingKey, collectAdrFindings, freezeAdrBaseline } from "./adr.js"; +import { deriveAdrs } from "./adr-derive.js"; import { baselineGet, type Baseline } from "../baseline.js"; import type { AdrViolation } from "../adr.js"; @@ -84,3 +85,62 @@ describe("collectAdrFindings + freezeAdrBaseline (temp repo)", () => { assert.equal(live.length, 0, "the frozen violation is suppressed on re-check"); }); }); + +describe("deriveAdrs — a proposal is not shown until the repo disproves nothing", () => { + let dir = ""; + + /** Write a repo where `commands` imports `utils` 5× and `utils` never reciprocates. */ + const seed = (extra: Record = {}): string => { + const root = mkdtempSync(join(tmpdir(), "kit-derive-")); + mkdirSync(join(root, "src", "commands"), { recursive: true }); + mkdirSync(join(root, "src", "utils"), { recursive: true }); + for (let i = 0; i < 5; i++) { + writeFileSync(join(root, "src", "utils", `u${i}.ts`), "export const x = 1;\n"); + writeFileSync(join(root, "src", "commands", `c${i}.ts`), `import "../utils/u${i}.js";\n`); + } + for (const [relPath, body] of Object.entries(extra)) { + mkdirSync(join(root, relPath, ".."), { recursive: true }); + writeFileSync(join(root, relPath), body); + } + return root; + }; + + after(() => rmSync(dir, { recursive: true, force: true })); + + it("proposes the asymmetric direction, verified against the real repo", async () => { + dir = seed(); + const { buildRepoGraph } = await import("./repomap.js"); + const out = deriveAdrs(dir, buildRepoGraph(dir), { minSupport: 5 }); + assert.ok(out); + assert.equal(out.root, "src"); + assert.equal(out.rejected.length, 0); + const hit = out.proposed.find((p) => p.candidate.from === "utils"); + assert.ok(hit, "utils → commands should survive verification"); + assert.equal(hit.candidate.to, "commands"); + assert.match(hit.draft, /status: proposed/); + }); + + it("REJECTS a candidate whose rule fires today — the graph proposes, the evaluator disproves", async () => { + // The over-match the verification pass exists for: a nested dir named after another + // bucket, so `../commands/` resolves INSIDE utils and no cross-bucket edge exists. + dir = seed({ + "src/utils/commands/x.ts": "export const y = 1;\n", + "src/utils/deep/z.ts": 'import "../commands/x.js";\n', + }); + const { buildRepoGraph } = await import("./repomap.js"); + const out = deriveAdrs(dir, buildRepoGraph(dir), { minSupport: 5 }); + assert.ok(out); + assert.equal(out.proposed.length, 0, "an over-matching rule must not be proposed"); + assert.equal(out.rejected.length, 1); + assert.match(out.rejected[0].reason, /fires today/); + assert.match(out.rejected[0].reason, /src\/utils\/deep\/z\.ts/); + }); + + it("returns null when there is no source root to reason about", async () => { + dir = mkdtempSync(join(tmpdir(), "kit-derive-empty-")); + mkdirSync(join(dir, "pkg", "a"), { recursive: true }); + writeFileSync(join(dir, "pkg", "a", "x.ts"), "export const x = 1;\n"); + const { buildRepoGraph } = await import("./repomap.js"); + assert.equal(deriveAdrs(dir, buildRepoGraph(dir), {}), null); + }); +}); diff --git a/src/commands/adr.ts b/src/commands/adr.ts index db198a09..a45b0d4c 100644 --- a/src/commands/adr.ts +++ b/src/commands/adr.ts @@ -356,7 +356,7 @@ export async function adrCheck(cwd = process.cwd()): Promise { export async function cmdAdr(): Promise { const args = process.argv.slice(3); const sub = - args[0] === "list" || args[0] === "check" || args[0] === "freeze" + args[0] === "list" || args[0] === "check" || args[0] === "freeze" || args[0] === "derive" ? args[0] : args[0] ? "help" @@ -368,9 +368,22 @@ export async function cmdAdr(): Promise { console.log(" kit adr list ADRs + status + enforced/documented"); console.log(" kit adr check gate the repo on accepted ADRs' rules (default)"); console.log(" kit adr freeze snapshot current findings into the baseline"); + console.log( + " kit adr derive propose ADRs the code already obeys, each verified against the repo", + ); + console.log( + `\n ${c.dim}derive: --root --min-support --json --emit /${c.reset}`, + ); return true; } + if (sub === "derive") { + // One module deeper so `kit adr derive`'s flags stay out of review/baseline/standards + // (see src/commands/adr-derive.ts) — and so `adr check` never loads the repo map. + const { adrDerive } = await import("./adr-derive.js"); + return adrDerive(cwd); + } + if (sub === "list") { const adrs = loadAdrs(cwd); if (adrs.length === 0) { diff --git a/src/flag-surface.ts b/src/flag-surface.ts index 4a74e3ee..3efa4500 100644 --- a/src/flag-surface.ts +++ b/src/flag-surface.ts @@ -87,7 +87,7 @@ export const COMMAND_FLAGS: Record = { "--with-migrate", "--yes", ], - adr: [], + adr: ["--emit", "--json", "--min-support", "--root"], "agent-audit": ["--attest", "--json", "--mode", "--no-auto-install", "--non-interactive"], "agent-config": [ "--broker-gate", From d64c58e49d6ed8c9fc31cbfe29b161fe5ba70975 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 22:08:14 +0000 Subject: [PATCH 2/4] =?UTF-8?q?feat(skill):=20make=20the=20skill=20linter?= =?UTF-8?q?=20a=20gate=20=E2=80=94=20declare=20the=20shipped=20skill's=20s?= =?UTF-8?q?cope,=20wire=20it=20into=20review=20and=20CI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `: ` 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. --- .github/workflows/ci.yml | 17 +++ AGENTS.md | 2 +- CLAUDE.md | 2 +- README.md | 4 +- contracts/kit.opencli.json | 2 +- docs/COMMANDS.md | 2 +- docs/MCP_TOOLS_GUIDE.md | 2 +- skills/triage/.kit-skill.snapshot.json | 8 ++ skills/triage/SKILL.md | 10 ++ src/ci-adr-gate.test.ts | 48 ++++++++ src/cli.ts | 2 +- src/commands/review.test.ts | 4 +- src/commands/review.ts | 25 +++- src/mcp-server.test.ts | 4 +- src/mcp-server.ts | 4 +- src/skill-run.test.ts | 162 +++++++++++++++++++++++++ src/skill-run.ts | 142 ++++++++++++++++++++++ 17 files changed, 420 insertions(+), 20 deletions(-) create mode 100644 skills/triage/.kit-skill.snapshot.json create mode 100644 src/skill-run.test.ts create mode 100644 src/skill-run.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6b84d80d..14a11142 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -61,3 +61,20 @@ jobs: # `ci-adr-gate.test.ts` pins this step's existence — deleting it fails the suite. - name: ADR gate (accepted ADRs' kit-enforce rules) run: node dist/cli.js adr check + + # SKILL DISCIPLINE — same failure class as the ADR gate above, 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 the day this landed, kit's + # ONLY shipped SKILL.md failed kit's own linter (no `allowed-tools` — the skill + # implicitly claimed every tool) for as long as the linter had existed, and no pipeline + # said a word. A tool that lints skills while shipping one that fails it is not a gate, + # it is advice. + # + # This runs the `skill` stage of `kit review`, so it covers every SKILL.md the repo + # ships rather than one hard-coded path — a second skill cannot be added un-gated. + # Module discipline only: contract, trigger collision, BOUNDED tool scope, and drift + # from the committed snapshot. Whether a skill's output is any GOOD is a model + # judgement and stays outside kit (ADR-0001). A repo with no skills skips honestly. + # `ci-adr-gate.test.ts` pins this step's existence — deleting it fails the suite. + - name: Skill gate (module discipline for every shipped SKILL.md) + run: node dist/cli.js review --stages skill diff --git a/AGENTS.md b/AGENTS.md index 8eeafcd0..34caf471 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -25,7 +25,7 @@ makes them four deterministic rules — not prose: - **ADR-0003** the check path imports no coverage-framework mappings. `node dist/cli.js adr check` runs them and **fails CI hard** on a violation. `kit check` -does **not** include the ADR stage — only `kit review` (check + design + standards + adr) +does **not** include the ADR stage — only `kit review` (check + design + standards + adr + skill) does. So before opening a PR that adds a dependency, moves an import, or touches `src/check*.ts`, run `kit review`, not `kit check` alone. diff --git a/CLAUDE.md b/CLAUDE.md index 3167c200..c26b8c72 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -38,7 +38,7 @@ makes them four deterministic rules — not prose: - **ADR-0003** the check path imports no coverage-framework mappings. `node dist/cli.js adr check` runs them and **fails CI hard** on a violation. `kit check` -does **not** include the ADR stage — only `kit review` (check + design + standards + adr) +does **not** include the ADR stage — only `kit review` (check + design + standards + adr + skill) does. So before opening a PR that adds a dependency, moves an import, or touches `src/check*.ts`, run `kit review`, not `kit check` alone. diff --git a/README.md b/README.md index 0b4983d6..bdd00723 100644 --- a/README.md +++ b/README.md @@ -267,7 +267,7 @@ Complete reference: [`docs/COMMANDS.md`](./docs/COMMANDS.md). The shortlist: - `kit setup`: Full pipeline: install → hooks → login → secrets → check - `kit check`: Status of tools, services, secrets, hooks, deploy env, security, tests - `kit fix`: Auto-remediate gaps (tools, gitignore, hooks, .env.template, declared deploy env) and print HITL blocks for auth / DSN setup -- `kit review` / `kit heal`: One-gate repo audit (check + design + standards + ADR); bounded self-heal loop +- `kit review` / `kit heal`: One-gate repo audit (check + design + standards + ADR + skill discipline); bounded self-heal loop - `kit adr {check,list,freeze}`: Turn an Architecture Decision Record into a deterministic gate — enforce a `kit-enforce` block (`forbid_pattern` / `require_pattern` / `forbid_import`, incl. transitive and across npm package boundaries) cited back to the ADR. Zero-LLM (prose is never interpreted) - `.kit/standards.d/*.toml`: Declarative house-rule plugins support `mode = "forbid"` and `mode = "require"`; directory excludes like `scripts/` mean `scripts/**`, with zero-match warnings - `kit scan`: Run external scanners (snyk/trivy/grype/semgrep/osv/socket) → one merged, air-gap-aware verdict @@ -1021,7 +1021,7 @@ For Cline, add the same config to your `cline_mcp_settings.json`. | Tool | Description | | --------------- | ----------------------------------------------------------------------------------------------- | | `kit_check` | Run all checks, return structured status JSON | -| `kit_review` | Full repo audit — check + design + standards + ADR gates as one structured report | +| `kit_review` | Full repo audit — check + design + standards + ADR + skill gates as one structured report | | `kit_fix` | Auto-fix issues (install tools, generate lock files) | | `kit_triage` | Security-triage a dependency BEFORE installing it — a pass satisfies the install gate | | `kit_memory` | Search cross-session memory + the repo's curated shared decisions (search-only) | diff --git a/contracts/kit.opencli.json b/contracts/kit.opencli.json index ce4f2afd..1791ff6a 100644 --- a/contracts/kit.opencli.json +++ b/contracts/kit.opencli.json @@ -3079,7 +3079,7 @@ }, "review": { "kind": "command", - "summary": "Full repo audit — runs check + design + standards + adr in one gate (for agents / PR checks; --json emits one structured report; --stages check,standards scopes the run, --category scopes the standards stage)", + "summary": "Full repo audit — runs check + design + standards + adr + skill in one gate (for agents / PR checks; --json emits one structured report; --stages check,standards scopes the run, --category scopes the standards stage)", "x-kit-accepted-flags": [ "--attest", "--category", diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index e5270f11..0ede4e6f 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -97,7 +97,7 @@ port = 3107 | `kit design` | A11y + design-token checks, baseline-aware. | | `kit standards [--category general\|specific\|plugins\|platform\|] [--enforce]` | Dev-standards gate: general metrics (complexity/duplication/size via lizard/jscpd/scc) + per-language linters (11 langs) + user plugins (`.kit/standards.d/`) + container (hadolint). Warn by default; `--enforce` fails net-new findings AND setup gaps. | | `kit standards freeze` | Snapshot only the standards dimensions into `.kit-baseline.json`. | -| `kit review` | Meta-runner — `check + design + standards + adr` gate for PR. | +| `kit review` | Meta-runner — `check + design + standards + adr + skill` gate for PR. The `skill` stage runs module discipline over every shipped `SKILL.md` (contract, trigger collision, bounded tool scope, snapshot drift); a repo with no skills skips honestly. | | `kit adr [check\|list\|freeze\|derive]` | ADR → gate: enforce accepted ADRs' `kit-enforce` rules (`forbid_pattern` / `require_pattern` / `forbid_import`, incl. transitive and cross-package via `follow_packages`), cited to the ADR. `list` shows status; `freeze` baselines existing findings; `derive` proposes ADRs the code already obeys (absent import edges with a populated reverse), each re-run through the real evaluator before it is shown and emitted as `status: proposed` so it gates nothing until a human accepts it. Zero-LLM (prose is never interpreted). | | `kit baseline [freeze]` | Snapshot current acceptable warnings (incl. standards + ADR) to `.kit-baseline.json`. | | `kit analyze [--write]` | Mine git history + framework markers → draft `CLAUDE.md` / `RULES.md`. | diff --git a/docs/MCP_TOOLS_GUIDE.md b/docs/MCP_TOOLS_GUIDE.md index 27d9387b..c305c15c 100644 --- a/docs/MCP_TOOLS_GUIDE.md +++ b/docs/MCP_TOOLS_GUIDE.md @@ -40,7 +40,7 @@ kit_triage → REQUIRED before installing anything the install gate has not MCP-run triage satisfies it identically to a CLI-run one kit_memory → recall prior cross-session decisions before answering project-specific questions -kit_review → the full audit (check + design + standards + ADR) as one +kit_review → the full audit (check + design + standards + ADR + skill) as one structured report — run before merging; concise:true trims pass/skip rows for context economy kit_run → escape hatch: any other kit command diff --git a/skills/triage/.kit-skill.snapshot.json b/skills/triage/.kit-skill.snapshot.json new file mode 100644 index 00000000..67db9a0a --- /dev/null +++ b/skills/triage/.kit-skill.snapshot.json @@ -0,0 +1,8 @@ +{ + "name": "triage", + "triggerKey": "security triage a dependency before installing it", + "scope": [ + "Bash" + ], + "fingerprint": "sha256:77a14838365b108a" +} diff --git a/skills/triage/SKILL.md b/skills/triage/SKILL.md index fd3ed30d..98ad6af9 100644 --- a/skills/triage/SKILL.md +++ b/skills/triage/SKILL.md @@ -1,6 +1,7 @@ --- name: triage description: "Security-triage a dependency before installing it." +allowed-tools: Bash --- # Triage @@ -33,6 +34,15 @@ python3 scripts/triage.py that is a CRITICAL ("cannot verify"), so the pass is withheld and kit blocks the install. Set `GITHUB_TOKEN` to avoid GitHub rate limits on `repo` checks. +## Scope + +`allowed-tools: Bash` — the whole skill is one subprocess call to +`scripts/triage.py`. It reads no files through the agent (the script opens what it +needs itself), fetches nothing through the agent, and writes nothing. Anything +broader would be a claim this skill cannot cash: an undeclared `allowed-tools` +implicitly claims EVERY tool, which is the opposite of what a gate should assert +about itself. + ## Rules - Stdlib only (urllib). No third-party deps, no network calls other than the diff --git a/src/ci-adr-gate.test.ts b/src/ci-adr-gate.test.ts index 40d43469..7fad91c5 100644 --- a/src/ci-adr-gate.test.ts +++ b/src/ci-adr-gate.test.ts @@ -42,6 +42,20 @@ function workflowBodies(): Record { /** `kit adr check` and `kit review` both run the ADR rules; either satisfies the requirement. */ const INVOKES_ADR = /\b(adr\s+check|kit_review\b|cli\.js\s+review\b|kit\s+review\b)/; +/** + * `kit skill test --gate` gates one skill; `kit review --stages skill` gates every SKILL.md the + * repo ships. Either satisfies the requirement, but only the second scales to a second skill. + */ +// NOTE the boundary placement: a leading \b before an alternative starting with `-` +// can never match (space→hyphen is not a word boundary), so each alternative carries +// its own. Caught by deleting the CI step and watching this test stay green. +const INVOKES_SKILL = /(\bskill\s+test\b|--stages[= ][^\n]*\bskill\b|\bkit_review\b)/; + +/** The step block that runs `re`, or undefined when no workflow step does. */ +function stepRunning(body: string, re: RegExp): string | undefined { + return body.split(/\n(?=\s*- name:)/).find((block) => re.test(block) && /run:/.test(block)); +} + describe("the ADR gate is wired into CI", () => { it("is invoked by at least one workflow, outside a comment", () => { const hits = Object.entries(workflowBodies()) @@ -73,3 +87,37 @@ describe("the ADR gate is wired into CI", () => { } }); }); + +describe("the skill gate is wired into CI", () => { + // Same rule as above, for the gate that was found unfired the same way: `kit skill test` + // had a working `--gate` (exit 1) and no workflow, hook or `kit review` stage called it, + // while kit's own only SKILL.md failed its `scope` check. A linter nobody runs cannot be + // told apart, in a green build, from a repo whose skills are clean. + it("is invoked by at least one workflow, outside a comment", () => { + const hits = Object.entries(workflowBodies()) + .filter(([, body]) => INVOKES_SKILL.test(body)) + .map(([file]) => file); + assert.ok( + hits.length > 0, + "no workflow runs the skill gate (`kit review --stages skill` or `kit skill test --gate`). " + + "Every shipped SKILL.md then declares its scope, or fails to, with nothing checking — " + + "which looks exactly like having no skills at all.", + ); + }); + + it("runs it as a hard failure, not a reported-and-ignored step", () => { + for (const [file, body] of Object.entries(workflowBodies())) { + if (!INVOKES_SKILL.test(body)) continue; + const step = stepRunning(body, INVOKES_SKILL); + if (!step) continue; + assert.ok( + !/continue-on-error:\s*true/.test(step), + `${file} runs the skill gate with continue-on-error — a gate that cannot fail the build is a report`, + ); + assert.ok( + !/\|\|\s*true/.test(step), + `${file} swallows the skill gate's exit code with \`|| true\``, + ); + } + }); +}); diff --git a/src/cli.ts b/src/cli.ts index be61a170..065c9e32 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -825,7 +825,7 @@ const COMMAND_REGISTRY: Record = { review: { handler: cmdReview, stability: "stable", - help: "Full repo audit — runs check + design + standards + adr in one gate (for agents / PR checks; --json emits one structured report; --stages check,standards scopes the run, --category scopes the standards stage)", + help: "Full repo audit — runs check + design + standards + adr + skill in one gate (for agents / PR checks; --json emits one structured report; --stages check,standards scopes the run, --category scopes the standards stage)", // MCP-exposed: THE one-shot audit for shell-less agents (kit_review), // superseding kit_standards on that surface (deprecated, leaves in 6.0). mcp: true, diff --git a/src/commands/review.test.ts b/src/commands/review.test.ts index 6905ea98..d5ab304a 100644 --- a/src/commands/review.test.ts +++ b/src/commands/review.test.ts @@ -30,10 +30,10 @@ describe("collectReview", () => { await rm(tempDir, { recursive: true, force: true }); }); - it("returns the four stages in the order the CLI always ran them", () => { + it("returns every stage in the order the CLI always ran them", () => { assert.deepEqual( report.stages.map((s) => s.stage), - ["check", "design", "standards", "adr"], + ["check", "design", "standards", "adr", "skill"], ); }); diff --git a/src/commands/review.ts b/src/commands/review.ts index e4637bde..c794ade2 100644 --- a/src/commands/review.ts +++ b/src/commands/review.ts @@ -1,9 +1,9 @@ /** - * `kit review` — meta-runner: check + design + standards + ADR in one shot. + * `kit review` — meta-runner: check + design + standards + ADR + skill discipline in one shot. * Convenient single-command gate for AI agents and PR checks. * * collectReview is the structured core: it runs every stage's shared gate - * (check-run, design, standards-run, adr) and returns one ReviewReport. + * (check-run, design, standards-run, adr, skill-run) and returns one ReviewReport. * cmdReview is a renderer on top; the MCP `kit_review` tool serializes the * same report — the computeCheckVerdict pattern, so the CLI and MCP surfaces * can never diverge on what a review runs or what "green" means. @@ -20,10 +20,11 @@ import { runCheckGate, checkRunToJsonChecks } from "../check-run.js"; import { runStandardsGate } from "../standards-run.js"; import { runDesignGate } from "./design.js"; import { runAdrGate } from "./adr.js"; +import { runSkillGate } from "../skill-run.js"; import type { JsonCheck } from "../cli-checks-shared.js"; import type { GateOpts } from "../check-security.js"; -export type ReviewStageName = "check" | "design" | "standards" | "adr"; +export type ReviewStageName = "check" | "design" | "standards" | "adr" | "skill"; export interface ReviewStageReport { stage: ReviewStageName; @@ -42,7 +43,13 @@ export interface ReviewReport { stages: ReviewStageReport[]; } -export const REVIEW_STAGES: readonly ReviewStageName[] = ["check", "design", "standards", "adr"]; +export const REVIEW_STAGES: readonly ReviewStageName[] = [ + "check", + "design", + "standards", + "adr", + "skill", +]; export interface CollectReviewOptions { cwd?: string; @@ -58,7 +65,7 @@ export interface CollectReviewOptions { * THE scoped, read-only path: an agent iterating on standards findings runs * `stages: ["standards"]` in seconds instead of paying the full audit's * security scan per loop — and it survives kit_standards' 6.0 removal. - * Undefined ⇒ all four. */ + * Undefined ⇒ every stage. */ stages?: ReviewStageName[]; /** Standards-stage scope (general | specific | plugins | platform | ), * passed through to the standards gate — parity with `kit standards --category`. */ @@ -131,7 +138,7 @@ function adrFindings(adr: Awaited>): JsonCheck[] { /** * Run the requested review stages (default: all four) and return the structured * report. Read-only; stages run in the order the CLI always ran them - * (check → design → standards → adr) regardless of the input order. The report + * (check → design → standards → adr → skill) regardless of the input order. The report * covers exactly the stages that ran — a scoped run's `ok` says nothing about * the stages it skipped, and the `stages` array shows the scope honestly. */ @@ -164,6 +171,12 @@ export async function collectReview(opts: CollectReviewOptions = {}): Promise !s.ok).map((s) => s.stage); return { ok: failed.length === 0, failed, stages }; diff --git a/src/mcp-server.test.ts b/src/mcp-server.test.ts index 34199485..0468f59b 100644 --- a/src/mcp-server.test.ts +++ b/src/mcp-server.test.ts @@ -363,7 +363,7 @@ describe("kit_review", () => { await rm(tempDir, { recursive: true, force: true }); }); - it("returns the structured report: ok, failed, and the four stages in order", async () => { + it("returns the structured report: ok, failed, and every stage in order", async () => { const { client, cleanup } = await createTestClient(); try { const result = await client.callTool({ name: "kit_review", arguments: { cwd: tempDir } }); @@ -376,7 +376,7 @@ describe("kit_review", () => { assert.ok(Array.isArray(data.failed)); assert.deepEqual( data.stages.map((s) => s.stage), - ["check", "design", "standards", "adr"], + ["check", "design", "standards", "adr", "skill"], ); assert.equal( data.ok, diff --git a/src/mcp-server.ts b/src/mcp-server.ts index 27c7e22b..94706bf3 100644 --- a/src/mcp-server.ts +++ b/src/mcp-server.ts @@ -64,7 +64,7 @@ const KIT_MCP_INSTRUCTIONS = `kit is a deterministic, local-first dev-environmen If you have shell access, prefer running \`kit \` directly — the CLI covers far more than these tools and \`kit --help\` documents everything. These MCP tools exist for shell-less clients. -Typical loop: kit_check (verify env + security) → kit_fix (auto-repair) → kit_triage (REQUIRED before installing any package kit's gate has not already cleared — the gate blocks untriaged installs) → kit_memory (recall prior cross-session decisions before answering project-specific questions) → kit_review (full audit — check + design + standards + ADR — before merging) → kit_run (escape hatch for any other kit command).`; +Typical loop: kit_check (verify env + security) → kit_fix (auto-repair) → kit_triage (REQUIRED before installing any package kit's gate has not already cleared — the gate blocks untriaged installs) → kit_memory (recall prior cross-session decisions before answering project-specific questions) → kit_review (full audit — check + design + standards + ADR + skill discipline — before merging) → kit_run (escape hatch for any other kit command).`; function configPath(cwd?: string): string { return resolve(cwd ?? process.cwd(), KIT_FILE); @@ -275,7 +275,7 @@ function register_kit_check(server: McpServer): void { } function register_kit_review(server: McpServer): void { - // kit_review — the full repo audit (check + design + standards + ADR) as ONE + // kit_review — the full repo audit (check + design + standards + ADR + skill) as ONE // structured report, via the same collectReview core `kit review` renders. // Replaces kit_standards on the MCP surface (that stage is one of its four). // Read-only: no writes, so no governance/read-only gating. diff --git a/src/skill-run.test.ts b/src/skill-run.test.ts new file mode 100644 index 00000000..1da089d0 --- /dev/null +++ b/src/skill-run.test.ts @@ -0,0 +1,162 @@ +import { describe, it, after } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { discoverSkills, runSkillGate, SNAPSHOT_NAME } from "./skill-run.js"; +import { parseSkillManifest, snapshotOf } from "./skill/test.js"; + +const dirs: string[] = []; +after(() => { + for (const d of dirs) rmSync(d, { recursive: true, force: true }); +}); + +function repo(): string { + const d = mkdtempSync(join(tmpdir(), "kit-skillgate-")); + dirs.push(d); + return d; +} + +/** Write a SKILL.md at `///SKILL.md`. Returns its directory. */ +function writeSkill( + root: string, + where: string, + name: string, + frontmatter: string, + body = "Body text that is long enough to be a real skill body.\n", +): string { + const dir = join(root, where, name); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "SKILL.md"), `---\n${frontmatter}\n---\n\n# ${name}\n\n${body}`); + return dir; +} + +const GOOD = (name: string): string => + `name: ${name}\ndescription: "Does one specific, describable thing for ${name}."\nallowed-tools: Bash`; + +/** Pin the snapshot the way `kit skill test --update-snapshot` does. */ +function pin(skillDir: string, frontmatter: string, body: string): void { + const raw = `---\n${frontmatter}\n---\n\n${body}`; + writeFileSync( + join(skillDir, SNAPSHOT_NAME), + JSON.stringify(snapshotOf(parseSkillManifest(raw)), null, 2), + ); +} + +describe("discoverSkills", () => { + it("finds SKILL.md under both skills/ and .claude/skills/", () => { + const root = repo(); + writeSkill(root, "skills", "alpha", GOOD("alpha")); + writeSkill(root, ".claude/skills", "beta", GOOD("beta")); + const found = discoverSkills(root).map((s) => s.path); + assert.deepEqual(found, [".claude/skills/beta/SKILL.md", "skills/alpha/SKILL.md"]); + }); + + it("ignores markdown that is not a SKILL.md", () => { + const root = repo(); + mkdirSync(join(root, "skills", "alpha"), { recursive: true }); + writeFileSync(join(root, "skills", "alpha", "README.md"), "# not a skill\n"); + assert.deepEqual(discoverSkills(root), []); + }); + + it("returns an empty list for a repo with no skills directory", () => { + assert.deepEqual(discoverSkills(repo()), []); + }); +}); + +describe("runSkillGate", () => { + it("passes a well-formed skill and names each check after it", () => { + const root = repo(); + const body = "Body text that is long enough to be a real skill body.\n"; + const dir = writeSkill(root, "skills", "alpha", GOOD("alpha"), body); + pin(dir, GOOD("alpha"), `# alpha\n\n${body}`); + + const r = runSkillGate(root); + assert.equal(r.ok, true); + assert.equal(r.skillCount, 1); + assert.ok(r.checks.every((c) => c.name.startsWith("alpha: "))); + assert.ok(r.checks.every((c) => c.files?.[0] === "skills/alpha/SKILL.md")); + assert.equal( + r.checks.find((c) => c.name === "alpha: scope")?.status, + "pass", + "a bounded allowed-tools list is the passing case", + ); + }); + + it("FAILS a skill that declares no tool scope — the defect this gate was built for", () => { + const root = repo(); + writeSkill(root, "skills", "alpha", `name: alpha\ndescription: "Does one specific thing."`); + const r = runSkillGate(root); + assert.equal(r.ok, false); + const scope = r.checks.find((c) => c.name === "alpha: scope"); + assert.equal(scope?.status, "fail"); + assert.match(scope!.detail, /allowed-tools/); + assert.equal(scope?.severity, "medium"); + }); + + it("skips honestly when the repo ships no skills — never a silent pass", () => { + const r = runSkillGate(repo()); + assert.equal(r.ok, true); + assert.equal(r.skillCount, 0); + assert.equal(r.checks.length, 1); + assert.equal(r.checks[0].status, "skip"); + // Not-applicable, not lost coverage: nothing was prevented from running. + assert.equal(r.checks[0].didNotRun, false); + assert.match(r.checks[0].detail, /nothing to check/); + }); + + it("warns rather than passes on an unpinned surface, without failing the gate", () => { + const root = repo(); + writeSkill(root, "skills", "alpha", GOOD("alpha")); // no snapshot pinned + const r = runSkillGate(root); + assert.equal(r.ok, true, "an unproven check must not fail the build on its own"); + const reg = r.checks.find((c) => c.name === "alpha: regression"); + assert.equal(reg?.status, "warn", "a skipped check is surfaced, never rendered as a pass"); + }); + + it("fails on snapshot drift — silently widening a skill's privileges is caught", () => { + const root = repo(); + const body = "Body text that is long enough to be a real skill body.\n"; + const dir = writeSkill(root, "skills", "alpha", GOOD("alpha"), body); + pin(dir, GOOD("alpha"), `# alpha\n\n${body}`); + // Re-write the skill with a broader scope, leaving the old snapshot in place. + writeSkill( + root, + "skills", + "alpha", + `name: alpha\ndescription: "Does one specific, describable thing for alpha."\nallowed-tools: Bash, Write`, + body, + ); + const r = runSkillGate(root); + assert.equal(r.ok, false); + assert.equal(r.checks.find((c) => c.name === "alpha: regression")?.status, "fail"); + }); + + it("compares siblings, so two skills cannot share one trigger unnoticed", () => { + const root = repo(); + const shared = 'description: "Exactly the same trigger sentence for both skills."'; + writeSkill(root, "skills", "alpha", `name: alpha\n${shared}\nallowed-tools: Bash`); + writeSkill(root, "skills", "beta", `name: beta\n${shared}\nallowed-tools: Bash`); + const r = runSkillGate(root); + assert.equal(r.skillCount, 2); + const triggers = r.checks.filter((c) => c.name.endsWith(": trigger")); + assert.equal(triggers.length, 2); + assert.ok( + triggers.some((c) => c.status === "fail"), + "an identical trigger across siblings must be reported, not silently tolerated", + ); + assert.equal(r.ok, false); + }); + + it("a single bad skill turns the whole gate red", () => { + const root = repo(); + const body = "Body text that is long enough to be a real skill body.\n"; + const dir = writeSkill(root, "skills", "alpha", GOOD("alpha"), body); + pin(dir, GOOD("alpha"), `# alpha\n\n${body}`); + writeSkill(root, "skills", "beta", `name: beta\ndescription: "Does one specific thing."`); + const r = runSkillGate(root); + assert.equal(r.ok, false); + assert.equal(r.checks.find((c) => c.name === "alpha: scope")?.status, "pass"); + assert.equal(r.checks.find((c) => c.name === "beta: scope")?.status, "fail"); + }); +}); diff --git a/src/skill-run.ts b/src/skill-run.ts new file mode 100644 index 00000000..d12cc56d --- /dev/null +++ b/src/skill-run.ts @@ -0,0 +1,142 @@ +/** + * The skill-discipline gate — the embeddable half of `kit skill test`, for `kit review` and CI. + * + * WHY THIS FILE EXISTS. `kit skill test --gate` has always exited 1 on a failing skill, and + * nothing ever called it: not CI, not `kit review`, not `verify-suite.sh`. Measured on kit + * itself the day this landed, kit's ONLY shipped `SKILL.md` failed kit's own linter (`scope: + * no allowed-tools declared`) and no pipeline noticed for as long as the linter had existed. + * That is the repo's own curated finding, verbatim: *"A gate that exists but is never invoked + * is the default failure, not the exception."* This module is the invocation. + * + * WHAT IT DECIDES, AND WHAT IT REFUSES TO. Only module discipline: the contract is declared, + * the trigger does not collide with a sibling, the tool scope is BOUNDED, and the module + * surface still matches its committed snapshot. It never judges whether a skill's output is + * good — that is a model judgement, delegated to an eval harness and never run by kit + * (ADR-0001). A skill that passes here is well-engineered, not necessarily useful. + * + * NO SKILLS IS NOT A FAILURE, AND NOT A SILENT PASS. A repo that ships no `SKILL.md` gets an + * honest not-applicable skip — `didNotRun` stays false, because nothing was prevented from + * running; there was simply nothing to check. A repo that ships one gets a verdict. + * + * Deterministic and offline: parse, compare, hash. Pure except for reading the repo. + */ +import { existsSync, readFileSync } from "node:fs"; +import { join, dirname, relative } from "node:path"; +import { walkSourceFiles } from "./source-walk.js"; +import { + parseSkillManifest, + testSkill, + triggerKey, + type SkillManifest, + type SkillSnapshot, + type SiblingSkill, +} from "./skill/test.js"; +import type { JsonCheck } from "./cli-checks-shared.js"; + +/** Committed snapshot filename, kept in step with `kit skill test --update-snapshot`. */ +export const SNAPSHOT_NAME = ".kit-skill.snapshot.json"; + +/** + * Where a repo keeps skills. `skills/` is kit's own layout; `.claude/skills/` is the + * agent-harness convention. Both are scanned, neither is required. + */ +export const SKILL_DIRS: readonly string[] = ["skills", ".claude/skills"]; + +export interface DiscoveredSkill { + /** Repo-relative path to the SKILL.md. */ + path: string; + manifest: SkillManifest; + snapshot: SkillSnapshot | null; +} + +function loadSnapshot(absSkillPath: string): SkillSnapshot | null { + const p = join(dirname(absSkillPath), SNAPSHOT_NAME); + if (!existsSync(p)) return null; + try { + return JSON.parse(readFileSync(p, "utf-8")) as SkillSnapshot; + } catch { + return null; // malformed snapshot ⇒ treated as absent, which the regression check surfaces + } +} + +/** Every `SKILL.md` the repo ships, in a stable order. */ +export function discoverSkills(cwd: string): DiscoveredSkill[] { + const out: DiscoveredSkill[] = []; + for (const dir of SKILL_DIRS) { + const abs = join(cwd, dir); + if (!existsSync(abs)) continue; + for (const file of walkSourceFiles(abs, { exts: [".md"] })) { + if (!file.endsWith("SKILL.md")) continue; + let raw: string; + try { + raw = readFileSync(file, "utf-8"); + } catch { + continue; // unreadable ⇒ not a skill we can judge; the walk is best-effort + } + out.push({ + path: relative(cwd, file).split("\\").join("/"), + manifest: parseSkillManifest(raw), + snapshot: loadSnapshot(file), + }); + } + } + return out.sort((a, b) => (a.path < b.path ? -1 : a.path > b.path ? 1 : 0)); +} + +export interface SkillGateResult { + ok: boolean; + /** How many `SKILL.md` files were found — 0 is a skip, never a pass. */ + skillCount: number; + checks: JsonCheck[]; +} + +/** + * Run module discipline over every skill in the repo. + * + * Each skill contributes one row per check, named `: `, so a red row points at + * the file and the rule rather than at "skills". Siblings are every OTHER discovered skill, + * which is what makes trigger-collision detection meaningful in a repo that ships several. + */ +export function runSkillGate(cwd: string = process.cwd()): SkillGateResult { + const skills = discoverSkills(cwd); + if (skills.length === 0) { + return { + ok: true, + skillCount: 0, + checks: [ + { + name: "skills", + status: "skip", + // Not-applicable, NOT a coverage loss: nothing was prevented from running. + didNotRun: false, + detail: `no SKILL.md found in ${SKILL_DIRS.join(" or ")} — nothing to check`, + category: "skill", + }, + ], + }; + } + + const checks: JsonCheck[] = []; + let ok = true; + for (const skill of skills) { + const label = skill.manifest.name ?? skill.path; + const siblings: SiblingSkill[] = skills + .filter((s) => s !== skill && s.manifest.name) + .map((s) => ({ name: s.manifest.name!, triggerKey: triggerKey(s.manifest) })); + const report = testSkill(skill.manifest, { siblings, snapshot: skill.snapshot }); + if (!report.ok) ok = false; + for (const c of report.checks) { + checks.push({ + name: `${label}: ${c.id}`, + // A skipped module check is a real gap in what was proven, so it warns rather than + // passing — but it never fails the gate on its own (`report.ok` ignores skips). + status: c.status === "skip" ? "warn" : c.status, + detail: c.detail, + category: "skill", + files: [skill.path], + ...(c.status === "fail" ? { severity: "medium" as const } : {}), + }); + } + } + return { ok, skillCount: skills.length, checks }; +} From 0d09a6bdf94bcb5e80ed34b1261ec1c2c03f87af Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 22:18:04 +0000 Subject: [PATCH 3/4] test(adr-derive): anchor the derived import rule, proved by a mutation that survived MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/adr-derive.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/adr-derive.test.ts b/src/adr-derive.test.ts index 9731fcec..4099dec3 100644 --- a/src/adr-derive.test.ts +++ b/src/adr-derive.test.ts @@ -79,6 +79,9 @@ describe("importRegexFor", () => { assert.ok(!re.test("../../other/commands/adr.js")); assert.ok(!re.test("./commands/adr.js")); assert.ok(!re.test("../commandsx/adr.js")); + // Anchored: an unanchored rule matches mid-specifier and turns a package path into + // a violation. A surviving mutation (dropping `^`) proved nothing else caught this. + assert.ok(!re.test("@scope/pkg/../commands/adr.js")); }); it("escapes regex metacharacters in a bucket name", () => { From d71ef898c6d812572523175375514af048523581 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 22:46:30 +0000 Subject: [PATCH 4/4] test(adr-derive): move the deriveAdrs tests to the module they now test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/commands/adr-derive.test.ts | 65 +++++++++++++++++++++++++++++++++ src/commands/adr.test.ts | 60 ------------------------------ 2 files changed, 65 insertions(+), 60 deletions(-) create mode 100644 src/commands/adr-derive.test.ts diff --git a/src/commands/adr-derive.test.ts b/src/commands/adr-derive.test.ts new file mode 100644 index 00000000..e056ec98 --- /dev/null +++ b/src/commands/adr-derive.test.ts @@ -0,0 +1,65 @@ +import { describe, it, after } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { deriveAdrs } from "./adr-derive.js"; + +describe("deriveAdrs — a proposal is not shown until the repo disproves nothing", () => { + let dir = ""; + + /** Write a repo where `commands` imports `utils` 5× and `utils` never reciprocates. */ + const seed = (extra: Record = {}): string => { + const root = mkdtempSync(join(tmpdir(), "kit-derive-")); + mkdirSync(join(root, "src", "commands"), { recursive: true }); + mkdirSync(join(root, "src", "utils"), { recursive: true }); + for (let i = 0; i < 5; i++) { + writeFileSync(join(root, "src", "utils", `u${i}.ts`), "export const x = 1;\n"); + writeFileSync(join(root, "src", "commands", `c${i}.ts`), `import "../utils/u${i}.js";\n`); + } + for (const [relPath, body] of Object.entries(extra)) { + mkdirSync(join(root, relPath, ".."), { recursive: true }); + writeFileSync(join(root, relPath), body); + } + return root; + }; + + after(() => rmSync(dir, { recursive: true, force: true })); + + it("proposes the asymmetric direction, verified against the real repo", async () => { + dir = seed(); + const { buildRepoGraph } = await import("./repomap.js"); + const out = deriveAdrs(dir, buildRepoGraph(dir), { minSupport: 5 }); + assert.ok(out); + assert.equal(out.root, "src"); + assert.equal(out.rejected.length, 0); + const hit = out.proposed.find((p) => p.candidate.from === "utils"); + assert.ok(hit, "utils → commands should survive verification"); + assert.equal(hit.candidate.to, "commands"); + assert.match(hit.draft, /status: proposed/); + }); + + it("REJECTS a candidate whose rule fires today — the graph proposes, the evaluator disproves", async () => { + // The over-match the verification pass exists for: a nested dir named after another + // bucket, so `../commands/` resolves INSIDE utils and no cross-bucket edge exists. + dir = seed({ + "src/utils/commands/x.ts": "export const y = 1;\n", + "src/utils/deep/z.ts": 'import "../commands/x.js";\n', + }); + const { buildRepoGraph } = await import("./repomap.js"); + const out = deriveAdrs(dir, buildRepoGraph(dir), { minSupport: 5 }); + assert.ok(out); + assert.equal(out.proposed.length, 0, "an over-matching rule must not be proposed"); + assert.equal(out.rejected.length, 1); + assert.match(out.rejected[0].reason, /fires today/); + assert.match(out.rejected[0].reason, /src\/utils\/deep\/z\.ts/); + }); + + it("returns null when there is no source root to reason about", async () => { + dir = mkdtempSync(join(tmpdir(), "kit-derive-empty-")); + mkdirSync(join(dir, "pkg", "a"), { recursive: true }); + writeFileSync(join(dir, "pkg", "a", "x.ts"), "export const x = 1;\n"); + const { buildRepoGraph } = await import("./repomap.js"); + assert.equal(deriveAdrs(dir, buildRepoGraph(dir), {}), null); + }); +}); diff --git a/src/commands/adr.test.ts b/src/commands/adr.test.ts index 2a66ce19..36464ac8 100644 --- a/src/commands/adr.test.ts +++ b/src/commands/adr.test.ts @@ -4,7 +4,6 @@ import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { adrFindingKey, collectAdrFindings, freezeAdrBaseline } from "./adr.js"; -import { deriveAdrs } from "./adr-derive.js"; import { baselineGet, type Baseline } from "../baseline.js"; import type { AdrViolation } from "../adr.js"; @@ -85,62 +84,3 @@ describe("collectAdrFindings + freezeAdrBaseline (temp repo)", () => { assert.equal(live.length, 0, "the frozen violation is suppressed on re-check"); }); }); - -describe("deriveAdrs — a proposal is not shown until the repo disproves nothing", () => { - let dir = ""; - - /** Write a repo where `commands` imports `utils` 5× and `utils` never reciprocates. */ - const seed = (extra: Record = {}): string => { - const root = mkdtempSync(join(tmpdir(), "kit-derive-")); - mkdirSync(join(root, "src", "commands"), { recursive: true }); - mkdirSync(join(root, "src", "utils"), { recursive: true }); - for (let i = 0; i < 5; i++) { - writeFileSync(join(root, "src", "utils", `u${i}.ts`), "export const x = 1;\n"); - writeFileSync(join(root, "src", "commands", `c${i}.ts`), `import "../utils/u${i}.js";\n`); - } - for (const [relPath, body] of Object.entries(extra)) { - mkdirSync(join(root, relPath, ".."), { recursive: true }); - writeFileSync(join(root, relPath), body); - } - return root; - }; - - after(() => rmSync(dir, { recursive: true, force: true })); - - it("proposes the asymmetric direction, verified against the real repo", async () => { - dir = seed(); - const { buildRepoGraph } = await import("./repomap.js"); - const out = deriveAdrs(dir, buildRepoGraph(dir), { minSupport: 5 }); - assert.ok(out); - assert.equal(out.root, "src"); - assert.equal(out.rejected.length, 0); - const hit = out.proposed.find((p) => p.candidate.from === "utils"); - assert.ok(hit, "utils → commands should survive verification"); - assert.equal(hit.candidate.to, "commands"); - assert.match(hit.draft, /status: proposed/); - }); - - it("REJECTS a candidate whose rule fires today — the graph proposes, the evaluator disproves", async () => { - // The over-match the verification pass exists for: a nested dir named after another - // bucket, so `../commands/` resolves INSIDE utils and no cross-bucket edge exists. - dir = seed({ - "src/utils/commands/x.ts": "export const y = 1;\n", - "src/utils/deep/z.ts": 'import "../commands/x.js";\n', - }); - const { buildRepoGraph } = await import("./repomap.js"); - const out = deriveAdrs(dir, buildRepoGraph(dir), { minSupport: 5 }); - assert.ok(out); - assert.equal(out.proposed.length, 0, "an over-matching rule must not be proposed"); - assert.equal(out.rejected.length, 1); - assert.match(out.rejected[0].reason, /fires today/); - assert.match(out.rejected[0].reason, /src\/utils\/deep\/z\.ts/); - }); - - it("returns null when there is no source root to reason about", async () => { - dir = mkdtempSync(join(tmpdir(), "kit-derive-empty-")); - mkdirSync(join(dir, "pkg", "a"), { recursive: true }); - writeFileSync(join(dir, "pkg", "a", "x.ts"), "export const x = 1;\n"); - const { buildRepoGraph } = await import("./repomap.js"); - assert.equal(deriveAdrs(dir, buildRepoGraph(dir), {}), null); - }); -});