From ca7f0be060e9719daeb09b43d503dd050a7951f0 Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Mon, 21 Sep 2026 06:31:00 +0200 Subject: [PATCH 1/4] chore(framework): the architecture rules are enforced where every tool passes The write-time refusal was a Claude Code PreToolUse hook. It bought one turn of latency for a mechanism no other tool can host: across every plugin shipped here the portable hook surface is SessionStart, and a write-time refusal has no equivalent on Codex, Cursor or Copilot. A guard this repository can only enforce for one of the tools it supports contradicts the premise of the repository. The pre-commit job already covers every edit that reaches a commit, whichever tool or person made it, and validate.yml replays it over the whole tree on each pull request, so nothing that was enforced stops being enforced. The hook also failed open by design, and nothing lints scripts/ or .claude/hooks/, so it could have stopped working without anything saying so. architecture-scan.js keeps only the three functions the command uses. Refs #250 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2 --- .claude/hooks/check-architecture-rules.js | 135 ------ .claude/settings.json | 12 - docs/ARCHITECTURE.md | 6 +- scripts/__tests__/architecture-hook.test.js | 500 -------------------- scripts/lib/architecture-scan.js | 7 +- 5 files changed, 7 insertions(+), 653 deletions(-) delete mode 100755 .claude/hooks/check-architecture-rules.js delete mode 100644 scripts/__tests__/architecture-hook.test.js diff --git a/.claude/hooks/check-architecture-rules.js b/.claude/hooks/check-architecture-rules.js deleted file mode 100755 index b50d0867b..000000000 --- a/.claude/hooks/check-architecture-rules.js +++ /dev/null @@ -1,135 +0,0 @@ -#!/usr/bin/env node - -/** - * PreToolUse guard: refuses a Write, Edit or MultiEdit that would leave a plugin's dispatch - * surface breaking one of the two architecture rules of issue #250. The rules live in - * `scripts/lib/architecture-rules.js` and their filesystem layer in `architecture-scan.js`; - * this script only reconstructs the unwritten content and speaks the host's refusal. - * - * It is the fast path, not the gate: `scripts/check-architecture-rules.js` runs on every commit, - * whichever tool made the edit. This one only ever sees Claude Code, and it fails open at every - * step — an unreadable payload, an unknown tool, an edit it cannot reconstruct or a crash lets - * the write through rather than halting unrelated work. - */ - -const fs = require("node:fs"); -const path = require("node:path"); - -const WRITE_TOOLS = new Set(["Write", "Edit", "MultiEdit"]); -const PROCEED = 0; - -function architecture() { - try { - const lib = path.resolve(__dirname, "..", "..", "scripts", "lib"); - return { - ...require(path.join(lib, "architecture-scan.js")), - classifyFile: require(path.join(lib, "architecture-rules.js")).classifyFile, - }; - } catch { - return null; - } -} - -function payloadFromStdin() { - try { - const parsed = JSON.parse(fs.readFileSync(0, "utf8")); - return parsed && typeof parsed === "object" ? parsed : null; - } catch { - return null; - } -} - -function repoRelative(absolutePath, root) { - const relative = path.relative(root, absolutePath); - if (relative === "" || relative.startsWith("..") || path.isAbsolute(relative)) return null; - return relative.split(path.sep).join("/"); -} - -function readFile(absolutePath) { - try { - return fs.readFileSync(absolutePath, "utf8"); - } catch { - return null; - } -} - -/** - * Splices literally. `String.prototype.replace` reads `$&`, `` $` ``, `$'` and `$1` in its - * replacement even when the pattern is a plain string, which corrupts a `new_string` holding one. - */ -function spliced(content, { old_string: oldString, new_string: newString, replace_all: replaceAll }) { - if (typeof oldString !== "string" || typeof newString !== "string") return null; - if (!content.includes(oldString)) return null; - if (replaceAll) return content.split(oldString).join(newString); - - const at = content.indexOf(oldString); - return content.slice(0, at) + newString + content.slice(at + oldString.length); -} - -function editedContent(absolutePath, edits) { - let content = readFile(absolutePath); - if (content === null) return null; - - for (const edit of edits) { - if (!edit || typeof edit !== "object") return null; - content = spliced(content, edit); - if (content === null) return null; - } - return content; -} - -/** What the file will hold if this call goes through, or null when that cannot be determined. */ -function prospectiveContent(toolName, toolInput, absolutePath) { - if (toolName === "Write") { - return typeof toolInput.content === "string" ? toolInput.content : null; - } - if (toolName === "Edit") return editedContent(absolutePath, [toolInput]); - - const { edits } = toolInput; - if (!Array.isArray(edits) || edits.length === 0) return null; - return editedContent(absolutePath, edits); -} - -function refuse(violations, describeFix) { - const reason = violations.map((v) => `${v.message}. Fix: ${describeFix(v)}.`).join("\n"); - process.stdout.write( - JSON.stringify({ - hookSpecificOutput: { - hookEventName: "PreToolUse", - permissionDecision: "deny", - permissionDecisionReason: reason, - }, - }) - ); -} - -function main() { - const rules = architecture(); - const payload = payloadFromStdin(); - if (!rules || !payload) return PROCEED; - - const { tool_name: toolName, tool_input: toolInput } = payload; - if (!WRITE_TOOLS.has(toolName)) return PROCEED; - if (!toolInput || typeof toolInput.file_path !== "string" || toolInput.file_path === "") return PROCEED; - - // Resolved against the project root, never the process cwd: the two can differ, and a readdir - // against the wrong one comes back empty instead of failing. - const root = process.env.CLAUDE_PROJECT_DIR || process.cwd(); - const absolutePath = path.resolve(root, toolInput.file_path); - const relativePath = repoRelative(absolutePath, root); - if (!relativePath || !rules.classifyFile(relativePath)) return PROCEED; - - const content = prospectiveContent(toolName, toolInput, absolutePath); - if (content === null) return PROCEED; - - const violations = rules.violationsForFile(relativePath, content, absolutePath); - if (violations.length > 0) refuse(violations, rules.describeFix); - - return PROCEED; -} - -try { - process.exitCode = main(); -} catch { - process.exitCode = PROCEED; -} diff --git a/.claude/settings.json b/.claude/settings.json index 61f2e989b..1850d4990 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -11,18 +11,6 @@ ] } ], - "PreToolUse": [ - { - "matcher": "Write|Edit|MultiEdit", - "hooks": [ - { - "type": "command", - "command": "node \"$CLAUDE_PROJECT_DIR/.claude/hooks/check-architecture-rules.js\"", - "timeout": 60 - } - ] - } - ], "PostToolUse": [ { "matcher": "Edit|Write|MultiEdit", diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index fc2232633..b52819e99 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -155,7 +155,11 @@ Address a capability only where the dispatch is declared: a router's `## Actions Recipe skills never hardcode a sibling provider. They discover cross-plugin capabilities at runtime through description matching. Agent permission lists and orchestration references are responsibility maps, so they name the current provider with its canonical `/plugin:folder` or `@plugin:agent` address. The orchestrator must verify that provider is installed before calling it. -`scripts/lib/architecture-rules.js` is the guard that decides both rules from a path and its content, and `architecture-scan.js` is the only place that reads the disk for it. Two callers share them. `scripts/check-architecture-rules.js` runs as the `architecture-rules` pre-commit job, so every edit that reaches a commit is covered whichever tool or person made it, and `validate.yml` replays it over the whole tree on each pull request. The `PreToolUse` hook on `Write`/`Edit`/`MultiEdit` is the fast path on top: it only ever sees Claude Code, but it refuses the write in the same turn instead of at commit time. `plugins/aidd-context/skills/00-onboard/**` is exempt from rule one: its reference menus name addresses because those are what the skill hands a person to type, not a hardcoded sibling provider. That exemption is temporary and ends with the follow-up issue on making that skill resolve its providers at runtime. The guard refuses an edit leaving a `## Actions` section without a citation for an action file that exists; it never refuses a citation with no file behind it, because that is how a skill legitimately grows one. +`scripts/lib/architecture-rules.js` is the guard that decides both rules from a path and its content, and `architecture-scan.js` is the only place that reads the disk for it. `scripts/check-architecture-rules.js` runs them as the `architecture-rules` pre-commit job, so every edit reaching a commit is covered whichever tool or person made it, and `validate.yml` replays the job over the whole tree on each pull request. + +It is enforced at commit time on purpose. An earlier revision also refused the write itself, through a Claude Code `PreToolUse` hook, which bought one turn of latency for a mechanism no other tool can host: across every plugin shipped here the portable hook surface is `SessionStart`, and a write-time refusal has no equivalent on Codex, Cursor or Copilot. A guard this repository could only enforce for one of the tools it supports contradicts the premise of the repository, so it was removed rather than duplicated. + +`plugins/aidd-context/skills/00-onboard/**` is exempt from rule one: its reference menus name addresses because those are what the skill hands a person to type, not a hardcoded sibling provider. That exemption is temporary and ends with the follow-up issue on making that skill resolve its providers at runtime. The guard refuses an edit leaving a `## Actions` section without a citation for an action file that exists; it never refuses a citation with no file behind it, because that is how a skill legitimately grows one. This distinction keeps recipe plugins swappable while making orchestration handoffs explicit and auditable. diff --git a/scripts/__tests__/architecture-hook.test.js b/scripts/__tests__/architecture-hook.test.js deleted file mode 100644 index fadb1ee17..000000000 --- a/scripts/__tests__/architecture-hook.test.js +++ /dev/null @@ -1,500 +0,0 @@ -const assert = require("node:assert/strict"); -const fs = require("node:fs"); -const os = require("node:os"); -const path = require("node:path"); -const test = require("node:test"); -const { spawnSync } = require("node:child_process"); - -const REPO_ROOT = path.resolve(__dirname, "../.."); -const HOOK = path.join(REPO_ROOT, ".claude/hooks/check-architecture-rules.js"); - -const CLEAN_SKILL = [ - "# Clean skill", - "", - "## Actions", - "", - "| # | Action | Role |", - "| --- | --- | --- |", - "| 01 | `step` | Do the one thing |", - "", -].join("\n"); - -const SIBLING_ADDRESS_SKILL = [ - "# Clean skill", - "", - "See @aidd-fixture-b:02-thing for the other half.", - "", - "## Actions", - "", - "| # | Action | Role |", - "| --- | --- | --- |", - "| 01 | `step` | Do the one thing |", - "", -].join("\n"); - -/** A fresh temp tree with a `plugins/` root, so the hook's own file classifier applies to it - * exactly as it would to this repository, without touching this repository's tracked files. */ -function makeProjectDir() { - const dir = fs.mkdtempSync(path.join(os.tmpdir(), "architecture-hook-test-")); - return dir; -} - -function writeFixtureFile(projectDir, relPath, content) { - const abs = path.join(projectDir, relPath); - fs.mkdirSync(path.dirname(abs), { recursive: true }); - fs.writeFileSync(abs, content, "utf8"); - return abs; -} - -function runHook(projectDir, payload) { - const input = typeof payload === "string" ? payload : JSON.stringify(payload); - const result = spawnSync(process.execPath, [HOOK], { - input, - encoding: "utf8", - env: { ...process.env, CLAUDE_PROJECT_DIR: projectDir }, - cwd: projectDir, - }); - return result; -} - -function parseDenyReason(stdout) { - const parsed = JSON.parse(stdout); - return parsed.hookSpecificOutput.permissionDecisionReason; -} - -test("a clean Write over a plugin skill exits zero with no output", () => { - const projectDir = makeProjectDir(); - const filePath = path.join(projectDir, "plugins/aidd-fixture-a/skills/01-clean/SKILL.md"); - - const result = runHook(projectDir, { - tool_name: "Write", - tool_input: { file_path: filePath, content: CLEAN_SKILL }, - }); - - assert.equal(result.status, 0); - assert.equal(result.stdout, ""); -}); - -test("a Write introducing a sibling address is denied, naming file, line and owning plugin", () => { - const projectDir = makeProjectDir(); - const filePath = path.join(projectDir, "plugins/aidd-fixture-a/skills/01-clean/SKILL.md"); - - const result = runHook(projectDir, { - tool_name: "Write", - tool_input: { file_path: filePath, content: SIBLING_ADDRESS_SKILL }, - }); - - assert.equal(result.status, 0); - const reason = parseDenyReason(result.stdout); - assert.match(reason, /plugins\/aidd-fixture-a\/skills\/01-clean\/SKILL\.md:3/); - assert.match(reason, /aidd-fixture-b/); -}); - -test("an Edit reconstructing the same content as the denied Write yields the same verdict", () => { - const projectDir = makeProjectDir(); - const filePath = writeFixtureFile( - projectDir, - "plugins/aidd-fixture-a/skills/01-clean/SKILL.md", - CLEAN_SKILL - ); - - const result = runHook(projectDir, { - tool_name: "Edit", - tool_input: { - file_path: filePath, - old_string: "# Clean skill\n", - new_string: "# Clean skill\n\nSee @aidd-fixture-b:02-thing for the other half.\n", - }, - }); - - assert.equal(result.status, 0); - const writeResult = runHook(projectDir, { - tool_name: "Write", - tool_input: { file_path: filePath, content: SIBLING_ADDRESS_SKILL }, - }); - const editReason = parseDenyReason(result.stdout); - const writeReason = parseDenyReason(writeResult.stdout); - assert.equal(editReason, writeReason); -}); - -test("an Edit whose new_string is a $-replacement token splices it literally, not specially", () => { - const projectDir = makeProjectDir(); - const AGENT_WITH_PERMISSION_LIST = [ - "# Guardrails", - "", - "- Never delegate to another agent.", - "", - "# Skills you may invoke", - "", - "- `/aidd-dev:02-implement`", - "- `/aidd-vcs:01-commit`", - "", - ].join("\n"); - const filePath = writeFixtureFile( - projectDir, - "plugins/aidd-dev/agents/executor.md", - AGENT_WITH_PERMISSION_LIST - ); - - // Removing the heading unmasks the one real sibling address below it (it is no longer under - // an exempt "# Skills you may invoke" heading) — a single, genuine violation. A `String.replace` - // reconstruction corrupts this: "$'" is a special token even against a plain-string search, so - // it re-inserts (and thereby duplicates) everything after the match, producing a second, - // spurious violation at a line that holds no such content in the real result. - const result = runHook(projectDir, { - tool_name: "Edit", - tool_input: { - file_path: filePath, - old_string: "# Skills you may invoke", - new_string: "$'", - }, - }); - - assert.equal(result.status, 0); - const reason = parseDenyReason(result.stdout); - assert.match(reason, /plugins\/aidd-dev\/agents\/executor\.md:8 /); - // Line 12 only exists in the corrupted (duplicated-tail) reconstruction — its absence here is - // what proves the splice was literal, not merely that some deny happened. - assert.doesNotMatch(reason, /plugins\/aidd-dev\/agents\/executor\.md:12 /); -}); - -test("an Edit whose old_string is absent from the current file exits zero", () => { - const projectDir = makeProjectDir(); - const filePath = writeFixtureFile( - projectDir, - "plugins/aidd-fixture-a/skills/01-clean/SKILL.md", - CLEAN_SKILL - ); - - const result = runHook(projectDir, { - tool_name: "Edit", - tool_input: { - file_path: filePath, - old_string: "this text is not in the file", - new_string: "See @aidd-fixture-b:02-thing for the other half.", - }, - }); - - assert.equal(result.status, 0); - assert.equal(result.stdout, ""); -}); - -test("a path under assets/ exits zero even when its content addresses a sibling", () => { - const projectDir = makeProjectDir(); - const filePath = path.join( - projectDir, - "plugins/aidd-fixture-a/skills/01-clean/assets/notes.md" - ); - - const result = runHook(projectDir, { - tool_name: "Write", - tool_input: { file_path: filePath, content: SIBLING_ADDRESS_SKILL }, - }); - - assert.equal(result.status, 0); - assert.equal(result.stdout, ""); -}); - -test("writing a new action file is always silent, named or not — rule two fires on the SKILL.md write only", () => { - // Order A of adding an action: the action file lands before the SKILL.md row that names it. - // A sibling-side check here would deny this exact, routine sequence (issue: adding an action - // is impossible in either order); the gap it trades for — an unnamed action file going - // uncaught until the next SKILL.md write — is real and recorded outside this file, not hidden. - const projectDir = makeProjectDir(); - writeFixtureFile(projectDir, "plugins/aidd-fixture-a/skills/01-clean/SKILL.md", CLEAN_SKILL); - writeFixtureFile( - projectDir, - "plugins/aidd-fixture-a/skills/01-clean/actions/01-step.md", - "# Step\n" - ); - const filePath = path.join( - projectDir, - "plugins/aidd-fixture-a/skills/01-clean/actions/02-extra.md" - ); - - const result = runHook(projectDir, { - tool_name: "Write", - tool_input: { file_path: filePath, content: "# Extra\n\nDoes something new.\n" }, - }); - - assert.equal(result.status, 0); - assert.equal(result.stdout, ""); -}); - -test("a malformed payload exits zero with no output", () => { - const projectDir = makeProjectDir(); - - const result = runHook(projectDir, "{ not json at all"); - - assert.equal(result.status, 0); - assert.equal(result.stdout, ""); -}); - -test("a path outside plugins/ exits zero even when its content addresses a sibling", () => { - const projectDir = makeProjectDir(); - const filePath = path.join(projectDir, "README.md"); - - const result = runHook(projectDir, { - tool_name: "Write", - tool_input: { file_path: filePath, content: SIBLING_ADDRESS_SKILL }, - }); - - assert.equal(result.status, 0); - assert.equal(result.stdout, ""); -}); - -test("an agent's permission list naming a sibling canonically is applied with no complaint", () => { - const projectDir = makeProjectDir(); - const filePath = path.join(projectDir, "plugins/aidd-fixture-a/agents/thing.md"); - const content = [ - "# Thing", - "", - "# Skills you may invoke", - "", - "- `/aidd-fixture-b:02-thing`", - "", - ].join("\n"); - - const result = runHook(projectDir, { - tool_name: "Write", - tool_input: { file_path: filePath, content }, - }); - - assert.equal(result.status, 0); - assert.equal(result.stdout, ""); -}); - -test("an Edit with replace_all denies on both occurrences the reconstruction introduces", () => { - const projectDir = makeProjectDir(); - const twoAnchors = [ - "# Clean skill", - "", - "ANCHOR", - "", - "ANCHOR", - "", - "## Actions", - "", - "| # | Action | Role |", - "| --- | --- | --- |", - "| 01 | `step` | Do the one thing |", - "", - ].join("\n"); - const filePath = writeFixtureFile( - projectDir, - "plugins/aidd-fixture-a/skills/01-clean/SKILL.md", - twoAnchors - ); - - const result = runHook(projectDir, { - tool_name: "Edit", - tool_input: { - file_path: filePath, - old_string: "ANCHOR", - new_string: "See @aidd-fixture-b:02-thing here.", - replace_all: true, - }, - }); - - assert.equal(result.status, 0); - const reason = parseDenyReason(result.stdout); - assert.match(reason, /:3\b/); - assert.match(reason, /:5\b/); -}); - -test("a MultiEdit that introduces a sibling address is denied", () => { - const projectDir = makeProjectDir(); - const rel = "plugins/aidd-fixture-a/skills/01-clean/SKILL.md"; - const filePath = writeFixtureFile(projectDir, rel, CLEAN_SKILL); - - const result = runHook(projectDir, { - tool_name: "MultiEdit", - tool_input: { - file_path: filePath, - edits: [ - { old_string: "# Clean skill", new_string: "# Clean skill\n\nstill clean here" }, - { old_string: "still clean here", new_string: "See @aidd-fixture-b:02-thing for the other half." }, - ], - }, - }); - - assert.equal(result.status, 0); - const reason = parseDenyReason(result.stdout); - assert.match(reason, /aidd-fixture-b/); - assert.match(reason, new RegExp(`${rel}:3`)); -}); - -test("a MultiEdit whose edits stay clean exits zero with no output", () => { - const projectDir = makeProjectDir(); - const rel = "plugins/aidd-fixture-a/skills/01-clean/SKILL.md"; - const filePath = writeFixtureFile(projectDir, rel, CLEAN_SKILL); - - const result = runHook(projectDir, { - tool_name: "MultiEdit", - tool_input: { - file_path: filePath, - edits: [{ old_string: "# Clean skill", new_string: "# Still a clean skill" }], - }, - }); - - assert.equal(result.status, 0); - assert.equal(result.stdout, ""); -}); - -test("a MultiEdit whose first edit cannot be applied exits zero", () => { - const projectDir = makeProjectDir(); - const rel = "plugins/aidd-fixture-a/skills/01-clean/SKILL.md"; - const filePath = writeFixtureFile(projectDir, rel, CLEAN_SKILL); - - const result = runHook(projectDir, { - tool_name: "MultiEdit", - tool_input: { - file_path: filePath, - edits: [ - { old_string: "a string this file never holds", new_string: "@aidd-fixture-b:02-thing" }, - ], - }, - }); - - assert.equal(result.status, 0); - assert.equal(result.stdout, ""); -}); - -test("a relative file_path from a differing cwd still resolves the actions directory against the project root", () => { - // The hook must resolve tool_input.file_path against CLAUDE_PROJECT_DIR exactly once, the same - // way for every filesystem read it performs. Before that fix, actionFileNamesFor derived its - // directory from the raw (relative) file_path, which Node then resolves against the process's - // actual cwd rather than the project root — silently emptying the action-file list, and with - // it, rule two, whenever the two differ. - const projectDir = makeProjectDir(); - const otherCwd = fs.mkdtempSync(path.join(os.tmpdir(), "architecture-hook-othercwd-")); - writeFixtureFile(projectDir, "plugins/aidd-fixture-a/skills/01-clean/SKILL.md", CLEAN_SKILL); - writeFixtureFile( - projectDir, - "plugins/aidd-fixture-a/skills/01-clean/actions/01-step.md", - "# Step\n" - ); - writeFixtureFile( - projectDir, - "plugins/aidd-fixture-a/skills/01-clean/actions/02-extra.md", - "# Extra\n" - ); - - const result = spawnSync(process.execPath, [HOOK], { - input: JSON.stringify({ - tool_name: "Write", - tool_input: { - file_path: "plugins/aidd-fixture-a/skills/01-clean/SKILL.md", - content: CLEAN_SKILL, - }, - }), - encoding: "utf8", - env: { ...process.env, CLAUDE_PROJECT_DIR: projectDir }, - cwd: otherCwd, - }); - - assert.equal(result.status, 0); - const reason = parseDenyReason(result.stdout); - assert.match(reason, /never names action file "02-extra\.md"/); -}); - -test("adding an action to an existing skill succeeds file-first (order A)", () => { - const projectDir = makeProjectDir(); - writeFixtureFile(projectDir, "plugins/aidd-fixture-a/skills/01-clean/SKILL.md", CLEAN_SKILL); - writeFixtureFile( - projectDir, - "plugins/aidd-fixture-a/skills/01-clean/actions/01-step.md", - "# Step\n" - ); - - const actionFilePath = path.join( - projectDir, - "plugins/aidd-fixture-a/skills/01-clean/actions/02-refine.md" - ); - const createAction = runHook(projectDir, { - tool_name: "Write", - tool_input: { file_path: actionFilePath, content: "# Refine\n" }, - }); - assert.equal(createAction.status, 0); - assert.equal(createAction.stdout, ""); - - fs.writeFileSync(actionFilePath, "# Refine\n", "utf8"); - const SKILL_NAMING_BOTH = [ - "# Clean skill", - "", - "## Actions", - "", - "| # | Action | Role |", - "| --- | --- | --- |", - "| 01 | `step` | Do the one thing |", - "| 02 | `refine` | Do the new thing |", - "", - ].join("\n"); - const skillFilePath = path.join(projectDir, "plugins/aidd-fixture-a/skills/01-clean/SKILL.md"); - const nameAction = runHook(projectDir, { - tool_name: "Write", - tool_input: { file_path: skillFilePath, content: SKILL_NAMING_BOTH }, - }); - assert.equal(nameAction.status, 0); - assert.equal(nameAction.stdout, ""); -}); - -test("adding an action to an existing skill succeeds citation-first (order B)", () => { - const projectDir = makeProjectDir(); - writeFixtureFile(projectDir, "plugins/aidd-fixture-a/skills/01-clean/SKILL.md", CLEAN_SKILL); - writeFixtureFile( - projectDir, - "plugins/aidd-fixture-a/skills/01-clean/actions/01-step.md", - "# Step\n" - ); - - const SKILL_CITING_REFINE = [ - "# Clean skill", - "", - "## Actions", - "", - "| # | Action | Role |", - "| --- | --- | --- |", - "| 01 | `step` | Do the one thing |", - "| 02 | `refine` | Do the new thing |", - "", - ].join("\n"); - const skillFilePath = path.join(projectDir, "plugins/aidd-fixture-a/skills/01-clean/SKILL.md"); - const citeFirst = runHook(projectDir, { - tool_name: "Write", - tool_input: { file_path: skillFilePath, content: SKILL_CITING_REFINE }, - }); - assert.equal(citeFirst.status, 0); - assert.equal(citeFirst.stdout, ""); - - fs.writeFileSync(skillFilePath, SKILL_CITING_REFINE, "utf8"); - const actionFilePath = path.join( - projectDir, - "plugins/aidd-fixture-a/skills/01-clean/actions/02-refine.md" - ); - const createAction = runHook(projectDir, { - tool_name: "Write", - tool_input: { file_path: actionFilePath, content: "# Refine\n" }, - }); - assert.equal(createAction.status, 0); - assert.equal(createAction.stdout, ""); -}); - -test("a router refusal says what a citation is, not only that one is missing", () => { - const projectDir = makeProjectDir(); - const rel = "plugins/aidd-fixture-a/skills/01-clean/SKILL.md"; - const filePath = writeFixtureFile(projectDir, rel, CLEAN_SKILL); - writeFixtureFile(projectDir, "plugins/aidd-fixture-a/skills/01-clean/actions/01-step.md", "# step\n"); - writeFixtureFile(projectDir, "plugins/aidd-fixture-a/skills/01-clean/actions/02-refine.md", "# refine\n"); - - const result = runHook(projectDir, { - tool_name: "Write", - tool_input: { file_path: filePath, content: `${CLEAN_SKILL}\nThe refine step tidies up.\n` }, - }); - - assert.equal(result.status, 0); - const reason = parseDenyReason(result.stdout); - assert.match(reason, /02-refine\.md/); - assert.match(reason, /table header that reads Action/); - assert.match(reason, /a word in prose does not count/); -}); diff --git a/scripts/lib/architecture-scan.js b/scripts/lib/architecture-scan.js index 75b8cbafb..61baf8848 100644 --- a/scripts/lib/architecture-scan.js +++ b/scripts/lib/architecture-scan.js @@ -1,7 +1,7 @@ /** * The filesystem layer the two rules need: `architecture-rules.js` stays pure, and everything - * that reads a directory or a file lives here, so the write-time hook and the commit-time check - * ask the same questions the same way. + * that reads a directory or a file lives here, so the rules can be decided without the engine + * ever learning what a directory is. */ "use strict"; @@ -27,7 +27,6 @@ function actionFileNames(relativePath, absolutePath) { } } -/** Violations for content that may not be on disk yet, which is what the write-time hook holds. */ function violationsForFile(relativePath, content, absolutePath) { try { const found = checkArchitecture(relativePath, content, actionFileNames(relativePath, absolutePath)); @@ -79,9 +78,7 @@ function scan(root, relativePaths) { } module.exports = { - actionFileNames, describeFix, governedPaths, scan, - violationsForFile, }; From 16d0419653877bdf0879052ff18e0b9f125a77fb Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Mon, 21 Sep 2026 06:36:43 +0200 Subject: [PATCH 2/4] docs(framework): the enforcement gets a line, not three paragraphs 231 words to say a pre-commit job runs. The file breakdown is readable in the code, the reason the write-time hook went lives in its own commit and pull request, and "never refuses a citation with no file behind it" is a test's job to state, not a sentence's. What stays is the rule, and the one exemption a reader applying that rule would otherwise trip over. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2 --- docs/ARCHITECTURE.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index b52819e99..eefcea94c 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -155,11 +155,9 @@ Address a capability only where the dispatch is declared: a router's `## Actions Recipe skills never hardcode a sibling provider. They discover cross-plugin capabilities at runtime through description matching. Agent permission lists and orchestration references are responsibility maps, so they name the current provider with its canonical `/plugin:folder` or `@plugin:agent` address. The orchestrator must verify that provider is installed before calling it. -`scripts/lib/architecture-rules.js` is the guard that decides both rules from a path and its content, and `architecture-scan.js` is the only place that reads the disk for it. `scripts/check-architecture-rules.js` runs them as the `architecture-rules` pre-commit job, so every edit reaching a commit is covered whichever tool or person made it, and `validate.yml` replays the job over the whole tree on each pull request. +Both rules run as the `architecture-rules` pre-commit job, so a commit is refused whichever tool made the edit. -It is enforced at commit time on purpose. An earlier revision also refused the write itself, through a Claude Code `PreToolUse` hook, which bought one turn of latency for a mechanism no other tool can host: across every plugin shipped here the portable hook surface is `SessionStart`, and a write-time refusal has no equivalent on Codex, Cursor or Copilot. A guard this repository could only enforce for one of the tools it supports contradicts the premise of the repository, so it was removed rather than duplicated. - -`plugins/aidd-context/skills/00-onboard/**` is exempt from rule one: its reference menus name addresses because those are what the skill hands a person to type, not a hardcoded sibling provider. That exemption is temporary and ends with the follow-up issue on making that skill resolve its providers at runtime. The guard refuses an edit leaving a `## Actions` section without a citation for an action file that exists; it never refuses a citation with no file behind it, because that is how a skill legitimately grows one. +- `plugins/aidd-context/skills/00-onboard/**` is exempt from the first: its menus name addresses because a person types them. Temporary, until that skill resolves its providers at runtime. This distinction keeps recipe plugins swappable while making orchestration handoffs explicit and auditable. From 76a3acf7c5c396c2cd367d4d418bdb7ec0ac55b1 Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Mon, 21 Sep 2026 06:38:49 +0200 Subject: [PATCH 3/4] docs(framework): the mechanism moves to where mechanisms are read ARCHITECTURE.md states the rule and the one exception to it. What runs the rule belongs in the pre-commit gates table, which is where someone asks what a commit checks. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2 --- aidd_docs/memory/coding-assertions.md | 2 +- docs/ARCHITECTURE.md | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/aidd_docs/memory/coding-assertions.md b/aidd_docs/memory/coding-assertions.md index 957915a46..280b12221 100644 --- a/aidd_docs/memory/coding-assertions.md +++ b/aidd_docs/memory/coding-assertions.md @@ -26,7 +26,7 @@ Never state in a commit message or a report anything not just observed in output | Order | Command | Checks | | ----- | ------- | ------ | -| 1 | `pnpm exec lefthook run pre-commit` | JSON and YAML validity, `scripts/` tests, skill frontmatter and argument hints, context imports and reference form, markdown links, the paths the prose names and a sentence written in two documents (`scripts/check-doc-duplication.js`); `cli` lint, architecture, typecheck and type honesty when `cli/` changed. `cli` knip and the full `cli` suite are pre-push, not pre-commit — see below | +| 1 | `pnpm exec lefthook run pre-commit` | JSON and YAML validity, `scripts/` tests, skill frontmatter and argument hints, cross-plugin orthogonality and router coherence (`scripts/check-architecture-rules.js`), context imports and reference form, markdown links, the paths the prose names and a sentence written in two documents (`scripts/check-doc-duplication.js`); `cli` lint, architecture, typecheck and type honesty when `cli/` changed. `cli` knip and the full `cli` suite are pre-push, not pre-commit — see below | | 2 | `pnpm exec commitlint --edit` | the message against `commitlint.config.cjs` | Same hook regenerates each plugin's `CATALOG.md`, the README counts and `docs/prompts-documentation.md`, and stages them. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index eefcea94c..62cc4f7f1 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -155,9 +155,7 @@ Address a capability only where the dispatch is declared: a router's `## Actions Recipe skills never hardcode a sibling provider. They discover cross-plugin capabilities at runtime through description matching. Agent permission lists and orchestration references are responsibility maps, so they name the current provider with its canonical `/plugin:folder` or `@plugin:agent` address. The orchestrator must verify that provider is installed before calling it. -Both rules run as the `architecture-rules` pre-commit job, so a commit is refused whichever tool made the edit. - -- `plugins/aidd-context/skills/00-onboard/**` is exempt from the first: its menus name addresses because a person types them. Temporary, until that skill resolves its providers at runtime. +- `plugins/aidd-context/skills/00-onboard/**` is exempt: its menus name addresses because a person types them. Temporary, until that skill resolves its providers at runtime. This distinction keeps recipe plugins swappable while making orchestration handoffs explicit and auditable. From 06d430428322c230bbd173ef000111f48f4719ca Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Mon, 21 Sep 2026 11:43:21 +0200 Subject: [PATCH 4/4] fix(framework): three reviewers found what the author did not A critical pass over the two documents, one reader at a time, found five defects in this branch's own edits. All five are corrected here. - The `--no-verify` sentence was replaced by a second falsehood. Checked against all nine workflows this time: no workflow fires on a push to a feature branch, `ci.yml` and `cli-ci.yml` fire on any pull request whatever its base, and only `validate.yml` filters `main` and `next`, minus three `cli` jobs it excludes. - Reading "Claude Code" as "an AI coding tool" made line 9 false. The manifest it points at is `.claude-plugin/marketplace.json`; codex emits `.agents/plugins/`, cursor `.cursor-plugin/`, copilot `.plugin/`. All three substitutions are reverted. That framing needs a rewrite, not a word swap. - The exemption bullet named one of the two holes in rule one. `isExemptFromOrthogonality` also exempts every path under `plugins/aidd-orchestrator/`. - The doc test read tables by position, so a `Runs` cell could name a deleted script and stay green. Tables are now found by their header cells and read by column name. A fifth test opens the script the table names. - Its failure message named one exemption bullet after this branch split it into two. Nine attacks replayed against the rewritten parser: a missing script, an un-backticked name and a wrong event go red; a blank line inside a table, an escaped pipe and an inserted column stay green. Four semantic mutations each redden exactly the test that names them. 503 tests pass. Referenced paths 0 dead, doc duplication 0, architecture rules 350 governed files clean. Refs #250 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2 --- aidd_docs/memory/coding-assertions.md | 6 +- docs/ARCHITECTURE.md | 7 +- .../architecture-doc-matches-the-tree.test.js | 178 ++++++++++++++++++ scripts/check-architecture-rules.js | 5 +- scripts/lib/architecture-scan.js | 2 +- 5 files changed, 191 insertions(+), 7 deletions(-) create mode 100644 scripts/__tests__/architecture-doc-matches-the-tree.test.js diff --git a/aidd_docs/memory/coding-assertions.md b/aidd_docs/memory/coding-assertions.md index 280b12221..f68a25294 100644 --- a/aidd_docs/memory/coding-assertions.md +++ b/aidd_docs/memory/coding-assertions.md @@ -45,7 +45,11 @@ Every `cli` job is globbed on `cli/**`. A change under `kanban/` alone fires no Each runs through `scripts/gate-witness.js`, which skips a gate this exact tree already passed: same index, same unstaged edits, same untracked files. A tree that changed while the gate ran is never stamped. -`--no-verify` buys nothing: `validate.yml` re-runs the whole pre-commit over the whole tree on every push and pull request. +`--no-verify` buys little, not nothing: + +- A push to a feature branch fires no workflow. +- Any pull request fires `ci.yml` and `cli-ci.yml`, whatever its base. +- Only a pull request targeting `main` or `next` replays the pre-commit, and `validate.yml` drops `cli-biome`, `cli-architecture` and `cli-typecheck` from it. ## Behavior diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 62cc4f7f1..cf063a76f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -135,7 +135,7 @@ flowchart LR Recipe skills route to self-contained actions with inputs, outputs, process steps, and tests. An orchestrator with no domain logic may instead route through numbered reference protocols that define handoffs and delegate the work to capabilities discovered at runtime. -A skill never links outside itself. The same tree ships flat, where the skill folder is renamed `-`, or as a marketplace, so no relative path survives both. A bundled script is named plugin-relative in backticks, never linked. +A skill never links outside itself (`scripts/__tests__/a-skill-links-only-inside-itself.test.js`). The same tree ships flat, where the skill folder is renamed `-`, or as a marketplace, so no relative path survives both. A bundled script is named plugin-relative in backticks, never linked. ## 🤖 Skills and agents @@ -155,7 +155,10 @@ Address a capability only where the dispatch is declared: a router's `## Actions Recipe skills never hardcode a sibling provider. They discover cross-plugin capabilities at runtime through description matching. Agent permission lists and orchestration references are responsibility maps, so they name the current provider with its canonical `/plugin:folder` or `@plugin:agent` address. The orchestrator must verify that provider is installed before calling it. -- `plugins/aidd-context/skills/00-onboard/**` is exempt: its menus name addresses because a person types them. Temporary, until that skill resolves its providers at runtime. +Two paths are exempt, both in `isExemptFromOrthogonality`: + +- `plugins/aidd-orchestrator/**`, whose references are responsibility maps. +- `plugins/aidd-context/skills/00-onboard/**`, whose menus name addresses a person types. Temporary: it ends when that skill resolves its providers at runtime. This distinction keeps recipe plugins swappable while making orchestration handoffs explicit and auditable. diff --git a/scripts/__tests__/architecture-doc-matches-the-tree.test.js b/scripts/__tests__/architecture-doc-matches-the-tree.test.js new file mode 100644 index 000000000..482b7564f --- /dev/null +++ b/scripts/__tests__/architecture-doc-matches-the-tree.test.js @@ -0,0 +1,178 @@ +const assert = require("node:assert/strict"); +const fs = require("node:fs"); +const path = require("node:path"); +const test = require("node:test"); + +const { checkOrthogonality } = require("../lib/architecture-rules.js"); + +const ROOT = path.resolve(__dirname, "../.."); +const DOC = path.join(ROOT, "docs/ARCHITECTURE.md"); + +/** + * `docs/ARCHITECTURE.md` describes the tree in two of its tables: the bundled hooks each plugin + * declares, and the plugin roster. Both were right when written and nothing compared them to the + * tree afterwards, which is how a document goes quietly wrong. These tests are that comparison. + * + * Tables are found by their header cells and read by column name, never by position, so adding a + * column, reordering two tables under one heading or leaving a blank line between rows changes + * nothing here. What must not change is a header cell this file names. + */ + +const CELL_SEPARATOR = /(? cell.trim()); +} + +function isSeparatorRow(cells) { + return cells.every((cell) => /^:?-+:?$/.test(cell)); +} + +/** + * Every table in the file, as `{ header, rows }`. A blank line does not end a table, because a + * table split for grouping is still one table. Any other non-row line does, because otherwise two + * tables separated by prose read as one. + */ +function tables() { + const found = []; + let current = null; + + for (const line of fs.readFileSync(DOC, "utf8").split("\n")) { + if (!line.trim().startsWith("|")) { + if (line.trim() !== "") current = null; + continue; + } + const cells = rowCells(line); + if (isSeparatorRow(cells)) continue; + if (current === null) { + current = { header: cells, rows: [] }; + found.push(current); + } else { + current.rows.push(cells); + } + } + return found; +} + +/** The one table whose header holds every name given. Never two, never none. */ +function tableWithColumns(...names) { + const matches = tables().filter((table) => + names.every((name) => table.header.some((cell) => cell.toLowerCase() === name.toLowerCase())) + ); + assert.equal( + matches.length, + 1, + `expected exactly one table in ARCHITECTURE.md with columns ${names.join(", ")}, found ${matches.length}` + ); + return matches[0]; +} + +function column(table, name) { + const at = table.header.findIndex((cell) => cell.toLowerCase() === name.toLowerCase()); + return (cells) => cells[at] ?? ""; +} + +/** `` `aidd-ui` 🚧 `` and `` `SessionStart` · `Stop` `` both mean the backticked tokens. */ +function backticked(cell) { + return [...cell.matchAll(/`([^`]+)`/g)].map((match) => match[1]); +} + +function onlyBacktickedName(cell, where) { + const names = backticked(cell); + assert.equal(names.length, 1, `${where}: expected one backticked name, got ${JSON.stringify(cell)}`); + return names[0]; +} + +function pluginDirectories() { + return fs + .readdirSync(path.join(ROOT, "plugins"), { withFileTypes: true }) + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name) + .sort(); +} + +function declaredHookEvents(plugin) { + const manifest = path.join(ROOT, "plugins", plugin, "hooks/hooks.json"); + if (!fs.existsSync(manifest)) return null; + return Object.keys(JSON.parse(fs.readFileSync(manifest, "utf8")).hooks ?? {}).sort(); +} + +const hooksTable = () => tableWithColumns("Plugin", "Event", "Runs"); +const concernsTable = () => tableWithColumns("Plugin", "Concern", "Layer"); + +test("the bundled hooks table names every plugin that declares hooks, and no other", () => { + const table = hooksTable(); + const plugin = column(table, "Plugin"); + const documented = table.rows.map((cells) => onlyBacktickedName(plugin(cells), "hooks table")).sort(); + assert.deepEqual(documented, pluginDirectories().filter((name) => declaredHookEvents(name) !== null)); +}); + +test("the bundled hooks table names the events each plugin actually declares", () => { + const table = hooksTable(); + const plugin = column(table, "Plugin"); + const event = column(table, "Event"); + for (const cells of table.rows) { + const name = onlyBacktickedName(plugin(cells), "hooks table"); + assert.deepEqual( + backticked(event(cells)).sort(), + declaredHookEvents(name), + `${name}: the table and plugins/${name}/hooks/hooks.json disagree` + ); + } +}); + +test("the bundled hooks table names a script that exists", () => { + // Without this the table can point at a deleted hook and still read as current. + const table = hooksTable(); + const plugin = column(table, "Plugin"); + const runs = column(table, "Runs"); + for (const cells of table.rows) { + const name = onlyBacktickedName(plugin(cells), "hooks table"); + const script = onlyBacktickedName(runs(cells), `hooks table, ${name} Runs cell`); + const full = path.join(ROOT, "plugins", name, script); + assert.ok(fs.existsSync(full), `plugins/${name}/${script} is named in ARCHITECTURE.md and does not exist`); + } +}); + +test("the plugin concerns table has one row per plugin in the tree", () => { + const table = concernsTable(); + const plugin = column(table, "Plugin"); + const documented = table.rows.map((cells) => onlyBacktickedName(plugin(cells), "concerns table")).sort(); + assert.deepEqual(documented, pluginDirectories()); +}); + +/** + * Rule one has two holes, both deliberate, both in `isExemptFromOrthogonality`. The 00-onboard one + * is load-bearing only while that skill still hardcodes the addresses it hands a person to type. + * Once it resolves its providers at runtime the hole protects nothing and should go, and nothing + * else would say so. + */ +test("the 00-onboard exemption still protects something", () => { + const skillDir = path.join(ROOT, "plugins/aidd-context/skills/00-onboard"); + const files = []; + const walk = (dir) => { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) walk(full); + else if (entry.name.endsWith(".md")) files.push(full); + } + }; + walk(skillDir); + + // Judged under a path the exemption does not cover, so the rule speaks instead of skipping. + const probePath = "plugins/aidd-context/skills/99-exemption-probe/SKILL.md"; + const wouldViolate = files.filter( + (file) => checkOrthogonality(probePath, fs.readFileSync(file, "utf8")).length > 0 + ); + + assert.ok( + wouldViolate.length > 0, + "00-onboard no longer hardcodes a sibling address. Delete ONBOARD_EXEMPT_PREFIX from " + + "scripts/lib/architecture-rules.js, its exemption bullet in docs/ARCHITECTURE.md, and this test." + ); +}); diff --git a/scripts/check-architecture-rules.js b/scripts/check-architecture-rules.js index 18b2ec53d..c59cd0df3 100644 --- a/scripts/check-architecture-rules.js +++ b/scripts/check-architecture-rules.js @@ -4,9 +4,8 @@ * The two architecture rules of issue #250, at commit time. The write-time hook only ever sees * Claude Code; this sees every edit that reaches a commit, whichever tool or person made it. * - * Paths given as arguments are checked; with none, the whole governed tree is. `--root ` - * moves both, which is what lets the failing path be exercised against a tree that is not this - * repository — a gate whose refusal nothing tests is a gate nobody can trust. + * Paths given as arguments are checked; with none, the whole governed tree is. + * `--root ` moves both, so the refusal can be exercised against another tree. */ "use strict"; diff --git a/scripts/lib/architecture-scan.js b/scripts/lib/architecture-scan.js index 61baf8848..886c469c6 100644 --- a/scripts/lib/architecture-scan.js +++ b/scripts/lib/architecture-scan.js @@ -13,7 +13,7 @@ const { checkArchitecture, classifyFile } = require("./architecture-rules.js"); const CITATION_SHAPES = "a cell under a table header that reads Action, a fenced `actions/.md` path, or a " + - "backticked `.md` file name — a word in prose does not count"; + "backticked `.md` file name. A word in prose does not count"; /** Rule two needs the skill's action files; the engine never reads them itself. */ function actionFileNames(relativePath, absolutePath) {