From 00c5ac23734e3b4da898705658488231b4e85200 Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Fri, 18 Sep 2026 11:57:15 +0200 Subject: [PATCH 01/11] feat(framework): refuse an AI edit that breaks a named architecture rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docs/ARCHITECTURE.md stated cross-plugin orthogonality and nothing verified it, so a hardcoded sibling address landed and no one noticed. Two rules now decide a prospective edit: a dispatch surface never names a sibling plugin, and a skill's `## Actions` section names exactly the actions it provides. They live in a pure module, so a test hands them the same inputs the hook does, without either touching disk through them. A PreToolUse hook is the enforcement, not lefthook and not CI: the decision is to prevent the write, and PostToolUse can only report one already made. The hook fails open on every shape it does not recognise, because it gates every write in this repository. The rule's own exceptions are encoded rather than assumed. An agent's permission list, an orchestration reference and everything a plugin keeps under assets/ stay silent, so the guard reports nothing on the tree as it stands — 474 script tests, and the 374 governed files replayed through the hook as writes. Closes #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 | 159 ++++++++++ .claude/settings.json | 11 + .../backlog-link.json | 5 + .../phase-1.md | 106 +++++++ .../phase-2.md | 103 ++++++ .../plan.md | 39 +++ .../spec.md | 52 ++++ docs/ARCHITECTURE.md | 2 + scripts/__tests__/architecture-hook.test.js | 294 ++++++++++++++++++ scripts/__tests__/architecture-rules.test.js | 145 +++++++++ .../agent-permission-list.md | 12 + .../architecture-rules/assets-note.md | 3 + .../architecture-rules/clean-skill/SKILL.md | 15 + .../cross-address-skill/SKILL.md | 11 + .../orchestrator-skill/SKILL.md | 7 + .../phantom-citation-skill/SKILL.md | 8 + .../unnamed-action-skill/SKILL.md | 9 + scripts/lib/architecture-rules.js | 247 +++++++++++++++ 18 files changed, 1228 insertions(+) create mode 100755 .claude/hooks/check-architecture-rules.js create mode 100644 aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/backlog-link.json create mode 100644 aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/phase-1.md create mode 100644 aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/phase-2.md create mode 100644 aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md create mode 100644 aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md create mode 100644 scripts/__tests__/architecture-hook.test.js create mode 100644 scripts/__tests__/architecture-rules.test.js create mode 100644 scripts/__tests__/fixtures/architecture-rules/agent-permission-list.md create mode 100644 scripts/__tests__/fixtures/architecture-rules/assets-note.md create mode 100644 scripts/__tests__/fixtures/architecture-rules/clean-skill/SKILL.md create mode 100644 scripts/__tests__/fixtures/architecture-rules/cross-address-skill/SKILL.md create mode 100644 scripts/__tests__/fixtures/architecture-rules/orchestrator-skill/SKILL.md create mode 100644 scripts/__tests__/fixtures/architecture-rules/phantom-citation-skill/SKILL.md create mode 100644 scripts/__tests__/fixtures/architecture-rules/unnamed-action-skill/SKILL.md create mode 100644 scripts/lib/architecture-rules.js diff --git a/.claude/hooks/check-architecture-rules.js b/.claude/hooks/check-architecture-rules.js new file mode 100755 index 000000000..b84eb9834 --- /dev/null +++ b/.claude/hooks/check-architecture-rules.js @@ -0,0 +1,159 @@ +#!/usr/bin/env node + +/** + * PreToolUse guard for Write, Edit and MultiEdit: refuses an AI-authored edit that would leave a plugin's + * dispatch surface breaking one of the two named architecture rules (issue #250) — cross-plugin + * orthogonality and router coherence. The rules themselves live in `scripts/lib/architecture- + * rules.js`, a pure engine this script is the only caller of at write time. + * + * Fails open on purpose: any unrecognised shape, unparseable payload, unreconstructable edit, or + * path outside the governed surface exits 0 with no output. This hook gates every write tool call + * in the repository, so a crash here must never block unrelated work. + */ + +const fs = require("node:fs"); +const path = require("node:path"); + +function loadEngine() { + try { + // Fixed relative path: this hook always lives two levels below the repo root, at + // `.claude/hooks/`, regardless of CLAUDE_PROJECT_DIR or the process cwd. + return require(path.resolve(__dirname, "..", "..", "scripts", "lib", "architecture-rules.js")); + } catch { + return null; + } +} + +function readPayload() { + try { + const parsed = JSON.parse(fs.readFileSync(0, "utf8")); + return parsed && typeof parsed === "object" ? parsed : null; + } catch { + return null; + } +} + +/** Repository-relative, forward-slashed path, or null when it resolves outside the project. */ +function toRepoRelative(filePath) { + const root = process.env.CLAUDE_PROJECT_DIR || process.cwd(); + try { + const rel = path.relative(root, path.resolve(root, filePath)); + if (rel === "" || rel.startsWith("..") || path.isAbsolute(rel)) return null; + return rel.split(path.sep).join("/"); + } catch { + return null; + } +} + +function applyEdit(current, oldString, newString, replaceAll) { + if (typeof oldString !== "string" || typeof newString !== "string") return null; + if (!current.includes(oldString)) return null; + return replaceAll ? current.split(oldString).join(newString) : current.replace(oldString, newString); +} + +/** The prospective file content, or null when it cannot be determined (Write with no string + * content, an Edit whose old_string is absent, or the current file cannot be read). */ +function prospectiveContent(toolName, toolInput) { + if (toolName === "Write") { + return typeof toolInput.content === "string" ? toolInput.content : null; + } + if (toolName !== "Edit" && toolName !== "MultiEdit") return null; + + let current; + try { + current = fs.readFileSync(toolInput.file_path, "utf8"); + } catch { + return null; + } + + if (toolName === "Edit") { + return applyEdit(current, toolInput.old_string, toolInput.new_string, Boolean(toolInput.replace_all)); + } + + // MultiEdit applies its edits in order, each to the result of the one before. + if (!Array.isArray(toolInput.edits) || toolInput.edits.length === 0) return null; + for (const edit of toolInput.edits) { + if (!edit || typeof edit !== "object") return null; + current = applyEdit(current, edit.old_string, edit.new_string, Boolean(edit.replace_all)); + if (current === null) return null; + } + return current; +} + +/** The action file names for a SKILL.md path, read from its sibling `actions/` directory. + * Undefined for anything else, matching the engine's own contract. */ +function actionFileNamesFor(relPath, absPath) { + if (path.basename(relPath) !== "SKILL.md") return undefined; + const actionsDir = path.join(path.dirname(absPath), "actions"); + try { + return fs.readdirSync(actionsDir).filter((name) => name.endsWith(".md")); + } catch { + return []; + } +} + +function fixFor(rule, plugin) { + return rule === "orthogonality" + ? `name the concept ${plugin} owns instead of addressing it directly` + : `keep "## Actions" naming exactly the action files this skill provides, no more, no fewer`; +} + +function denyReason(violations) { + return violations + .map((v) => `${v.message}. Fix: ${fixFor(v.rule, v.plugin)}.`) + .join("\n"); +} + +function deny(reason) { + process.stdout.write( + JSON.stringify({ + hookSpecificOutput: { + hookEventName: "PreToolUse", + permissionDecision: "deny", + permissionDecisionReason: reason, + }, + }) + ); +} + +function main() { + const engine = loadEngine(); + if (!engine) return 0; + + const payload = readPayload(); + if (!payload) return 0; + + const toolName = payload.tool_name; + if (toolName !== "Write" && toolName !== "Edit" && toolName !== "MultiEdit") return 0; + + const toolInput = payload.tool_input; + if (!toolInput || typeof toolInput.file_path !== "string" || toolInput.file_path === "") return 0; + + const relPath = toRepoRelative(toolInput.file_path); + if (!relPath) return 0; + + if (!engine.classifyFile(relPath)) return 0; + + const content = prospectiveContent(toolName, toolInput); + if (content === null) return 0; + + const actionFileNames = actionFileNamesFor(relPath, toolInput.file_path); + + let violations; + try { + violations = engine.checkArchitecture(relPath, content, actionFileNames); + } catch { + return 0; + } + + if (!Array.isArray(violations) || violations.length === 0) return 0; + + deny(denyReason(violations)); + return 0; +} + +try { + process.exitCode = main(); +} catch { + process.exitCode = 0; +} diff --git a/.claude/settings.json b/.claude/settings.json index 1850d4990..ceddf9728 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -11,6 +11,17 @@ ] } ], + "PreToolUse": [ + { + "matcher": "Write|Edit|MultiEdit", + "hooks": [ + { + "type": "command", + "command": "node \"$CLAUDE_PROJECT_DIR/.claude/hooks/check-architecture-rules.js\"" + } + ] + } + ], "PostToolUse": [ { "matcher": "Edit|Write|MultiEdit", diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/backlog-link.json b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/backlog-link.json new file mode 100644 index 000000000..75e33474d --- /dev/null +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/backlog-link.json @@ -0,0 +1,5 @@ +{ + "backlog": "ai-driven-dev/framework#250", + "written_at": "2026-09-18T09:24:15Z", + "written_by": "aidd-pm:04-spec" +} diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/phase-1.md b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/phase-1.md new file mode 100644 index 000000000..c57349b1c --- /dev/null +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/phase-1.md @@ -0,0 +1,106 @@ + +# Instruction: The two rules, as a tested engine + +## Architecture projection + +> Tree of the final files. ✅ create · ✏️ modify · ❌ delete + +```txt +. +├── scripts +│ ├── lib +│ │ └── architecture-rules.js ✅ the two rules, as pure functions +│ └── __tests__ +│ ├── architecture-rules.test.js ✅ both rules, both directions, plus the clean-tree sweep +│ └── fixtures +│ └── architecture-rules ✅ synthetic plugin trees, written for this task +└── docs + └── ARCHITECTURE.md ✏️ one line pointing at the guard that now enforces the rule +``` + +## User Journey + +```mermaid +flowchart TD + A[A contributor edits a plugin source] --> B{Does the prospective content break a rule?} + B -- no --> C[Nothing is said] + B -- "a sibling plugin is addressed" --> D[File, line, owning plugin] + B -- "the Actions section is out of step" --> E[File, line, the action that does not match] + D --> F[The contributor corrects it and edits again] + E --> F +``` + +## Test Scope + +```mermaid +--- +title: Test scope +--- +journey + section Setup + Write the synthetic plugin fixtures under the test fixtures directory => fixtures on disk: 5: system + section Happy path + Run the rule engine over a fixture whose skill addresses only its own plugin => no violation: 5: cli + Run the rule engine over the repository's own plugins directory => no violation: 5: cli + section Edge case - a recipe skill addresses a sibling + A skill file names another plugin's skill => run the engine over it => one violation naming the file, the line and the owning plugin: 1: cli + section Edge case - a permission list addresses a sibling + An agent lists a sibling skill under its permission heading => run the engine over it => no violation: 1: cli + section Edge case - an orchestration reference addresses a sibling + An orchestrator reference names a provider => run the engine over it => no violation: 1: cli + section Edge case - an action file no section names + A skill gains an action file its Actions section never mentions => run the engine over it => one violation naming the file: 1: cli + section Edge case - a section names an action that does not exist + An Actions row cites a name with no file behind it => run the engine over it => one violation naming the file and the row: 1: cli +``` + +## Tasks to do + +### `1)` The failing tests come first + +> Every rule gets its red before it gets its engine. + +1. Write the synthetic fixtures: a plugin tree with a clean skill, a skill addressing a sibling, an agent permission list addressing a sibling, an orchestrator reference addressing a sibling, a skill with an unnamed action file, a skill citing an absent action. +2. Write `architecture-rules.test.js` covering each fixture and each expected verdict, plus one case that sweeps the repository's real `plugins/` and expects zero violations. +3. Run the suite and watch every case fail for the absence of the module, not for a typo. + +### `2)` The orthogonality rule + +> A dispatch surface never names a sibling plugin. + +1. Expose a function taking a repository-relative path and the prospective content, returning violations with a line, a 1-indexed number, the address found and the plugin that owns it. +2. Match `/:` and `@:` addresses; a match whose plugin equals the file's own owner is not a violation. +3. Govern only `SKILL.md`, `actions/*.md`, `references/*.md` and `agents/*.md`. Anything under `assets/` returns nothing. +4. Exempt a file owned by an orchestrator plugin, and exempt the lines under an agent's `# Skills you may invoke` heading. + +### `3)` The router coherence rule + +> An Actions section names exactly the actions that exist. + +1. Take the prospective `SKILL.md` content and the names of the skill's action files. +2. Isolate the `## Actions` section, up to the next second-level heading. +3. Report an action file the section names by neither its stem nor its file name, and report a name the section cites that no file backs. +4. Report the line of the section heading when the violation is an absent mention, and the line of the citation when it is a phantom one. + +### `4)` The rule the repository already follows + +> A guard that is red on a clean tree is a guard nobody keeps. + +1. Run the sweep case over the real `plugins/` tree. +2. When a rule fires there, the rule is wrong, not the tree. Narrow it and record why in `plan.md`'s decisions. + +### `5)` The rule document points at its enforcement + +> `docs/ARCHITECTURE.md` states the rule; say where it is now checked. + +1. Add one sentence naming the guard, plugin-relative and in backticks, never as a link. + +## Test acceptance criteria + +| Task | Acceptance criteria | +| --- | --- | +| 1 | Every case in the suite fails before the engine exists, each for the missing module | +| 2 | A skill addressing a sibling yields a violation naming file, line and owning plugin; an agent permission list and an orchestrator reference yield none; a file under `assets/` yields none | +| 3 | An action file the section never names yields one violation; a cited name with no file yields one violation; a skill whose section and files agree yields none | +| 4 | Sweeping the repository's own `plugins/` yields zero violations | +| 5 | `docs/ARCHITECTURE.md` names the guard, and the markdown-link check still passes | diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/phase-2.md b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/phase-2.md new file mode 100644 index 000000000..27c71e1f2 --- /dev/null +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/phase-2.md @@ -0,0 +1,103 @@ + +# Instruction: The refusal, at the AI host's write moment + +## Architecture projection + +> Tree of the final files. ✅ create · ✏️ modify · ❌ delete + +```txt +. +├── .claude +│ ├── hooks +│ │ └── check-architecture-rules.js ✅ reads the pending edit, calls the engine, refuses +│ └── settings.json ✏️ one PreToolUse entry matching Write and Edit +└── scripts + └── __tests__ + └── architecture-hook.test.js ✅ the payload shapes and the refusal contract +``` + +## User Journey + +```mermaid +flowchart TD + A[An agent calls Write or Edit on a plugin source] --> B[PreToolUse hands the hook the pending content] + B --> C{Does the prospective content break a rule?} + C -- no --> D[The write proceeds] + C -- yes --> E[The call is denied, with file, line and owning plugin] + E --> F[The agent corrects the content and calls again] + F --> B +``` + +## Test Scope + +```mermaid +--- +title: Test scope +--- +journey + section Setup + Build a PreToolUse payload for a plugin file => payload on stdin: 5: system + section Happy path + Run the hook on a payload whose content breaks no rule => exit zero and no output: 5: cli + section Edge case - a Write that introduces a sibling address + The payload carries content addressing another plugin => run the hook => a deny decision naming file, line and owning plugin: 1: cli + section Edge case - an Edit that introduces a sibling address + The payload carries an old and a new string => run the hook => the reconstructed content is judged, and the call is denied: 1: cli + section Edge case - an Edit whose old string is absent + The payload cannot be reconstructed => run the hook => exit zero, because the edit will fail on its own: 1: cli + section Edge case - a file outside the governed surface + The payload names a file under assets => run the hook => exit zero: 1: cli + section Teardown + Remove the temporary payload files => baseline restored: 5: system +``` + +## Tasks to do + +### `1)` The hook, generated by the capability that owns hooks + +> The decider asked for a host-native lifecycle hook produced by the hook capability, not a hand-placed script. + +1. Run `/aidd-context:08-hook-generate` for a `PreToolUse` hook matching `Write|Edit`, at the project scope, for Claude Code — the one host this repository configures hooks for, per the plan's decisions. +2. Keep the generated entry and script; give the script the name the projection states. + +### `2)` The prospective content + +> Judge what the file will hold, not what it holds now. + +1. Read the payload from standard input. Take `tool_input.file_path`. +2. For a `Write`, the prospective content is `tool_input.content`. +3. For an `Edit`, read the file and apply `old_string` to `new_string`, once or everywhere per `replace_all`. +4. When the file path is outside the governed surface, or the reconstruction fails, exit zero and say nothing. + +### `3)` The refusal + +> A refusal a reader cannot act on is noise. + +1. Call the engine with the path, the prospective content, and the skill's action file names when the path is a `SKILL.md`. +2. On a violation, emit the `PreToolUse` deny decision, with a reason naming each file, line, and the plugin that owns the address, and what to write instead. +3. On no violation, exit zero with no output. + +### `4)` The failing tests come first + +> The hook is a contract with the host; test it as one. + +1. Write `architecture-hook.test.js` driving the hook as a subprocess with each payload shape. +2. Watch each case fail before the hook exists. +3. Assert the decision is a deny and the reason carries the file and the line, never only the rule. + +### `5)` The proof + +> A guard nobody watched fire is a comment. + +1. Write a plugin file addressing a sibling, through the agent's own Write tool, and record that the call was refused and what it said. +2. Write an agent permission list naming a sibling the same way, and record that it was applied. + +## Test acceptance criteria + +| Task | Acceptance criteria | +| --- | --- | +| 1 | The hook entry exists in `.claude/settings.json` at the project scope and fires on `Write` and `Edit` | +| 2 | A `Write` payload and an `Edit` payload over the same file yield the same verdict for the same resulting content | +| 3 | A denied call returns a reason naming file, line and owning plugin; a clean call returns nothing at all | +| 4 | Every case fails before the hook exists, then passes | +| 5 | An attempted write of a sibling address is refused in the same turn; an attempted write of a permission list is applied | diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md new file mode 100644 index 000000000..c481e8d4c --- /dev/null +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md @@ -0,0 +1,39 @@ + +# Plan: Architecture guard on AI-authored edits + +## Overview + +| Field | Value | +| --- | --- | +| **Goal** | Refuse an AI edit that would hardcode a sibling plugin's address, or leave a skill's `## Actions` section out of step with its action files, before the edit lands. | +| **Source** | [`spec.md`](./spec.md), from [ai-driven-dev/framework#250](https://github.com/ai-driven-dev/framework/issues/250) | + +## Phases + +| # | Phase | File | +| --- | --- | --- | +| 1 | The two rules, as a tested engine | [`phase-1.md`](./phase-1.md) | +| 2 | The refusal, at the AI host's write moment | [`phase-2.md`](./phase-2.md) | + +## Resources + +| Source | Verified | +| --- | --- | +| `docs/ARCHITECTURE.md`, capability addressing | A capability is addressed only where dispatch is declared: a router's `## Actions` table, an agent's `# Skills you may invoke` list. Agent permission lists and orchestration references legitimately name a provider; recipe skills never do. | +| | `PreToolUse` receives `tool_input` before the tool runs — `content` for `Write` — and refuses the call with `hookSpecificOutput.permissionDecision: "deny"` plus a `permissionDecisionReason` the model reads. `PostToolUse` cannot refuse, it only reports after the fact. | +| `.claude/hooks/check-written-file.js` | The in-repo precedent for a hook that reads the payload from stdin, resolves the written file, and hands a report back in the same turn. | +| Issue #250, comment of 2026-09-14 | Supersedes the issue body's "Guardrail local et CI": no Git hook, no CI gate, synthetic fixtures, #406 neither blocker nor fixture. | +| Probe over the 51 skills and 8 plugins in the tree | Both rules, as scoped below, report zero violations on the repository as it stands — orthogonality across 37 real cross-plugin addresses, router coherence across 48 skills that hold action files, in both directions. | + +## Decisions + +| Decision | Why | +| --- | --- | +| `PreToolUse`, not `PostToolUse` | The decision is "prevent the write until it is corrected". `PostToolUse` fires after the file is already on disk; Biome gets away with it only because it rewrites in place, and this guard cannot rewrite prose. | +| The engine is a plain module, the hook is a thin caller | A rule that is a pure function of (path, prospective content, action-file listing) is testable without a hook, a host, or a tree. The hook contributes only the payload and the refusal. | +| The governed surface is `SKILL.md`, `actions/`, `references/`, `agents/` | That is where dispatch is declared. `assets/` hold sheets a reader reads — `12-cook`'s recipes name 20 cross-plugin commands on purpose — so they are excluded by a stated rule, never by a quiet path filter. | +| An orchestrator plugin is exempt wholesale | `docs/ARCHITECTURE.md` makes orchestration references responsibility maps. `aidd-orchestrator` holds 14 of the tree's cross-plugin addresses for exactly that reason. | +| The router rule reads the `## Actions` section, not the table | `10-todo` names its one action as a path in a fenced block rather than a table row. Scoping to the section and matching either the stem or the file name covers both shapes and still reports zero on the tree. | +| Unit tests live under `scripts/__tests__/`, and that is not a CI gate on the rules | Pre-commit runs those tests against synthetic fixtures, proving the engine works. Nothing scans the tree at commit time or in CI, which is what the decider ruled out. | +| Fixtures are written for this task | The spec forbids #406's historical code. Each rule gets a breaking fixture and a legitimate-naming fixture, so a guard that flags a permission list fails its own suite. | +| The hook is wired for Claude Code alone | It is the only host this repository configures hooks for: `.codex/config.toml` carries a sandbox mode and nothing else. Codex, Cursor and Copilot all expose `PreToolUse` and the same deny shape, but Codex delivers a file edit as an `apply_patch` command string rather than a path and a content, which is a different parse. The engine is host-agnostic, so each adapter is additive and none of them touches a rule. | diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md new file mode 100644 index 000000000..2cd5fe475 --- /dev/null +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md @@ -0,0 +1,52 @@ +# Architecture guard on AI-authored edits + +## Target + +An AI-authored edit that would break one of this repository's two named architecture rules is refused before it lands, with the offending file and line named. + +## Hard constraints + +- The guard decides before the edit is applied and refuses it. It is not a commit-time gate and not a continuous-integration job. +- The guard is deterministic: the same prospective file content yields the same verdict, with no dependence on model judgement and none on which AI tool made the edit. +- The guard is produced by the project's own hook-generation capability, so it exists for the AI hosts this repository configures rather than for one. +- Rule one, cross-plugin orthogonality: a plugin's dispatch surface must not name a sibling plugin by a hardcoded address. +- Rule two, router coherence: a skill's `## Actions` section must name exactly the actions that skill provides — no action file the section never names, no name the section cites without a file behind it. +- The governed surface is the dispatch surface: a skill's `SKILL.md`, its actions and its references, and an agent's definition. A plugin's `assets/` hold content shown to a reader, not dispatch, and are out of the guard's reach by this rule rather than by an unstated path filter. +- Naming that `docs/ARCHITECTURE.md` declares legitimate stays silent: an agent's permission list and an orchestration reference are responsibility maps and name their provider canonically. Flagging either is a defect of the guard, not of the tree. +- Every refusal names the file, the line, and the plugin that owns the addressed capability, in terms a reader can act on without opening the rule document. +- The guard is silent on the repository as it stands. A rule that reports a violation in the current tree is miscalibrated, not vindicated. +- The rules are exercised by purpose-built fixtures. No historical code from #406 is used as a fixture. +- Each rule is proved by a fixture that breaks it and turns exactly the test named for that rule red, and by a fixture of legitimate naming that stays green. + +## Non-goals + +- A `lefthook` pre-commit gate for these rules. The decider ruled it out on 2026-09-14. +- A CI job enforcing these rules. +- Judging whether a skill's prose `description` has gone stale. Nothing mechanically separates stale prose from current prose, so the issue's "router/description mismatch" is served by the router half alone. +- Reaching into a plugin's `assets/`. A recipe sheet names the commands a reader types; that is its subject, not a dispatch this rule governs. +- Repairing violations that exist in the tree today. That was #406, now closed. +- Enforcing any architecture rule beyond the two named above. +- Catching a violation introduced outside an AI tool, by a human editing by hand. +- Shipping the guard into projects that install this marketplace. The rules govern this repository's own plugin sources. + +## Done-when + +- An edit that would leave a skill's dispatch surface holding a sibling plugin's hardcoded address is refused, and the refusal names that file, its line, and the plugin that owns the address. +- An edit that would leave a skill's `## Actions` section naming an action the skill does not provide, or omitting one it does, is refused, and the refusal names the file and the line. +- An edit that writes an agent permission list, or an orchestration reference, naming its provider canonically is applied with no complaint. +- Breaking each rule in its fixture turns red exactly the test named for that rule, and no other test. +- The refusal reaches the author in the same turn as the edit that caused it, before any commit, push, or CI run. +- Every plugin source in the repository as it stands passes both rules. +- A contributor who has never read `docs/ARCHITECTURE.md` can correct a refused edit from the refusal text alone. + +## Stakeholders + +- Decider: the repository owner, who ruled out the commit-time and CI mechanisms on 2026-09-14. +- Owner: the framework maintainers. +- Consumer: every contributor and every agent that edits a plugin source in this repository. + +## Context + +- Backlog item: [ai-driven-dev/framework#250](https://github.com/ai-driven-dev/framework/issues/250). +- The issue body's "Guardrail local et CI" section predates the 2026-09-14 comment on the same issue and is superseded by it. The comment is the governing statement: no Git hook, no CI gate, synthetic fixtures, #406 neither blocker nor fixture. +- The orthogonality rule and its legitimate exceptions are stated in `docs/ARCHITECTURE.md`, under capability addressing: a capability is addressed only where the dispatch is declared, and elsewhere the concept is named instead of the skill that owns it. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 1b0f28551..7e274ee4d 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -155,6 +155,8 @@ 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 prospective edit's path and content. + This distinction keeps recipe plugins swappable while making orchestration handoffs explicit and auditable. ## 🔎 See also diff --git a/scripts/__tests__/architecture-hook.test.js b/scripts/__tests__/architecture-hook.test.js new file mode 100644 index 000000000..fbd620182 --- /dev/null +++ b/scripts/__tests__/architecture-hook.test.js @@ -0,0 +1,294 @@ +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 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("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, ""); +}); diff --git a/scripts/__tests__/architecture-rules.test.js b/scripts/__tests__/architecture-rules.test.js new file mode 100644 index 000000000..f79ca4670 --- /dev/null +++ b/scripts/__tests__/architecture-rules.test.js @@ -0,0 +1,145 @@ +const assert = require("node:assert/strict"); +const fs = require("node:fs"); +const path = require("node:path"); +const test = require("node:test"); + +const { checkArchitecture } = require("../lib/architecture-rules.js"); + +const ROOT = path.resolve(__dirname, "../.."); +const FIXTURES = path.join(__dirname, "fixtures/architecture-rules"); + +function fixture(relPath) { + return fs.readFileSync(path.join(FIXTURES, relPath), "utf8"); +} + +/** + * Thin tree walker for the sweep test only. The engine itself never reads the filesystem — + * this is the caller that supplies its inputs from the real `plugins/` tree. + */ +function sweepPlugins() { + const files = []; + const pluginsDir = path.join(ROOT, "plugins"); + + function walk(dir) { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + if (entry.name === "assets") continue; + walk(full); + } else if (entry.isFile() && entry.name.endsWith(".md")) { + files.push(full); + } + } + } + walk(pluginsDir); + + const violations = []; + for (const absPath of files) { + const relPath = path.relative(ROOT, absPath).split(path.sep).join("/"); + const content = fs.readFileSync(absPath, "utf8"); + let actionFileNames; + if (path.basename(absPath) === "SKILL.md") { + const actionsDir = path.join(path.dirname(absPath), "actions"); + actionFileNames = fs.existsSync(actionsDir) + ? fs.readdirSync(actionsDir).filter((f) => f.endsWith(".md")) + : []; + } + violations.push(...checkArchitecture(relPath, content, actionFileNames)); + } + return violations; +} + +test("a clean skill with no sibling address and a fully-named action yields no violations", () => { + const content = fixture("clean-skill/SKILL.md"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-clean/SKILL.md", + content, + ["01-step.md"] + ); + assert.deepEqual(violations, []); +}); + +test("a skill addressing a sibling plugin yields a violation naming file, line and owning plugin", () => { + const content = fixture("cross-address-skill/SKILL.md"); + const filePath = "plugins/aidd-fixture-a/skills/02-cross-address/SKILL.md"; + const violations = checkArchitecture(filePath, content, ["01-step.md"]); + + assert.equal(violations.length, 1); + const [violation] = violations; + assert.equal(violation.file, filePath); + assert.equal(violation.plugin, "aidd-fixture-b"); + const lines = content.split("\n"); + assert.equal(lines[violation.line - 1].includes("/aidd-fixture-b:01-noop"), true); +}); + +test("an agent's Skills you may invoke list addressing siblings yields no violations", () => { + const content = fixture("agent-permission-list.md"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/agents/reviewer.md", + content + ); + assert.deepEqual(violations, []); +}); + +test("an orchestrator plugin is exempt wholesale from both rules", () => { + const content = fixture("orchestrator-skill/SKILL.md"); + const violations = checkArchitecture( + "plugins/aidd-orchestrator/skills/99-fixture/SKILL.md", + content, + ["01-route.md"] // never named in the section; would violate rule 2 if not exempt + ); + assert.deepEqual(violations, []); +}); + +test("a file under assets/ yields no violations regardless of content", () => { + const content = fixture("assets-note.md"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-clean/assets/note.md", + content + ); + assert.deepEqual(violations, []); +}); + +test("an action file the Actions section never names yields one violation", () => { + const content = fixture("unnamed-action-skill/SKILL.md"); + const filePath = "plugins/aidd-fixture-a/skills/03-unnamed-action/SKILL.md"; + const violations = checkArchitecture(filePath, content, [ + "01-step-one.md", + "02-step-two.md", + ]); + + assert.equal(violations.length, 1); + const [violation] = violations; + assert.equal(violation.file, filePath); + assert.equal(violation.plugin, "aidd-fixture-a"); + const lines = content.split("\n"); + assert.equal(lines[violation.line - 1].trim(), "## Actions"); +}); + +test("a name the Actions section cites with no action file behind it yields one violation", () => { + const content = fixture("phantom-citation-skill/SKILL.md"); + const filePath = "plugins/aidd-fixture-a/skills/04-phantom-citation/SKILL.md"; + const violations = checkArchitecture(filePath, content, ["01-step.md"]); + + assert.equal(violations.length, 1); + const [violation] = violations; + assert.equal(violation.file, filePath); + assert.equal(violation.plugin, "aidd-fixture-a"); + const lines = content.split("\n"); + assert.equal(lines[violation.line - 1].includes("ghost-step"), true); +}); + +test("a skill whose Actions section and action files agree yields no violations", () => { + const content = fixture("clean-skill/SKILL.md"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-clean/SKILL.md", + content, + ["01-step.md"] + ); + assert.deepEqual(violations, []); +}); + +test("sweeping the repository's own plugins/ tree yields zero violations", () => { + const violations = sweepPlugins(); + assert.deepEqual(violations, []); +}); diff --git a/scripts/__tests__/fixtures/architecture-rules/agent-permission-list.md b/scripts/__tests__/fixtures/architecture-rules/agent-permission-list.md new file mode 100644 index 000000000..71057c071 --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/agent-permission-list.md @@ -0,0 +1,12 @@ +# Role + +You are the reviewer. + +# Behavior + +- Do your job. + +# Skills you may invoke + +- `/aidd-fixture-b:01-noop` +- `/aidd-fixture-c:02-other` diff --git a/scripts/__tests__/fixtures/architecture-rules/assets-note.md b/scripts/__tests__/fixtures/architecture-rules/assets-note.md new file mode 100644 index 000000000..891150a5d --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/assets-note.md @@ -0,0 +1,3 @@ +# Recipe + +Type `/aidd-fixture-b:01-noop` to trigger the sibling flow from your terminal. diff --git a/scripts/__tests__/fixtures/architecture-rules/clean-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/clean-skill/SKILL.md new file mode 100644 index 000000000..1712689ab --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/clean-skill/SKILL.md @@ -0,0 +1,15 @@ +# Clean + +A minimal skill with no sibling address and one action. + +## Actions + +Run `step`. + +| Action | Does | +| --- | --- | +| step | does the one thing | + +## Notes + +Nothing else here. diff --git a/scripts/__tests__/fixtures/architecture-rules/cross-address-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/cross-address-skill/SKILL.md new file mode 100644 index 000000000..ee2e8806a --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/cross-address-skill/SKILL.md @@ -0,0 +1,11 @@ +# Cross Address + +This skill wrongly calls a sibling plugin directly. + +## Actions + +Run `step`, then hand off to `/aidd-fixture-b:01-noop` for the rest. + +| Action | Does | +| --- | --- | +| step | does the one thing | diff --git a/scripts/__tests__/fixtures/architecture-rules/orchestrator-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/orchestrator-skill/SKILL.md new file mode 100644 index 000000000..6e9d99c02 --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/orchestrator-skill/SKILL.md @@ -0,0 +1,7 @@ +# Orchestrated Router + +Dispatches broadly across plugins. + +## Actions + +Route to `/aidd-fixture-a:02-cross-address` or `/aidd-fixture-b:01-noop` depending on intent. diff --git a/scripts/__tests__/fixtures/architecture-rules/phantom-citation-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/phantom-citation-skill/SKILL.md new file mode 100644 index 000000000..844d62f27 --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/phantom-citation-skill/SKILL.md @@ -0,0 +1,8 @@ +# One Step + +## Actions + +| # | Action | Does | +| --- | --- | --- | +| 01 | `step` | does the one thing | +| 02 | `ghost-step` | invents a step that does not exist | diff --git a/scripts/__tests__/fixtures/architecture-rules/unnamed-action-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/unnamed-action-skill/SKILL.md new file mode 100644 index 000000000..1347d874a --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/unnamed-action-skill/SKILL.md @@ -0,0 +1,9 @@ +# Two Steps + +## Actions + +Run `step-one` to begin. + +| Action | Does | +| --- | --- | +| step-one | begins | diff --git a/scripts/lib/architecture-rules.js b/scripts/lib/architecture-rules.js new file mode 100644 index 000000000..2efd4235f --- /dev/null +++ b/scripts/lib/architecture-rules.js @@ -0,0 +1,247 @@ +/** + * The two named architecture rules (issue #250), as pure functions of a repository-relative + * path, the prospective file content, and — for a SKILL.md — the names of the skill's action + * files. Never reads the filesystem: the caller supplies every input, so a hook and a test can + * hand it the same shape without either touching disk through it. + * + * Rule one, cross-plugin orthogonality: a plugin's dispatch surface must not name a sibling + * plugin by a hardcoded address. + * Rule two, router coherence: a skill's `## Actions` section must name exactly the actions that + * skill provides. + */ + +"use strict"; + +const ORCHESTRATOR_PLUGIN = "aidd-orchestrator"; + +const HEADING_RE = /^#{1,6}\s/; +const SKILLS_INVOKE_HEADING_RE = /^#{1,6}\s+Skills you may invoke\s*$/i; +const ACTIONS_HEADING_RE = /^##\s+Actions\s*$/i; +const SECOND_LEVEL_HEADING_RE = /^##\s+/; +const ADDRESS_RE = /[@/](aidd-[a-z0-9]+(?:-[a-z0-9]+)*):([A-Za-z0-9][\w.-]*)/g; +const TABLE_TOKEN_RE = /`([a-z][a-z0-9-]*)`/g; +const ACTION_PATH_RE = /actions\/([A-Za-z0-9._-]+)\.md/g; + +function toLines(content) { + return content.split("\n"); +} + +/** + * Classifies a repository-relative path against the governed surface. Returns null for + * anything else, including everything under an `assets/` directory at any depth. + */ +function classifyFile(filePath) { + const parts = filePath.split("/").filter(Boolean); + if (parts[0] !== "plugins" || parts.length < 3) return null; + if (parts.includes("assets")) return null; + + const owner = parts[1]; + + if (parts[2] === "agents") { + if (parts.length === 4 && parts[3].endsWith(".md")) { + return { owner, kind: "agent" }; + } + return null; + } + + if (parts[2] !== "skills") return null; + const rest = parts.slice(3); + if (rest.length < 2) return null; + + const last = rest[rest.length - 1]; + const parent = rest[rest.length - 2]; + + if (rest.length === 2 && last === "SKILL.md") { + return { owner, kind: "skill", skillDir: rest[0] }; + } + if (parent === "actions" && last.endsWith(".md")) { + return { owner, kind: "action", skillDir: rest.slice(0, -2).join("/") }; + } + if (parent === "references" && last.endsWith(".md")) { + return { owner, kind: "reference", skillDir: rest.slice(0, -2).join("/") }; + } + return null; +} + +/** Every `/plugin:name` or `@plugin:name` address, with its 1-indexed line. */ +function findAddresses(content) { + const matches = []; + toLines(content).forEach((line, idx) => { + ADDRESS_RE.lastIndex = 0; + let match; + while ((match = ADDRESS_RE.exec(line)) !== null) { + matches.push({ line: idx + 1, plugin: match[1], text: match[0] }); + } + }); + return matches; +} + +/** 1-indexed lines under an agent's `# Skills you may invoke` heading, any level, until the + * next heading of any level. */ +function exemptAgentLines(lines) { + const exempt = new Set(); + let inSection = false; + lines.forEach((line, idx) => { + if (HEADING_RE.test(line)) { + inSection = SKILLS_INVOKE_HEADING_RE.test(line); + return; + } + if (inSection) exempt.add(idx + 1); + }); + return exempt; +} + +/** + * Rule one: a dispatch surface never names a sibling plugin by a hardcoded address. + */ +function checkOrthogonality(filePath, content) { + const info = classifyFile(filePath); + if (!info || info.owner === ORCHESTRATOR_PLUGIN) return []; + + const lines = toLines(content); + const exempt = info.kind === "agent" ? exemptAgentLines(lines) : new Set(); + + const violations = []; + for (const address of findAddresses(content)) { + if (address.plugin === info.owner) continue; + if (exempt.has(address.line)) continue; + violations.push({ + file: filePath, + line: address.line, + plugin: address.plugin, + rule: "orthogonality", + message: `${filePath}:${address.line} addresses sibling plugin "${address.plugin}" via "${address.text}"`, + }); + } + return violations; +} + +/** Strips a leading `NN-` and a trailing `.md` — "01-frame.md" -> "frame". */ +function stemOf(fileName) { + return fileName.replace(/\.md$/i, "").replace(/^[0-9]+-/, ""); +} + +/** The `## Actions` section: from the line after its heading up to the next `##` heading (or + * end of file). Returns null when no such heading exists. */ +function findActionsSection(lines) { + const headingIdx = lines.findIndex((line) => ACTIONS_HEADING_RE.test(line)); + if (headingIdx === -1) return null; + + let endIdx = lines.length; + for (let i = headingIdx + 1; i < lines.length; i += 1) { + if (SECOND_LEVEL_HEADING_RE.test(lines[i])) { + endIdx = i; + break; + } + } + return { headingLine: headingIdx + 1, startIdx: headingIdx + 1, endIdx }; +} + +/** + * Rule two: a skill's `## Actions` section names exactly the actions that skill provides. + */ +function checkRouterCoherence(filePath, content, actionFileNames) { + const info = classifyFile(filePath); + if (!info || info.kind !== "skill" || info.owner === ORCHESTRATOR_PLUGIN) return []; + + const names = actionFileNames || []; + if (names.length === 0) return []; + + const lines = toLines(content); + const section = findActionsSection(lines); + + if (!section) { + return [ + { + file: filePath, + line: 1, + plugin: info.owner, + rule: "router-coherence", + message: `${filePath} has action files but no "## Actions" section`, + }, + ]; + } + + const violations = []; + const sectionLines = lines.slice(section.startIdx, section.endIdx); + const sectionText = sectionLines.join("\n"); + + const backed = new Set(); + for (const name of names) { + backed.add(name.toLowerCase()); + backed.add(name.replace(/\.md$/i, "").toLowerCase()); + backed.add(stemOf(name).toLowerCase()); + } + + for (const name of names) { + const stem = stemOf(name); + const fullNoExt = name.replace(/\.md$/i, ""); + if ( + sectionText.includes(name) || + sectionText.includes(fullNoExt) || + sectionText.includes(stem) + ) { + continue; + } + violations.push({ + file: filePath, + line: section.headingLine, + plugin: info.owner, + rule: "router-coherence", + message: `${filePath}:${section.headingLine} "## Actions" never names action file "${name}"`, + }); + } + + sectionLines.forEach((line, offset) => { + const lineNo = section.startIdx + offset + 1; + + if (/^\s*\|/.test(line)) { + TABLE_TOKEN_RE.lastIndex = 0; + let match; + while ((match = TABLE_TOKEN_RE.exec(line)) !== null) { + const token = match[1].toLowerCase(); + if (!backed.has(token)) { + violations.push({ + file: filePath, + line: lineNo, + plugin: info.owner, + rule: "router-coherence", + message: `${filePath}:${lineNo} "## Actions" cites "${match[1]}" with no action file behind it`, + }); + } + } + } + + ACTION_PATH_RE.lastIndex = 0; + let pathMatch; + while ((pathMatch = ACTION_PATH_RE.exec(line)) !== null) { + const cited = pathMatch[1].toLowerCase(); + if (!backed.has(cited)) { + violations.push({ + file: filePath, + line: lineNo, + plugin: info.owner, + rule: "router-coherence", + message: `${filePath}:${lineNo} "## Actions" cites "actions/${pathMatch[1]}.md" with no action file behind it`, + }); + } + } + }); + + return violations; +} + +/** Both rules, combined. `actionFileNames` is read only when `filePath` is a SKILL.md. */ +function checkArchitecture(filePath, content, actionFileNames) { + return [ + ...checkOrthogonality(filePath, content), + ...checkRouterCoherence(filePath, content, actionFileNames), + ]; +} + +module.exports = { + checkArchitecture, + checkOrthogonality, + checkRouterCoherence, + classifyFile, +}; From 6da8c47298870dbda7eb1af7f7ec425efb947505 Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Fri, 18 Sep 2026 12:48:18 +0200 Subject: [PATCH 02/11] fix(framework): the guard reads architecture, not notation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An independent review broke the first cut in seven places. The worst: the address pattern required a leading slash or at-sign, so the bare form `aidd-pm:04-spec` — the one a contributor types by hand — walked straight past it. Eight such addresses were already in the tree, six of them in reference directories the classifier never looked into. The rule now matches an address by its own shape at any depth, and the router rule matches a stem as a whole token: `assert` no longer counts as named because `assert-architecture` happens to contain it. A citation is read from the column that actually carries action names, so a table with a keyword column is no longer refused. Writing an action file now re-checks its own skill's router, which is the direction a contributor creates one. The hook spliced an Edit with String.replace, which expands `$&` and `$'` as replacement patterns where the tool writes them literally. Both branches splice literally now. Two exemptions were removed and one added. Router coherence no longer skips the orchestrator — addressing and coherence are different rules. Two branches that no test could kill got the fixtures they were missing. `00-onboard` is exempt because its menus hand a person the command to type; that exemption is temporary and #883 closes it. 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 | 51 ++++++- .claude/settings.json | 3 +- .../spec.md | 3 + docs/ARCHITECTURE.md | 2 +- .../skills/01-plan/actions/04-plan.md | 2 +- scripts/__tests__/architecture-hook.test.js | 97 +++++++++++++ scripts/__tests__/architecture-rules.test.js | 110 ++++++++++++++- .../bare-address-skill/SKILL.md | 10 ++ .../architecture-rules/nested-action.md | 1 + .../nested-assets-skill/SKILL.md | 5 + .../architecture-rules/nested-reference.md | 4 + .../architecture-rules/nested-skill/SKILL.md | 4 + .../architecture-rules/onboard-menu.md | 5 + .../orchestrator-incoherent-skill/SKILL.md | 8 ++ .../phantom-column-skill/SKILL.md | 7 + .../phantom-path-skill/SKILL.md | 9 ++ .../stem-substring-skill/SKILL.md | 7 + scripts/lib/architecture-rules.js | 133 +++++++++++++++--- 18 files changed, 433 insertions(+), 28 deletions(-) create mode 100644 scripts/__tests__/fixtures/architecture-rules/bare-address-skill/SKILL.md create mode 100644 scripts/__tests__/fixtures/architecture-rules/nested-action.md create mode 100644 scripts/__tests__/fixtures/architecture-rules/nested-assets-skill/SKILL.md create mode 100644 scripts/__tests__/fixtures/architecture-rules/nested-reference.md create mode 100644 scripts/__tests__/fixtures/architecture-rules/nested-skill/SKILL.md create mode 100644 scripts/__tests__/fixtures/architecture-rules/onboard-menu.md create mode 100644 scripts/__tests__/fixtures/architecture-rules/orchestrator-incoherent-skill/SKILL.md create mode 100644 scripts/__tests__/fixtures/architecture-rules/phantom-column-skill/SKILL.md create mode 100644 scripts/__tests__/fixtures/architecture-rules/phantom-path-skill/SKILL.md create mode 100644 scripts/__tests__/fixtures/architecture-rules/stem-substring-skill/SKILL.md diff --git a/.claude/hooks/check-architecture-rules.js b/.claude/hooks/check-architecture-rules.js index b84eb9834..30db21d41 100755 --- a/.claude/hooks/check-architecture-rules.js +++ b/.claude/hooks/check-architecture-rules.js @@ -45,10 +45,17 @@ function toRepoRelative(filePath) { } } +/** Splices `newString` in for `oldString` literally — never through `String.prototype.replace`, + * whose replacement string treats `$&`, `` $` ``, `$'` and `$1`-style tokens specially even + * when the search pattern is a plain string, corrupting a `new_string` that happens to contain + * one. */ function applyEdit(current, oldString, newString, replaceAll) { if (typeof oldString !== "string" || typeof newString !== "string") return null; if (!current.includes(oldString)) return null; - return replaceAll ? current.split(oldString).join(newString) : current.replace(oldString, newString); + if (replaceAll) return current.split(oldString).join(newString); + + const idx = current.indexOf(oldString); + return current.slice(0, idx) + newString + current.slice(idx + oldString.length); } /** The prospective file content, or null when it cannot be determined (Write with no string @@ -92,6 +99,42 @@ function actionFileNamesFor(relPath, absPath) { } } +/** Rule two also fires from the action-file side: writing `actions/NN-new.md` can leave the + * sibling `SKILL.md`'s "## Actions" section out of step just as writing the `SKILL.md` itself + * can. Reads the sibling `SKILL.md` as it stands on disk, and the action file listing as it + * will be after this write, then runs the same coherence check the `SKILL.md` write path + * runs. Returns [] — fails open — when the sibling `SKILL.md` cannot be read. */ +function siblingRouterCoherence(engine, info, actionAbsPath) { + if (!info.skillDir) return []; + const root = process.env.CLAUDE_PROJECT_DIR || process.cwd(); + const skillMdRelPath = `${info.skillDir}/SKILL.md`; + const skillMdAbsPath = path.join(root, info.skillDir, "SKILL.md"); + + let skillContent; + try { + skillContent = fs.readFileSync(skillMdAbsPath, "utf8"); + } catch { + return []; + } + + const actionsDirAbs = path.join(root, info.skillDir, "actions"); + let names; + try { + names = fs.readdirSync(actionsDirAbs).filter((name) => name.endsWith(".md")); + } catch { + names = []; + } + const newBasename = path.basename(actionAbsPath); + const nameSet = new Set(names); + if (newBasename.endsWith(".md")) nameSet.add(newBasename); + + try { + return engine.checkRouterCoherence(skillMdRelPath, skillContent, Array.from(nameSet)); + } catch { + return []; + } +} + function fixFor(rule, plugin) { return rule === "orthogonality" ? `name the concept ${plugin} owns instead of addressing it directly` @@ -132,7 +175,8 @@ function main() { const relPath = toRepoRelative(toolInput.file_path); if (!relPath) return 0; - if (!engine.classifyFile(relPath)) return 0; + const info = engine.classifyFile(relPath); + if (!info) return 0; const content = prospectiveContent(toolName, toolInput); if (content === null) return 0; @@ -142,6 +186,9 @@ function main() { let violations; try { violations = engine.checkArchitecture(relPath, content, actionFileNames); + if (info.kind === "action") { + violations = violations.concat(siblingRouterCoherence(engine, info, toolInput.file_path)); + } } catch { return 0; } diff --git a/.claude/settings.json b/.claude/settings.json index ceddf9728..61f2e989b 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -17,7 +17,8 @@ "hooks": [ { "type": "command", - "command": "node \"$CLAUDE_PROJECT_DIR/.claude/hooks/check-architecture-rules.js\"" + "command": "node \"$CLAUDE_PROJECT_DIR/.claude/hooks/check-architecture-rules.js\"", + "timeout": 60 } ] } diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md index 2cd5fe475..c889b1d2d 100644 --- a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md @@ -27,6 +27,8 @@ An AI-authored edit that would break one of this repository's two named architec - Repairing violations that exist in the tree today. That was #406, now closed. - Enforcing any architecture rule beyond the two named above. - Catching a violation introduced outside an AI tool, by a human editing by hand. +- Catching a write performed through a shell command rather than a write tool. Deciding whether a shell line writes a plugin source means parsing arbitrary shell, which is not the deterministic verdict rule one requires. An agent told to edit through `sed` or a heredoc is therefore ungoverned, and that is stated here rather than left to be discovered. +- Making `aidd-context:00-onboard` resolve its providers at runtime. Its reference menus name addresses because those addresses are what it hands a person to type, so it is exempt by a named rule with a follow-up issue behind it, not by silence. - Shipping the guard into projects that install this marketplace. The rules govern this repository's own plugin sources. ## Done-when @@ -48,5 +50,6 @@ An AI-authored edit that would break one of this repository's two named architec ## Context - Backlog item: [ai-driven-dev/framework#250](https://github.com/ai-driven-dev/framework/issues/250). +- The `00-onboard` exemption has an expiry, not a pass: [ai-driven-dev/framework#883](https://github.com/ai-driven-dev/framework/issues/883) makes that skill resolve its providers at runtime, and closing it closes the exemption. - The issue body's "Guardrail local et CI" section predates the 2026-09-14 comment on the same issue and is superseded by it. The comment is the governing statement: no Git hook, no CI gate, synthetic fixtures, #406 neither blocker nor fixture. - The orthogonality rule and its legitimate exceptions are stated in `docs/ARCHITECTURE.md`, under capability addressing: a capability is addressed only where the dispatch is declared, and elsewhere the concept is named instead of the skill that owns it. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 7e274ee4d..ddb87742c 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -155,7 +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. -`scripts/lib/architecture-rules.js` is the guard that decides both rules from a prospective edit's path and content. +`scripts/lib/architecture-rules.js` is the guard that decides both rules from a prospective edit's path and content. It fires on this repository's AI write path for Claude Code — a `PreToolUse` hook on `Write`/`Edit`/`MultiEdit` — so a contributor on another AI tool is not covered by it. `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. This distinction keeps recipe plugins swappable while making orchestration handoffs explicit and auditable. diff --git a/plugins/aidd-dev/skills/01-plan/actions/04-plan.md b/plugins/aidd-dev/skills/01-plan/actions/04-plan.md index 238b141c8..0803da0ff 100644 --- a/plugins/aidd-dev/skills/01-plan/actions/04-plan.md +++ b/plugins/aidd-dev/skills/01-plan/actions/04-plan.md @@ -13,7 +13,7 @@ A feature folder, always at `aidd_docs/tasks//_ { + 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( @@ -154,6 +195,62 @@ test("a path under assets/ exits zero even when its content addresses a sibling" assert.equal(result.stdout, ""); }); +test("a new action file left unnamed in the sibling SKILL.md's Actions section is denied", () => { + 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); + const reason = parseDenyReason(result.stdout); + assert.match(reason, /never names action file "02-extra\.md"/); +}); + +test("a new action file the sibling SKILL.md already names is applied with no complaint", () => { + const projectDir = makeProjectDir(); + const SKILL_NAMING_BOTH = [ + "# Clean skill", + "", + "## Actions", + "", + "| # | Action | Role |", + "| --- | --- | --- |", + "| 01 | `step` | Do the one thing |", + "| 02 | `extra` | Do the new thing |", + "", + ].join("\n"); + writeFixtureFile(projectDir, "plugins/aidd-fixture-a/skills/01-clean/SKILL.md", SKILL_NAMING_BOTH); + 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(); diff --git a/scripts/__tests__/architecture-rules.test.js b/scripts/__tests__/architecture-rules.test.js index f79ca4670..2b09a7bdf 100644 --- a/scripts/__tests__/architecture-rules.test.js +++ b/scripts/__tests__/architecture-rules.test.js @@ -3,7 +3,7 @@ const fs = require("node:fs"); const path = require("node:path"); const test = require("node:test"); -const { checkArchitecture } = require("../lib/architecture-rules.js"); +const { checkArchitecture, classifyFile } = require("../lib/architecture-rules.js"); const ROOT = path.resolve(__dirname, "../.."); const FIXTURES = path.join(__dirname, "fixtures/architecture-rules"); @@ -24,7 +24,6 @@ function sweepPlugins() { for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { const full = path.join(dir, entry.name); if (entry.isDirectory()) { - if (entry.name === "assets") continue; walk(full); } else if (entry.isFile() && entry.name.endsWith(".md")) { files.push(full); @@ -36,6 +35,9 @@ function sweepPlugins() { const violations = []; for (const absPath of files) { const relPath = path.relative(ROOT, absPath).split(path.sep).join("/"); + // The engine's own classifier decides what is governed — including `assets/` — so the + // walker never keeps a second copy of that rule that could drift from it. + if (!classifyFile(relPath)) continue; const content = fs.readFileSync(absPath, "utf8"); let actionFileNames; if (path.basename(absPath) === "SKILL.md") { @@ -81,16 +83,27 @@ test("an agent's Skills you may invoke list addressing siblings yields no violat assert.deepEqual(violations, []); }); -test("an orchestrator plugin is exempt wholesale from both rules", () => { +test("an orchestrator plugin is exempt from orthogonality alone", () => { const content = fixture("orchestrator-skill/SKILL.md"); + // No actionFileNames: isolates rule one, since rule two short-circuits on an empty list. const violations = checkArchitecture( "plugins/aidd-orchestrator/skills/99-fixture/SKILL.md", - content, - ["01-route.md"] // never named in the section; would violate rule 2 if not exempt + content ); assert.deepEqual(violations, []); }); +test("router coherence still applies to an orchestrator plugin", () => { + const content = fixture("orchestrator-incoherent-skill/SKILL.md"); + const filePath = "plugins/aidd-orchestrator/skills/99-fixture/SKILL.md"; + const violations = checkArchitecture(filePath, content, ["01-route.md"]); + + assert.equal(violations.length, 1); + const [violation] = violations; + assert.equal(violation.rule, "router-coherence"); + assert.equal(violation.plugin, "aidd-orchestrator"); +}); + test("a file under assets/ yields no violations regardless of content", () => { const content = fixture("assets-note.md"); const violations = checkArchitecture( @@ -100,6 +113,45 @@ test("a file under assets/ yields no violations regardless of content", () => { assert.deepEqual(violations, []); }); +test("an action file nested a level deeper than usual is still governed", () => { + const content = fixture("nested-action.md"); + const filePath = "plugins/aidd-fixture-a/skills/01-clean/actions/group/nested-action.md"; + const violations = checkArchitecture(filePath, content); + + assert.equal(violations.length, 1); + const [violation] = violations; + assert.equal(violation.plugin, "aidd-fixture-b"); +}); + +test("a SKILL.md nested a level deeper than usual is still governed", () => { + const content = fixture("nested-skill/SKILL.md"); + const filePath = "plugins/aidd-fixture-a/skills/01-clean/variant/SKILL.md"; + const violations = checkArchitecture(filePath, content); + + assert.equal(violations.length, 1); + const [violation] = violations; + assert.equal(violation.plugin, "aidd-fixture-b"); +}); + +test("a reference file nested two levels under references/ is still governed", () => { + const content = fixture("nested-reference.md"); + const filePath = "plugins/aidd-fixture-a/skills/01-clean/references/state/nested-reference.md"; + const violations = checkArchitecture(filePath, content); + + assert.equal(violations.length, 1); + const [violation] = violations; + assert.equal(violation.plugin, "aidd-fixture-b"); +}); + +test("a SKILL.md nested under assets/ is ungoverned for that reason alone", () => { + const content = fixture("nested-assets-skill/SKILL.md"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-clean/assets/nested/SKILL.md", + content + ); + assert.deepEqual(violations, []); +}); + test("an action file the Actions section never names yields one violation", () => { const content = fixture("unnamed-action-skill/SKILL.md"); const filePath = "plugins/aidd-fixture-a/skills/03-unnamed-action/SKILL.md"; @@ -129,6 +181,54 @@ test("a name the Actions section cites with no action file behind it yields one assert.equal(lines[violation.line - 1].includes("ghost-step"), true); }); +test("00-onboard's own reference menus are exempt from orthogonality", () => { + const content = fixture("onboard-menu.md"); + const filePath = "plugins/aidd-context/skills/00-onboard/references/order/onboard-menu.md"; + const violations = checkArchitecture(filePath, content); + assert.deepEqual(violations, []); +}); + +test("a bare plugin:skill address is caught, but not one embedded in a longer identifier", () => { + const content = fixture("bare-address-skill/SKILL.md"); + const filePath = "plugins/aidd-fixture-a/skills/07-bare-address/SKILL.md"; + const violations = checkArchitecture(filePath, content, ["01-step.md"]); + + assert.equal(violations.length, 1); + const [violation] = violations; + assert.equal(violation.plugin, "aidd-fixture-b"); + assert.match(violation.message, /"aidd-fixture-b:01-noop"/); +}); + +test("a backticked ordinary word outside the action column is not a phantom citation", () => { + const content = fixture("phantom-column-skill/SKILL.md"); + const filePath = "plugins/aidd-fixture-a/skills/06-phantom-column/SKILL.md"; + const violations = checkArchitecture(filePath, content, ["01-step.md"]); + assert.deepEqual(violations, []); +}); + +test("a stem that is a substring of a sibling action's stem is not mistaken for a mention", () => { + const content = fixture("stem-substring-skill/SKILL.md"); + const filePath = "plugins/aidd-fixture-a/skills/05-stem-substring/SKILL.md"; + const violations = checkArchitecture(filePath, content, [ + "01-assert.md", + "02-assert-architecture.md", + ]); + + assert.equal(violations.length, 1); + const [violation] = violations; + assert.match(violation.message, /"01-assert\.md"/); +}); + +test("a fenced actions/ path citing a file with no action behind it yields one violation", () => { + const content = fixture("phantom-path-skill/SKILL.md"); + const filePath = "plugins/aidd-fixture-a/skills/08-phantom-path/SKILL.md"; + const violations = checkArchitecture(filePath, content, ["01-step.md"]); + + assert.equal(violations.length, 1); + const [violation] = violations; + assert.match(violation.message, /actions\/99-absent\.md/); +}); + test("a skill whose Actions section and action files agree yields no violations", () => { const content = fixture("clean-skill/SKILL.md"); const violations = checkArchitecture( diff --git a/scripts/__tests__/fixtures/architecture-rules/bare-address-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/bare-address-skill/SKILL.md new file mode 100644 index 000000000..99822bb50 --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/bare-address-skill/SKILL.md @@ -0,0 +1,10 @@ +# Bare Address + +Declare it the same way `aidd-fixture-b:01-noop` does. Do not confuse this with +some-aidd-fixture-b:02-other, which names no real plugin and must stay silent. + +## Actions + +| Action | Does | +| --- | --- | +| `step` | does the one thing | diff --git a/scripts/__tests__/fixtures/architecture-rules/nested-action.md b/scripts/__tests__/fixtures/architecture-rules/nested-action.md new file mode 100644 index 000000000..701a6a7a6 --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/nested-action.md @@ -0,0 +1 @@ +Nested one level under `actions/`, this still names a sibling directly: `aidd-fixture-b:01-noop`. diff --git a/scripts/__tests__/fixtures/architecture-rules/nested-assets-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/nested-assets-skill/SKILL.md new file mode 100644 index 000000000..47b6d98b3 --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/nested-assets-skill/SKILL.md @@ -0,0 +1,5 @@ +# Nested Under Assets + +This file sits under an `assets/` segment even though it is named `SKILL.md` and would +otherwise be governed at this depth. Naming a sibling here, `aidd-fixture-b:01-noop`, must +stay silent. diff --git a/scripts/__tests__/fixtures/architecture-rules/nested-reference.md b/scripts/__tests__/fixtures/architecture-rules/nested-reference.md new file mode 100644 index 000000000..1875629ed --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/nested-reference.md @@ -0,0 +1,4 @@ +# Nested reference + +Nested two levels under `references/`, this still names a sibling directly: +`aidd-fixture-b:01-noop`. diff --git a/scripts/__tests__/fixtures/architecture-rules/nested-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/nested-skill/SKILL.md new file mode 100644 index 000000000..4b93cd5be --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/nested-skill/SKILL.md @@ -0,0 +1,4 @@ +# Nested Skill + +Nested one level deeper than usual, this still names a sibling directly: +`aidd-fixture-b:01-noop`. diff --git a/scripts/__tests__/fixtures/architecture-rules/onboard-menu.md b/scripts/__tests__/fixtures/architecture-rules/onboard-menu.md new file mode 100644 index 000000000..2c8cbd610 --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/onboard-menu.md @@ -0,0 +1,5 @@ +# Onboard routing menu + +| Situation | Route to | +| --- | --- | +| start new work | `aidd-orchestrator:01-sdlc` | diff --git a/scripts/__tests__/fixtures/architecture-rules/orchestrator-incoherent-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/orchestrator-incoherent-skill/SKILL.md new file mode 100644 index 000000000..32addd9da --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/orchestrator-incoherent-skill/SKILL.md @@ -0,0 +1,8 @@ +# Orchestrated Router + +Dispatches broadly across plugins. + +## Actions + +Hand off to `/aidd-fixture-a:02-cross-address` depending on intent, and say nothing else about +what this skill itself provides. diff --git a/scripts/__tests__/fixtures/architecture-rules/phantom-column-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/phantom-column-skill/SKILL.md new file mode 100644 index 000000000..46e023eca --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/phantom-column-skill/SKILL.md @@ -0,0 +1,7 @@ +# One Step With A Trigger Column + +## Actions + +| # | Action | Trigger | Role | +| --- | --- | --- | --- | +| 01 | `step` | `yes` | Do the one thing | diff --git a/scripts/__tests__/fixtures/architecture-rules/phantom-path-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/phantom-path-skill/SKILL.md new file mode 100644 index 000000000..eb629289e --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/phantom-path-skill/SKILL.md @@ -0,0 +1,9 @@ +# Fenced Path Citation + +## Actions + +Run `step` first. + +```md +actions/99-absent.md +``` diff --git a/scripts/__tests__/fixtures/architecture-rules/stem-substring-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/stem-substring-skill/SKILL.md new file mode 100644 index 000000000..b2b7bbf35 --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/stem-substring-skill/SKILL.md @@ -0,0 +1,7 @@ +# Two similarly named actions + +## Actions + +| # | Action | Facet | +| --- | --- | --- | +| 02 | `assert-architecture` | Report where the code breaks the documented architecture | diff --git a/scripts/lib/architecture-rules.js b/scripts/lib/architecture-rules.js index 2efd4235f..b554fbae7 100644 --- a/scripts/lib/architecture-rules.js +++ b/scripts/lib/architecture-rules.js @@ -14,11 +14,20 @@ const ORCHESTRATOR_PLUGIN = "aidd-orchestrator"; +// Temporary: `00-onboard`'s reference menus are routing menus whose addresses are what the +// skill hands a person to type, not a hardcoded sibling provider — so orthogonality stays +// silent on this one skill directory. See the follow-up issue on 00-onboard runtime discovery. +const ONBOARD_EXEMPT_PREFIX = "plugins/aidd-context/skills/00-onboard/"; + const HEADING_RE = /^#{1,6}\s/; const SKILLS_INVOKE_HEADING_RE = /^#{1,6}\s+Skills you may invoke\s*$/i; const ACTIONS_HEADING_RE = /^##\s+Actions\s*$/i; const SECOND_LEVEL_HEADING_RE = /^##\s+/; -const ADDRESS_RE = /[@/](aidd-[a-z0-9]+(?:-[a-z0-9]+)*):([A-Za-z0-9][\w.-]*)/g; +// Matches `/plugin:name`, `@plugin:name`, and the bare `plugin:name` form. The lookbehind keeps +// the left edge from firing inside a longer identifier — `some-aidd-dev:01-x` never matches, +// because the character right before "aidd-" (a hyphen, a letter, a digit, or another `/`/`@`) +// rules it out — while a backtick, space, or start of line still lets a bare address through. +const ADDRESS_RE = /(?/skills/<...>`) of the skill folder itself: the + * directory holding `SKILL.md`, or the directory the `actions/`/`references/` segment sits in. */ function classifyFile(filePath) { const parts = filePath.split("/").filter(Boolean); @@ -46,20 +60,27 @@ function classifyFile(filePath) { if (parts[2] !== "skills") return null; const rest = parts.slice(3); - if (rest.length < 2) return null; + if (rest.length < 1) return null; const last = rest[rest.length - 1]; - const parent = rest[rest.length - 2]; + if (!last.endsWith(".md")) return null; + + const skillDirFor = (segments) => ["plugins", owner, "skills", ...segments].join("/"); - if (rest.length === 2 && last === "SKILL.md") { - return { owner, kind: "skill", skillDir: rest[0] }; + if (last === "SKILL.md") { + return { owner, kind: "skill", skillDir: skillDirFor(rest.slice(0, -1)) }; } - if (parent === "actions" && last.endsWith(".md")) { - return { owner, kind: "action", skillDir: rest.slice(0, -2).join("/") }; + + const actionsIdx = rest.indexOf("actions"); + if (actionsIdx !== -1) { + return { owner, kind: "action", skillDir: skillDirFor(rest.slice(0, actionsIdx)) }; } - if (parent === "references" && last.endsWith(".md")) { - return { owner, kind: "reference", skillDir: rest.slice(0, -2).join("/") }; + + const referencesIdx = rest.indexOf("references"); + if (referencesIdx !== -1) { + return { owner, kind: "reference", skillDir: skillDirFor(rest.slice(0, referencesIdx)) }; } + return null; } @@ -97,6 +118,7 @@ function exemptAgentLines(lines) { function checkOrthogonality(filePath, content) { const info = classifyFile(filePath); if (!info || info.owner === ORCHESTRATOR_PLUGIN) return []; + if (filePath.startsWith(ONBOARD_EXEMPT_PREFIX)) return []; const lines = toLines(content); const exempt = info.kind === "agent" ? exemptAgentLines(lines) : new Set(); @@ -121,6 +143,19 @@ function stemOf(fileName) { return fileName.replace(/\.md$/i, "").replace(/^[0-9]+-/, ""); } +function escapeRegExp(text) { + return text.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +/** Whether `token` occurs in `text` as a whole identifier, not merely as a substring of a + * longer hyphenated one — "assert" is present in "the `assert` action" but not in + * "assert-architecture", because hyphen is a token character here, not a boundary. */ +function tokenPresent(text, token) { + if (!token) return false; + const re = new RegExp(`(? cell.trim()); +} + +function isTableSeparatorRow(cells) { + return cells.length > 0 && cells.every((cell) => /^:?-+:?$/.test(cell)); +} + +/** Groups of consecutive table-row offsets (into `sectionLines`) — a `## Actions` section may + * hold more than one pipe table (an action table, and unrelated prose table such as a trigger + * glossary), and each is scoped to its own action column independently. */ +function tableBlocks(sectionLines) { + const blocks = []; + let current = null; + sectionLines.forEach((line, offset) => { + if (/^\s*\|/.test(line)) { + if (!current) { + current = []; + blocks.push(current); + } + current.push(offset); + } else { + current = null; + } + }); + return blocks; +} + +/** The column index that carries action names in this table block, found by locating a cell + * backed by a real action file — the only column a backticked token can be a phantom citation + * in. -1 when no row backs any column, so an unrelated table (a keyword or trigger glossary, + * never an action listing) is left unchecked rather than guessed at. */ +function actionColumnOf(offsets, sectionLines, backed) { + for (const offset of offsets) { + const cells = splitTableCells(sectionLines[offset]); + if (isTableSeparatorRow(cells)) continue; + for (let col = 0; col < cells.length; col += 1) { + TABLE_TOKEN_RE.lastIndex = 0; + let match; + while ((match = TABLE_TOKEN_RE.exec(cells[col])) !== null) { + if (backed.has(match[1].toLowerCase())) return col; + } + } + } + return -1; +} + /** * Rule two: a skill's `## Actions` section names exactly the actions that skill provides. */ function checkRouterCoherence(filePath, content, actionFileNames) { const info = classifyFile(filePath); - if (!info || info.kind !== "skill" || info.owner === ORCHESTRATOR_PLUGIN) return []; + if (!info || info.kind !== "skill") return []; const names = actionFileNames || []; if (names.length === 0) return []; @@ -177,9 +263,9 @@ function checkRouterCoherence(filePath, content, actionFileNames) { const stem = stemOf(name); const fullNoExt = name.replace(/\.md$/i, ""); if ( - sectionText.includes(name) || - sectionText.includes(fullNoExt) || - sectionText.includes(stem) + tokenPresent(sectionText, name) || + tokenPresent(sectionText, fullNoExt) || + tokenPresent(sectionText, stem) ) { continue; } @@ -192,13 +278,20 @@ function checkRouterCoherence(filePath, content, actionFileNames) { }); } - sectionLines.forEach((line, offset) => { - const lineNo = section.startIdx + offset + 1; + for (const offsets of tableBlocks(sectionLines)) { + const actionColumn = actionColumnOf(offsets, sectionLines, backed); + if (actionColumn === -1) continue; // no row backs any column: not an action table, leave it alone - if (/^\s*\|/.test(line)) { + for (const offset of offsets) { + const cells = splitTableCells(sectionLines[offset]); + if (isTableSeparatorRow(cells)) continue; + const cell = cells[actionColumn]; + if (cell === undefined) continue; + + const lineNo = section.startIdx + offset + 1; TABLE_TOKEN_RE.lastIndex = 0; let match; - while ((match = TABLE_TOKEN_RE.exec(line)) !== null) { + while ((match = TABLE_TOKEN_RE.exec(cell)) !== null) { const token = match[1].toLowerCase(); if (!backed.has(token)) { violations.push({ @@ -211,6 +304,10 @@ function checkRouterCoherence(filePath, content, actionFileNames) { } } } + } + + sectionLines.forEach((line, offset) => { + const lineNo = section.startIdx + offset + 1; ACTION_PATH_RE.lastIndex = 0; let pathMatch; From 29489f7dc6f9623d3f7638fc536407bcdbb22cf0 Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Fri, 18 Sep 2026 13:28:55 +0200 Subject: [PATCH 03/11] fix(framework): rule two enforces the direction that can be decided MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second review found the guard refused adding an action to a skill in either order. Creating the file first was refused for not being named; naming it first was refused for having no file behind it. The second order is the one `aidd-context:04-skill-generate` documents, so the framework's own generator was refused by the framework's own guard, with no way out in the refusal text. A citation with no file behind it cannot be told apart from a citation written seconds before the file it names, so that direction is gone, and with it the column heuristics and table parsing that existed only to serve it. What remains is decidable at any moment: an action file the section never cites. The section names an action by citing it — a table cell, a fenced `actions/.md` path, a backticked file name — never by a word in running prose. Containment over prose let `plan` pass because the same section reads "the plan is the culmination"; measured over the tree it missed 20 of 78 row deletions. Citation matching misses none that has a row. Rule two fires on a `SKILL.md` write alone. An action file created and never cited is caught at the next write to its router, not at its own creation — recorded in the spec's non-goals rather than left to be discovered. 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 | 53 +----- .../plan.md | 4 +- .../spec.md | 7 +- docs/ARCHITECTURE.md | 2 +- scripts/__tests__/architecture-hook.test.js | 160 ++++++++++++---- scripts/__tests__/architecture-rules.test.js | 42 ++--- .../SKILL.md | 9 + .../phantom-citation-skill/SKILL.md | 8 - .../phantom-column-skill/SKILL.md | 7 - .../phantom-path-skill/SKILL.md | 9 - scripts/lib/architecture-rules.js | 171 ++++++------------ 11 files changed, 222 insertions(+), 250 deletions(-) create mode 100644 scripts/__tests__/fixtures/architecture-rules/deleted-row-masked-by-prose-skill/SKILL.md delete mode 100644 scripts/__tests__/fixtures/architecture-rules/phantom-citation-skill/SKILL.md delete mode 100644 scripts/__tests__/fixtures/architecture-rules/phantom-column-skill/SKILL.md delete mode 100644 scripts/__tests__/fixtures/architecture-rules/phantom-path-skill/SKILL.md diff --git a/.claude/hooks/check-architecture-rules.js b/.claude/hooks/check-architecture-rules.js index 30db21d41..d5ad95acb 100755 --- a/.claude/hooks/check-architecture-rules.js +++ b/.claude/hooks/check-architecture-rules.js @@ -34,8 +34,7 @@ function readPayload() { } /** Repository-relative, forward-slashed path, or null when it resolves outside the project. */ -function toRepoRelative(filePath) { - const root = process.env.CLAUDE_PROJECT_DIR || process.cwd(); +function toRepoRelative(filePath, root) { try { const rel = path.relative(root, path.resolve(root, filePath)); if (rel === "" || rel.startsWith("..") || path.isAbsolute(rel)) return null; @@ -99,42 +98,6 @@ function actionFileNamesFor(relPath, absPath) { } } -/** Rule two also fires from the action-file side: writing `actions/NN-new.md` can leave the - * sibling `SKILL.md`'s "## Actions" section out of step just as writing the `SKILL.md` itself - * can. Reads the sibling `SKILL.md` as it stands on disk, and the action file listing as it - * will be after this write, then runs the same coherence check the `SKILL.md` write path - * runs. Returns [] — fails open — when the sibling `SKILL.md` cannot be read. */ -function siblingRouterCoherence(engine, info, actionAbsPath) { - if (!info.skillDir) return []; - const root = process.env.CLAUDE_PROJECT_DIR || process.cwd(); - const skillMdRelPath = `${info.skillDir}/SKILL.md`; - const skillMdAbsPath = path.join(root, info.skillDir, "SKILL.md"); - - let skillContent; - try { - skillContent = fs.readFileSync(skillMdAbsPath, "utf8"); - } catch { - return []; - } - - const actionsDirAbs = path.join(root, info.skillDir, "actions"); - let names; - try { - names = fs.readdirSync(actionsDirAbs).filter((name) => name.endsWith(".md")); - } catch { - names = []; - } - const newBasename = path.basename(actionAbsPath); - const nameSet = new Set(names); - if (newBasename.endsWith(".md")) nameSet.add(newBasename); - - try { - return engine.checkRouterCoherence(skillMdRelPath, skillContent, Array.from(nameSet)); - } catch { - return []; - } -} - function fixFor(rule, plugin) { return rule === "orthogonality" ? `name the concept ${plugin} owns instead of addressing it directly` @@ -172,23 +135,25 @@ function main() { const toolInput = payload.tool_input; if (!toolInput || typeof toolInput.file_path !== "string" || toolInput.file_path === "") return 0; - const relPath = toRepoRelative(toolInput.file_path); + // Resolved once, against the project root, and reused for every filesystem access below — a + // relative `file_path` must never be read against the process's own cwd, which can differ + // from CLAUDE_PROJECT_DIR and would otherwise silently empty a readdir this hook depends on. + const root = process.env.CLAUDE_PROJECT_DIR || process.cwd(); + const relPath = toRepoRelative(toolInput.file_path, root); if (!relPath) return 0; const info = engine.classifyFile(relPath); if (!info) return 0; - const content = prospectiveContent(toolName, toolInput); + const absPath = path.resolve(root, toolInput.file_path); + const content = prospectiveContent(toolName, { ...toolInput, file_path: absPath }); if (content === null) return 0; - const actionFileNames = actionFileNamesFor(relPath, toolInput.file_path); + const actionFileNames = actionFileNamesFor(relPath, absPath); let violations; try { violations = engine.checkArchitecture(relPath, content, actionFileNames); - if (info.kind === "action") { - violations = violations.concat(siblingRouterCoherence(engine, info, toolInput.file_path)); - } } catch { return 0; } diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md index c481e8d4c..d2644b26d 100644 --- a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md @@ -32,8 +32,10 @@ | `PreToolUse`, not `PostToolUse` | The decision is "prevent the write until it is corrected". `PostToolUse` fires after the file is already on disk; Biome gets away with it only because it rewrites in place, and this guard cannot rewrite prose. | | The engine is a plain module, the hook is a thin caller | A rule that is a pure function of (path, prospective content, action-file listing) is testable without a hook, a host, or a tree. The hook contributes only the payload and the refusal. | | The governed surface is `SKILL.md`, `actions/`, `references/`, `agents/` | That is where dispatch is declared. `assets/` hold sheets a reader reads — `12-cook`'s recipes name 20 cross-plugin commands on purpose — so they are excluded by a stated rule, never by a quiet path filter. | -| An orchestrator plugin is exempt wholesale | `docs/ARCHITECTURE.md` makes orchestration references responsibility maps. `aidd-orchestrator` holds 14 of the tree's cross-plugin addresses for exactly that reason. | +| An orchestrator plugin is exempt from rule one, and only rule one | `docs/ARCHITECTURE.md` makes orchestration references responsibility maps. `aidd-orchestrator` holds 14 of the tree's cross-plugin addresses for exactly that reason. Router coherence is a different rule and applies to it like any other plugin. | | The router rule reads the `## Actions` section, not the table | `10-todo` names its one action as a path in a fenced block rather than a table row. Scoping to the section and matching either the stem or the file name covers both shapes and still reports zero on the tree. | | Unit tests live under `scripts/__tests__/`, and that is not a CI gate on the rules | Pre-commit runs those tests against synthetic fixtures, proving the engine works. Nothing scans the tree at commit time or in CI, which is what the decider ruled out. | | Fixtures are written for this task | The spec forbids #406's historical code. Each rule gets a breaking fixture and a legitimate-naming fixture, so a guard that flags a permission list fails its own suite. | | The hook is wired for Claude Code alone | It is the only host this repository configures hooks for: `.codex/config.toml` carries a sandbox mode and nothing else. Codex, Cursor and Copilot all expose `PreToolUse` and the same deny shape, but Codex delivers a file edit as an `apply_patch` command string rather than a path and a content, which is a different parse. The engine is host-agnostic, so each adapter is additive and none of them touches a rule. | +| Rule two enforces one direction, not two | A citation with no file behind it cannot be told apart from a citation written just before the file it names. Enforcing it deadlocked adding an action: creating the file first was refused for not being named, naming it first was refused for having no file, and `aidd-context:04-skill-generate` documents the second order. The decidable direction is kept. | +| An action is named by a citation, never by a word | Plain containment over the section let ordinary prose pass for a mention: `plan` appears in "the plan is the culmination", so deleting that action's row went unnoticed. Measured over the tree, containment missed 20 of 78 row deletions; citation matching misses none that has a row. | diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md index c889b1d2d..8b3ae0f62 100644 --- a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md @@ -10,7 +10,7 @@ An AI-authored edit that would break one of this repository's two named architec - The guard is deterministic: the same prospective file content yields the same verdict, with no dependence on model judgement and none on which AI tool made the edit. - The guard is produced by the project's own hook-generation capability, so it exists for the AI hosts this repository configures rather than for one. - Rule one, cross-plugin orthogonality: a plugin's dispatch surface must not name a sibling plugin by a hardcoded address. -- Rule two, router coherence: a skill's `## Actions` section must name exactly the actions that skill provides — no action file the section never names, no name the section cites without a file behind it. +- Rule two, router coherence: a skill's `## Actions` section must cite every action file that skill provides. A citation is a table cell, a fenced `actions/.md` path, or a backticked file name — never a bare word in running prose. - The governed surface is the dispatch surface: a skill's `SKILL.md`, its actions and its references, and an agent's definition. A plugin's `assets/` hold content shown to a reader, not dispatch, and are out of the guard's reach by this rule rather than by an unstated path filter. - Naming that `docs/ARCHITECTURE.md` declares legitimate stays silent: an agent's permission list and an orchestration reference are responsibility maps and name their provider canonically. Flagging either is a defect of the guard, not of the tree. - Every refusal names the file, the line, and the plugin that owns the addressed capability, in terms a reader can act on without opening the rule document. @@ -23,6 +23,9 @@ An AI-authored edit that would break one of this repository's two named architec - A `lefthook` pre-commit gate for these rules. The decider ruled it out on 2026-09-14. - A CI job enforcing these rules. - Judging whether a skill's prose `description` has gone stale. Nothing mechanically separates stale prose from current prose, so the issue's "router/description mismatch" is served by the router half alone. +- Refusing a citation with no action file behind it. A citation written seconds before the file it names is indistinguishable from a stale one, and this project's own skill generator writes the router first, so enforcing that direction makes adding an action impossible in either order. The decidable direction — an action file the section never cites — is the one the issue names, and the one enforced. +- Catching an action file created and never cited. Rule two decides on a write to a `SKILL.md`; an orphan action file is caught at the next write to its skill's router, not at its own creation. Firing on the action file instead is what made both orders of adding an action refuse each other. +- Reaching into a plugin's `README.md`, which addresses a sibling in two places today. Like `assets/`, it is a document a reader reads, not a dispatch the skill executes. - Reaching into a plugin's `assets/`. A recipe sheet names the commands a reader types; that is its subject, not a dispatch this rule governs. - Repairing violations that exist in the tree today. That was #406, now closed. - Enforcing any architecture rule beyond the two named above. @@ -34,7 +37,7 @@ An AI-authored edit that would break one of this repository's two named architec ## Done-when - An edit that would leave a skill's dispatch surface holding a sibling plugin's hardcoded address is refused, and the refusal names that file, its line, and the plugin that owns the address. -- An edit that would leave a skill's `## Actions` section naming an action the skill does not provide, or omitting one it does, is refused, and the refusal names the file and the line. +- An edit that would leave a skill's `## Actions` section not naming an action file the skill provides is refused, and the refusal names the file and the line. The section names an action by citing it, so a word in prose that happens to match a stem never passes for a mention. - An edit that writes an agent permission list, or an orchestration reference, naming its provider canonically is applied with no complaint. - Breaking each rule in its fixture turns red exactly the test named for that rule, and no other test. - The refusal reaches the author in the same turn as the edit that caused it, before any commit, push, or CI run. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index ddb87742c..c0417b143 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -155,7 +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. -`scripts/lib/architecture-rules.js` is the guard that decides both rules from a prospective edit's path and content. It fires on this repository's AI write path for Claude Code — a `PreToolUse` hook on `Write`/`Edit`/`MultiEdit` — so a contributor on another AI tool is not covered by it. `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. +`scripts/lib/architecture-rules.js` is the guard that decides both rules from a prospective edit's path and content. It fires on this repository's AI write path for Claude Code — a `PreToolUse` hook on `Write`/`Edit`/`MultiEdit` — so a contributor on another AI tool is not covered by it. `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 index 4ed66387e..67b31b091 100644 --- a/scripts/__tests__/architecture-hook.test.js +++ b/scripts/__tests__/architecture-hook.test.js @@ -195,7 +195,11 @@ test("a path under assets/ exits zero even when its content addresses a sibling" assert.equal(result.stdout, ""); }); -test("a new action file left unnamed in the sibling SKILL.md's Actions section is denied", () => { +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( @@ -213,40 +217,6 @@ test("a new action file left unnamed in the sibling SKILL.md's Actions section i tool_input: { file_path: filePath, content: "# Extra\n\nDoes something new.\n" }, }); - assert.equal(result.status, 0); - const reason = parseDenyReason(result.stdout); - assert.match(reason, /never names action file "02-extra\.md"/); -}); - -test("a new action file the sibling SKILL.md already names is applied with no complaint", () => { - const projectDir = makeProjectDir(); - const SKILL_NAMING_BOTH = [ - "# Clean skill", - "", - "## Actions", - "", - "| # | Action | Role |", - "| --- | --- | --- |", - "| 01 | `step` | Do the one thing |", - "| 02 | `extra` | Do the new thing |", - "", - ].join("\n"); - writeFixtureFile(projectDir, "plugins/aidd-fixture-a/skills/01-clean/SKILL.md", SKILL_NAMING_BOTH); - 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, ""); }); @@ -389,3 +359,123 @@ test("a MultiEdit whose first edit cannot be applied exits zero", () => { 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, ""); +}); diff --git a/scripts/__tests__/architecture-rules.test.js b/scripts/__tests__/architecture-rules.test.js index 2b09a7bdf..812236db6 100644 --- a/scripts/__tests__/architecture-rules.test.js +++ b/scripts/__tests__/architecture-rules.test.js @@ -33,6 +33,7 @@ function sweepPlugins() { walk(pluginsDir); const violations = []; + let skillsWithActions = 0; for (const absPath of files) { const relPath = path.relative(ROOT, absPath).split(path.sep).join("/"); // The engine's own classifier decides what is governed — including `assets/` — so the @@ -45,10 +46,11 @@ function sweepPlugins() { actionFileNames = fs.existsSync(actionsDir) ? fs.readdirSync(actionsDir).filter((f) => f.endsWith(".md")) : []; + if (actionFileNames.length > 0) skillsWithActions += 1; } violations.push(...checkArchitecture(relPath, content, actionFileNames)); } - return violations; + return { violations, skillsWithActions }; } test("a clean skill with no sibling address and a fully-named action yields no violations", () => { @@ -168,19 +170,6 @@ test("an action file the Actions section never names yields one violation", () = assert.equal(lines[violation.line - 1].trim(), "## Actions"); }); -test("a name the Actions section cites with no action file behind it yields one violation", () => { - const content = fixture("phantom-citation-skill/SKILL.md"); - const filePath = "plugins/aidd-fixture-a/skills/04-phantom-citation/SKILL.md"; - const violations = checkArchitecture(filePath, content, ["01-step.md"]); - - assert.equal(violations.length, 1); - const [violation] = violations; - assert.equal(violation.file, filePath); - assert.equal(violation.plugin, "aidd-fixture-a"); - const lines = content.split("\n"); - assert.equal(lines[violation.line - 1].includes("ghost-step"), true); -}); - test("00-onboard's own reference menus are exempt from orthogonality", () => { const content = fixture("onboard-menu.md"); const filePath = "plugins/aidd-context/skills/00-onboard/references/order/onboard-menu.md"; @@ -199,13 +188,6 @@ test("a bare plugin:skill address is caught, but not one embedded in a longer id assert.match(violation.message, /"aidd-fixture-b:01-noop"/); }); -test("a backticked ordinary word outside the action column is not a phantom citation", () => { - const content = fixture("phantom-column-skill/SKILL.md"); - const filePath = "plugins/aidd-fixture-a/skills/06-phantom-column/SKILL.md"; - const violations = checkArchitecture(filePath, content, ["01-step.md"]); - assert.deepEqual(violations, []); -}); - test("a stem that is a substring of a sibling action's stem is not mistaken for a mention", () => { const content = fixture("stem-substring-skill/SKILL.md"); const filePath = "plugins/aidd-fixture-a/skills/05-stem-substring/SKILL.md"; @@ -219,14 +201,17 @@ test("a stem that is a substring of a sibling action's stem is not mistaken for assert.match(violation.message, /"01-assert\.md"/); }); -test("a fenced actions/ path citing a file with no action behind it yields one violation", () => { - const content = fixture("phantom-path-skill/SKILL.md"); - const filePath = "plugins/aidd-fixture-a/skills/08-phantom-path/SKILL.md"; - const violations = checkArchitecture(filePath, content, ["01-step.md"]); +test("a deleted table row is still caught when unrelated prose loosely contains its word", () => { + // Regression for the substring-over-prose bug: "Run them in order, `01 → 04`. The plan is + // the culmination." contains the word "plan" in ordinary prose, not as a table cell, an + // actions/ path, or a backticked .md filename — so it must not count as naming 04-plan.md. + const content = fixture("deleted-row-masked-by-prose-skill/SKILL.md"); + const filePath = "plugins/aidd-fixture-a/skills/09-deleted-row/SKILL.md"; + const violations = checkArchitecture(filePath, content, ["01-frame.md", "04-plan.md"]); assert.equal(violations.length, 1); const [violation] = violations; - assert.match(violation.message, /actions\/99-absent\.md/); + assert.match(violation.message, /"04-plan\.md"/); }); test("a skill whose Actions section and action files agree yields no violations", () => { @@ -240,6 +225,9 @@ test("a skill whose Actions section and action files agree yields no violations" }); test("sweeping the repository's own plugins/ tree yields zero violations", () => { - const violations = sweepPlugins(); + const { violations, skillsWithActions } = sweepPlugins(); assert.deepEqual(violations, []); + // A sweep that never actually exercised a skill with action files would pass the same way — + // this pins the sweep to the measured count so it cannot go vacuously green. + assert.equal(skillsWithActions, 48); }); diff --git a/scripts/__tests__/fixtures/architecture-rules/deleted-row-masked-by-prose-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/deleted-row-masked-by-prose-skill/SKILL.md new file mode 100644 index 000000000..4e8fc0967 --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/deleted-row-masked-by-prose-skill/SKILL.md @@ -0,0 +1,9 @@ +# Multi Step + +## Actions + +Run them in order, `01 → 04`. The plan is the culmination. + +| # | Action | Role | +| --- | --- | --- | +| 01 | `frame` | Frame it | diff --git a/scripts/__tests__/fixtures/architecture-rules/phantom-citation-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/phantom-citation-skill/SKILL.md deleted file mode 100644 index 844d62f27..000000000 --- a/scripts/__tests__/fixtures/architecture-rules/phantom-citation-skill/SKILL.md +++ /dev/null @@ -1,8 +0,0 @@ -# One Step - -## Actions - -| # | Action | Does | -| --- | --- | --- | -| 01 | `step` | does the one thing | -| 02 | `ghost-step` | invents a step that does not exist | diff --git a/scripts/__tests__/fixtures/architecture-rules/phantom-column-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/phantom-column-skill/SKILL.md deleted file mode 100644 index 46e023eca..000000000 --- a/scripts/__tests__/fixtures/architecture-rules/phantom-column-skill/SKILL.md +++ /dev/null @@ -1,7 +0,0 @@ -# One Step With A Trigger Column - -## Actions - -| # | Action | Trigger | Role | -| --- | --- | --- | --- | -| 01 | `step` | `yes` | Do the one thing | diff --git a/scripts/__tests__/fixtures/architecture-rules/phantom-path-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/phantom-path-skill/SKILL.md deleted file mode 100644 index eb629289e..000000000 --- a/scripts/__tests__/fixtures/architecture-rules/phantom-path-skill/SKILL.md +++ /dev/null @@ -1,9 +0,0 @@ -# Fenced Path Citation - -## Actions - -Run `step` first. - -```md -actions/99-absent.md -``` diff --git a/scripts/lib/architecture-rules.js b/scripts/lib/architecture-rules.js index b554fbae7..eb7864a0f 100644 --- a/scripts/lib/architecture-rules.js +++ b/scripts/lib/architecture-rules.js @@ -6,8 +6,14 @@ * * Rule one, cross-plugin orthogonality: a plugin's dispatch surface must not name a sibling * plugin by a hardcoded address. - * Rule two, router coherence: a skill's `## Actions` section must name exactly the actions that - * skill provides. + * Rule two, router coherence: a skill's `## Actions` section must name every action file that + * skill provides. It checks one direction only — an action file the section never cites. It + * used to also flag the opposite direction, a citation with no action file behind it, but that + * is indistinguishable from a citation written seconds before the file it names, which is the + * order this project's own skill generator documents (create the action, then have the router + * name it — or name it first, then create the file). Enforcing it made adding an action to an + * existing skill impossible in either order. The direction that remains is decidable at any + * moment content is proposed, regardless of what gets written next. */ "use strict"; @@ -16,7 +22,8 @@ const ORCHESTRATOR_PLUGIN = "aidd-orchestrator"; // Temporary: `00-onboard`'s reference menus are routing menus whose addresses are what the // skill hands a person to type, not a hardcoded sibling provider — so orthogonality stays -// silent on this one skill directory. See the follow-up issue on 00-onboard runtime discovery. +// silent on this one skill directory. See #883, the follow-up issue on 00-onboard runtime +// discovery. const ONBOARD_EXEMPT_PREFIX = "plugins/aidd-context/skills/00-onboard/"; const HEADING_RE = /^#{1,6}\s/; @@ -28,8 +35,13 @@ const SECOND_LEVEL_HEADING_RE = /^##\s+/; // because the character right before "aidd-" (a hyphen, a letter, a digit, or another `/`/`@`) // rules it out — while a backtick, space, or start of line still lets a bare address through. const ADDRESS_RE = /(?.md` path, or a backticked `.md` filename. +// Deliberately narrow — a word loose in prose is never a citation, which is what let a deleted +// table row hide behind unrelated text that happened to contain the same word. const ACTION_PATH_RE = /actions\/([A-Za-z0-9._-]+)\.md/g; +const BACKTICKED_MD_RE = /`([A-Za-z0-9][A-Za-z0-9._-]*\.md)`/g; +const CITATION_TOKEN_RE = /^[A-Za-z0-9][A-Za-z0-9._-]*$/; function toLines(content) { return content.split("\n"); @@ -143,17 +155,11 @@ function stemOf(fileName) { return fileName.replace(/\.md$/i, "").replace(/^[0-9]+-/, ""); } -function escapeRegExp(text) { - return text.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); -} - -/** Whether `token` occurs in `text` as a whole identifier, not merely as a substring of a - * longer hyphenated one — "assert" is present in "the `assert` action" but not in - * "assert-architecture", because hyphen is a token character here, not a boundary. */ -function tokenPresent(text, token) { - if (!token) return false; - const re = new RegExp(`(? cell.trim()); } -function isTableSeparatorRow(cells) { - return cells.length > 0 && cells.every((cell) => /^:?-+:?$/.test(cell)); -} - -/** Groups of consecutive table-row offsets (into `sectionLines`) — a `## Actions` section may - * hold more than one pipe table (an action table, and unrelated prose table such as a trigger - * glossary), and each is scoped to its own action column independently. */ -function tableBlocks(sectionLines) { - const blocks = []; - let current = null; - sectionLines.forEach((line, offset) => { - if (/^\s*\|/.test(line)) { - if (!current) { - current = []; - blocks.push(current); - } - current.push(offset); - } else { - current = null; +/** Every citation the "## Actions" section makes to an action file: a table cell that reads as + * a plain name, an `actions/.md` path, or a backticked `.md` filename — the three + * shapes rule two's own comment names, and nothing else. A word merely present in prose is not + * collected here, on purpose: that is exactly what let a deleted table row hide behind + * unrelated text that happened to contain the same word. */ +function citationsIn(sectionLines, sectionText) { + const citations = new Set(); + + for (const line of sectionLines) { + if (!/^\s*\|/.test(line)) continue; + for (const rawCell of splitTableCells(line)) { + const cell = stripBackticks(rawCell); + if (CITATION_TOKEN_RE.test(cell)) citations.add(cell.toLowerCase()); } - }); - return blocks; -} + } -/** The column index that carries action names in this table block, found by locating a cell - * backed by a real action file — the only column a backticked token can be a phantom citation - * in. -1 when no row backs any column, so an unrelated table (a keyword or trigger glossary, - * never an action listing) is left unchecked rather than guessed at. */ -function actionColumnOf(offsets, sectionLines, backed) { - for (const offset of offsets) { - const cells = splitTableCells(sectionLines[offset]); - if (isTableSeparatorRow(cells)) continue; - for (let col = 0; col < cells.length; col += 1) { - TABLE_TOKEN_RE.lastIndex = 0; - let match; - while ((match = TABLE_TOKEN_RE.exec(cells[col])) !== null) { - if (backed.has(match[1].toLowerCase())) return col; - } - } + ACTION_PATH_RE.lastIndex = 0; + let pathMatch; + while ((pathMatch = ACTION_PATH_RE.exec(sectionText)) !== null) { + citations.add(pathMatch[1].toLowerCase()); + } + + BACKTICKED_MD_RE.lastIndex = 0; + let mdMatch; + while ((mdMatch = BACKTICKED_MD_RE.exec(sectionText)) !== null) { + citations.add(mdMatch[1].toLowerCase()); } - return -1; + + return citations; } /** - * Rule two: a skill's `## Actions` section names exactly the actions that skill provides. + * Rule two: a skill's `## Actions` section cites every action file that skill provides. It + * checks this one direction only — see the module header comment for why the opposite + * direction (a citation with no file behind it) is gone rather than narrowed. */ function checkRouterCoherence(filePath, content, actionFileNames) { const info = classifyFile(filePath); @@ -251,24 +247,14 @@ function checkRouterCoherence(filePath, content, actionFileNames) { const violations = []; const sectionLines = lines.slice(section.startIdx, section.endIdx); const sectionText = sectionLines.join("\n"); + const citations = citationsIn(sectionLines, sectionText); - const backed = new Set(); for (const name of names) { - backed.add(name.toLowerCase()); - backed.add(name.replace(/\.md$/i, "").toLowerCase()); - backed.add(stemOf(name).toLowerCase()); - } + const full = name.toLowerCase(); + const fullNoExt = name.replace(/\.md$/i, "").toLowerCase(); + const stem = stemOf(name).toLowerCase(); + if (citations.has(full) || citations.has(fullNoExt) || citations.has(stem)) continue; - for (const name of names) { - const stem = stemOf(name); - const fullNoExt = name.replace(/\.md$/i, ""); - if ( - tokenPresent(sectionText, name) || - tokenPresent(sectionText, fullNoExt) || - tokenPresent(sectionText, stem) - ) { - continue; - } violations.push({ file: filePath, line: section.headingLine, @@ -278,53 +264,6 @@ function checkRouterCoherence(filePath, content, actionFileNames) { }); } - for (const offsets of tableBlocks(sectionLines)) { - const actionColumn = actionColumnOf(offsets, sectionLines, backed); - if (actionColumn === -1) continue; // no row backs any column: not an action table, leave it alone - - for (const offset of offsets) { - const cells = splitTableCells(sectionLines[offset]); - if (isTableSeparatorRow(cells)) continue; - const cell = cells[actionColumn]; - if (cell === undefined) continue; - - const lineNo = section.startIdx + offset + 1; - TABLE_TOKEN_RE.lastIndex = 0; - let match; - while ((match = TABLE_TOKEN_RE.exec(cell)) !== null) { - const token = match[1].toLowerCase(); - if (!backed.has(token)) { - violations.push({ - file: filePath, - line: lineNo, - plugin: info.owner, - rule: "router-coherence", - message: `${filePath}:${lineNo} "## Actions" cites "${match[1]}" with no action file behind it`, - }); - } - } - } - } - - sectionLines.forEach((line, offset) => { - const lineNo = section.startIdx + offset + 1; - - ACTION_PATH_RE.lastIndex = 0; - let pathMatch; - while ((pathMatch = ACTION_PATH_RE.exec(line)) !== null) { - const cited = pathMatch[1].toLowerCase(); - if (!backed.has(cited)) { - violations.push({ - file: filePath, - line: lineNo, - plugin: info.owner, - rule: "router-coherence", - message: `${filePath}:${lineNo} "## Actions" cites "actions/${pathMatch[1]}.md" with no action file behind it`, - }); - } - } - }); - return violations; } From 3d3f68c7a6c319d3ec5cae1401f870b971e64208 Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Fri, 18 Sep 2026 13:48:09 +0200 Subject: [PATCH 04/11] fix(framework): a citation comes from the column a table calls Action MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A third review found the refusal told a contributor to keep the section "naming exactly the action files, no more, no fewer" — the second half of which the guard stopped enforcing a commit ago — and never said what counts as naming one. Someone who had written the file's name in prose had already done what the text asked. The refusal now names the three shapes a citation takes. Citations were collected from every cell of every table in the section, so a glossary satisfied rule two while routing nothing, and an action whose row was deleted stayed covered by its name sitting in a "next step" column. Each table now declares its own action column and only that column cites. Resolving that column once per section rather than once per table was a defect the mutation caught: a glossary standing before the router made the router's own column unreadable. A table is a table. phase-1.md still specified the direction commit 29489f7d deleted. The sweep's reason, the fixtures it names, and its acceptance criteria now say what the engine does. One non-goal blamed the deadlock on the wrong half of the mechanism, and claimed an orphan action file is caught at the next router write — there is no such guarantee, and it now says so. 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 | 2 +- .../phase-1.md | 12 ++-- .../spec.md | 4 +- scripts/__tests__/architecture-hook.test.js | 19 +++++ scripts/__tests__/architecture-rules.test.js | 71 +++++++++++++++++++ .../glossary-column-skill/SKILL.md | 14 ++++ .../no-actions-section-skill/SKILL.md | 7 ++ scripts/lib/architecture-rules.js | 53 ++++++++++---- 8 files changed, 161 insertions(+), 21 deletions(-) create mode 100644 scripts/__tests__/fixtures/architecture-rules/glossary-column-skill/SKILL.md create mode 100644 scripts/__tests__/fixtures/architecture-rules/no-actions-section-skill/SKILL.md diff --git a/.claude/hooks/check-architecture-rules.js b/.claude/hooks/check-architecture-rules.js index d5ad95acb..c843a9739 100755 --- a/.claude/hooks/check-architecture-rules.js +++ b/.claude/hooks/check-architecture-rules.js @@ -101,7 +101,7 @@ function actionFileNamesFor(relPath, absPath) { function fixFor(rule, plugin) { return rule === "orthogonality" ? `name the concept ${plugin} owns instead of addressing it directly` - : `keep "## Actions" naming exactly the action files this skill provides, no more, no fewer`; + : 'cite every action file the skill provides in its "## Actions" section. A citation is a cell in the table\'s action column, a fenced `actions/.md` path, or a backticked `.md` file name — a word in prose does not count'; } function denyReason(violations) { diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/phase-1.md b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/phase-1.md index c57349b1c..61e8d98f7 100644 --- a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/phase-1.md +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/phase-1.md @@ -50,8 +50,8 @@ journey An orchestrator reference names a provider => run the engine over it => no violation: 1: cli section Edge case - an action file no section names A skill gains an action file its Actions section never mentions => run the engine over it => one violation naming the file: 1: cli - section Edge case - a section names an action that does not exist - An Actions row cites a name with no file behind it => run the engine over it => one violation naming the file and the row: 1: cli + section Edge case - a section whose mention is only prose + A word in the section's prose matches an action's stem but no row cites it => run the engine over it => one violation naming the file: 1: cli ``` ## Tasks to do @@ -75,12 +75,12 @@ journey ### `3)` The router coherence rule -> An Actions section names exactly the actions that exist. +> An Actions section cites every action that exists. 1. Take the prospective `SKILL.md` content and the names of the skill's action files. 2. Isolate the `## Actions` section, up to the next second-level heading. -3. Report an action file the section names by neither its stem nor its file name, and report a name the section cites that no file backs. -4. Report the line of the section heading when the violation is an absent mention, and the line of the citation when it is a phantom one. +3. Report an action file the section cites by neither its stem, its file name, nor an `actions/.md` path. Collect a citation from the table's action column, never from running prose. +4. Report the line of the section heading. A citation with no file behind it is deliberately not reported — it cannot be told apart from one written just before the file it names. ### `4)` The rule the repository already follows @@ -101,6 +101,6 @@ journey | --- | --- | | 1 | Every case in the suite fails before the engine exists, each for the missing module | | 2 | A skill addressing a sibling yields a violation naming file, line and owning plugin; an agent permission list and an orchestrator reference yield none; a file under `assets/` yields none | -| 3 | An action file the section never names yields one violation; a cited name with no file yields one violation; a skill whose section and files agree yields none | +| 3 | An action file the section never cites yields one violation; a stem appearing only in prose does not count as a citation; a skill whose section and files agree yields none | | 4 | Sweeping the repository's own `plugins/` yields zero violations | | 5 | `docs/ARCHITECTURE.md` names the guard, and the markdown-link check still passes | diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md index 8b3ae0f62..a192f1f1f 100644 --- a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md @@ -14,7 +14,7 @@ An AI-authored edit that would break one of this repository's two named architec - The governed surface is the dispatch surface: a skill's `SKILL.md`, its actions and its references, and an agent's definition. A plugin's `assets/` hold content shown to a reader, not dispatch, and are out of the guard's reach by this rule rather than by an unstated path filter. - Naming that `docs/ARCHITECTURE.md` declares legitimate stays silent: an agent's permission list and an orchestration reference are responsibility maps and name their provider canonically. Flagging either is a defect of the guard, not of the tree. - Every refusal names the file, the line, and the plugin that owns the addressed capability, in terms a reader can act on without opening the rule document. -- The guard is silent on the repository as it stands. A rule that reports a violation in the current tree is miscalibrated, not vindicated. +- The guard is silent on the repository as it stands. Two shapes it reads naively, both absent from the tree today: a fenced `## Actions` inside a code block is taken for the section itself, and a fenced `##` line inside the section ends it early. A rule that reports a violation in the current tree is miscalibrated, not vindicated. - The rules are exercised by purpose-built fixtures. No historical code from #406 is used as a fixture. - Each rule is proved by a fixture that breaks it and turns exactly the test named for that rule red, and by a fixture of legitimate naming that stays green. @@ -24,7 +24,7 @@ An AI-authored edit that would break one of this repository's two named architec - A CI job enforcing these rules. - Judging whether a skill's prose `description` has gone stale. Nothing mechanically separates stale prose from current prose, so the issue's "router/description mismatch" is served by the router half alone. - Refusing a citation with no action file behind it. A citation written seconds before the file it names is indistinguishable from a stale one, and this project's own skill generator writes the router first, so enforcing that direction makes adding an action impossible in either order. The decidable direction — an action file the section never cites — is the one the issue names, and the one enforced. -- Catching an action file created and never cited. Rule two decides on a write to a `SKILL.md`; an orphan action file is caught at the next write to its skill's router, not at its own creation. Firing on the action file instead is what made both orders of adding an action refuse each other. +- Catching an action file created and never cited. Rule two decides on a write to a `SKILL.md`; an orphan action file is caught at the next write to its skill's router, not at its own creation. Firing on the action file is what broke the order that creates the file first, and dropping it was the cheaper half of breaking the deadlock. The window is unbounded: that next write may never come, so this is a hole, not a delay. - Reaching into a plugin's `README.md`, which addresses a sibling in two places today. Like `assets/`, it is a document a reader reads, not a dispatch the skill executes. - Reaching into a plugin's `assets/`. A recipe sheet names the commands a reader types; that is its subject, not a dispatch this rule governs. - Repairing violations that exist in the tree today. That was #406, now closed. diff --git a/scripts/__tests__/architecture-hook.test.js b/scripts/__tests__/architecture-hook.test.js index 67b31b091..babfb0fbe 100644 --- a/scripts/__tests__/architecture-hook.test.js +++ b/scripts/__tests__/architecture-hook.test.js @@ -479,3 +479,22 @@ test("adding an action to an existing skill succeeds citation-first (order B)", 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, /action column/); + assert.match(reason, /a word in prose does not count/); +}); diff --git a/scripts/__tests__/architecture-rules.test.js b/scripts/__tests__/architecture-rules.test.js index 812236db6..315125541 100644 --- a/scripts/__tests__/architecture-rules.test.js +++ b/scripts/__tests__/architecture-rules.test.js @@ -231,3 +231,74 @@ test("sweeping the repository's own plugins/ tree yields zero violations", () => // this pins the sweep to the measured count so it cannot go vacuously green. assert.equal(skillsWithActions, 48); }); + +test("a table that declares no action column cites nothing, whatever its cells read", () => { + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-glossary/SKILL.md", + fixture("glossary-column-skill/SKILL.md"), + ["01-step.md"] + ); + assert.equal(violations.length, 1); + assert.equal(violations[0].rule, "router-coherence"); + assert.match(violations[0].message, /never names action file "01-step\.md"/); +}); + +test("a name in a column other than the action column does not cite that action", () => { + const content = [ + "# Two column skill", + "", + "## Actions", + "", + "| # | Action | Next |", + "| --- | --- | --- |", + "| 01 | `first` | second |", + "", + "## Transversal rules", + "", + "- Nothing.", + ].join("\n"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-two-column/SKILL.md", + content, + ["01-first.md", "02-second.md"] + ); + assert.equal(violations.length, 1); + assert.match(violations[0].message, /never names action file "02-second\.md"/); +}); + +test("a skill holding action files with no Actions section at all is refused", () => { + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-no-section/SKILL.md", + fixture("no-actions-section-skill/SKILL.md"), + ["01-step.md"] + ); + assert.equal(violations.length, 1); + assert.equal(violations[0].rule, "router-coherence"); + assert.match(violations[0].message, /no "## Actions" section/); +}); + +test("a glossary table before the router does not blind the router's own column", () => { + const content = [ + "# Two table skill", + "", + "## Actions", + "", + "| Term | Synonym |", + "| --- | --- |", + "| step | move |", + "", + "| # | Action | Role |", + "| --- | --- | --- |", + "| 01 | `first` | Do it |", + "", + "## Transversal rules", + "", + "- Nothing.", + ].join("\n"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-two-table/SKILL.md", + content, + ["01-first.md"] + ); + assert.deepEqual(violations, []); +}); diff --git a/scripts/__tests__/fixtures/architecture-rules/glossary-column-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/glossary-column-skill/SKILL.md new file mode 100644 index 000000000..a9959b5f3 --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/glossary-column-skill/SKILL.md @@ -0,0 +1,14 @@ +# Glossary column skill + +## Actions + +Nothing routes here. The table below defines words, it does not dispatch. + +| Term | Synonym | +| --- | --- | +| step | move | +| other | further | + +## Transversal rules + +- Nothing. diff --git a/scripts/__tests__/fixtures/architecture-rules/no-actions-section-skill/SKILL.md b/scripts/__tests__/fixtures/architecture-rules/no-actions-section-skill/SKILL.md new file mode 100644 index 000000000..a9022d99d --- /dev/null +++ b/scripts/__tests__/fixtures/architecture-rules/no-actions-section-skill/SKILL.md @@ -0,0 +1,7 @@ +# No actions section skill + +This skill ships an action file and never declares a router for it. + +## Transversal rules + +- Nothing. diff --git a/scripts/lib/architecture-rules.js b/scripts/lib/architecture-rules.js index eb7864a0f..276947e43 100644 --- a/scripts/lib/architecture-rules.js +++ b/scripts/lib/architecture-rules.js @@ -52,9 +52,7 @@ function toLines(content) { * anything else, including everything under an `assets/` segment at any depth. * * An `actions/` or `references/` segment governs everything beneath it, at any depth, and a - * `SKILL.md` is governed at any depth under `skills/` — not only one level down. `skillDir` is - * the repository-relative path (`plugins//skills/<...>`) of the skill folder itself: the - * directory holding `SKILL.md`, or the directory the `actions/`/`references/` segment sits in. + * `SKILL.md` is governed at any depth under `skills/` — not only one level down. */ function classifyFile(filePath) { const parts = filePath.split("/").filter(Boolean); @@ -77,20 +75,18 @@ function classifyFile(filePath) { const last = rest[rest.length - 1]; if (!last.endsWith(".md")) return null; - const skillDirFor = (segments) => ["plugins", owner, "skills", ...segments].join("/"); - if (last === "SKILL.md") { - return { owner, kind: "skill", skillDir: skillDirFor(rest.slice(0, -1)) }; + return { owner, kind: "skill" }; } const actionsIdx = rest.indexOf("actions"); if (actionsIdx !== -1) { - return { owner, kind: "action", skillDir: skillDirFor(rest.slice(0, actionsIdx)) }; + return { owner, kind: "action" }; } const referencesIdx = rest.indexOf("references"); if (referencesIdx !== -1) { - return { owner, kind: "reference", skillDir: skillDirFor(rest.slice(0, referencesIdx)) }; + return { owner, kind: "reference" }; } return null; @@ -186,6 +182,31 @@ function splitTableCells(line) { return withoutEdges.split("|").map((cell) => cell.trim()); } +/** The contiguous runs of table rows in a section: each run is one table, so one table's + * header never speaks for the next one's columns. */ +function tableBlocks(sectionLines) { + const blocks = []; + let current = []; + for (const line of sectionLines) { + if (/^\s*\|/.test(line)) { + current.push(line); + continue; + } + if (current.length > 0) { + blocks.push(current); + current = []; + } + } + if (current.length > 0) blocks.push(current); + return blocks; +} + +/** A `| --- | --- |` row: every cell is dashes and colons, so it declares no column and cites + * nothing. */ +function isTableSeparatorRow(cells) { + return cells.length > 0 && cells.every((cell) => /^:?-{3,}:?$/.test(cell)); +} + /** Every citation the "## Actions" section makes to an action file: a table cell that reads as * a plain name, an `actions/.md` path, or a backticked `.md` filename — the three * shapes rule two's own comment names, and nothing else. A word merely present in prose is not @@ -194,10 +215,18 @@ function splitTableCells(line) { function citationsIn(sectionLines, sectionText) { const citations = new Set(); - for (const line of sectionLines) { - if (!/^\s*\|/.test(line)) continue; - for (const rawCell of splitTableCells(line)) { - const cell = stripBackticks(rawCell); + // Only the column a table declares as its action column counts. A glossary, a trigger column + // or a "next step" column names things that are not dispatch, and reading them as citations + // would let a section satisfy rule two while routing nothing. Each table decides for itself: + // a section may hold a glossary next to its router, and the router must still be read. + for (const block of tableBlocks(sectionLines)) { + const rows = block.map(splitTableCells); + const actionColumn = rows[0].findIndex((cell) => /\baction\b/i.test(stripBackticks(cell))); + if (actionColumn === -1) continue; // this table declares no action column: it cites nothing + + for (const cells of rows.slice(1)) { + if (isTableSeparatorRow(cells)) continue; + const cell = stripBackticks(cells[actionColumn] ?? ""); if (CITATION_TOKEN_RE.test(cell)) citations.add(cell.toLowerCase()); } } From 472330ce2cd5d67e6038efd8e17cd3c98413cef1 Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Fri, 18 Sep 2026 14:03:07 +0200 Subject: [PATCH 05/11] fix(framework): a table resumed after a blank line is still that table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fourth review caught a regression the third commit's own hot path introduced. A blank line inside a router table started a new block, that block had no header row, so no column declared itself and every row below the blank line cited nothing. Inserting one blank line — changing no content — refused three actions the section visibly lists. A run of rows with no `| --- |` under it is the same table resumed, and it keeps the column its header declared. Two header shapes were wrong in opposite directions. `Next action` was read as an action column, so a deleted row could hide behind a routing hint; the match is exact now. `Actions` was not read at all, so a router echoing its own section heading refused every action it provides; the match takes the plural. And a separator written `| -- |` — which `00-onboard` does, and GitHub renders — was not recognised as a separator at all. The sweep over the real tree is what caught that one, which is the whole reason it exists. plan.md still claimed both directions of rule two were measured green, and nothing recorded that one plugin source was repaired. `04-plan.md:16` addressed a sibling skill in prose; the spec's non-goal keeps existing violations out of scope, so the exception is now named where a reviewer reads the contract, not left to be found in a diff. Refs #250 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2 --- .../plan.md | 3 +- .../spec.md | 4 +- scripts/__tests__/architecture-rules.test.js | 68 +++++++++++++++++++ scripts/lib/architecture-rules.js | 26 +++++-- 4 files changed, 91 insertions(+), 10 deletions(-) diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md index d2644b26d..5340719ff 100644 --- a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md @@ -23,7 +23,7 @@ | | `PreToolUse` receives `tool_input` before the tool runs — `content` for `Write` — and refuses the call with `hookSpecificOutput.permissionDecision: "deny"` plus a `permissionDecisionReason` the model reads. `PostToolUse` cannot refuse, it only reports after the fact. | | `.claude/hooks/check-written-file.js` | The in-repo precedent for a hook that reads the payload from stdin, resolves the written file, and hands a report back in the same turn. | | Issue #250, comment of 2026-09-14 | Supersedes the issue body's "Guardrail local et CI": no Git hook, no CI gate, synthetic fixtures, #406 neither blocker nor fixture. | -| Probe over the 51 skills and 8 plugins in the tree | Both rules, as scoped below, report zero violations on the repository as it stands — orthogonality across 37 real cross-plugin addresses, router coherence across 48 skills that hold action files, in both directions. | +| Probe over the 51 skills and 8 plugins in the tree | Both rules, as scoped below, report zero violations on the repository as it stands — orthogonality across 37 real cross-plugin addresses, router coherence across 48 skills that hold action files. The probe measured both directions; only the decidable one ships, per the decision below. | ## Decisions @@ -39,3 +39,4 @@ | The hook is wired for Claude Code alone | It is the only host this repository configures hooks for: `.codex/config.toml` carries a sandbox mode and nothing else. Codex, Cursor and Copilot all expose `PreToolUse` and the same deny shape, but Codex delivers a file edit as an `apply_patch` command string rather than a path and a content, which is a different parse. The engine is host-agnostic, so each adapter is additive and none of them touches a rule. | | Rule two enforces one direction, not two | A citation with no file behind it cannot be told apart from a citation written just before the file it names. Enforcing it deadlocked adding an action: creating the file first was refused for not being named, naming it first was refused for having no file, and `aidd-context:04-skill-generate` documents the second order. The decidable direction is kept. | | An action is named by a citation, never by a word | Plain containment over the section let ordinary prose pass for a mention: `plan` appears in "the plan is the culmination", so deleting that action's row went unnoticed. Measured over the tree, containment missed 20 of 78 row deletions; citation matching misses none that has a row. | +| One plugin source was repaired, and it is the exception | `plugins/aidd-dev/skills/01-plan/actions/04-plan.md:16` read "declare it the same way `aidd-pm:04-spec` does", which is rule one's own violation in prose. The spec's non-goal keeps existing violations out of scope, and the hard constraint says a rule red on the tree is miscalibrated — so the one line that was genuinely wrong was fixed, and the seven in `00-onboard` were exempted with #883 behind them instead. No other plugin source is touched. | diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md index a192f1f1f..5065ce14d 100644 --- a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md @@ -14,7 +14,7 @@ An AI-authored edit that would break one of this repository's two named architec - The governed surface is the dispatch surface: a skill's `SKILL.md`, its actions and its references, and an agent's definition. A plugin's `assets/` hold content shown to a reader, not dispatch, and are out of the guard's reach by this rule rather than by an unstated path filter. - Naming that `docs/ARCHITECTURE.md` declares legitimate stays silent: an agent's permission list and an orchestration reference are responsibility maps and name their provider canonically. Flagging either is a defect of the guard, not of the tree. - Every refusal names the file, the line, and the plugin that owns the addressed capability, in terms a reader can act on without opening the rule document. -- The guard is silent on the repository as it stands. Two shapes it reads naively, both absent from the tree today: a fenced `## Actions` inside a code block is taken for the section itself, and a fenced `##` line inside the section ends it early. A rule that reports a violation in the current tree is miscalibrated, not vindicated. +- The guard is silent on the repository as it stands. Three shapes it reads naively, all absent from the tree today: a fenced `## Actions` inside a code block is taken for the section itself, a fenced `##` line inside the section ends it early, and a fenced example table inside the section cites like a real router. A rule that reports a violation in the current tree is miscalibrated, not vindicated. - The rules are exercised by purpose-built fixtures. No historical code from #406 is used as a fixture. - Each rule is proved by a fixture that breaks it and turns exactly the test named for that rule red, and by a fixture of legitimate naming that stays green. @@ -27,7 +27,7 @@ An AI-authored edit that would break one of this repository's two named architec - Catching an action file created and never cited. Rule two decides on a write to a `SKILL.md`; an orphan action file is caught at the next write to its skill's router, not at its own creation. Firing on the action file is what broke the order that creates the file first, and dropping it was the cheaper half of breaking the deadlock. The window is unbounded: that next write may never come, so this is a hole, not a delay. - Reaching into a plugin's `README.md`, which addresses a sibling in two places today. Like `assets/`, it is a document a reader reads, not a dispatch the skill executes. - Reaching into a plugin's `assets/`. A recipe sheet names the commands a reader types; that is its subject, not a dispatch this rule governs. -- Repairing violations that exist in the tree today. That was #406, now closed. +- Repairing violations that exist in the tree today. That was #406, now closed. One line is the stated exception, named in the plan's decisions: a prose sentence in `aidd-dev`'s planning action addressed a sibling skill outright, and leaving it would have made the guard red on its own tree. - Enforcing any architecture rule beyond the two named above. - Catching a violation introduced outside an AI tool, by a human editing by hand. - Catching a write performed through a shell command rather than a write tool. Deciding whether a shell line writes a plugin source means parsing arbitrary shell, which is not the deterministic verdict rule one requires. An agent told to edit through `sed` or a heredoc is therefore ungoverned, and that is stated here rather than left to be discovered. diff --git a/scripts/__tests__/architecture-rules.test.js b/scripts/__tests__/architecture-rules.test.js index 315125541..477bd8fa4 100644 --- a/scripts/__tests__/architecture-rules.test.js +++ b/scripts/__tests__/architecture-rules.test.js @@ -302,3 +302,71 @@ test("a glossary table before the router does not blind the router's own column" ); assert.deepEqual(violations, []); }); + +test("a blank line splitting the router table does not unname the rows below it", () => { + const content = [ + "# Split table skill", + "", + "## Actions", + "", + "| # | Action | Role |", + "| --- | --- | --- |", + "| 01 | `first` | Do it |", + "", + "| 02 | `second` | Then this |", + "", + "## Transversal rules", + "", + "- Nothing.", + ].join("\n"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-split/SKILL.md", + content, + ["01-first.md", "02-second.md"] + ); + assert.deepEqual(violations, []); +}); + +test("a two-dash separator row still marks the header it follows", () => { + const content = [ + "# Short rule skill", + "", + "## Actions", + "", + "| # | Action | Does |", + "| -- | --- | --- |", + "| 01 | first | Do it |", + "", + "## Transversal rules", + "", + "- Nothing.", + ].join("\n"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-short-rule/SKILL.md", + content, + ["01-first.md"] + ); + assert.deepEqual(violations, []); +}); + +test("a plural Actions header declares the action column too", () => { + const content = [ + "# Plural header skill", + "", + "## Actions", + "", + "| # | Actions | Role |", + "| --- | --- | --- |", + "| 01 | `first` | Do it |", + "", + "## Transversal rules", + "", + "- Nothing.", + ].join("\n"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-plural/SKILL.md", + content, + ["01-first.md"] + ); + assert.deepEqual(violations, []); +}); diff --git a/scripts/lib/architecture-rules.js b/scripts/lib/architecture-rules.js index 276947e43..d67f54524 100644 --- a/scripts/lib/architecture-rules.js +++ b/scripts/lib/architecture-rules.js @@ -41,6 +41,9 @@ const ADDRESS_RE = /(? 0 && cells.every((cell) => /^:?-{3,}:?$/.test(cell)); + return cells.length > 0 && cells.every((cell) => /^:?-+:?$/.test(cell)); } /** Every citation the "## Actions" section makes to an action file: a table cell that reads as @@ -212,19 +215,28 @@ function isTableSeparatorRow(cells) { * shapes rule two's own comment names, and nothing else. A word merely present in prose is not * collected here, on purpose: that is exactly what let a deleted table row hide behind * unrelated text that happened to contain the same word. */ -function citationsIn(sectionLines, sectionText) { +function citationsIn(sectionLines) { + const sectionText = sectionLines.join("\n"); const citations = new Set(); // Only the column a table declares as its action column counts. A glossary, a trigger column // or a "next step" column names things that are not dispatch, and reading them as citations // would let a section satisfy rule two while routing nothing. Each table decides for itself: // a section may hold a glossary next to its router, and the router must still be read. + let actionColumn = -1; for (const block of tableBlocks(sectionLines)) { const rows = block.map(splitTableCells); - const actionColumn = rows[0].findIndex((cell) => /\baction\b/i.test(stripBackticks(cell))); - if (actionColumn === -1) continue; // this table declares no action column: it cites nothing + const isNewTable = rows.length > 1 && isTableSeparatorRow(rows[1]); + + // A header row is followed by its `| --- |` separator. A run of rows without one is the + // same table resumed after a blank line, and it keeps the column its header declared — + // otherwise a purely cosmetic edit refuses every row below the blank line. + if (isNewTable) { + actionColumn = rows[0].findIndex((cell) => ACTION_HEADER_RE.test(stripBackticks(cell))); + } + if (actionColumn === -1) continue; // no header declared an action column: this table cites nothing - for (const cells of rows.slice(1)) { + for (const cells of isNewTable ? rows.slice(1) : rows) { if (isTableSeparatorRow(cells)) continue; const cell = stripBackticks(cells[actionColumn] ?? ""); if (CITATION_TOKEN_RE.test(cell)) citations.add(cell.toLowerCase()); @@ -276,7 +288,7 @@ function checkRouterCoherence(filePath, content, actionFileNames) { const violations = []; const sectionLines = lines.slice(section.startIdx, section.endIdx); const sectionText = sectionLines.join("\n"); - const citations = citationsIn(sectionLines, sectionText); + const citations = citationsIn(sectionLines); for (const name of names) { const full = name.toLowerCase(); From d60652be8e2521d4cf12c657d7d6b337c87a5d3b Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Fri, 18 Sep 2026 14:16:56 +0200 Subject: [PATCH 06/11] fix(framework): a fence holds an example, and a shared stem cites nobody MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fifth review asked for the one thing no test could supply: a recorded refusal through a real tool call. Everything until now was verified upstream of the wire — the tests spawn the script and hand it a payload, which proves the script. The matcher, the `$CLAUDE_PROJECT_DIR` expansion and `node` on `PATH` are only exercised by an actual Write, and the hook fails open at every one of those steps, so a broken wire looks exactly like a clean tree. `wiring-proof.md` records both halves: the refusal, verbatim, and the permission list carrying the same address through untouched. Two holes the review named are closed rather than disclosed. `01-plan.md` and `04-plan.md` both reduce to `plan`, so one citation covered both and deleting either row went unnoticed; a shared stem now cites nobody and the numbered name still does. And a fenced block is an example: a documented `## Actions` is no longer mistaken for the section, a `##` inside one no longer ends it early, and an example table inside one no longer cites. The exception is a fenced `actions/.md` path, which `10-todo` uses for its only action — the sweep over the real tree caught that the moment fences went blank. 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 | 2 +- .../plan.md | 2 + .../spec.md | 2 +- .../wiring-proof.md | 53 +++++++++ scripts/__tests__/architecture-hook.test.js | 2 +- scripts/__tests__/architecture-rules.test.js | 101 ++++++++++++++++++ scripts/lib/architecture-rules.js | 43 ++++++-- 7 files changed, 196 insertions(+), 9 deletions(-) create mode 100644 aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/wiring-proof.md diff --git a/.claude/hooks/check-architecture-rules.js b/.claude/hooks/check-architecture-rules.js index c843a9739..609de1a96 100755 --- a/.claude/hooks/check-architecture-rules.js +++ b/.claude/hooks/check-architecture-rules.js @@ -101,7 +101,7 @@ function actionFileNamesFor(relPath, absPath) { function fixFor(rule, plugin) { return rule === "orthogonality" ? `name the concept ${plugin} owns instead of addressing it directly` - : 'cite every action file the skill provides in its "## Actions" section. A citation is a cell in the table\'s action column, a fenced `actions/.md` path, or a backticked `.md` file name — a word in prose does not count'; + : 'cite every action file the skill provides in its "## Actions" section. A citation is 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'; } function denyReason(violations) { diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md index 5340719ff..7553619fd 100644 --- a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md @@ -40,3 +40,5 @@ | Rule two enforces one direction, not two | A citation with no file behind it cannot be told apart from a citation written just before the file it names. Enforcing it deadlocked adding an action: creating the file first was refused for not being named, naming it first was refused for having no file, and `aidd-context:04-skill-generate` documents the second order. The decidable direction is kept. | | An action is named by a citation, never by a word | Plain containment over the section let ordinary prose pass for a mention: `plan` appears in "the plan is the culmination", so deleting that action's row went unnoticed. Measured over the tree, containment missed 20 of 78 row deletions; citation matching misses none that has a row. | | One plugin source was repaired, and it is the exception | `plugins/aidd-dev/skills/01-plan/actions/04-plan.md:16` read "declare it the same way `aidd-pm:04-spec` does", which is rule one's own violation in prose. The spec's non-goal keeps existing violations out of scope, and the hard constraint says a rule red on the tree is miscalibrated — so the one line that was genuinely wrong was fixed, and the seven in `00-onboard` were exempted with #883 behind them instead. No other plugin source is touched. | +| A stem speaks for its action only when no sibling shares it | `01-plan.md` and `04-plan.md` both reduce to `plan`, so one row would cover both and deleting either would go unnoticed. A shared stem cites nobody; the numbered name still does. No skill in the tree has a collision today, which is why it had to be reasoned about rather than observed. | +| A fenced block is an example, never a router | The section scan blanks fences before reading headings and tables, so a documented `## Actions`, a `##` inside the section, and an example table all stop steering the verdict. Path citations are read from the unblanked lines, because `10-todo` legitimately cites `actions/01-todo.md` inside a fence. | diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md index 5065ce14d..0ffed7bce 100644 --- a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md @@ -14,7 +14,7 @@ An AI-authored edit that would break one of this repository's two named architec - The governed surface is the dispatch surface: a skill's `SKILL.md`, its actions and its references, and an agent's definition. A plugin's `assets/` hold content shown to a reader, not dispatch, and are out of the guard's reach by this rule rather than by an unstated path filter. - Naming that `docs/ARCHITECTURE.md` declares legitimate stays silent: an agent's permission list and an orchestration reference are responsibility maps and name their provider canonically. Flagging either is a defect of the guard, not of the tree. - Every refusal names the file, the line, and the plugin that owns the addressed capability, in terms a reader can act on without opening the rule document. -- The guard is silent on the repository as it stands. Three shapes it reads naively, all absent from the tree today: a fenced `## Actions` inside a code block is taken for the section itself, a fenced `##` line inside the section ends it early, and a fenced example table inside the section cites like a real router. A rule that reports a violation in the current tree is miscalibrated, not vindicated. +- The guard is silent on the repository as it stands. A fenced block holds an example, never a router: a `## Actions` inside one is not the section, a `##` inside one does not end it, and a table inside one cites nothing. A fenced `actions/.md` path is the one exception, because `aidd-dev:10-todo` cites its only action that way. A rule that reports a violation in the current tree is miscalibrated, not vindicated. - The rules are exercised by purpose-built fixtures. No historical code from #406 is used as a fixture. - Each rule is proved by a fixture that breaks it and turns exactly the test named for that rule red, and by a fixture of legitimate naming that stays green. diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/wiring-proof.md b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/wiring-proof.md new file mode 100644 index 000000000..7d81908d9 --- /dev/null +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/wiring-proof.md @@ -0,0 +1,53 @@ +# Wiring proof + +Phase 2's task 5 asks for a recorded live refusal, because everything else about this guard +is verified upstream of the seam that matters: the tests spawn the hook script and pipe it a +payload, which proves the script and never proves the wire. The matcher string, the +`$CLAUDE_PROJECT_DIR` expansion inside the command, and `node` being on `PATH` are only +exercised by a real tool call — and the hook fails open at every one of those steps, so a +broken wire looks exactly like a clean tree. + +Both calls below were made with the Write tool, in this repository, against +`.claude/settings.json` as committed. + +## A refusal + +Writing `plugins/aidd-dev/skills/03-assert/references/wiring-proof.md`, a reference file in a +recipe skill, with this body: + +```md +# Wiring proof + +Hand the result to `/aidd-vcs:01-commit` once every assertion passes. +``` + +The call was refused. Verbatim, as it reached the author: + +```text +plugins/aidd-dev/skills/03-assert/references/wiring-proof.md:3 addresses sibling plugin "aidd-vcs" via "/aidd-vcs:01-commit". Fix: name the concept aidd-vcs owns instead of addressing it directly. +``` + +The file was never created — `ls` on that path answers `No such file or directory`. + +## The same address, legitimately + +Writing `plugins/aidd-dev/agents/wiring-proof.md` with the same address under the heading +`docs/ARCHITECTURE.md` sanctions: + +```md +# Skills you may invoke + +- `/aidd-vcs:01-commit` +``` + +The call was applied, with no output. The probe was removed in the same turn; +`git status --porcelain plugins/` is empty. + +## What this settles + +| Claim | Settled by | +| --- | --- | +| The hook is reachable from the matcher as wired | the refusal arrived at all | +| It decides before the write, not after | the refused path does not exist | +| The refusal names file, line and owning plugin | the verbatim text above | +| A legitimate permission list is not refused | the second call was applied silently | diff --git a/scripts/__tests__/architecture-hook.test.js b/scripts/__tests__/architecture-hook.test.js index babfb0fbe..fadb1ee17 100644 --- a/scripts/__tests__/architecture-hook.test.js +++ b/scripts/__tests__/architecture-hook.test.js @@ -495,6 +495,6 @@ test("a router refusal says what a citation is, not only that one is missing", ( assert.equal(result.status, 0); const reason = parseDenyReason(result.stdout); assert.match(reason, /02-refine\.md/); - assert.match(reason, /action column/); + assert.match(reason, /table header that reads Action/); assert.match(reason, /a word in prose does not count/); }); diff --git a/scripts/__tests__/architecture-rules.test.js b/scripts/__tests__/architecture-rules.test.js index 477bd8fa4..279ab24e1 100644 --- a/scripts/__tests__/architecture-rules.test.js +++ b/scripts/__tests__/architecture-rules.test.js @@ -370,3 +370,104 @@ test("a plural Actions header declares the action column too", () => { ); assert.deepEqual(violations, []); }); + +test("two action files sharing a stem are not both covered by one citation", () => { + const content = [ + "# Shared stem skill", + "", + "## Actions", + "", + "| # | Action | Role |", + "| --- | --- | --- |", + "| 01 | `plan` | Do it |", + "", + "## Transversal rules", + "", + "- Nothing.", + ].join("\n"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-shared-stem/SKILL.md", + content, + ["01-plan.md", "04-plan.md"] + ); + assert.equal(violations.length, 2); + assert.match(violations[0].message, /never names action file "01-plan\.md"/); + assert.match(violations[1].message, /never names action file "04-plan\.md"/); +}); + +test("a fenced example table inside the Actions section cites nothing", () => { + const content = [ + "# Fenced example skill", + "", + "## Actions", + "", + "| # | Action | Role |", + "| --- | --- | --- |", + "| 01 | `first` | Do it |", + "", + "An example of the shape a router takes:", + "", + "```md", + "| 02 | second | Then this |", + "```", + "", + "## Transversal rules", + "", + "- Nothing.", + ].join("\n"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-fenced/SKILL.md", + content, + ["01-first.md", "02-second.md"] + ); + assert.equal(violations.length, 1); + assert.match(violations[0].message, /never names action file "02-second\.md"/); +}); + +test("a fenced actions/.md path still cites, the way 10-todo does", () => { + const content = [ + "# Fenced path skill", + "", + "## Actions", + "", + "```md", + "actions/01-only.md", + "```", + "", + "## Transversal rules", + "", + "- Nothing.", + ].join("\n"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-fenced-path/SKILL.md", + content, + ["01-only.md"] + ); + assert.deepEqual(violations, []); +}); + +test("a fenced ## heading inside the Actions section does not end it early", () => { + const content = [ + "# Fenced heading skill", + "", + "## Actions", + "", + "```md", + "## Actions", + "```", + "", + "| # | Action | Role |", + "| --- | --- | --- |", + "| 01 | `first` | Do it |", + "", + "## Transversal rules", + "", + "- Nothing.", + ].join("\n"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-fenced-heading/SKILL.md", + content, + ["01-first.md"] + ); + assert.deepEqual(violations, []); +}); diff --git a/scripts/lib/architecture-rules.js b/scripts/lib/architecture-rules.js index d67f54524..ed7251bdc 100644 --- a/scripts/lib/architecture-rules.js +++ b/scripts/lib/architecture-rules.js @@ -163,6 +163,27 @@ function stripBackticks(text) { /** The `## Actions` section: from the line after its heading up to the next `##` heading (or * end of file). Returns null when no such heading exists. */ +/** The lines of a document with every fenced block blanked, keeping the line count intact so + * a reported number still points at the right line. A fence holds an example: a `## Actions` + * inside one is not the section, a `##` inside one does not end it, and a table inside one + * routes nothing. A fenced `actions/.md` path is the exception — `10-todo` cites its one + * action that way — so path citations are read from the unblanked lines. */ +function withoutFences(lines) { + let fence = null; + return lines.map((line) => { + const match = line.match(/^\s*(`{3,}|~{3,})/); + if (fence === null && match) { + fence = match[1][0]; + return ""; + } + if (fence !== null) { + if (match && match[1][0] === fence) fence = null; + return ""; + } + return line; + }); +} + function findActionsSection(lines) { const headingIdx = lines.findIndex((line) => ACTIONS_HEADING_RE.test(line)); if (headingIdx === -1) return null; @@ -215,8 +236,8 @@ function isTableSeparatorRow(cells) { * shapes rule two's own comment names, and nothing else. A word merely present in prose is not * collected here, on purpose: that is exactly what let a deleted table row hide behind * unrelated text that happened to contain the same word. */ -function citationsIn(sectionLines) { - const sectionText = sectionLines.join("\n"); +function citationsIn(sectionLines, rawSectionLines) { + const sectionText = (rawSectionLines ?? sectionLines).join("\n"); const citations = new Set(); // Only the column a table declares as its action column counts. A glossary, a trigger column @@ -270,7 +291,8 @@ function checkRouterCoherence(filePath, content, actionFileNames) { const names = actionFileNames || []; if (names.length === 0) return []; - const lines = toLines(content); + const rawLines = toLines(content); + const lines = withoutFences(rawLines); const section = findActionsSection(lines); if (!section) { @@ -287,14 +309,23 @@ function checkRouterCoherence(filePath, content, actionFileNames) { const violations = []; const sectionLines = lines.slice(section.startIdx, section.endIdx); - const sectionText = sectionLines.join("\n"); - const citations = citationsIn(sectionLines); + const citations = citationsIn(sectionLines, rawLines.slice(section.startIdx, section.endIdx)); + + // Two action files can share a stem — `01-plan.md` and `04-plan.md` both reduce to `plan`. + // One citation would then cover both, and deleting either row would go unnoticed, so a + // shared stem speaks for nobody and only the numbered name does. + const stemCount = new Map(); + for (const name of names) { + const stem = stemOf(name).toLowerCase(); + stemCount.set(stem, (stemCount.get(stem) ?? 0) + 1); + } for (const name of names) { const full = name.toLowerCase(); const fullNoExt = name.replace(/\.md$/i, "").toLowerCase(); const stem = stemOf(name).toLowerCase(); - if (citations.has(full) || citations.has(fullNoExt) || citations.has(stem)) continue; + const stemIsOwn = stemCount.get(stem) === 1; + if (citations.has(full) || citations.has(fullNoExt) || (stemIsOwn && citations.has(stem))) continue; violations.push({ file: filePath, From cf448aa26736c06581971675264cd86d64d0070a Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Fri, 18 Sep 2026 14:33:21 +0200 Subject: [PATCH 07/11] fix(framework): only the path shape reads through a fence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A sixth review found the raw view carried two exceptions where the contract named one. Reading a backticked `.md` through a fence let an example cite, so a deleted router row hid behind a fenced table — the exact defect blanking fences was written for. `10-todo` needs the path shape and nothing needs the other, so the exception is now one shape wide. A fence nobody closed used to blank the rest of the file, so a `## Actions` that is visibly there produced "has action files but no ## Actions section". An unclosed fence is not a fence. Two shapes stay generous and now say so in the spec instead of being found: a separator-less run below the router is read as that router resumed, and rule one reads addresses through fences. Both were measured against a width check that did not discriminate them — the reviewer's own glossary has the router's column count — so the check is gone and the limit is written down. Refs #250 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2 --- .../plan.md | 1 + .../spec.md | 2 +- scripts/__tests__/architecture-rules.test.js | 49 +++++++++++++++++++ scripts/lib/architecture-rules.js | 26 ++++++++-- 4 files changed, 73 insertions(+), 5 deletions(-) diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md index 7553619fd..5eb300349 100644 --- a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md @@ -42,3 +42,4 @@ | One plugin source was repaired, and it is the exception | `plugins/aidd-dev/skills/01-plan/actions/04-plan.md:16` read "declare it the same way `aidd-pm:04-spec` does", which is rule one's own violation in prose. The spec's non-goal keeps existing violations out of scope, and the hard constraint says a rule red on the tree is miscalibrated — so the one line that was genuinely wrong was fixed, and the seven in `00-onboard` were exempted with #883 behind them instead. No other plugin source is touched. | | A stem speaks for its action only when no sibling shares it | `01-plan.md` and `04-plan.md` both reduce to `plan`, so one row would cover both and deleting either would go unnoticed. A shared stem cites nobody; the numbered name still does. No skill in the tree has a collision today, which is why it had to be reasoned about rather than observed. | | A fenced block is an example, never a router | The section scan blanks fences before reading headings and tables, so a documented `## Actions`, a `##` inside the section, and an example table all stop steering the verdict. Path citations are read from the unblanked lines, because `10-todo` legitimately cites `actions/01-todo.md` inside a fence. | +| Only the path shape reads through a fence | Both shapes read the unblanked lines at first, which let a backticked `.md` inside a fenced example cite — the very hole blanking fences was for. `10-todo` needs the path shape and nothing needs the other, so the exception is one shape wide. | diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md index 0ffed7bce..9addb4bbf 100644 --- a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/spec.md @@ -14,7 +14,7 @@ An AI-authored edit that would break one of this repository's two named architec - The governed surface is the dispatch surface: a skill's `SKILL.md`, its actions and its references, and an agent's definition. A plugin's `assets/` hold content shown to a reader, not dispatch, and are out of the guard's reach by this rule rather than by an unstated path filter. - Naming that `docs/ARCHITECTURE.md` declares legitimate stays silent: an agent's permission list and an orchestration reference are responsibility maps and name their provider canonically. Flagging either is a defect of the guard, not of the tree. - Every refusal names the file, the line, and the plugin that owns the addressed capability, in terms a reader can act on without opening the rule document. -- The guard is silent on the repository as it stands. A fenced block holds an example, never a router: a `## Actions` inside one is not the section, a `##` inside one does not end it, and a table inside one cites nothing. A fenced `actions/.md` path is the one exception, because `aidd-dev:10-todo` cites its only action that way. A rule that reports a violation in the current tree is miscalibrated, not vindicated. +- The guard is silent on the repository as it stands. A fenced block holds an example, never a router: a `## Actions` inside one is not the section, a `##` inside one does not end it, and a table inside one cites nothing. A fenced `actions/.md` path is the one exception, because `aidd-dev:10-todo` cites its only action that way — so a fenced example writing that shape does cite, and can mask a row deleted elsewhere. Two more shapes are read generously and stay that way: a run of table rows with no separator under it is taken for the table above it resumed after a blank line, so a separator-less glossary written just below the router would donate its cells; and rule one reads addresses through fences, so a governed file teaching a counter-example is refused. Neither shape is a table a renderer accepts, and a refusal nobody can act on is the worse failure. A rule that reports a violation in the current tree is miscalibrated, not vindicated. - The rules are exercised by purpose-built fixtures. No historical code from #406 is used as a fixture. - Each rule is proved by a fixture that breaks it and turns exactly the test named for that rule red, and by a fixture of legitimate naming that stays green. diff --git a/scripts/__tests__/architecture-rules.test.js b/scripts/__tests__/architecture-rules.test.js index 279ab24e1..9eb7df1b8 100644 --- a/scripts/__tests__/architecture-rules.test.js +++ b/scripts/__tests__/architecture-rules.test.js @@ -471,3 +471,52 @@ test("a fenced ## heading inside the Actions section does not end it early", () ); assert.deepEqual(violations, []); }); + +test("a backticked file name inside a fenced example does not cite", () => { + const fence = "```"; + const content = [ + "# Fenced filename skill", + "", + "## Actions", + "", + `${fence}md`, + "| Action | When |", + "| --- | --- |", + `| ${"`"}02-gone.md${"`"} | b |`, + fence, + "", + "| Action | When |", + "| --- | --- |", + "| 01-keep | a |", + "", + "## Transversal rules", + "", + "- Nothing.", + ].join("\n"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-fenced-name/SKILL.md", + content, + ["01-keep.md", "02-gone.md"] + ); + assert.equal(violations.length, 1); + assert.match(violations[0].message, /never names action file "02-gone\.md"/); +}); + +test("a fence nobody closed is not read as a fence at all", () => { + const content = [ + "# Unterminated fence skill", + "", + "## Actions", + "", + "```md", + "| # | Action | Role |", + "| --- | --- | --- |", + "| 01 | `first` | Do it |", + ].join("\n"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-unterminated/SKILL.md", + content, + ["01-first.md"] + ); + assert.deepEqual(violations, []); +}); diff --git a/scripts/lib/architecture-rules.js b/scripts/lib/architecture-rules.js index ed7251bdc..127df9cf3 100644 --- a/scripts/lib/architecture-rules.js +++ b/scripts/lib/architecture-rules.js @@ -169,6 +169,17 @@ function stripBackticks(text) { * routes nothing. A fenced `actions/.md` path is the exception — `10-todo` cites its one * action that way — so path citations are read from the unblanked lines. */ function withoutFences(lines) { + // An unterminated fence would blank the rest of the file, hiding a `## Actions` that is + // visibly there and producing a refusal nobody can act on. Treat it as not a fence at all. + let open = null; + for (const line of lines) { + const match = line.match(/^\s*(`{3,}|~{3,})/); + if (!match) continue; + if (open === null) open = match[1][0]; + else if (match[1][0] === open) open = null; + } + if (open !== null) return lines; + let fence = null; return lines.map((line) => { const match = line.match(/^\s*(`{3,}|~{3,})/); @@ -237,7 +248,11 @@ function isTableSeparatorRow(cells) { * collected here, on purpose: that is exactly what let a deleted table row hide behind * unrelated text that happened to contain the same word. */ function citationsIn(sectionLines, rawSectionLines) { - const sectionText = (rawSectionLines ?? sectionLines).join("\n"); + const blankedText = sectionLines.join("\n"); + // Only the `actions/.md` path shape is read through fences, because `10-todo` cites its + // one action that way. Every other shape reads the blanked view: a backticked file name inside + // a fenced example is an example, and letting it cite reopened the hole fences were blanked for. + const pathText = (rawSectionLines ?? sectionLines).join("\n"); const citations = new Set(); // Only the column a table declares as its action column counts. A glossary, a trigger column @@ -251,7 +266,10 @@ function citationsIn(sectionLines, rawSectionLines) { // A header row is followed by its `| --- |` separator. A run of rows without one is the // same table resumed after a blank line, and it keeps the column its header declared — - // otherwise a purely cosmetic edit refuses every row below the blank line. + // otherwise a purely cosmetic edit refuses every row below the blank line. The cost is + // stated in the spec: a separator-less run following the router is read as part of it, + // so a glossary written without a separator would donate its cells. Neither shape is a + // table any renderer accepts, and the false refusal is the worse of the two. if (isNewTable) { actionColumn = rows[0].findIndex((cell) => ACTION_HEADER_RE.test(stripBackticks(cell))); } @@ -266,13 +284,13 @@ function citationsIn(sectionLines, rawSectionLines) { ACTION_PATH_RE.lastIndex = 0; let pathMatch; - while ((pathMatch = ACTION_PATH_RE.exec(sectionText)) !== null) { + while ((pathMatch = ACTION_PATH_RE.exec(pathText)) !== null) { citations.add(pathMatch[1].toLowerCase()); } BACKTICKED_MD_RE.lastIndex = 0; let mdMatch; - while ((mdMatch = BACKTICKED_MD_RE.exec(sectionText)) !== null) { + while ((mdMatch = BACKTICKED_MD_RE.exec(blankedText)) !== null) { citations.add(mdMatch[1].toLowerCase()); } From 997207b13d48655ba2c554d1a8a8ce8d1f856b35 Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Fri, 18 Sep 2026 22:32:54 +0200 Subject: [PATCH 08/11] test(framework): rule two decided by one property over every router shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six review rounds found the same class of defect one shape at a time: a separator written with two dashes, a plural header, a blank line splitting the table, a fenced example citing like a router, prose carrying a stem. Each got its own test after the fact, which is how the next shape gets missed rather than found. One property replaces the hunt: a section citing every action it provides is silent, and dropping any one citation yields exactly that one violation. It runs over 432 generated shapes — three headers, three separators, four citation forms, six kinds of surrounding noise, split and unsplit — and over every router in the tree, removing each of the 152 citations in turn. The shapes are enumerated, not random. A guard that fails on a seed nobody can reproduce is worse than no guard, and the repository root carries six dev dependencies, none of them a generator library — `fast-check` lives in `cli/`, which this suite does not run in. Five of the six engine mutations these rounds fixed are killed by the property alone. The sixth, a stem shared by two action files, cannot live in the matrix because a collision changes the expected verdict; it keeps the unit test written for it. Refs #250 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2 --- .../architecture-rules.property.test.js | 248 ++++++++++++++++++ 1 file changed, 248 insertions(+) create mode 100644 scripts/__tests__/architecture-rules.property.test.js diff --git a/scripts/__tests__/architecture-rules.property.test.js b/scripts/__tests__/architecture-rules.property.test.js new file mode 100644 index 000000000..ec8434428 --- /dev/null +++ b/scripts/__tests__/architecture-rules.property.test.js @@ -0,0 +1,248 @@ +const assert = require("node:assert/strict"); +const fs = require("node:fs"); +const path = require("node:path"); +const test = require("node:test"); + +const { checkRouterCoherence } = require("../lib/architecture-rules.js"); + +const ROOT = path.resolve(__dirname, "../.."); +const TICK = "`"; + +/** + * Rule two decided by one property, over every shape a router is written in, instead of one + * test per shape someone happened to think of. + * + * The property: a section that cites every action file it provides is silent, and removing any + * one of those citations yields exactly one violation — the one naming that file. + * + * The shapes are generated rather than random. A guard that fails on a seed nobody can reproduce + * is worse than no guard, and the repository's own root holds six dev dependencies, none of them + * a generator library. The matrix below is small enough to enumerate and wide enough to cover + * what six rounds of review found by hand. + */ + +const HEADERS = { + bare: ["| Action | Does |"], + numbered: ["| # | Action | Role |"], + plural: ["| # | Actions | Role |"], +}; + +const SEPARATORS = { + three: "---", + two: "--", + aligned: ":---", +}; + +/** How a row names the action it routes to. Each is a citation shape the engine accepts. */ +const CITATIONS = { + stem: (name) => stemOf(name), + backtickedStem: (name) => `${TICK}${stemOf(name)}${TICK}`, + numberedName: (name) => name.replace(/\.md$/, ""), + backtickedFile: (name) => `${TICK}${name}${TICK}`, +}; + +/** Text placed around the router that must not change any verdict. */ +const NOISE = { + none: () => [], + prose: (names) => [ + "", + `Run them in order. The ${stemOf(names[0])} step is the culmination, and ${stemOf( + names[names.length - 1] + )} closes it.`, + ], + glossaryTable: () => ["", "| Term | Synonym |", "| --- | --- |", "| step | move |"], + fencedExample: (names) => [ + "", + "An example of the shape a router takes:", + "", + "```md", + "| # | Action | Role |", + "| --- | --- | --- |", + `| 99 | ${stemOf(names[names.length - 1])} | an example, not a router |`, + "```", + ], + /** The same example, citing a file name rather than a stem: the shape that reads through a + * fence for the path form and must not for this one. */ + fencedFilename: (names) => [ + "", + "```md", + "| # | Action | Role |", + "| --- | --- | --- |", + `| 99 | ${TICK}${names[names.length - 1]}${TICK} | an example, not a router |`, + "```", + ], + trailingParagraph: () => ["", "Before running an action, read its file in `actions/`."], +}; + +function stemOf(name) { + return name.replace(/\.md$/, "").replace(/^\d+-/, ""); +} + +/** One `SKILL.md` body, built from the matrix. `omit` is the action file left uncited. */ +function buildSkill({ header, separator, citation, noise, names, omit, splitAfter }) { + const headerRow = HEADERS[header][0]; + const columns = headerRow.split("|").filter((cell) => cell.trim() !== "").length; + const separatorRow = `| ${Array.from({ length: columns }, () => SEPARATORS[separator]).join(" | ")} |`; + + const rows = []; + names.forEach((name, index) => { + if (name === omit) return; + if (splitAfter !== undefined && index === splitAfter) rows.push(""); + const cited = CITATIONS[citation](name); + rows.push(columns === 2 ? `| ${cited} | does it |` : `| 0${index + 1} | ${cited} | does it |`); + }); + + return [ + "# Generated skill", + "", + "## Actions", + "", + headerRow, + separatorRow, + ...rows, + ...NOISE[noise](names), + "", + "## Transversal rules", + "", + "- Nothing.", + ].join("\n"); +} + +const NAMES = ["01-first.md", "02-second.md", "03-third.md"]; +const FILE = "plugins/aidd-fixture-a/skills/01-generated/SKILL.md"; + +function shapes() { + const out = []; + for (const header of Object.keys(HEADERS)) { + for (const separator of Object.keys(SEPARATORS)) { + for (const citation of Object.keys(CITATIONS)) { + for (const noise of Object.keys(NOISE)) { + for (const splitAfter of [undefined, 1]) { + out.push({ header, separator, citation, noise, splitAfter }); + } + } + } + } + } + return out; +} + +function describe(shape, omit) { + const split = shape.splitAfter === undefined ? "unsplit" : "split by a blank line"; + return `${shape.header} header, ${shape.separator} separator, ${shape.citation} citation, ${shape.noise} noise, ${split}${ + omit ? `, omitting ${omit}` : "" + }`; +} + +test("every shape that cites all its actions is silent", () => { + const failures = []; + for (const shape of shapes()) { + const content = buildSkill({ ...shape, names: NAMES, omit: null }); + const violations = checkRouterCoherence(FILE, content, NAMES); + if (violations.length !== 0) { + failures.push(`${describe(shape)} => ${violations.map((v) => v.message).join(" | ")}`); + } + } + assert.deepEqual(failures, [], `a router shape was refused although it cites every action:\n${failures.join("\n")}`); +}); + +test("every shape that drops one citation yields exactly that one violation", () => { + const failures = []; + for (const shape of shapes()) { + for (const omit of NAMES) { + const content = buildSkill({ ...shape, names: NAMES, omit }); + const violations = checkRouterCoherence(FILE, content, NAMES); + if (violations.length !== 1) { + failures.push(`${describe(shape, omit)} => ${violations.length} violations`); + continue; + } + if (!violations[0].message.includes(omit)) { + failures.push(`${describe(shape, omit)} => named ${violations[0].message}`); + } + } + } + assert.deepEqual(failures, [], `a dropped citation was missed or misattributed:\n${failures.join("\n")}`); +}); + +test("the shape matrix is wide enough to be worth running", () => { + // A generator that silently produces nothing passes every property above. Pin the count so a + // matrix that collapses fails here rather than going quietly green. + // 3 headers x 3 separators x 4 citation shapes x 6 noises x 2 split positions. + assert.equal(shapes().length, 432); +}); + +/** + * The same property against the repository's own routers, which is where it has to hold: every + * action the tree provides is cited, and removing the line that cites it is caught. + */ +function realSkills() { + const skills = []; + const pluginsDir = path.join(ROOT, "plugins"); + for (const owner of fs.readdirSync(pluginsDir)) { + const skillsDir = path.join(pluginsDir, owner, "skills"); + if (!fs.existsSync(skillsDir)) continue; + for (const skill of fs.readdirSync(skillsDir)) { + const skillMd = path.join(skillsDir, skill, "SKILL.md"); + const actionsDir = path.join(skillsDir, skill, "actions"); + if (!fs.existsSync(skillMd) || !fs.existsSync(actionsDir)) continue; + const names = fs.readdirSync(actionsDir).filter((name) => name.endsWith(".md")); + if (names.length === 0) continue; + skills.push({ + relPath: `plugins/${owner}/skills/${skill}/SKILL.md`, + content: fs.readFileSync(skillMd, "utf8"), + names, + }); + } + } + return skills; +} + +/** The skill's content with every table row citing `name` removed. */ +function withoutCitationOf(content, name) { + const stem = stemOf(name).toLowerCase(); + const noExt = name.replace(/\.md$/, "").toLowerCase(); + return content + .split("\n") + .filter((line) => { + if (!/^\s*\|/.test(line)) return true; + const cells = line + .trim() + .replace(/^\|/, "") + .replace(/\|$/, "") + .split("|") + .map((cell) => cell.trim().replace(/^`|`$/g, "").toLowerCase()); + return !cells.includes(stem) && !cells.includes(noExt) && !cells.includes(name.toLowerCase()); + }) + .join("\n"); +} + +test("every router in the tree cites every action it provides", () => { + const failures = []; + for (const { relPath, content, names } of realSkills()) { + const violations = checkRouterCoherence(relPath, content, names); + if (violations.length > 0) failures.push(`${relPath} => ${violations.map((v) => v.message).join(" | ")}`); + } + assert.deepEqual(failures, [], failures.join("\n")); +}); + +test("removing a real router's citation is caught, for every action in the tree", () => { + // `aidd-dev:10-todo` names its one action as a fenced path rather than a table row, so there + // is no row to remove and nothing to catch. It is the one documented exception. + const FENCED_PATH_SKILL = "plugins/aidd-dev/skills/10-todo/SKILL.md"; + const missed = []; + let checked = 0; + + for (const { relPath, content, names } of realSkills()) { + if (relPath === FENCED_PATH_SKILL) continue; + for (const name of names) { + checked += 1; + const violations = checkRouterCoherence(relPath, withoutCitationOf(content, name), names); + if (!violations.some((violation) => violation.message.includes(name))) { + missed.push(`${relPath} ${name}`); + } + } + } + + assert.deepEqual(missed, [], `a deleted citation went unnoticed:\n${missed.join("\n")}`); + assert.ok(checked > 100, `expected the tree to offer more than 100 citations to remove, got ${checked}`); +}); From 9a3248b68b7e8d85563732ebc5744eae32923dd3 Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Fri, 18 Sep 2026 23:00:02 +0200 Subject: [PATCH 09/11] refactor(framework): the guard reads without its commentary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six rounds of review left the rules correct and unreadable: 36% of the engine was comment, and four functions had accumulated a branch per round without anyone designing them together. Names carry what the prose used to. `exemptAgentLines` is `permissionListLines`, `findAddresses` is `addressesIn`, and the three citation shapes are three functions instead of one paragraph and three inline loops. `withoutFences` no longer repeats its fence detection twice, `classifyFile` destructures the path instead of indexing it, and the violation object is built in one place rather than three. What stays is why, never what: the exemption that expires with #883, the literal splice that `String.replace` would corrupt, the project root a readdir must resolve against. The reasoning behind each rule already lives in the plan's decisions table, and the header now points there instead of restating it. Comments fall from 36% to 9% in the engine and 30% to 11% in the hook, and one function remains above the size threshold instead of six. Behaviour is unchanged: the same 506 tests pass, and nine mutations — separator, plural header, fences, backticked file name, resumed table, shared stem, action column, and both exemptions — each still turn red. 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 | 180 ++++----- scripts/lib/architecture-rules.js | 469 ++++++++++------------ 2 files changed, 287 insertions(+), 362 deletions(-) diff --git a/.claude/hooks/check-architecture-rules.js b/.claude/hooks/check-architecture-rules.js index 609de1a96..406538cb2 100755 --- a/.claude/hooks/check-architecture-rules.js +++ b/.claude/hooks/check-architecture-rules.js @@ -1,30 +1,35 @@ #!/usr/bin/env node /** - * PreToolUse guard for Write, Edit and MultiEdit: refuses an AI-authored edit that would leave a plugin's - * dispatch surface breaking one of the two named architecture rules (issue #250) — cross-plugin - * orthogonality and router coherence. The rules themselves live in `scripts/lib/architecture- - * rules.js`, a pure engine this script is the only caller of at write time. + * 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`; this script only supplies their inputs and speaks the + * host's refusal. * - * Fails open on purpose: any unrecognised shape, unparseable payload, unreconstructable edit, or - * path outside the governed surface exits 0 with no output. This hook gates every write tool call - * in the repository, so a crash here must never block unrelated work. + * It fails open at every step. This hook gates every write in the repository, so an unreadable + * payload, an unknown tool, an edit it cannot reconstruct or a crash must let the write through + * rather than halt unrelated work. */ const fs = require("node:fs"); const path = require("node:path"); -function loadEngine() { +const WRITE_TOOLS = new Set(["Write", "Edit", "MultiEdit"]); +const PROCEED = 0; + +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"; + +function architectureRules() { try { - // Fixed relative path: this hook always lives two levels below the repo root, at - // `.claude/hooks/`, regardless of CLAUDE_PROJECT_DIR or the process cwd. return require(path.resolve(__dirname, "..", "..", "scripts", "lib", "architecture-rules.js")); } catch { return null; } } -function readPayload() { +function payloadFromStdin() { try { const parsed = JSON.parse(fs.readFileSync(0, "utf8")); return parsed && typeof parsed === "object" ? parsed : null; @@ -33,84 +38,75 @@ function readPayload() { } } -/** Repository-relative, forward-slashed path, or null when it resolves outside the project. */ -function toRepoRelative(filePath, root) { +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 { - const rel = path.relative(root, path.resolve(root, filePath)); - if (rel === "" || rel.startsWith("..") || path.isAbsolute(rel)) return null; - return rel.split(path.sep).join("/"); + return fs.readFileSync(absolutePath, "utf8"); } catch { return null; } } -/** Splices `newString` in for `oldString` literally — never through `String.prototype.replace`, - * whose replacement string treats `$&`, `` $` ``, `$'` and `$1`-style tokens specially even - * when the search pattern is a plain string, corrupting a `new_string` that happens to contain - * one. */ -function applyEdit(current, oldString, newString, replaceAll) { +/** + * 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 (!current.includes(oldString)) return null; - if (replaceAll) return current.split(oldString).join(newString); + if (!content.includes(oldString)) return null; + if (replaceAll) return content.split(oldString).join(newString); - const idx = current.indexOf(oldString); - return current.slice(0, idx) + newString + current.slice(idx + oldString.length); + const at = content.indexOf(oldString); + return content.slice(0, at) + newString + content.slice(at + oldString.length); } -/** The prospective file content, or null when it cannot be determined (Write with no string - * content, an Edit whose old_string is absent, or the current file cannot be read). */ -function prospectiveContent(toolName, toolInput) { - if (toolName === "Write") { - return typeof toolInput.content === "string" ? toolInput.content : null; - } - if (toolName !== "Edit" && toolName !== "MultiEdit") return null; +function editedContent(absolutePath, edits) { + let content = readFile(absolutePath); + if (content === null) return null; - let current; - try { - current = fs.readFileSync(toolInput.file_path, "utf8"); - } catch { - 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; +} - if (toolName === "Edit") { - return applyEdit(current, toolInput.old_string, toolInput.new_string, Boolean(toolInput.replace_all)); +/** 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]); - // MultiEdit applies its edits in order, each to the result of the one before. - if (!Array.isArray(toolInput.edits) || toolInput.edits.length === 0) return null; - for (const edit of toolInput.edits) { - if (!edit || typeof edit !== "object") return null; - current = applyEdit(current, edit.old_string, edit.new_string, Boolean(edit.replace_all)); - if (current === null) return null; - } - return current; + const { edits } = toolInput; + if (!Array.isArray(edits) || edits.length === 0) return null; + return editedContent(absolutePath, edits); } -/** The action file names for a SKILL.md path, read from its sibling `actions/` directory. - * Undefined for anything else, matching the engine's own contract. */ -function actionFileNamesFor(relPath, absPath) { - if (path.basename(relPath) !== "SKILL.md") return undefined; - const actionsDir = path.join(path.dirname(absPath), "actions"); +/** Rule two needs the skill's action files; the engine never reads them itself. */ +function actionFileNames(relativePath, absolutePath) { + if (path.basename(relativePath) !== "SKILL.md") return undefined; try { - return fs.readdirSync(actionsDir).filter((name) => name.endsWith(".md")); + return fs.readdirSync(path.join(path.dirname(absolutePath), "actions")).filter((name) => name.endsWith(".md")); } catch { return []; } } -function fixFor(rule, plugin) { +function howToFix({ rule, plugin }) { return rule === "orthogonality" ? `name the concept ${plugin} owns instead of addressing it directly` - : 'cite every action file the skill provides in its "## Actions" section. A citation is 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'; -} - -function denyReason(violations) { - return violations - .map((v) => `${v.message}. Fix: ${fixFor(v.rule, v.plugin)}.`) - .join("\n"); + : `cite every action file the skill provides in its "## Actions" section. A citation is ${CITATION_SHAPES}`; } -function deny(reason) { +function refuse(violations) { + const reason = violations.map((v) => `${v.message}. Fix: ${howToFix(v)}.`).join("\n"); process.stdout.write( JSON.stringify({ hookSpecificOutput: { @@ -122,50 +118,42 @@ function deny(reason) { ); } -function main() { - const engine = loadEngine(); - if (!engine) return 0; - - const payload = readPayload(); - if (!payload) return 0; +function violationsFor(engine, relativePath, content, actions) { + try { + const found = engine.checkArchitecture(relativePath, content, actions); + return Array.isArray(found) ? found : []; + } catch { + return []; + } +} - const toolName = payload.tool_name; - if (toolName !== "Write" && toolName !== "Edit" && toolName !== "MultiEdit") return 0; +function main() { + const engine = architectureRules(); + const payload = payloadFromStdin(); + if (!engine || !payload) return PROCEED; - const toolInput = payload.tool_input; - if (!toolInput || typeof toolInput.file_path !== "string" || toolInput.file_path === "") return 0; + 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 once, against the project root, and reused for every filesystem access below — a - // relative `file_path` must never be read against the process's own cwd, which can differ - // from CLAUDE_PROJECT_DIR and would otherwise silently empty a readdir this hook depends on. + // 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 relPath = toRepoRelative(toolInput.file_path, root); - if (!relPath) return 0; - - const info = engine.classifyFile(relPath); - if (!info) return 0; - - const absPath = path.resolve(root, toolInput.file_path); - const content = prospectiveContent(toolName, { ...toolInput, file_path: absPath }); - if (content === null) return 0; + const absolutePath = path.resolve(root, toolInput.file_path); + const relativePath = repoRelative(absolutePath, root); + if (!relativePath || !engine.classifyFile(relativePath)) return PROCEED; - const actionFileNames = actionFileNamesFor(relPath, absPath); - - let violations; - try { - violations = engine.checkArchitecture(relPath, content, actionFileNames); - } catch { - return 0; - } + const content = prospectiveContent(toolName, toolInput, absolutePath); + if (content === null) return PROCEED; - if (!Array.isArray(violations) || violations.length === 0) return 0; + const violations = violationsFor(engine, relativePath, content, actionFileNames(relativePath, absolutePath)); + if (violations.length > 0) refuse(violations); - deny(denyReason(violations)); - return 0; + return PROCEED; } try { process.exitCode = main(); } catch { - process.exitCode = 0; + process.exitCode = PROCEED; } diff --git a/scripts/lib/architecture-rules.js b/scripts/lib/architecture-rules.js index 127df9cf3..a7b76ac06 100644 --- a/scripts/lib/architecture-rules.js +++ b/scripts/lib/architecture-rules.js @@ -1,229 +1,178 @@ /** - * The two named architecture rules (issue #250), as pure functions of a repository-relative - * path, the prospective file content, and — for a SKILL.md — the names of the skill's action - * files. Never reads the filesystem: the caller supplies every input, so a hook and a test can - * hand it the same shape without either touching disk through it. + * The two architecture rules of issue #250, as pure functions: the caller supplies the path, the + * prospective content and a SKILL.md's action file names, and nothing here reads the filesystem. * - * Rule one, cross-plugin orthogonality: a plugin's dispatch surface must not name a sibling - * plugin by a hardcoded address. - * Rule two, router coherence: a skill's `## Actions` section must name every action file that - * skill provides. It checks one direction only — an action file the section never cites. It - * used to also flag the opposite direction, a citation with no action file behind it, but that - * is indistinguishable from a citation written seconds before the file it names, which is the - * order this project's own skill generator documents (create the action, then have the router - * name it — or name it first, then create the file). Enforcing it made adding an action to an - * existing skill impossible in either order. The direction that remains is decidable at any - * moment content is proposed, regardless of what gets written next. + * Why each rule is shaped the way it is — the one-directional router check, the exemptions, the + * two table shapes read generously — is in the decisions table of + * `aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md`. */ "use strict"; const ORCHESTRATOR_PLUGIN = "aidd-orchestrator"; -// Temporary: `00-onboard`'s reference menus are routing menus whose addresses are what the -// skill hands a person to type, not a hardcoded sibling provider — so orthogonality stays -// silent on this one skill directory. See #883, the follow-up issue on 00-onboard runtime -// discovery. +/** Expires with #883, which makes 00-onboard resolve its providers at runtime. */ const ONBOARD_EXEMPT_PREFIX = "plugins/aidd-context/skills/00-onboard/"; -const HEADING_RE = /^#{1,6}\s/; -const SKILLS_INVOKE_HEADING_RE = /^#{1,6}\s+Skills you may invoke\s*$/i; -const ACTIONS_HEADING_RE = /^##\s+Actions\s*$/i; -const SECOND_LEVEL_HEADING_RE = /^##\s+/; -// Matches `/plugin:name`, `@plugin:name`, and the bare `plugin:name` form. The lookbehind keeps -// the left edge from firing inside a longer identifier — `some-aidd-dev:01-x` never matches, -// because the character right before "aidd-" (a hyphen, a letter, a digit, or another `/`/`@`) -// rules it out — while a backtick, space, or start of line still lets a bare address through. -const ADDRESS_RE = /(?.md` path, or a backticked `.md` filename. -// Deliberately narrow — a word loose in prose is never a citation, which is what let a deleted -// table row hide behind unrelated text that happened to contain the same word. -const ACTION_PATH_RE = /actions\/([A-Za-z0-9._-]+)\.md/g; -const BACKTICKED_MD_RE = /`([A-Za-z0-9][A-Za-z0-9._-]*\.md)`/g; -/** A header cell that declares the action column. Exact on purpose: "Next action" heads a - * routing hint, not a dispatch, and reading it as one lets a deleted row hide behind it. */ -const ACTION_HEADER_RE = /^actions?$/i; -const CITATION_TOKEN_RE = /^[A-Za-z0-9][A-Za-z0-9._-]*$/; +const ANY_HEADING = /^#{1,6}\s/; +const PERMISSION_LIST_HEADING = /^#{1,6}\s+Skills you may invoke\s*$/i; +const ACTIONS_HEADING = /^##\s+Actions\s*$/i; +const SECOND_LEVEL_HEADING = /^##\s+/; +const FENCE_MARKER = /^\s*(`{3,}|~{3,})/; +const TABLE_ROW = /^\s*\|/; + +const PLUGIN_ADDRESS = /(? { - ADDRESS_RE.lastIndex = 0; +// --- Rule one: cross-plugin orthogonality -------------------------------------------------- + +function addressesIn(content) { + const addresses = []; + toLines(content).forEach((line, index) => { + PLUGIN_ADDRESS.lastIndex = 0; let match; - while ((match = ADDRESS_RE.exec(line)) !== null) { - matches.push({ line: idx + 1, plugin: match[1], text: match[0] }); + while ((match = PLUGIN_ADDRESS.exec(line)) !== null) { + addresses.push({ line: index + 1, plugin: match[1], text: match[0] }); } }); - return matches; + return addresses; } -/** 1-indexed lines under an agent's `# Skills you may invoke` heading, any level, until the - * next heading of any level. */ -function exemptAgentLines(lines) { - const exempt = new Set(); - let inSection = false; - lines.forEach((line, idx) => { - if (HEADING_RE.test(line)) { - inSection = SKILLS_INVOKE_HEADING_RE.test(line); - return; - } - if (inSection) exempt.add(idx + 1); +/** An agent's `# Skills you may invoke` list names its providers on purpose. */ +function permissionListLines(lines) { + const listed = new Set(); + let inList = false; + lines.forEach((line, index) => { + if (ANY_HEADING.test(line)) inList = PERMISSION_LIST_HEADING.test(line); + else if (inList) listed.add(index + 1); }); - return exempt; + return listed; +} + +function isExemptFromOrthogonality(filePath, owner) { + return owner === ORCHESTRATOR_PLUGIN || filePath.startsWith(ONBOARD_EXEMPT_PREFIX); } -/** - * Rule one: a dispatch surface never names a sibling plugin by a hardcoded address. - */ function checkOrthogonality(filePath, content) { - const info = classifyFile(filePath); - if (!info || info.owner === ORCHESTRATOR_PLUGIN) return []; - if (filePath.startsWith(ONBOARD_EXEMPT_PREFIX)) return []; + const surface = classifyFile(filePath); + if (!surface || isExemptFromOrthogonality(filePath, surface.owner)) return []; const lines = toLines(content); - const exempt = info.kind === "agent" ? exemptAgentLines(lines) : new Set(); - - const violations = []; - for (const address of findAddresses(content)) { - if (address.plugin === info.owner) continue; - if (exempt.has(address.line)) continue; - violations.push({ - file: filePath, - line: address.line, - plugin: address.plugin, - rule: "orthogonality", - message: `${filePath}:${address.line} addresses sibling plugin "${address.plugin}" via "${address.text}"`, - }); - } - return violations; + const exemptLines = surface.kind === "agent" ? permissionListLines(lines) : new Set(); + + return addressesIn(content) + .filter(({ plugin, line }) => plugin !== surface.owner && !exemptLines.has(line)) + .map(({ plugin, line, text }) => + violation( + "orthogonality", + filePath, + line, + plugin, + `${filePath}:${line} addresses sibling plugin "${plugin}" via "${text}"` + ) + ); } -/** Strips a leading `NN-` and a trailing `.md` — "01-frame.md" -> "frame". */ -function stemOf(fileName) { - return fileName.replace(/\.md$/i, "").replace(/^[0-9]+-/, ""); -} +// --- Reading a `## Actions` section -------------------------------------------------------- -/** Strips one layer of matching backticks around a trimmed cell or token, if present. */ -function stripBackticks(text) { - const trimmed = text.trim(); - const match = /^`(.*)`$/.exec(trimmed); - return match ? match[1].trim() : trimmed; +function fenceCharacter(line) { + return line.match(FENCE_MARKER)?.[1][0] ?? null; } -/** The `## Actions` section: from the line after its heading up to the next `##` heading (or - * end of file). Returns null when no such heading exists. */ -/** The lines of a document with every fenced block blanked, keeping the line count intact so - * a reported number still points at the right line. A fence holds an example: a `## Actions` - * inside one is not the section, a `##` inside one does not end it, and a table inside one - * routes nothing. A fenced `actions/.md` path is the exception — `10-todo` cites its one - * action that way — so path citations are read from the unblanked lines. */ -function withoutFences(lines) { - // An unterminated fence would blank the rest of the file, hiding a `## Actions` that is - // visibly there and producing a refusal nobody can act on. Treat it as not a fence at all. +function hasUnterminatedFence(lines) { let open = null; for (const line of lines) { - const match = line.match(/^\s*(`{3,}|~{3,})/); - if (!match) continue; - if (open === null) open = match[1][0]; - else if (match[1][0] === open) open = null; + const marker = fenceCharacter(line); + if (marker === null) continue; + open = open === null ? marker : open === marker ? null : open; } - if (open !== null) return lines; + return open !== null; +} + +/** + * Every fenced line blanked, line count intact so a reported number still points at its line. A + * fence holds an example, never a router — except an unterminated one, which is not a fence. + */ +function withoutFences(lines) { + if (hasUnterminatedFence(lines)) return lines; - let fence = null; + let open = null; return lines.map((line) => { - const match = line.match(/^\s*(`{3,}|~{3,})/); - if (fence === null && match) { - fence = match[1][0]; - return ""; - } - if (fence !== null) { - if (match && match[1][0] === fence) fence = null; - return ""; - } - return line; + const marker = fenceCharacter(line); + if (open === null && marker === null) return line; + if (open === null) open = marker; + else if (marker === open) open = null; + return ""; }); } -function findActionsSection(lines) { - const headingIdx = lines.findIndex((line) => ACTIONS_HEADING_RE.test(line)); - if (headingIdx === -1) return null; +function actionsSection(lines) { + const headingIndex = lines.findIndex((line) => ACTIONS_HEADING.test(line)); + if (headingIndex === -1) return null; - let endIdx = lines.length; - for (let i = headingIdx + 1; i < lines.length; i += 1) { - if (SECOND_LEVEL_HEADING_RE.test(lines[i])) { - endIdx = i; - break; - } - } - return { headingLine: headingIdx + 1, startIdx: headingIdx + 1, endIdx }; + const after = lines.slice(headingIndex + 1); + const nextHeading = after.findIndex((line) => SECOND_LEVEL_HEADING.test(line)); + + return { + headingLine: headingIndex + 1, + startIndex: headingIndex + 1, + endIndex: nextHeading === -1 ? lines.length : headingIndex + 1 + nextHeading, + }; } -/** A table row's cells, trimmed, with the leading and trailing empty cell a `| a | b |` line - * produces stripped off. */ -function splitTableCells(line) { - const trimmed = line.trim(); - const withoutEdges = trimmed.replace(/^\|/, "").replace(/\|$/, ""); - return withoutEdges.split("|").map((cell) => cell.trim()); +function tableCells(line) { + return line.trim().replace(/^\|/, "").replace(/\|$/, "").split("|").map((cell) => cell.trim()); } -/** The contiguous runs of table rows in a section: each run is one table, so one table's - * header never speaks for the next one's columns. */ -function tableBlocks(sectionLines) { +/** Contiguous runs of table rows: one table's header never speaks for the next one's columns. */ +function tableBlocks(lines) { const blocks = []; let current = []; - for (const line of sectionLines) { - if (/^\s*\|/.test(line)) { + + for (const line of lines) { + if (TABLE_ROW.test(line)) { current.push(line); continue; } @@ -233,131 +182,119 @@ function tableBlocks(sectionLines) { } } if (current.length > 0) blocks.push(current); + return blocks; } -/** A `| --- | --- |` row: every cell is dashes and colons, so it declares no column and cites - * nothing. Two dashes count — `00-onboard` writes `| -- |` and GitHub renders it. */ -function isTableSeparatorRow(cells) { - return cells.length > 0 && cells.every((cell) => /^:?-+:?$/.test(cell)); +function isSeparatorRow(cells) { + return cells.length > 0 && cells.every((cell) => SEPARATOR_CELL.test(cell)); } -/** Every citation the "## Actions" section makes to an action file: a table cell that reads as - * a plain name, an `actions/.md` path, or a backticked `.md` filename — the three - * shapes rule two's own comment names, and nothing else. A word merely present in prose is not - * collected here, on purpose: that is exactly what let a deleted table row hide behind - * unrelated text that happened to contain the same word. */ -function citationsIn(sectionLines, rawSectionLines) { - const blankedText = sectionLines.join("\n"); - // Only the `actions/.md` path shape is read through fences, because `10-todo` cites its - // one action that way. Every other shape reads the blanked view: a backticked file name inside - // a fenced example is an example, and letting it cite reopened the hole fences were blanked for. - const pathText = (rawSectionLines ?? sectionLines).join("\n"); - const citations = new Set(); - - // Only the column a table declares as its action column counts. A glossary, a trigger column - // or a "next step" column names things that are not dispatch, and reading them as citations - // would let a section satisfy rule two while routing nothing. Each table decides for itself: - // a section may hold a glossary next to its router, and the router must still be read. +function declaredActionColumn(headerCells) { + return headerCells.findIndex((cell) => ACTION_COLUMN_HEADER.test(stripBackticks(cell))); +} + +// --- Rule two: router coherence ------------------------------------------------------------- + +/** + * Names cited from the column a table calls `Action`. A run of rows with no separator beneath it + * is that table resumed after a blank line, so it keeps the column its header declared. + */ +function citationsFromActionColumns(sectionLines) { + const cited = new Set(); let actionColumn = -1; + for (const block of tableBlocks(sectionLines)) { - const rows = block.map(splitTableCells); - const isNewTable = rows.length > 1 && isTableSeparatorRow(rows[1]); - - // A header row is followed by its `| --- |` separator. A run of rows without one is the - // same table resumed after a blank line, and it keeps the column its header declared — - // otherwise a purely cosmetic edit refuses every row below the blank line. The cost is - // stated in the spec: a separator-less run following the router is read as part of it, - // so a glossary written without a separator would donate its cells. Neither shape is a - // table any renderer accepts, and the false refusal is the worse of the two. - if (isNewTable) { - actionColumn = rows[0].findIndex((cell) => ACTION_HEADER_RE.test(stripBackticks(cell))); - } - if (actionColumn === -1) continue; // no header declared an action column: this table cites nothing + const rows = block.map(tableCells); + const declaresItsOwnColumns = rows.length > 1 && isSeparatorRow(rows[1]); - for (const cells of isNewTable ? rows.slice(1) : rows) { - if (isTableSeparatorRow(cells)) continue; + if (declaresItsOwnColumns) actionColumn = declaredActionColumn(rows[0]); + if (actionColumn === -1) continue; + + for (const cells of declaresItsOwnColumns ? rows.slice(1) : rows) { + if (isSeparatorRow(cells)) continue; const cell = stripBackticks(cells[actionColumn] ?? ""); - if (CITATION_TOKEN_RE.test(cell)) citations.add(cell.toLowerCase()); + if (CITATION_TOKEN.test(cell)) cited.add(cell.toLowerCase()); } } + return cited; +} - ACTION_PATH_RE.lastIndex = 0; - let pathMatch; - while ((pathMatch = ACTION_PATH_RE.exec(pathText)) !== null) { - citations.add(pathMatch[1].toLowerCase()); - } +function matchesOf(pattern, text) { + pattern.lastIndex = 0; + const found = new Set(); + let match; + while ((match = pattern.exec(text)) !== null) found.add(match[1].toLowerCase()); + return found; +} + +/** + * The three citation shapes. Only the `actions/.md` path is read through a fence, because + * `aidd-dev:10-todo` cites its one action that way; a backticked file name inside a fence is an + * example. A word loose in prose is never a citation. + */ +function citationsIn(blankedLines, rawLines) { + return new Set([ + ...citationsFromActionColumns(blankedLines), + ...matchesOf(ACTION_PATH, (rawLines ?? blankedLines).join("\n")), + ...matchesOf(BACKTICKED_FILE_NAME, blankedLines.join("\n")), + ]); +} - BACKTICKED_MD_RE.lastIndex = 0; - let mdMatch; - while ((mdMatch = BACKTICKED_MD_RE.exec(blankedText)) !== null) { - citations.add(mdMatch[1].toLowerCase()); +/** A stem two action files share cites neither: one row would otherwise cover both. */ +function ambiguousStems(actionFileNames) { + const seen = new Set(); + const shared = new Set(); + for (const name of actionFileNames) { + const stem = stemOf(name).toLowerCase(); + if (seen.has(stem)) shared.add(stem); + seen.add(stem); } + return shared; +} - return citations; +function isCited(actionFileName, citations, sharedStems) { + const stem = stemOf(actionFileName).toLowerCase(); + return ( + citations.has(actionFileName.toLowerCase()) || + citations.has(actionFileName.replace(/\.md$/i, "").toLowerCase()) || + (!sharedStems.has(stem) && citations.has(stem)) + ); } -/** - * Rule two: a skill's `## Actions` section cites every action file that skill provides. It - * checks this one direction only — see the module header comment for why the opposite - * direction (a citation with no file behind it) is gone rather than narrowed. - */ function checkRouterCoherence(filePath, content, actionFileNames) { - const info = classifyFile(filePath); - if (!info || info.kind !== "skill") return []; + const surface = classifyFile(filePath); + if (!surface || surface.kind !== "skill") return []; - const names = actionFileNames || []; + const names = actionFileNames ?? []; if (names.length === 0) return []; const rawLines = toLines(content); const lines = withoutFences(rawLines); - const section = findActionsSection(lines); + const section = actionsSection(lines); if (!section) { - return [ - { - file: filePath, - line: 1, - plugin: info.owner, - rule: "router-coherence", - message: `${filePath} has action files but no "## Actions" section`, - }, - ]; - } - - const violations = []; - const sectionLines = lines.slice(section.startIdx, section.endIdx); - const citations = citationsIn(sectionLines, rawLines.slice(section.startIdx, section.endIdx)); - - // Two action files can share a stem — `01-plan.md` and `04-plan.md` both reduce to `plan`. - // One citation would then cover both, and deleting either row would go unnoticed, so a - // shared stem speaks for nobody and only the numbered name does. - const stemCount = new Map(); - for (const name of names) { - const stem = stemOf(name).toLowerCase(); - stemCount.set(stem, (stemCount.get(stem) ?? 0) + 1); - } - - for (const name of names) { - const full = name.toLowerCase(); - const fullNoExt = name.replace(/\.md$/i, "").toLowerCase(); - const stem = stemOf(name).toLowerCase(); - const stemIsOwn = stemCount.get(stem) === 1; - if (citations.has(full) || citations.has(fullNoExt) || (stemIsOwn && citations.has(stem))) continue; - - violations.push({ - file: filePath, - line: section.headingLine, - plugin: info.owner, - rule: "router-coherence", - message: `${filePath}:${section.headingLine} "## Actions" never names action file "${name}"`, - }); + const message = `${filePath} has action files but no "## Actions" section`; + return [violation("router-coherence", filePath, 1, surface.owner, message)]; } - return violations; + const { startIndex, endIndex, headingLine } = section; + const citations = citationsIn(lines.slice(startIndex, endIndex), rawLines.slice(startIndex, endIndex)); + const sharedStems = ambiguousStems(names); + + return names + .filter((name) => !isCited(name, citations, sharedStems)) + .map((name) => + violation( + "router-coherence", + filePath, + headingLine, + surface.owner, + `${filePath}:${headingLine} "## Actions" never names action file "${name}"` + ) + ); } -/** Both rules, combined. `actionFileNames` is read only when `filePath` is a SKILL.md. */ function checkArchitecture(filePath, content, actionFileNames) { return [ ...checkOrthogonality(filePath, content), From 07e794313d21010c932a7435900aa009123824ef Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Fri, 18 Sep 2026 23:30:45 +0200 Subject: [PATCH 10/11] docs(framework): the decisions table holds what the code stopped saying MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The refactor cut its comments on the premise that the why lives in the plan's decisions table, then pointed there for three decisions the table never held: a table resumed after a blank line keeping its column, a separator row counting from two dashes, and an unterminated fence not being a fence. A pointer that misses is worse than the comment it replaced, and it undermines the refactor's own argument. `citationsIn` also defended a parameter its single caller always passes. The default is gone. Both found by an independent review that also ran the old engine against the new one over 4550 content cases, 60000 fuzzed paths and 26 hook payloads — zero differences. That is stronger evidence of an unchanged behaviour than the nine mutations the refactor commit cited. Refs #250 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2 --- .../2026_09_18_cross-plugin-orthogonality-guard/plan.md | 3 +++ scripts/lib/architecture-rules.js | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md index 5eb300349..03e781163 100644 --- a/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md +++ b/aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/plan.md @@ -43,3 +43,6 @@ | A stem speaks for its action only when no sibling shares it | `01-plan.md` and `04-plan.md` both reduce to `plan`, so one row would cover both and deleting either would go unnoticed. A shared stem cites nobody; the numbered name still does. No skill in the tree has a collision today, which is why it had to be reasoned about rather than observed. | | A fenced block is an example, never a router | The section scan blanks fences before reading headings and tables, so a documented `## Actions`, a `##` inside the section, and an example table all stop steering the verdict. Path citations are read from the unblanked lines, because `10-todo` legitimately cites `actions/01-todo.md` inside a fence. | | Only the path shape reads through a fence | Both shapes read the unblanked lines at first, which let a backticked `.md` inside a fenced example cite — the very hole blanking fences was for. `10-todo` needs the path shape and nothing needs the other, so the exception is one shape wide. | +| A table resumed after a blank line keeps its column | A run of rows with no `| --- |` beneath it is read as the table above it, resumed. Otherwise inserting one blank line — changing no content — refuses every row below it. The cost: a separator-less glossary written just under the router donates its cells, so a deleted row could hide behind it. Neither shape is a table a renderer accepts, and the false refusal is the worse failure. A column-count check does not separate them: a glossary often has the router's width. | +| A separator row counts from two dashes | `aidd-context:00-onboard` writes `| -- |` and GitHub renders it. Requiring three silently stopped the header being recognised, which unnamed every action in that skill. The real-tree sweep caught it. | +| A fence nobody closed is not a fence | Blanking from an unterminated fence to end of file hides a `## Actions` that is visibly there, and the refusal then reads "has action files but no ## Actions section" — unactionable. The whole document is read as unfenced instead. | diff --git a/scripts/lib/architecture-rules.js b/scripts/lib/architecture-rules.js index a7b76ac06..3ea9ade7a 100644 --- a/scripts/lib/architecture-rules.js +++ b/scripts/lib/architecture-rules.js @@ -236,7 +236,7 @@ function matchesOf(pattern, text) { function citationsIn(blankedLines, rawLines) { return new Set([ ...citationsFromActionColumns(blankedLines), - ...matchesOf(ACTION_PATH, (rawLines ?? blankedLines).join("\n")), + ...matchesOf(ACTION_PATH, rawLines.join("\n")), ...matchesOf(BACKTICKED_FILE_NAME, blankedLines.join("\n")), ]); } From e599505c08aa483ebdcaeaf785d91e7e62fa70ea Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Mon, 21 Sep 2026 06:11:43 +0200 Subject: [PATCH 11/11] feat(framework): the same two rules refuse an edit at commit time, for every tool The write-time hook only ever sees Claude Code, so a contributor on Cursor, Codex, Copilot or opencode met no guard at all. The rules now run as a pre-commit job, which every edit reaching a commit passes through, and validate.yml replays it over the whole tree on each pull request. Nothing is duplicated to get there: architecture-scan.js holds the one filesystem layer the pure engine refuses to own, and the hook and the command are both thin callers of it. The hook stays as the fast path, refusing in the same turn rather than at commit time. --root exists so the refusal itself is testable against a tree that is not this repository. A gate whose refusal nothing exercises is a gate nobody can trust. 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 | 62 ++++------ docs/ARCHITECTURE.md | 2 +- lefthook.yml | 8 ++ .../__tests__/architecture-check-cli.test.js | 107 ++++++++++++++++++ scripts/check-architecture-rules.js | 49 ++++++++ scripts/lib/architecture-scan.js | 87 ++++++++++++++ 6 files changed, 271 insertions(+), 44 deletions(-) create mode 100644 scripts/__tests__/architecture-check-cli.test.js create mode 100644 scripts/check-architecture-rules.js create mode 100644 scripts/lib/architecture-scan.js diff --git a/.claude/hooks/check-architecture-rules.js b/.claude/hooks/check-architecture-rules.js index 406538cb2..b50d0867b 100755 --- a/.claude/hooks/check-architecture-rules.js +++ b/.claude/hooks/check-architecture-rules.js @@ -3,12 +3,13 @@ /** * 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`; this script only supplies their inputs and speaks the - * host's refusal. + * `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 fails open at every step. This hook gates every write in the repository, so an unreadable - * payload, an unknown tool, an edit it cannot reconstruct or a crash must let the write through - * rather than halt unrelated work. + * 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"); @@ -17,13 +18,13 @@ const path = require("node:path"); const WRITE_TOOLS = new Set(["Write", "Edit", "MultiEdit"]); const PROCEED = 0; -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"; - -function architectureRules() { +function architecture() { try { - return require(path.resolve(__dirname, "..", "..", "scripts", "lib", "architecture-rules.js")); + 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; } @@ -89,24 +90,8 @@ function prospectiveContent(toolName, toolInput, absolutePath) { return editedContent(absolutePath, edits); } -/** Rule two needs the skill's action files; the engine never reads them itself. */ -function actionFileNames(relativePath, absolutePath) { - if (path.basename(relativePath) !== "SKILL.md") return undefined; - try { - return fs.readdirSync(path.join(path.dirname(absolutePath), "actions")).filter((name) => name.endsWith(".md")); - } catch { - return []; - } -} - -function howToFix({ rule, plugin }) { - return rule === "orthogonality" - ? `name the concept ${plugin} owns instead of addressing it directly` - : `cite every action file the skill provides in its "## Actions" section. A citation is ${CITATION_SHAPES}`; -} - -function refuse(violations) { - const reason = violations.map((v) => `${v.message}. Fix: ${howToFix(v)}.`).join("\n"); +function refuse(violations, describeFix) { + const reason = violations.map((v) => `${v.message}. Fix: ${describeFix(v)}.`).join("\n"); process.stdout.write( JSON.stringify({ hookSpecificOutput: { @@ -118,19 +103,10 @@ function refuse(violations) { ); } -function violationsFor(engine, relativePath, content, actions) { - try { - const found = engine.checkArchitecture(relativePath, content, actions); - return Array.isArray(found) ? found : []; - } catch { - return []; - } -} - function main() { - const engine = architectureRules(); + const rules = architecture(); const payload = payloadFromStdin(); - if (!engine || !payload) return PROCEED; + if (!rules || !payload) return PROCEED; const { tool_name: toolName, tool_input: toolInput } = payload; if (!WRITE_TOOLS.has(toolName)) return PROCEED; @@ -141,13 +117,13 @@ function main() { const root = process.env.CLAUDE_PROJECT_DIR || process.cwd(); const absolutePath = path.resolve(root, toolInput.file_path); const relativePath = repoRelative(absolutePath, root); - if (!relativePath || !engine.classifyFile(relativePath)) return PROCEED; + if (!relativePath || !rules.classifyFile(relativePath)) return PROCEED; const content = prospectiveContent(toolName, toolInput, absolutePath); if (content === null) return PROCEED; - const violations = violationsFor(engine, relativePath, content, actionFileNames(relativePath, absolutePath)); - if (violations.length > 0) refuse(violations); + const violations = rules.violationsForFile(relativePath, content, absolutePath); + if (violations.length > 0) refuse(violations, rules.describeFix); return PROCEED; } diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index c0417b143..fc2232633 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -155,7 +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. -`scripts/lib/architecture-rules.js` is the guard that decides both rules from a prospective edit's path and content. It fires on this repository's AI write path for Claude Code — a `PreToolUse` hook on `Write`/`Edit`/`MultiEdit` — so a contributor on another AI tool is not covered by it. `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. 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. This distinction keeps recipe plugins swappable while making orchestration handoffs explicit and auditable. diff --git a/lefthook.yml b/lefthook.yml index 14eefeb00..00d136edb 100644 --- a/lefthook.yml +++ b/lefthook.yml @@ -61,6 +61,14 @@ pre-commit: exit 0 fi node scripts/check-skill-argument-hints.mjs + architecture-rules: + glob: "plugins/*/{skills,agents}/**/*.md" + run: | + if ! command -v node >/dev/null 2>&1; then + echo "ℹ️ node not available; skipping architecture-rules" + exit 0 + fi + node scripts/check-architecture-rules.js {files} context-imports: # Two patterns: "**/" needs a literal slash, so it never matches a repository-root file. glob: diff --git a/scripts/__tests__/architecture-check-cli.test.js b/scripts/__tests__/architecture-check-cli.test.js new file mode 100644 index 000000000..5505d948a --- /dev/null +++ b/scripts/__tests__/architecture-check-cli.test.js @@ -0,0 +1,107 @@ +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 { governedPaths, scan } = require("../lib/architecture-scan.js"); + +const REPO_ROOT = path.resolve(__dirname, "../.."); +const CHECK = path.join(REPO_ROOT, "scripts/check-architecture-rules.js"); + +/** + * The commit-time half of issue #250. The write-time hook only ever sees Claude Code; this + * check is what every other tool, and every person, runs into. Its tests are therefore the + * ones that matter for coverage, not the hook's. + */ + +const CLEAN_SKILL = [ + "# Clean skill", + "", + "## Actions", + "", + "| # | Action | Role |", + "| --- | --- | --- |", + "| 01 | `step` | Do the one thing |", + "", +].join("\n"); + +const SIBLING_ADDRESS_SKILL = CLEAN_SKILL.replace( + "# Clean skill", + "# Clean skill\n\nSee @aidd-fixture-b:02-thing for the other half." +); + +const UNCITED_ACTION_SKILL = CLEAN_SKILL.replace("| 01 | `step` | Do the one thing |", ""); + +/** A temp tree with a `plugins/` root, so the file classifier applies exactly as it does here. */ +function makeTree(skillContent, actionNames = ["01-step.md"]) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "architecture-check-")); + const skillDir = path.join(root, "plugins/aidd-fixture-a/skills/01-demo"); + fs.mkdirSync(path.join(skillDir, "actions"), { recursive: true }); + fs.writeFileSync(path.join(skillDir, "SKILL.md"), skillContent); + for (const name of actionNames) { + fs.writeFileSync(path.join(skillDir, "actions", name), "# action\n"); + } + return { root, skill: "plugins/aidd-fixture-a/skills/01-demo/SKILL.md" }; +} + +test("a skill citing every action it provides yields nothing", () => { + const { root, skill } = makeTree(CLEAN_SKILL); + assert.deepEqual(scan(root, [skill]), []); +}); + +test("a skill addressing a sibling plugin is caught, and names the sibling", () => { + const { root, skill } = makeTree(SIBLING_ADDRESS_SKILL); + const found = scan(root, [skill]); + assert.equal(found.length, 1); + assert.equal(found[0].rule, "orthogonality"); + assert.equal(found[0].plugin, "aidd-fixture-b"); +}); + +test("an action file the router never cites is caught, and names the file", () => { + const { root, skill } = makeTree(UNCITED_ACTION_SKILL); + const found = scan(root, [skill]); + assert.equal(found.length, 1); + assert.equal(found[0].rule, "router-coherence"); + assert.match(found[0].message, /01-step\.md/); +}); + +test("a path outside the governed surface contributes nothing", () => { + const { root } = makeTree(SIBLING_ADDRESS_SKILL); + fs.writeFileSync(path.join(root, "README.md"), "See @aidd-fixture-b:02-thing.\n"); + assert.deepEqual(scan(root, ["README.md"]), []); +}); + +test("a path that does not exist is skipped rather than thrown over", () => { + const { root } = makeTree(CLEAN_SKILL); + assert.deepEqual(scan(root, ["plugins/aidd-fixture-a/skills/99-gone/SKILL.md"]), []); +}); + +test("the whole governed tree of this repository is silent, and is not empty", () => { + // A scan that matched nothing would pass every assertion above. Pin both halves. + const paths = governedPaths(REPO_ROOT); + assert.ok(paths.length > 100, `expected the tree to govern more than 100 files, got ${paths.length}`); + assert.deepEqual(scan(REPO_ROOT, paths), []); +}); + +test("the command exits 0 on this repository and says how much it checked", () => { + const result = spawnSync(process.execPath, [CHECK], { encoding: "utf8" }); + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /governed file\(s\) checked, no violation/); +}); + +test("the command exits 1 and prints the fix when a governed file breaks a rule", () => { + const { root, skill } = makeTree(SIBLING_ADDRESS_SKILL); + const result = spawnSync(process.execPath, [CHECK, "--root", root, skill], { encoding: "utf8" }); + assert.equal(result.status, 1); + assert.match(result.stderr, /addresses sibling plugin "aidd-fixture-b"/); + assert.match(result.stderr, /Fix: name the concept aidd-fixture-b owns/); +}); + +test("the command scans a whole tree it is pointed at, not only this repository", () => { + const { root } = makeTree(UNCITED_ACTION_SKILL); + const result = spawnSync(process.execPath, [CHECK, "--root", root], { encoding: "utf8" }); + assert.equal(result.status, 1); + assert.match(result.stderr, /never names action file "01-step\.md"/); +}); diff --git a/scripts/check-architecture-rules.js b/scripts/check-architecture-rules.js new file mode 100644 index 000000000..18b2ec53d --- /dev/null +++ b/scripts/check-architecture-rules.js @@ -0,0 +1,49 @@ +#!/usr/bin/env node + +/** + * 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. + */ + +"use strict"; + +const path = require("node:path"); + +const { describeFix, governedPaths, scan } = require("./lib/architecture-scan.js"); + +const DEFAULT_ROOT = path.resolve(__dirname, ".."); + +function parse(argv) { + const at = argv.indexOf("--root"); + if (at === -1) return { root: DEFAULT_ROOT, requested: argv.filter((a) => !a.startsWith("-")) }; + + const root = path.resolve(argv[at + 1] ?? "."); + const rest = [...argv.slice(0, at), ...argv.slice(at + 2)]; + return { root, requested: rest.filter((a) => !a.startsWith("-")) }; +} + +function main(argv) { + const { root, requested } = parse(argv); + const relative = (argument) => + path.relative(root, path.resolve(root, argument)).split(path.sep).join("/"); + const paths = requested.length > 0 ? requested.map(relative) : governedPaths(root); + const violations = scan(root, paths); + + if (violations.length === 0) { + console.log(`✅ Architecture rules: ${paths.length} governed file(s) checked, no violation`); + return 0; + } + + for (const violation of violations) { + console.error(`❌ ${violation.message}`); + console.error(` Fix: ${describeFix(violation)}.`); + } + console.error(`\n${violations.length} violation(s) across ${paths.length} checked file(s).`); + return 1; +} + +process.exitCode = main(process.argv.slice(2)); diff --git a/scripts/lib/architecture-scan.js b/scripts/lib/architecture-scan.js new file mode 100644 index 000000000..75b8cbafb --- /dev/null +++ b/scripts/lib/architecture-scan.js @@ -0,0 +1,87 @@ +/** + * 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. + */ + +"use strict"; + +const fs = require("node:fs"); +const path = require("node:path"); + +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"; + +/** Rule two needs the skill's action files; the engine never reads them itself. */ +function actionFileNames(relativePath, absolutePath) { + if (path.basename(relativePath) !== "SKILL.md") return undefined; + try { + return fs + .readdirSync(path.join(path.dirname(absolutePath), "actions")) + .filter((name) => name.endsWith(".md")); + } catch { + return []; + } +} + +/** 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)); + return Array.isArray(found) ? found : []; + } catch { + return []; + } +} + +function describeFix({ rule, plugin }) { + return rule === "orthogonality" + ? `name the concept ${plugin} owns instead of addressing it directly` + : `cite every action file the skill provides in its "## Actions" section. A citation is ${CITATION_SHAPES}`; +} + +function markdownUnder(directory, root, found) { + for (const entry of fs.readdirSync(directory, { withFileTypes: true })) { + const full = path.join(directory, entry.name); + if (entry.isDirectory()) markdownUnder(full, root, found); + else if (entry.name.endsWith(".md")) { + found.push(path.relative(root, full).split(path.sep).join("/")); + } + } + return found; +} + +/** Every governed path in the tree, for the whole-tree run CI does. */ +function governedPaths(root) { + const plugins = path.join(root, "plugins"); + if (!fs.existsSync(plugins)) return []; + return markdownUnder(plugins, root, []).filter((relativePath) => classifyFile(relativePath)); +} + +/** Violations across paths already on disk. A path that is not governed contributes none. */ +function scan(root, relativePaths) { + const violations = []; + for (const relativePath of relativePaths) { + if (!classifyFile(relativePath)) continue; + const absolutePath = path.join(root, relativePath); + let content; + try { + content = fs.readFileSync(absolutePath, "utf8"); + } catch { + continue; + } + violations.push(...violationsForFile(relativePath, content, absolutePath)); + } + return violations; +} + +module.exports = { + actionFileNames, + describeFix, + governedPaths, + scan, + violationsForFile, +};