diff --git a/aidd_docs/tasks/2026_10/2026_10_07_backlog-shape-guard/backlog-link.json b/aidd_docs/tasks/2026_10/2026_10_07_backlog-shape-guard/backlog-link.json new file mode 100644 index 000000000..c463362c0 --- /dev/null +++ b/aidd_docs/tasks/2026_10/2026_10_07_backlog-shape-guard/backlog-link.json @@ -0,0 +1,5 @@ +{ + "backlog": "ai-driven-dev/framework#972", + "written_at": "2026-10-07T15:20:00Z", + "written_by": "aidd-dev:01-plan" +} diff --git a/aidd_docs/tasks/2026_10/2026_10_07_backlog-shape-guard/phase-1.md b/aidd_docs/tasks/2026_10/2026_10_07_backlog-shape-guard/phase-1.md new file mode 100644 index 000000000..979044c3c --- /dev/null +++ b/aidd_docs/tasks/2026_10/2026_10_07_backlog-shape-guard/phase-1.md @@ -0,0 +1,92 @@ +--- +status: done +--- + +# Instruction: the taught example is parsed where it is watched +## Architecture projection +> Tree of the final files. ✅ create · ✏️ modify · ❌ delete + +```txt +. +└── scripts/__tests__/ + └── a-backlog-link-the-reader-can-read.test.js ✏️ parse the taught example, not its substrings +``` +## User Journey +```mermaid +flowchart TD + A[a change touches only the plugin files that teach the shape] --> B[scripts-tests runs, its glob covers plugins] + B --> C{does the taught example still parse, with the three fields?} + C -- no --> D[pre-commit fails, naming the file and the field] + C -- yes --> E[the commit proceeds] +``` +## Test Scope +```mermaid +--- +title: Test scope +--- +journey + section Setup + a taught example valid today => the suite is green: 5: system + section Happy path + run the guard on the tree as it stands => it passes: 5: system + section Edge case - the example keeps its field names and stops being JSON + break the example's syntax => run the guard => it fails, naming the file: 1: system + section Edge case - a field becomes empty + set written_by to an empty string => run the guard => it fails, naming the field: 1: system + section Edge case - the taught example is not the first fence + add a fence above it at line start => run the guard => it fails, and the cli sibling fails too: 1: system + section Edge case - a fence the sibling does not see + add one after text on the same line => run the guard => it passes, as the sibling does: 1: system + section Edge case - the fence markers are gone + leave the JSON as prose => run the guard => it fails: 1: system + section Edge case - the taught value is not an object + teach null or an array => run the guard => it fails, naming the shape: 1: system + section Teardown + restore the example => the suite is green again: 5: system +``` +## Tasks to do +### `1)` watch the guard stay green through the defect +> A guard nothing fails for is indistinguishable from a comment. + +1. Break the taught example's JSON syntax, keeping the three field names. +2. Run the guard. Record that it passes, which is the defect. +3. Restore the file. + +### `2)` parse the taught example +> The assertion becomes the one the reader actually makes. + +1. Parse each taught example instead of searching it for substrings. +2. Require the three fields present and non-empty strings. +3. Fail with the file and the field named, so the message locates the break. +4. Keep the `.md` and `.json` forms both readable: one skill fences the example, the other ships it as an asset. + +### `3)` agree with the sibling on which fence is the example +> Two guards reading different fences leave one green and the other red. + +1. Read the first json fence, as the `cli/` test does. +2. Match it with the sibling's own regex, line-anchored, so both read the same fence. +3. Fail naming the file, the field it must carry, and how many fences were read. +4. Report the parse error when that fence is not JSON, and refuse a value that is not an object. + +### `5)` keep only the comments the code cannot say +> Four rounds of accretion, cut back once the rule settled. + +1. Drop what the code states a line below. +2. Keep why the field list is restated rather than imported, that a drifting shape already produced two unreadable files, why the example is parsed, and which fence the sibling reads with its consequence. + +### `4)` watch it go red for its own reason +> The mutation that proves the guard ships with it. + +1. Break the syntax again. The guard fails, and the message names that file. +2. Empty one field. The guard fails, and the message names that field. +3. Strip the fence markers, leaving the JSON as prose. The guard fails. +4. Restore, and run the whole suite plus `pnpm exec lefthook run pre-commit`. + +## Test acceptance criteria +| Task | Acceptance criteria | +| --- | --- | +| 1 | the guard passes over a broken example before the change: measured `pass 3 / fail 0` on `origin/next` with the asset's syntax broken and its three field names kept | +| 2 | the guard parses both the fenced and the asset form, and names file and field on failure | +| 3 | a fence added above the taught one fails here and in the `cli/` sibling, with the same verdict | +| 4 | the fences stripped and an emptied field each turn this guard red, and the suite is green once restored | +| 5 | no comment restates the code, and no non-derivable reason was lost | diff --git a/aidd_docs/tasks/2026_10/2026_10_07_backlog-shape-guard/plan.md b/aidd_docs/tasks/2026_10/2026_10_07_backlog-shape-guard/plan.md new file mode 100644 index 000000000..67e41b801 --- /dev/null +++ b/aidd_docs/tasks/2026_10/2026_10_07_backlog-shape-guard/plan.md @@ -0,0 +1,38 @@ +--- +objective: "A change confined to the plugin files that teach backlog-link.json fails at pre-commit when it breaks what the reader accepts." +status: implemented +--- + +# Plan: the guard that runs on plugin changes parses what it guards + +## Overview +| Field | Value | +| --- | --- | +| **Goal** | move the structural assertions to the guard that already sees `plugins/**` | +| **Source** | [`source.md`](./source.md) | + +## Phases +| # | Phase | File | +| --- | --- | --- | +| 1 | the taught example is parsed where it is watched | [`phase-1.md`](./phase-1.md) | + +## Resources +| Source | Verified | +| --- | --- | +| `lefthook.yml` | `scripts-tests` carries `glob: "{scripts,plugins,cli/src}/**"`, so it runs on a plugin change; `cli-knip` and `cli-test` carry `glob: "cli/**"` and do not | +| `.github/workflows/cli-ci.yml:70` | the `changes` job matches `cli/*\|kanban/*\|scripts/__tests__/*\|README.md` and `plugins/aidd-telemetry/*` minus prose. `plugins/aidd-pm/**` matches nothing | +| `scripts/__tests__/a-backlog-link-the-reader-can-read.test.js:73` | asserts `text.includes('"backlog"')`, a substring check that never parses the example | +| `cli/tests/contexts/telemetry/infrastructure/task-backlog-skill-shape.integration.test.ts` | parses the example and feeds it to the real `TaskBacklogAdapter`, which a repository script test cannot reach | + +## Decisions +| Decision | Why | +| --- | --- | +| The structural assertions go to the `scripts/` guard | it is the only one of the two that runs on a change confined to `plugins/**` | +| Neither filter is widened | `plugins/**` would run 6800 cli tests on a prose edit, and a narrow path list is the hand-maintained duplication that caused this: those paths moved last week | +| The `cli/` test keeps the adapter crossing | a repository script test cannot import across that boundary, which is the whole reason that test exists | +| The guard parses the example rather than reading field names | a file that keeps the three strings while ceasing to be JSON is exactly the case that fell between the two guards | +| The taught example is the first json fence, matched with the sibling's own regex | two guards reading different fences leave one green and the other red, and no gate runs the `cli/` one for `plugins/**`. Measured three times: by content, by an unanchored position rule, and by the sibling's anchored one. Only the third agrees in every case | +| The taught value is required to be a plain object | the reader funnels every parse through `cli/src/kernel/reading/json-file.ts`, whose predicate this copies, so nothing the reader accepts is refused | +| The `cli/` test's pinned `owner/repo#123` and its field-set equality are left alone | #972 scopes the Affected file to the scripts guard. Changing a plugin's taught value still lands that test red on `next`, which is the same unwatched-test mechanism. Filed as #973 rather than deferred to nobody | +| One case stays green on both guards, knowingly | a valid decoy carrying `backlog` at line start, above a broken taught example, is read as the example by both. Both agree, so nothing lands red on `next`. Closing it here alone would make this guard red where the sibling is green, and would reinstate the one-fence rule this branch added and removed | +| The mutation ships with the guard | `aidd_docs/memory/coding-assertions.md` requires watching the named test go red for the reason it names | diff --git a/aidd_docs/tasks/2026_10/2026_10_07_backlog-shape-guard/review.md b/aidd_docs/tasks/2026_10/2026_10_07_backlog-shape-guard/review.md new file mode 100644 index 000000000..9c5dbc4dc --- /dev/null +++ b/aidd_docs/tasks/2026_10/2026_10_07_backlog-shape-guard/review.md @@ -0,0 +1,40 @@ +# Review: backlog-shape-guard + +- **Verdict**: changes-requested +- **Diff**: `origin/next...d0e3ea96` +- **Axes run**: code, functional, relevancy +- **Date**: 2026_10_07 +- **Findings**: 0 critical, 5 warning, 6 minor + +## Phases + +### Phase 1 — the taught example is parsed where it is watched + +- [x] the guard passes over a broken example before the change, recorded — `7f77e845` body ("the guard passed 3 of 3"); reproduced: `origin/next`'s guard returns `pass 3 / fail 0` over a syntax-broken asset and over a syntax-broken fence in `04-plan.md` +- [x] the guard parses both the fenced and the asset form, and names file and field on failure — `scripts/__tests__/a-backlog-link-the-reader-can-read.test.js:74-88`; measured `teaches an example that is not JSON: Expected ',' or '}' ... position 94` and `must teach the field "written_by" as a non-empty string` +- [ ] three mutations each turn this guard red, and the suite is green once restored — two of the plan's three discriminate. The third, `phase-1.md:35` "rename the taught file", also fails at `origin/next` (`ENOENT`, 1 failure): `fs.readFileSync` is unchanged context in the diff, so it proves nothing about the new assertion. Suite green once restored: `tests 550 / pass 550 / fail 0` + +## Findings + +| Sev | Kind | Phase | Location | Issue | Fix | +| --- | ---- | ----- | -------- | ----- | --- | +| 🟡 | functional | 1 | `scripts/__tests__/a-backlog-link-the-reader-can-read.test.js:73` | Criterion 3 unmet: the renamed-asset mutation fails identically at `origin/next`, so only 2 of 3 named mutations discriminate. `aidd_docs/memory/coding-assertions.md`: "Break the thing the test is named for and watch that test — not another — go red" | Swap it for the mutation that does discriminate: strip the fence markers from `04-plan.md` keeping the JSON lines as prose. Measured: `origin/next` 0 failures, current guard fails `must teach the example in a json fence` | +| 🟡 | fit | 1 | `:74-75` (`d0e3ea96`) | The stated reason for dropping the one-fence rule — it "would have failed a contributor adding an unrelated json block to 04-plan.md with a message about a rule they never read" — does not hold for a fence added *above* the example. Measured: the guard still fails, now with `04-plan.md must teach the field "backlog" as a non-empty string`, against a file that does teach it. The removal only helps a fence added *below* | Select the fence by content (the first whose parse carries `backlog`) rather than by position; both the explanatory message and the below-case then hold | +| 🟡 | fit | - | `cli/tests/contexts/telemetry/infrastructure/task-backlog-skill-shape.integration.test.ts:71,88` | #972's named harm is still reachable. Measured: a plugins-only edit changing the asset's `backlog` to `owner/repo#999`, or adding a fourth field to one taught file only, leaves this guard green (`pass 3 / fail 0`) and turns the `cli/` test red (`expected 'owner/repo#999' to be 'owner/repo#123'`; `expected [ 'backlog', 'note', …] to deeply equal [ 'backlog', 'written_at', …]`). No gate runs the `cli/` test on those paths | Drop the over-specified value pin, or move field-set agreement between the two taught files into this guard | +| 🟡 | conform | 1 | `7f77e845` commit body | Claims "a renamed asset" among the mutations proving the new guard. The pre-change guard fails on it too, so it is not proof. `coding-assertions.md`: "Never state in a commit message or a report anything not just observed in output" | Reword to the two mutations that discriminate, plus the fence-absence one | +| 🟡 | conform | 1 | `plan.md:3`, `phase-1.md:2` | Both still `status: pending` with every line of the phase shipped. `plugins/aidd-dev/skills/02-implement/actions/02-execute.md:17` requires `status: done` committed with the code; `03-finalize.md:7` needs all phases done | Set both to `done` | +| 🟢 | fit | 1 | `:74` | The residual weakness `d0e3ea96` accepts is real and shared by both guards. Measured: a valid-shape json fence above the taught example lets a *broken* taught example pass the scripts guard (`pass 3`) and the `cli/` test (`5 passed`). Nothing lands red on `next`, so the hole is in the guard's purpose, not in gate coverage | Covered by the content-selection fix above | +| 🟢 | rot | - | `source.md` Observed behaviour; `7f77e845` body | Overstates the gap for the `.json` half: pre-commit `json-validity` (`lefthook.yml:5`, glob `**/*.json`) already failed a syntax-broken asset — `invalid JSON (Expected ',' or '}' ...)`, exit 1. The genuinely new pre-commit coverage is the `.md` fence cases and empty or non-string field values | Scope the claim to the `.md` fence and the field-value cases | +| 🟢 | rot | 1 | `phase-1.md:41-57,69` | The shipped fence-presence assertion (`:75`) appears in no task, test-scope leg or criterion; nor does the one-fence rule added by `bd05645f` and removed by `d0e3ea96`. Task 2 step 4 covers only "both forms readable" | Add the fence-absence leg to Test Scope and criterion 3 | +| 🟢 | code | 1 | `:74` | 104 chars, where the surrounding code breaks such expressions across lines. Nothing enforces it: `biome.json` exists only under `cli/` | Split the ternary | +| 🟢 | code | 1 | `:75` | `assert.ok(source !== undefined, ...)` can never fire on the `.json` branch (`source = text`, always a string), so its message would be wrong if it did | Scope the assertion to the `.md` branch | +| 🟢 | code | 1 | `:81` | A taught file holding literal `null` fails with `TypeError: Cannot read properties of null (reading 'backlog')`, naming no file, where arrays and primitives fail cleanly. The reader absorbs this case (`asPlainObjectOrEmpty`) | Guard the parse result as a plain object before the field loop | + +## Verification + +| Metric | Value | +| ------------- | ------------------------------------------------- | +| Verified | 67% (2/3) | +| Files checked | `scripts/__tests__/a-backlog-link-the-reader-can-read.test.js`, `plugins/aidd-pm/skills/04-spec/assets/backlog-link-template.json`, `plugins/aidd-dev/skills/01-plan/actions/04-plan.md`, `cli/src/contexts/telemetry/infrastructure/task-backlog-adapter.ts`, `cli/src/contexts/telemetry/domain/task-backlog-link.ts`, `cli/tests/contexts/telemetry/infrastructure/task-backlog-skill-shape.integration.test.ts`, `lefthook.yml`, `.github/workflows/cli-ci.yml`, `.github/workflows/validate.yml`, `scripts/validate-json.mjs`, `plan.md`, `phase-1.md`, `backlog-link.json` | +| Unchecked | criterion 3, three discriminating mutations — fix | +| Unplanned | the fence-presence assertion (`:75`); the one-fence rule added by `bd05645f` and removed by `d0e3ea96`, in no plan document; `backlog-link.json` added by `6f021da4`, absent from the phase projection | diff --git a/aidd_docs/tasks/2026_10/2026_10_07_backlog-shape-guard/source.md b/aidd_docs/tasks/2026_10/2026_10_07_backlog-shape-guard/source.md new file mode 100644 index 000000000..061633a0d --- /dev/null +++ b/aidd_docs/tasks/2026_10/2026_10_07_backlog-shape-guard/source.md @@ -0,0 +1,36 @@ +## Description + +`cli/tests/contexts/telemetry/infrastructure/task-backlog-skill-shape.integration.test.ts` reads two plugin files and feeds each one's taught `backlog-link.json` example through the real `TaskBacklogAdapter`. Nothing runs it when only those plugin files change. + +- `lefthook.yml`'s pre-push jobs `cli-knip` and `cli-test` both carry `glob: "cli/**"`. +- `cli-ci.yml`'s `changes` job matches `cli/*|kanban/*|scripts/__tests__/*|README.md`, plus `plugins/aidd-telemetry/*` excluding prose. `plugins/aidd-pm/**` matches nothing. + +So a change confined to the plugin files that test reads breaks it, and neither the local gate nor CI runs it. + +The sibling guard `scripts/__tests__/a-backlog-link-the-reader-can-read.test.js` does run on such a change: `scripts-tests` carries `glob: "{scripts,plugins,cli/src}/**"`. But it asserts only that each field name appears as a substring (`text.includes('"backlog"')`). It never parses the example. The case that falls between the two guards: the taught example stops being valid JSON, or moves, while still containing the three strings. + +## Affected file(s) + +`scripts/__tests__/a-backlog-link-the-reader-can-read.test.js` + +## Expected behaviour + +A change confined to the plugin files that teach `backlog-link.json` fails locally, at pre-commit, when it breaks what the reader accepts. The structural assertions belong in the guard that already runs on `plugins/**`: parse the taught example and require the three fields as non-empty strings. + +## Observed behaviour + +Measured in PR #969. The taught shape moved from a fenced block in `actions/01-build.md` to `assets/backlog-link-template.json`. The `scripts/` guard failed at pre-commit on the field names, which is correct. The `cli/` test failed too, with `expected null not to be null` and `no fenced json example to test` — but only in CI, and only incidentally: that same push also touched `scripts/__tests__/`, which is in the CI filter. A push carrying the plugin change alone would have landed a red test on `next`. + +## AI tool + +Claude Code + +## OS + +macOS + +## Additional context + +Widening either filter to `plugins/**` is the wrong fix: it runs the full 6800-test cli suite on every prose edit. The narrow alternative, listing the two paths the test reads, is the hand-maintained duplication that produced this — those paths just moved. + +The `cli/` test keeps its own job, crossing the boundary into the real adapter, which a repository script test cannot do. diff --git a/scripts/__tests__/a-backlog-link-the-reader-can-read.test.js b/scripts/__tests__/a-backlog-link-the-reader-can-read.test.js index 5e388cbed..a533c6f84 100644 --- a/scripts/__tests__/a-backlog-link-the-reader-can-read.test.js +++ b/scripts/__tests__/a-backlog-link-the-reader-can-read.test.js @@ -7,16 +7,51 @@ const { describe, it } = require("node:test"); const ROOT = path.resolve(__dirname, "../.."); /** - * A task folder declares the backlog item it delivers in `backlog-link.json`, and the whole - * `by_backlog` axis rests on that one file being readable. A writer spelling the fields in - * camelCase while the reader looks for snake_case makes every declaration unreadable, in - * silence. + * The `by_backlog` axis rests on one file being readable, so a field spelled differently on + * either side makes every declaration unreadable in silence. * - * The reader is a `cli/` module and this is a repository script test, so the rule is restated - * here rather than imported across that boundary — and the second case below is what keeps - * the restatement honest. + * The reader lives in `cli/` and this is a repository script test, so the field list is + * restated rather than imported across that boundary. The third case keeps it honest. */ -const REQUIRED_FIELDS = ["backlog", "written_at", "written_by"]; +const IDENTIFYING_FIELD = "backlog"; +const REQUIRED_FIELDS = [IDENTIFYING_FIELD, "written_at", "written_by"]; + +function shapeOf(value) { + if (value === null) return "null"; + if (Array.isArray(value)) return "an array"; + return typeof value; +} + +/** The first json fence, matched with the pattern + * `cli/tests/contexts/telemetry/infrastructure/task-backlog-skill-shape.integration.test.ts` + * uses. No gate runs that sibling on `plugins/**`, so two guards reading different fences + * leave this one green and land its red on `next`. */ +function taughtInMarkdown(file, markdown) { + const fences = [...markdown.matchAll(/^[ \t]*```json\r?\n([\s\S]*?)\r?\n[ \t]*```/gmu)].map((m) => m[1]); + assert.ok(fences.length > 0, `${file} must teach the example in a json fence`); + + const taught = taughtAsJson(file, fences[0]); + assert.ok( + IDENTIFYING_FIELD in taught, + `${file}'s first json fence must be the taught example, carrying "${IDENTIFYING_FIELD}". ` + + `Read ${fences.length} fence(s).` + ); + return taught; +} + +function taughtAsJson(file, text) { + let taught; + try { + taught = JSON.parse(text); + } catch (error) { + assert.fail(`${file} teaches an example that is not JSON: ${error.message}`); + } + assert.ok( + taught !== null && typeof taught === "object" && !Array.isArray(taught), + `${file} must teach an object, not ${shapeOf(taught)}` + ); + return taught; +} function trackedBacklogLinks() { return cp @@ -56,11 +91,10 @@ describe("every backlog declaration in this repository is one the report can rea ); }); - /** The two skills that teach the file are the only things that decide what gets written, - * so they have to teach the same three names, wherever inside each skill the lesson lives — and the same three this test asks for. - * Read from the skills rather than trusted: a taught shape drifting away from the reader - * is exactly what produced the two unreadable files, and a guard restating the fields - * without checking the lesson would have stayed green through it. */ + /** A taught shape drifting away from the reader is what produced the two unreadable files, + * and a guard checking only its own field list would have stayed green through it. The + * example is parsed, not searched: one that keeps the three names while ceasing to be JSON + * is unreadable all the same. */ it("is the shape both skills that write it actually teach", () => { const TEACHING_FILES = [ "plugins/aidd-pm/skills/04-spec/assets/backlog-link-template.json", @@ -69,15 +103,18 @@ describe("every backlog declaration in this repository is one the report can rea for (const file of TEACHING_FILES) { const text = fs.readFileSync(path.join(ROOT, file), "utf8"); + const taught = file.endsWith(".md") ? taughtInMarkdown(file, text) : taughtAsJson(file, text); for (const field of REQUIRED_FIELDS) { - assert.ok(text.includes(`"${field}"`), `${file} must teach the field "${field}"`); + assert.ok( + typeof taught[field] === "string" && taught[field] !== "", + `${file} must teach the field "${field}" as a non-empty string` + ); } } }); - /** The check above pins the writer's side alone, which would stay green through a - * writer/reader disagreement. This reads `TaskBacklogAdapter.read`'s own field access, in - * `cli/` across the boundary this file's header explains it does not import across. */ + /** The check above pins the writer's side alone, so this reads the field access of + * `TaskBacklogAdapter.read` across the boundary the header explains. */ it("is the shape the reader in cli/ actually reads, not just the shape skills teach", () => { const READER_FILE = "cli/src/contexts/telemetry/infrastructure/task-backlog-adapter.ts"; const text = fs.readFileSync(path.join(ROOT, READER_FILE), "utf8");