diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6b84d80..14a1114 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 8eeafcd..34caf47 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 3167c20..c26b8c7 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 0b4983d..bdd0072 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 4ad1bf5..1791ff6 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, @@ -3075,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 1c8f55c..0ede4e6 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -97,8 +97,8 @@ 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 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 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 27d9387..c305c15 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 0000000..67db9a0 --- /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 fd3ed30..98ad6af 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/adr-derive.test.ts b/src/adr-derive.test.ts new file mode 100644 index 0000000..4099dec --- /dev/null +++ b/src/adr-derive.test.ts @@ -0,0 +1,264 @@ +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")); + // 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", () => { + 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 0000000..ff4bdc4 --- /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/ci-adr-gate.test.ts b/src/ci-adr-gate.test.ts index 40d4346..7fad91c 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 a28ae8e..065c9e3 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, @@ -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.test.ts b/src/commands/adr-derive.test.ts new file mode 100644 index 0000000..e056ec9 --- /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-derive.ts b/src/commands/adr-derive.ts new file mode 100644 index 0000000..958775b --- /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.ts b/src/commands/adr.ts index db198a0..a45b0d4 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/commands/review.test.ts b/src/commands/review.test.ts index 6905ea9..d5ab304 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 e4637bd..c794ade 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/flag-surface.ts b/src/flag-surface.ts index 4a74e3e..3efa450 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", diff --git a/src/mcp-server.test.ts b/src/mcp-server.test.ts index 3419948..0468f59 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 27c7e22..94706bf 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 0000000..1da089d --- /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 0000000..d12cc56 --- /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 }; +}