From df2d644f7bb67822ce428e0cfb7c2d0777785f09 Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Mon, 12 Oct 2026 14:49:25 +0200 Subject: [PATCH] fix(framework): close the architecture guard's three recorded gaps A run of table rows with no separator under the Actions table was read as the table resumed after a blank line, so a glossary there could donate a citation and hide a deleted row. A blank line ends a table, as every renderer reads it: such a run now draws one refusal at its first line, and cites nothing. Orthogonality keeps reading through fences while router coherence does not, on purpose and now said in the code: a fenced command is still one the model runs. A test pins it. R8 says that two actions sharing a slug keep their number, the form the guard already cites. The split-table unit test and the property test's split shapes now expect that single refusal. Every router in the tree still passes. Closes #884 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VE16AxC1V9wAMGmGgefe4z --- .../references/skill-authoring.md | 2 +- .../architecture-rules.property.test.js | 21 ++++- scripts/__tests__/architecture-rules.test.js | 94 ++++++++++++++++++- scripts/lib/architecture-rules.js | 49 +++++++--- 4 files changed, 146 insertions(+), 20 deletions(-) diff --git a/plugins/aidd-context/skills/04-skill-generate/references/skill-authoring.md b/plugins/aidd-context/skills/04-skill-generate/references/skill-authoring.md index 6072deba5..b0862c855 100644 --- a/plugins/aidd-context/skills/04-skill-generate/references/skill-authoring.md +++ b/plugins/aidd-context/skills/04-skill-generate/references/skill-authoring.md @@ -14,7 +14,7 @@ The contract every generated skill satisfies. `skill-generate` obeys it too. - **R6.** The router holds the flow, the action table, and the transversal rules. Nothing else. - **R7.** The mermaid flow shows every path a run can take: the nominal chain, one entry node per case, a back-edge per loop, a terminal node per outcome. A branch stated in prose is a branch missing from the flow. -- **R8.** The action table is `| Action | Does |`, one row per action file, in run order. `Action` is the bare slug, no backticks and no number. `Does` is a lowercase imperative half-line with no final period. Above the table, one sentence: what to read next, nothing more. +- **R8.** The action table is `| Action | Does |`, one row per action file, in run order. `Action` is the bare slug, no backticks and no number; two actions sharing a slug keep their number (`01-plan`, `04-plan`). `Does` is a lowercase imperative half-line with no final period. Above the table, one sentence: what to read next, nothing more. - **R9.** `## Transversal rules` holds the rules no single action or reference owns. A rule stated there is stated nowhere else. - **R10.** The router is loaded on every call, an action only when its turn comes: the router carries nothing an action or a reference could carry. diff --git a/scripts/__tests__/architecture-rules.property.test.js b/scripts/__tests__/architecture-rules.property.test.js index ec8434428..ad930c915 100644 --- a/scripts/__tests__/architecture-rules.property.test.js +++ b/scripts/__tests__/architecture-rules.property.test.js @@ -134,11 +134,24 @@ function describe(shape, omit) { }`; } -test("every shape that cites all its actions is silent", () => { +/** A blank line inside the router table is built only when the row it precedes is kept. */ +function isCut(shape, omit) { + return shape.splitAfter !== undefined && omit !== NAMES[shape.splitAfter]; +} + +function refusedOnceAtTheCut(violations) { + return violations.length === 1 && /no separator/.test(violations[0].message); +} + +test("every unsplit shape that cites all its actions is silent, and every split one is refused at the cut", () => { const failures = []; for (const shape of shapes()) { const content = buildSkill({ ...shape, names: NAMES, omit: null }); const violations = checkRouterCoherence(FILE, content, NAMES); + if (isCut(shape, null)) { + if (!refusedOnceAtTheCut(violations)) failures.push(`${describe(shape)} => ${violations.length} violations`); + continue; + } if (violations.length !== 0) { failures.push(`${describe(shape)} => ${violations.map((v) => v.message).join(" | ")}`); } @@ -146,12 +159,16 @@ test("every shape that cites all its actions is silent", () => { 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", () => { +test("every unsplit 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 (isCut(shape, omit)) { + if (!refusedOnceAtTheCut(violations)) failures.push(`${describe(shape, omit)} => ${violations.length} violations`); + continue; + } if (violations.length !== 1) { failures.push(`${describe(shape, omit)} => ${violations.length} violations`); continue; diff --git a/scripts/__tests__/architecture-rules.test.js b/scripts/__tests__/architecture-rules.test.js index 7a31f3908..33f5666fb 100644 --- a/scripts/__tests__/architecture-rules.test.js +++ b/scripts/__tests__/architecture-rules.test.js @@ -303,7 +303,7 @@ 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", () => { +test("a blank line splitting the router table is refused once, at the rows it cut off", () => { const content = [ "# Split table skill", "", @@ -324,7 +324,61 @@ test("a blank line splitting the router table does not unname the rows below it" content, ["01-first.md", "02-second.md"] ); - assert.deepEqual(violations, []); + assert.equal(violations.length, 1); + assert.match(violations[0].message, /:9 .*no separator/); +}); + +test("a glossary with no separator under the router cites nothing", () => { + const content = [ + "# Glossary skill", + "", + "## Actions", + "", + "| Action | When |", + "| --- | --- |", + "| 01-keep | a |", + "", + "| role | the caller |", + "| scope | the ask |", + "", + "## Transversal rules", + "", + "- Nothing.", + ].join("\n"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-glossary/SKILL.md", + content, + ["01-keep.md", "02-role.md"] + ); + assert.equal(violations.length, 1); + assert.match(violations[0].message, /:9 .*no separator/); +}); + +test("a glossary under the router with its own separator leaves a deleted row unnamed", () => { + const content = [ + "# Glossary skill", + "", + "## Actions", + "", + "| Action | When |", + "| --- | --- |", + "| 01-keep | a |", + "", + "| Term | Meaning |", + "| --- | --- |", + "| role | the caller |", + "", + "## Transversal rules", + "", + "- Nothing.", + ].join("\n"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-glossary-table/SKILL.md", + content, + ["01-keep.md", "02-role.md"] + ); + assert.equal(violations.length, 1); + assert.match(violations[0].message, /never names action file "02-role\.md"/); }); test("a two-dash separator row still marks the header it follows", () => { @@ -395,6 +449,42 @@ test("two action files sharing a stem are not both covered by one citation", () assert.match(violations[1].message, /never names action file "04-plan\.md"/); }); +test("two action files sharing a stem are each cited by their numbered name", () => { + const content = [ + "# Shared stem skill", + "", + "## Actions", + "", + "| Action | Does |", + "| --- | --- |", + "| 01-plan | draft it |", + "| 04-plan | revise it |", + "", + "## Transversal rules", + "", + "- Nothing.", + ].join("\n"); + const violations = checkArchitecture( + "plugins/aidd-fixture-a/skills/01-shared-stem-numbered/SKILL.md", + content, + ["01-plan.md", "04-plan.md"] + ); + assert.deepEqual(violations, []); +}); + +test("a sibling address inside a fence is still refused, unlike a fenced router table", () => { + const content = [ + "# Fenced address", + "", + "```shell", + "/aidd-fixture-b:01-thing", + "```", + ].join("\n"); + const violations = checkArchitecture("plugins/aidd-fixture-a/skills/01-x/references/teach.md", content, []); + assert.equal(violations.length, 1); + assert.match(violations[0].message, /:4 addresses sibling plugin "aidd-fixture-b"/); +}); + test("a fenced example table inside the Actions section cites nothing", () => { const content = [ "# Fenced example skill", diff --git a/scripts/lib/architecture-rules.js b/scripts/lib/architecture-rules.js index 3ea9ade7a..05a4a6f09 100644 --- a/scripts/lib/architecture-rules.js +++ b/scripts/lib/architecture-rules.js @@ -102,6 +102,7 @@ function checkOrthogonality(filePath, content) { const lines = toLines(content); const exemptLines = surface.kind === "agent" ? permissionListLines(lines) : new Set(); + // Unlike router coherence, read through fences: a fenced command is still one the model runs. return addressesIn(content) .filter(({ plugin, line }) => plugin !== surface.owner && !exemptLines.has(line)) .map(({ plugin, line, text }) => @@ -166,22 +167,24 @@ function tableCells(line) { return line.trim().replace(/^\|/, "").replace(/\|$/, "").split("|").map((cell) => cell.trim()); } -/** Contiguous runs of table rows: one table's header never speaks for the next one's columns. */ +/** Contiguous runs of table rows with the index of their first line: one table's header never speaks for the next one's columns. */ function tableBlocks(lines) { const blocks = []; let current = []; + let start = 0; - for (const line of lines) { + lines.forEach((line, index) => { if (TABLE_ROW.test(line)) { + if (current.length === 0) start = index; current.push(line); - continue; + return; } if (current.length > 0) { - blocks.push(current); + blocks.push({ start, lines: current }); current = []; } - } - if (current.length > 0) blocks.push(current); + }); + if (current.length > 0) blocks.push({ start, lines: current }); return blocks; } @@ -197,27 +200,33 @@ function declaredActionColumn(headerCells) { // --- 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. + * Names cited from the column a table calls `Action`, and the first line of each run of rows with + * no separator that follows such a table. A blank line ends a table, so that run is no table at all: + * read as one, a glossary below the router would donate its cells as citations. */ function citationsFromActionColumns(sectionLines) { const cited = new Set(); + const cutOff = []; let actionColumn = -1; for (const block of tableBlocks(sectionLines)) { - const rows = block.map(tableCells); + const rows = block.lines.map(tableCells); const declaresItsOwnColumns = rows.length > 1 && isSeparatorRow(rows[1]); - if (declaresItsOwnColumns) actionColumn = declaredActionColumn(rows[0]); + if (!declaresItsOwnColumns) { + if (actionColumn !== -1) cutOff.push(block.start); + continue; + } + actionColumn = declaredActionColumn(rows[0]); if (actionColumn === -1) continue; - for (const cells of declaresItsOwnColumns ? rows.slice(1) : rows) { + for (const cells of rows.slice(1)) { if (isSeparatorRow(cells)) continue; const cell = stripBackticks(cells[actionColumn] ?? ""); if (CITATION_TOKEN.test(cell)) cited.add(cell.toLowerCase()); } } - return cited; + return { cited, cutOff }; } function matchesOf(pattern, text) { @@ -233,9 +242,9 @@ function matchesOf(pattern, text) { * `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) { +function citationsIn(cited, blankedLines, rawLines) { return new Set([ - ...citationsFromActionColumns(blankedLines), + ...cited, ...matchesOf(ACTION_PATH, rawLines.join("\n")), ...matchesOf(BACKTICKED_FILE_NAME, blankedLines.join("\n")), ]); @@ -279,7 +288,17 @@ function checkRouterCoherence(filePath, content, actionFileNames) { } const { startIndex, endIndex, headingLine } = section; - const citations = citationsIn(lines.slice(startIndex, endIndex), rawLines.slice(startIndex, endIndex)); + const sectionLines = lines.slice(startIndex, endIndex); + const { cited, cutOff } = citationsFromActionColumns(sectionLines); + + // One refusal at the cut, not one per action the rows below it would have named. + if (cutOff.length > 0) { + const line = startIndex + cutOff[0] + 1; + const message = `${filePath}:${line} table rows with no separator follow the action table; a blank line ends a table, so remove it or give these rows their own header`; + return [violation("router-coherence", filePath, line, surface.owner, message)]; + } + + const citations = citationsIn(cited, sectionLines, rawLines.slice(startIndex, endIndex)); const sharedStems = ambiguousStems(names); return names