Repository navigation
feat(framework): refuse an AI edit that breaks a named architecture rule - #885
Merged
Merged
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2
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/<name>.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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2
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 29489f7 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2
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/<name>.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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2
A sixth review found the raw view carried two exceptions where the contract named one. Reading a backticked `<name>.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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2
7 tasks
…hape 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2
…r 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq AIDD-Session-Id: 2261efcd-a873-4e60-9893-2217f702bde2
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 What & why
docs/ARCHITECTURE.mdstated cross-plugin orthogonality and nothing verified it, so a hardcoded sibling address landed and nobody noticed. Two rules now decide an AI-authored edit before it lands:## Actionssection cites every action file that skill provides.Per the issue's 2026-09-14 comment, which supersedes the body's "Guardrail local et CI" section: no Git hook and no CI gate. The enforcement is a host-native
PreToolUsehook, because the decision is to prevent the write, andPostToolUsecan only report one already made.🛠️ How it works
scripts/lib/architecture-rules.jsis a pure function of a repository-relative path, the prospective content, and — for aSKILL.md— its action file names. It never reads the filesystem, so a test and a hook hand it the same shape..claude/hooks/check-architecture-rules.jssupplies the payload and emits the refusal; it fails open on every shape it does not recognise, because it gates every write in this repository.The rule's own exceptions are encoded, not assumed: an agent's
# Skills you may invokelist, an orchestration reference, and everything under a plugin'sassets/stay silent.aidd-context:00-onboardis exempt from rule one — its menus name addresses because those are what the skill hands a person to type — and that exemption is temporary, with #883 behind it.Rule two enforces one direction only. A citation with no file behind it cannot be told apart from one written seconds before the file it names, and
aidd-context:04-skill-generatedocuments writing the router first — enforcing that direction made adding an action refuse in both orders.🧪 How to verify
node scripts/check-tests-leave-git-alone.js -- node --test 'scripts/__tests__/**/*.test.js'→ 501 pass, 0 fail. Never run that suite bare; it can overwrite this repository's.git/hooks.pnpm exec lefthook run pre-push→ green, including the fullclisuite at 6237 tests.# Skills you may invokeand watch it apply. A recorded run of both is inaidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/wiring-proof.md.plugins/— 350 of them — through both rules and expects zero. It is the calibration guard, and it caught two regressions during this branch.PreToolUseand the same deny shape, but Codex delivers a file edit as anapply_patchcommand string — a different parse. The engine is host-agnostic, so each adapter is additive.plugins/aidd-dev/skills/01-plan/actions/04-plan.md:16addressed a sibling skill in prose, and leaving it would have made the guard red on its own tree.The spec, plan, phases and the wiring proof are in
aidd_docs/tasks/2026_09/2026_09_18_cross-plugin-orthogonality-guard/.🔗 Linked issue
Closes #250
✅ I certify
🤖 Generated with Claude Code
https://claude.ai/code/session_01FPREgkoNtK4PYh2YyZbztq