From 390fa30b2a6b61545530fe8869e2f8861d5f24f3 Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Thu, 8 Oct 2026 06:17:58 +0200 Subject: [PATCH] fix(aidd-dev): a review appends a round and scores the whole plan One phase of a 7-phase plan, one diff, three models: `100% (6/6)` on 7 blocks, `100% (6/6)` on 1 block, `53% (16/34)` where 16/34 is 47%, and boxes copied from the previous run. The skill never said what a phase outside the diff becomes, how the percent is computed, or where a past round lives. A review now appends one section per round and is never rewritten. A round carries its date, its author from `git config user.name`, its diff, the axes that ran, a verdict and a score; its `Criteria` list holds the plan's criteria and its `Findings` list the defects no criterion covers. The score counts over the plan's whole criteria total, in three numbers that sum to it, so a plan covered at a third can no longer read as finished. The skill follows `skill-generate`'s authoring contract: the router holds the flow, the five actions and two transversal rules, and nothing else. `01-prepare` resolves the diff once for the three axes and opens the round; `05-finalize` judges it and checks its shape. `references/report-contract.md` holds where the report lives, what diff is reviewed and how a round joins it. An action says what to decide; the template owns the names and the form, the validator the field set. The skill reads 1583 words, against 1785 on `next`. Severity reads `critical`, `major`, `minor`: one scale, where `warning` put a log level between two gravities. Three files outside this skill still say `warning`. A critical is the harm a merge would do to production, its users, their data or what depends on it, never how strictly a rule is worded. Before, a broken declared rule read critical in 7 of 12 runs and a crash in production blocked in 3 of 5; now 0 of 10 and 5 of 5, over 50 runs on ten defect fixtures with every verdict unanimous. Measured, each with the mutation that proves it: 31 assertions, 39 mutations, each reddening one assertion alone. The assertions spell a path the way the documents spell it, so Windows and macOS read the same tree. 71 real runs on the final text: 50 of 50 functional runs over 10 fixtures scored right, including 10 phases and 34 criteria reading `3/34 met, 5 unmet, 26 out of the diff`; 3 of 3 runs without a plan read `not scored`; 33 of 33 planted defects found across the three axes; no run patched a file. Against `next` on the same fixtures, counting checked boxes against the truth: 6 wrong runs of 18. Four rules come from those campaigns, each turning a split between runs into unanimity: a context line of the diff is evidence, a criterion whose subject is absent is a gap rather than `not-applicable`, a phase projecting no file is traced rather than out of the diff, and a label echoing the criterion's words is not evidence. Refs #858 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VE16AxC1V9wAMGmGgefe4z AIDD-Session-Id: bf713ca4-3c1b-491d-ac0a-4d4533330b5c --- .../backlog-link.json | 1 + .../2026_10_07_review-phase-scope/phase-1.md | 74 +++ .../2026_10_07_review-phase-scope/phase-2.md | 70 +++ .../2026_10_07_review-phase-scope/plan.md | 36 ++ .../2026_10_07_review-phase-scope/review.md | 454 ++++++++++++++++++ plugins/aidd-dev/CATALOG.md | 11 +- plugins/aidd-dev/skills/05-review/SKILL.md | 49 +- .../skills/05-review/actions/01-prepare.md | 25 + .../05-review/actions/01-review-code.md | 23 - .../05-review/actions/02-review-code.md | 24 + .../05-review/actions/02-review-functional.md | 26 - .../05-review/actions/03-review-functional.md | 33 ++ .../05-review/actions/03-review-relevancy.md | 24 - .../05-review/actions/04-review-relevancy.md | 26 + .../skills/05-review/actions/05-finalize.md | 23 + .../05-review/assets/review-template.md | 40 +- .../05-review/assets/review-validator.yml | 19 +- .../05-review/references/report-contract.md | 27 ++ .../05-review/references/review-rubric.md | 31 +- ...-a-round-and-scores-the-whole-plan.test.js | 328 +++++++++++++ 20 files changed, 1193 insertions(+), 151 deletions(-) create mode 100644 aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/backlog-link.json create mode 100644 aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/phase-1.md create mode 100644 aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/phase-2.md create mode 100644 aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/plan.md create mode 100644 aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/review.md create mode 100644 plugins/aidd-dev/skills/05-review/actions/01-prepare.md delete mode 100644 plugins/aidd-dev/skills/05-review/actions/01-review-code.md create mode 100644 plugins/aidd-dev/skills/05-review/actions/02-review-code.md delete mode 100644 plugins/aidd-dev/skills/05-review/actions/02-review-functional.md create mode 100644 plugins/aidd-dev/skills/05-review/actions/03-review-functional.md delete mode 100644 plugins/aidd-dev/skills/05-review/actions/03-review-relevancy.md create mode 100644 plugins/aidd-dev/skills/05-review/actions/04-review-relevancy.md create mode 100644 plugins/aidd-dev/skills/05-review/actions/05-finalize.md create mode 100644 plugins/aidd-dev/skills/05-review/references/report-contract.md create mode 100644 scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js diff --git a/aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/backlog-link.json b/aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/backlog-link.json new file mode 100644 index 000000000..734ddb990 --- /dev/null +++ b/aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/backlog-link.json @@ -0,0 +1 @@ +{"backlog":"ai-driven-dev/framework#858","written_at":"2026-10-07T20:05:00Z","written_by":"aidd-dev:01-plan"} diff --git a/aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/phase-1.md b/aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/phase-1.md new file mode 100644 index 000000000..b32c25f6d --- /dev/null +++ b/aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/phase-1.md @@ -0,0 +1,74 @@ +--- +status: done +--- + +# Instruction: the round section and the phase scope +## Architecture projection +```txt +. +├── plugins/aidd-dev/skills/05-review +│ ├── SKILL.md ✏️ the flow, the five actions, the one transversal rule +│ ├── actions/01-prepare.md ✏️ resolve the diff once, open the round +│ ├── actions/03-review-functional.md ✏️ the phase projection decides scope +│ ├── actions/05-finalize.md ✏️ judge the round, check its shape +│ ├── references/report-contract.md ✏️ the file, the diff, how a round joins it +│ ├── assets/review-template.md ✏️ the round section and its Out of the diff line +│ └── assets/review-validator.yml ✏️ a round's header, fields and lists +└── scripts/__tests__ + └── a-review-appends-a-round-and-scores-the-whole-plan.test.js ✅ the invariants +``` +## User Journey +```mermaid +flowchart TD + A[round 1 on the cart phase] --> B[section appended] + B --> C[round 2 on the same plan] + C --> D[a second section appended, round 1 untouched] + D --> E[nothing read from round 1] +``` +## Test Scope +```mermaid +--- +title: Test scope +--- +journey + section Setup + Write a plan with one phase in the diff and two outside => fixture ready: 5: cli + Seed a round 1 claiming every criterion met, citing files the diff never touched => trap ready: 5: cli + section Happy path + Run the functional axis with --plugin-dir => a round 2 derived from the diff alone: 5: cli + Run it again after a fix => a round 3, rounds 1 and 2 untouched: 5: cli + section Edge case - every phase outside the diff + A diff touching no projected file => one Out of the diff line per phase, no box: 1: cli + section Teardown + Delete the fixture folder => worktree clean: 5: cli +``` +## Tasks to do +### `1)` The guard goes red first +> Invariants nothing else holds. + +1. Assert the re-run rule appends, refuses an earlier round, and says where the number comes from. +2. Assert the validator's field set and the template render the same round, in order. +3. Assert a phase's projected files have one address. +4. Run the suite and watch each fail, nothing else. + +### `2)` A round becomes a section +> History gets a home that cannot be mistaken for the current state. + +1. In `SKILL.md`, append a round numbered one past the last, derived from the plan and the diff alone. +2. Add the round section to the template: its fields, then `Criteria` and `Findings`. +3. Put a round's fields and lists in the validator; drop the closed set of four sections. +4. Drop from the header what a round's fields already carry. + +### `3)` The projection decides scope +> A phase outside the diff is present and visibly unverified. + +1. A phase whose projected files the diff leaves untouched gets one `Out of the diff:` line, no box. +2. The line carries the plan's own phase name and its criteria count, written in plan order. +3. Name `## Architecture projection` as the one address where those files are read. + +## Test acceptance criteria +| Task | Acceptance criteria | +| --- | --- | +| 1 | Each round and scope assertion reddens alone under the mutation of the rule it names. | +| 2 | Runtime, no diff can show it: with a round 1 claiming every criterion met on files the diff never touched, a second run appends a round scored from the diff alone and leaves round 1 untouched. | +| 3 | A phase outside the diff gets one `Out of the diff:` line, named as the plan names it, in plan order, holding no box. | diff --git a/aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/phase-2.md b/aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/phase-2.md new file mode 100644 index 000000000..39850a36e --- /dev/null +++ b/aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/phase-2.md @@ -0,0 +1,70 @@ +--- +status: done +--- + +# Instruction: the score and the statuses +## Architecture projection +```txt +. +├── plugins/aidd-dev/skills/05-review +│ ├── actions/02-review-code.md ✏️ a finding is a line, keyed on a file:line +│ ├── actions/03-review-functional.md ✏️ the score, not-applicable, evidence over a label +│ ├── actions/04-review-relevancy.md ✏️ a misfit is a line, keyed on a file:line +│ ├── assets/review-template.md ✏️ the Score field and the finding's fix +│ └── references/review-rubric.md ✏️ fixed removed, the verdict rules scoped +└── scripts/__tests__ + └── a-review-appends-a-round-and-scores-the-whole-plan.test.js ✅ the score and status assertions +``` +## User Journey +```mermaid +flowchart TD + A[the plan's criteria total] --> B[met, unmet, out of the diff] + B --> C[three numbers that sum to it] + A -->|no plan given| D[not scored, never approve] + B -->|no diff could ever show it| E[not-applicable, counts as met] +``` +## Test Scope +```mermaid +--- +title: Test scope +--- +journey + section Happy path + One phase of two in the diff, one criterion met => 1/6 met, 1 unmet, 4 out of the diff: 5: cli + Run the three axes => findings keyed on a file:line, each carrying its fix: 5: cli + section Edge case - nothing countable + Run with no plan and no criteria => not scored, and no approve: 1: cli + section Edge case - nothing a diff can show + A criterion on runtime timing => not-applicable, counted as met: 2: cli + section Teardown + Delete the fixture folder => worktree clean: 5: cli +``` +## Tasks to do +### `1)` The guard goes red first +> One assertion per rule, each with the mutation that proves it. + +1. Assert the score counts over the plan's whole total and that its three numbers sum to it. +2. Assert an unmet criterion has one home, and that no file of the skill sends it to `Findings`. +3. Assert an unscored round never approves, that a criterion no diff could show is `not-applicable`, and that a finding carries its fix. + +### `2)` The score replaces the percent +> One denominator, and three numbers a reader can add up. + +1. `Score` counts met, unmet and out of the diff over the plan's criteria total. +2. The template renders the form; the action states the rule and names no placeholder. +3. `Score` reads "not scored" when the functional axis did not run. + +### `3)` The statuses say what they mean +> A tag the review applies, and a verdict that cannot approve nothing. + +1. `fixed` appears in no verdict. +2. `not-applicable` marks a criterion no diff could ever show, counts as met, and says so on its line. +3. A round without functional takes its verdict from its findings alone. +4. A finding is a `Findings` line keyed on a backticked `file:line`, carrying its fix, severity and kind. + +## Test acceptance criteria +| Task | Acceptance criteria | +| --- | --- | +| 1 | Each score and status assertion reddens alone under the mutation of the rule it names. | +| 2 | A plan of 6 criteria whose diff touches one phase of 2, with 1 met, reads `1/6 met, 1 unmet, 4 out of the diff`; with no plan it reads `not scored`. | +| 3 | A criterion on runtime timing is checked `not-applicable` and counted as met; a round with no criterion in scope does not read `approve`; a three-axis run writes findings keyed on a `file:line`, each with its fix. | diff --git a/aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/plan.md b/aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/plan.md new file mode 100644 index 000000000..e53ff5837 --- /dev/null +++ b/aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/plan.md @@ -0,0 +1,36 @@ +--- +objective: "A review appends one round per run and scores it over the whole plan, so three models produce one report." +status: implemented +--- + +# Plan: a review appends a round and scores the whole plan +## Overview +| Field | Value | +| --- | --- | +| **Goal** | `05-review` says what happens to a phase outside the diff, how the count is computed, and where a past round lives, so three models produce one report. | +| **Source** | [ai-driven-dev/framework#858](https://github.com/ai-driven-dev/framework/issues/858) | + +## Phases +| # | Phase | File | +| --- | --- | --- | +| 1 | The round section and the phase scope | [`phase-1.md`](./phase-1.md) | +| 2 | The score and the statuses | [`phase-2.md`](./phase-2.md) | + +## Resources +| Source | Verified | +| --- | --- | +| [#858](https://github.com/ai-driven-dev/framework/issues/858) | Three models, one diff, three reports: `100% (6/6)` on 7 blocks, `100% (6/6)` on 1 block, `53% (16/34)` where 16/34 is 47%. | + +## Decisions +| Decision | Why | +| --- | --- | +| A round is a section, appended, and an earlier one is never read | the Check zone re-enters after every candidate, so rounds are the norm, and the artifact held one. Rewriting the report shows a state and never a progression, which is why nobody could read what a round had fixed. A section whose criterion text repeats from round to round makes the fix readable by eye, and makes copying easy: measured on a fixture whose round 1 claims `6/6 met` with citations on files the diff never touched, 9 runs out of 9 scored `1/6 met, 1 unmet, 4 out of the diff` and left round 1 untouched. | +| The score counts over the plan's whole total, in three numbers that sum to it | two fractions with different denominators cannot be read. `100% (4/4)` on a plan covered at a third was the defect, and one denominator removes it: `1/6 met, 1 unmet, 4 out of the diff`. | +| An unmet criterion stays in `Criteria`, a finding is keyed on a `file:line` | only the functional axis owns criteria; `01-review-code` and `03-review-relevancy` emit findings keyed on a changed `file:line`. Two line types, one key each, told apart by the first token, so no sub-section is needed. Measured: before the rule, 1 run of 3 wrote the same unmet criterion in both lists. | +| `review.md` records no model name | the only use found was comparing models, a framework-development need. `architecture.md` gives observation its own layer and `aidd-telemetry` already journals the tool by hook, verifiably. Measured: the field self-declared three different spellings of one model, and a false one under another. The human name comes from `git config user.name`. | +| The diff has one home, in `report-contract.md` | the three actions each resolved it, and "a git ref range, defaulting to the default branch" gave `main` while the work targets `next`: measured, a two-dot `git diff origin/next` showed 88 files against the change's 13, so two runs on one change traced two diffs. One reference row, `...HEAD` plus the working tree, replaces five copies, and `01-prepare` resolves it once for the three axes. | +| Only a finding blocks, never a criterion | a `Criteria` line carries no severity, and the plan template gives a criterion none, so "an unchecked criterion the plan words as critical" was a signal no file holds: two models split `blocked` from `changes-requested` on one diff. An unchecked criterion already forces `changes-requested`, so nothing is lost. | +| The skill is written to `skill-authoring.md` | `04-skill-generate` carries the contract every generated skill satisfies, and 05-review broke R6, R8, R13 and R18: no flow, an `| # | Action | Axis |` table, three `## Test` lists, and `## References` and `## Assets` blocks. Reshaped, the five known fixtures reproduce their values to the word, so the format cost no behaviour. | +| The template owns the forms, the action owns the rules | `SKILL.md` already makes the template the rendering source. A rule naming `{{pct}}` couples a rule file to a rendering, and the guard refuses a placeholder inside a rule. | +| The guard lives in `scripts/__tests__/` | a plugin ships no tests of its own: `hooks/` copies recursively into user projects. The pre-commit `scripts-tests` job globs `{scripts,plugins,cli/src}/**`, so editing this skill runs the guard. | +| Every run is measured with `claude --plugin-dir "$PWD/plugins/aidd-dev"` | without it the Skill tool resolves `~/.claude/plugins/cache/aidd-framework/aidd-dev/2.6.0`, byte-identical to this branch until the first edit. Measured: the flag makes the skill answer `BASE=plugins/aidd-dev/skills/05-review`. | diff --git a/aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/review.md b/aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/review.md new file mode 100644 index 000000000..5a7d67585 --- /dev/null +++ b/aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/review.md @@ -0,0 +1,454 @@ +# Review: a review appends a round and scores the whole plan + +- Rounds: 19 +- Started: 2026-10-08 + + + +## Round 1 · r-4c1a + +- Date: 2026-10-08 +- By: Baptiste LAFOURCADE +- Diff: `origin/next...worktree` +- Axes: functional +- Verdict: changes-requested +- Score: 2/6 met, 4 unmet, 0 out of the diff + +### Criteria + +- [ ] each assertion of phase 1 reddens alone — no guard written yet for this shape +- [x] a round 1 claiming every criterion met leaves no trace in round 2 — 3 runs of 3 scored `1/6 met, 1 unmet, 4 out of the diff` and left round 1 untouched +- [ ] the `Out of the diff:` line is named as the plan names it, in plan order — 2 runs grouped the lines at the end, 1 kept plan order; phase named `auth` twice and `Phase 1 - auth` once +- [ ] each assertion of phase 2 reddens alone — no guard written yet for this shape +- [x] the score's three numbers sum to the plan's total — `1/6 met, 1 unmet, 4 out of the diff`, 3 runs of 3 +- [ ] `not-applicable`, the unscored verdict and the finding's fix hold — none of the three was written yet, and 1 run of 3 wrote an unmet criterion in `Findings` as well as in `Criteria` + +## Round 2 · r-9d72 + +- Date: 2026-10-08 +- By: Baptiste LAFOURCADE +- Diff: `origin/next...worktree` +- Axes: functional +- Verdict: changes-requested +- Score: 5/6 met, 1 unmet, 0 out of the diff + +### Criteria + +- [x] each assertion of phase 1 reddens alone — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:30, 37, 51 +- [x] a round 1 claiming every criterion met leaves no trace in round 2 — 3 further runs on the worktree tree, same score, round 1 untouched; and a round 3 over two past rounds read `2/6 met, 0 unmet, 4 out of the diff` +- [x] the `Out of the diff:` line is named as the plan names it, in plan order — actions/02-review-functional.md:16; 3 runs of 3 identical +- [x] each assertion of phase 2 reddens alone — same guard file, 5 assertions, each red under its own mutation +- [x] the score's three numbers sum to the plan's total — 6 runs of 6 +- [ ] `not-applicable`, the unscored verdict and the finding's fix hold — a run with no plan read `Score: not scored` and still `Verdict: approve`; a criterion on runtime timing was written as a plain gap, never tagged; a three-axis run had no slot for the fix, which bled into the issue text + +## Round 3 · r-6b05 + +- Date: 2026-10-08 +- By: Baptiste LAFOURCADE +- Diff: `origin/next...worktree` +- Axes: functional +- Verdict: approve +- Score: 6/6 met, 0 unmet, 0 out of the diff + +### Criteria + +- [x] each assertion of phase 1 reddens alone — 8 assertions, each red under its own mutation +- [x] a round 1 claiming every criterion met leaves no trace in round 2 — 9 runs of 9 across the session, round 1 untouched every time +- [x] the `Out of the diff:` line is named as the plan names it, in plan order — 6 runs of 6 identical +- [x] each assertion of phase 2 reddens alone — same guard file +- [x] the score's three numbers sum to the plan's total — including `2/7 met, 1 unmet, 4 out of the diff` with one criterion `not-applicable` +- [x] `not-applicable`, the unscored verdict and the finding's fix hold — fixed since r-9d72: no plan reads `changes-requested`; a timing criterion reads `[x] ... — not-applicable, runtime performance not observable in a static diff; counts as met`; a three-axis run writes `` `src/cart.js:7` : missing or non-numeric price turns total into NaN → validate it is a finite number — 🟡 warning, error-handling `` + +## Round 4 · r-b068 + +- Date: 2026-10-08 +- By: Baptiste LAFOURCADE +- Diff: `origin/next...worktree` +- Axes: code, functional, relevancy +- Verdict: changes-requested +- Score: 6/6 met, 0 unmet, 0 out of the diff + +### Criteria + +- [x] Each assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:29, 36, 43; 3 mutations, each red on its own test only +- [x] With a round 1 claiming every criterion met on files the diff never touched, a second run appends a round scored from the diff alone and leaves round 1 untouched. — plugins/aidd-dev/skills/05-review/SKILL.md:30 +- [x] A phase outside the diff gets one `Out of the diff:` line, named as the plan names it, in plan order, holding no box. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:12 +- [x] Each assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:54, 65, 69, 74, 78; 5 mutations, each red on its own test only +- [x] A plan of 6 criteria whose diff touches one phase of 2, with 1 met, reads `1/6 met, 1 unmet, 4 out of the diff`; with no plan it reads `not scored`. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:13 +- [x] A criterion on runtime timing is checked `not-applicable` and counted as met; a round scoring nothing does not read `approve`; a three-axis run writes findings keyed on a `file:line`, each with its fix. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:12, plugins/aidd-dev/skills/05-review/references/review-rubric.md:15, plugins/aidd-dev/skills/05-review/assets/review-template.md:25 + +### Findings + +- [ ] `.claude/settings.json:27` : unstaged edit empties `enabledPlugins` and `extraKnownMarketplaces`, so this checkout no longer registers itself as the local marketplace (`codebase-map.md`, `.claude/` row); unrelated to #858 → keep it out of the commit — 🟡 warning, fit +- [ ] `plugins/aidd-dev/skills/05-review/SKILL.md:30` : "says since which round" and the template's `fixed since r-{{id}}` line need an earlier round's ids, which the same rule forbids reading → either allow reading earlier round ids and finding keys only, or drop the `fixed since` line and the "since which round" clause — 🟡 warning, rot +- [ ] `plugins/aidd-dev/skills/05-review/references/review-rubric.md:15` : the verdict rule never says whether an `Out of the diff:` criterion blocks `approve`, so `1/6 met, 0 unmet, 5 out of the diff` can read `approve` in one run and `changes-requested` in another, the cross-model divergence #858 targets → state that out-of-the-diff criteria are neither unchecked nor blocking, or that they block — 🟡 warning, fit +- [ ] `plugins/aidd-dev/skills/05-review/SKILL.md:40` : still describes the validator as "the closed set of report sections", but it now holds a round's fields and lists → "the closed set of a round's fields and lists" — 🟢 minor, rot +- [ ] `plugins/aidd-dev/skills/05-review/assets/review-template.md:20` : the template owns the forms (plan decision) yet renders neither the `not-applicable` criterion line nor `Score: not scored`, both left to prose in `02-review-functional.md:11-13` → add both forms to the template — 🟢 minor, rot +- [ ] `.gitignore:73` : personal `tokenade` cache entries, unrelated to #858 → drop from this branch, or move to a global gitignore — 🟢 minor, fit + +## Round 5 · r-a52f + +- Date: 2026-10-08 +- By: Baptiste LAFOURCADE +- Diff: `84dafd69...c9c182f2+worktree` +- Axes: code, functional, relevancy +- Verdict: changes-requested +- Score: 6/6 met, 0 unmet, 0 out of the diff + +### Criteria + +- [x] Each assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:28 +- [x] With a round 1 claiming every criterion met on files the diff never touched, a second run appends a round scored from the diff alone and leaves round 1 untouched. — plugins/aidd-dev/skills/05-review/SKILL.md:30 +- [x] A phase outside the diff gets one `Out of the diff:` line, named as the plan names it, in plan order, holding no box. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:12 +- [x] Each assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:96 +- [x] A plan of 6 criteria whose diff touches one phase of 2, with 1 met, reads `1/6 met, 1 unmet, 4 out of the diff`; with no plan it reads `not scored`. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:13 +- [x] A criterion on runtime timing is checked `not-applicable` and counted as met; a round scoring nothing does not read `approve`; a three-axis run writes findings keyed on a `file:line`, each with its fix. — plugins/aidd-dev/skills/05-review/references/review-rubric.md:15 + +### Findings + +- `.claude/settings.json:27` : unstaged edit empties `enabledPlugins` and `extraKnownMarketplaces`, unregistering the checkout as the local marketplace `.claude/` exists for; no plan task asks for it → restore the file from `origin/next` and keep it out of the commit — 🔴 critical, fit +- `plugins/aidd-dev/skills/05-review/references/review-rubric.md:15` : "an unscored round never approves" also catches a code-only or relevancy-only round, which `SKILL.md` allows when the caller names one axis, so a clean single-axis review must read `changes-requested` with nothing to change → scope the rule to a round where functional ran without criteria — 🟡 warning, fit +- `plugins/aidd-dev/skills/05-review/assets/review-template.md:3` : `Rounds:` duplicates the round headings and no rule says who rewrites it, beside a Re-run rule that forbids editing earlier content → drop the field, or name it in `SKILL.md`'s Re-run rule as the one header line each round updates — 🟢 minor, rot +- `plugins/aidd-dev/skills/05-review/references/review-rubric.md:19` : "An unchecked criterion is a finding" contradicts the validator's `Findings # defects no criterion covers` and the plan's one-home decision → "is a gap that stays in `Criteria`" — 🟢 minor, rot +- `plugins/aidd-dev/skills/05-review/actions/01-review-code.md:17` : "One row per issue" survives the table's removal; the output is now a `Findings` line → "One finding per issue" — 🟢 minor, rot +- `scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:51` : `slice(-fields.length)` compares only the template's last six `- Word:` lines, so a field inserted in the round before `Date` passes (mutation `- Extra: x` above `- Date:` left all 11 tests green) → slice the template from `## Round` and compare the whole list — 🟢 minor, code +- `.gitignore:73` : unstaged tokenade cache entries are personal tooling, outside the plan → move them to `.git/info/exclude` or a global gitignore — 🟢 minor, fit +- `aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/phase-2.md:66` : phase 2's task-1 criterion repeats phase 1's text word for word, and `SKILL.md:27` makes a criterion's text its key with no phase grouping in `Criteria`, so the two lines are indistinguishable across rounds → reword one criterion, or key a criterion on its phase plus its text — 🟢 minor, rot + +## Round 6 · r-3e8d + +- Date: 2026-10-08 +- By: Baptiste LAFOURCADE +- Diff: `origin/next...worktree` +- Axes: code, functional, relevancy +- Verdict: changes-requested +- Score: 5/6 met, 1 unmet, 0 out of the diff + +### Criteria + +- [x] Each round and scope assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:29 +- [x] With a round 1 claiming every criterion met on files the diff never touched, a second run appends a round scored from the diff alone and leaves round 1 untouched. — plugins/aidd-dev/skills/05-review/SKILL.md:30 +- [x] A phase outside the diff gets one `Out of the diff:` line, named as the plan names it, in plan order, holding no box. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:12 +- [x] Each score and status assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:55 +- [x] A plan of 6 criteria whose diff touches one phase of 2, with 1 met, reads `1/6 met, 1 unmet, 4 out of the diff`; with no plan it reads `not scored`. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:13 +- [ ] A criterion on runtime timing is checked `not-applicable` and counted as met; a round scoring nothing does not read `approve`; a three-axis run writes findings keyed on a `file:line`, each with its fix. — `not-applicable` is `[x]` in assets/review-template.md:21 but sits under "leave `[ ]`" in actions/02-review-functional.md:12, and references/review-rubric.md:19 still calls it an unchecked criterion; the box a run writes depends on which file it follows + +### Findings + +- `plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:12` : a phase with no `## Architecture projection`, or one listing no file, has no defined scope: "projected files the diff leaves untouched" is vacuously true, so one run writes it out of the diff and another traces it → state that a phase projecting no file is traced, never out of the diff — 🟡 warning, fit +- `plugins/aidd-dev/skills/05-review/actions/01-review-code.md:7` : the plan is still an input "to tag each finding's phase", but the finding line has no phase slot since the table's `Phase` column went → drop the clause, or add a phase slot to the template's finding line — 🟢 minor, rot +- `plugins/aidd-dev/skills/05-review/SKILL.md:27` : "Boxes only, no prose" contradicts line 30's "A finding carries no box" → "Boxes and finding lines only, no prose" — 🟢 minor, rot +- `plugins/aidd-dev/skills/05-review/assets/review-validator.yml:10` : `Criteria` "Absent when functional did not run" omits the case `02-review-functional.md:11` adds, functional run with no criteria available → "Absent when functional did not run or had no criteria" — 🟢 minor, rot + +## Round 7 · r-356a + +- Date: 2026-10-08 +- By: Baptiste LAFOURCADE +- Diff: `84dafd69...worktree` +- Axes: code, functional, relevancy +- Verdict: changes-requested +- Score: 5/6 met, 1 unmet, 0 out of the diff + +### Criteria + +- [x] Each round and scope assertion reddens alone under the mutation of the rule it names. — not-applicable, a mutation run is runtime; one assertion per rule at `scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:29` +- [x] With a round 1 claiming every criterion met on files the diff never touched, a second run appends a round scored from the diff alone and leaves round 1 untouched. — plugins/aidd-dev/skills/05-review/SKILL.md:30 +- [x] A phase outside the diff gets one `Out of the diff:` line, named as the plan names it, in plan order, holding no box. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:12 +- [x] Each score and status assertion reddens alone under the mutation of the rule it names. — not-applicable, a mutation run is runtime; one assertion per rule at `scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:111` +- [x] A plan of 6 criteria whose diff touches one phase of 2, with 1 met, reads `1/6 met, 1 unmet, 4 out of the diff`; with no plan it reads `not scored`. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:13 +- [ ] A criterion on runtime timing is checked `not-applicable` and counted as met; a round scoring nothing does not read `approve`; a three-axis run writes findings keyed on a `file:line`, each with its fix. — a diff touching no projected file scores `0/6 met, 0 unmet, 6 out of the diff` and still reads `approve`: review-rubric.md:15 exempts out-of-the-diff criteria and refuses only a round "without criteria" + +### Findings + +- `plugins/aidd-dev/skills/05-review/references/review-rubric.md:17` : `blocked` keys on "an unchecked critical criterion", but a `Criteria` line carries no severity since the `functional` row was dropped, so nothing marks a criterion critical → state where a criterion's criticality is read (the plan's wording), or drop the clause — 🟢 minor, rot +- `plugins/aidd-dev/skills/05-review/SKILL.md:29` : a round where functional did not run reads `not scored` and may still `approve`, while phase-2 task 3.3 says "An unscored round never approves" and the rubric refuses only functional-without-criteria → align the rubric and the plan on one rule for an unscored round — 🟢 minor, fit +- `plugins/aidd-dev/skills/05-review/assets/review-template.md:8` : the round id `r-` is drawn every round but no rule, test or plan task reads it → name what it is for in `SKILL.md`, or drop it from the template and validator — 🟢 minor, rot +- `plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:2` : blank lines after the headings were removed, unlike `01-review-code.md` and `03-review-relevancy.md` → restore them — 🟢 minor, code + +## Round 8 · r-8d93 + +- Date: 2026-10-08 +- By: Baptiste LAFOURCADE +- Diff: `84dafd69...working tree` +- Axes: code, functional, relevancy +- Verdict: changes-requested +- Score: 6/6 met, 0 unmet, 0 out of the diff + +### Criteria + +- [x] Each round and scope assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:254 +- [x] With a round 1 claiming every criterion met on files the diff never touched, a second run appends a round scored from the diff alone and leaves round 1 untouched. — plugins/aidd-dev/skills/05-review/SKILL.md:30 +- [x] A phase outside the diff gets one `Out of the diff:` line, named as the plan names it, in plan order, holding no box. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:13 +- [x] Each score and status assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:297 +- [x] A plan of 6 criteria whose diff touches one phase of 2, with 1 met, reads `1/6 met, 1 unmet, 4 out of the diff`; with no plan it reads `not scored`. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:21 +- [x] A criterion on runtime timing is checked `not-applicable` and counted as met; a round with no criterion in scope does not read `approve`; a three-axis run writes findings keyed on a `file:line`, each with its fix. — plugins/aidd-dev/skills/05-review/references/review-rubric.md:15 + +### Findings + +- `plugins/aidd-dev/skills/05-review/references/review-rubric.md:15` : a clean round with no criterion in scope (a code-only or relevancy-only run, which SKILL.md allows, or a diff leaving every phase out) meets no verdict: `approve` is refused, and neither `changes-requested` nor `blocked` condition holds → name the verdict such a round gets — 🟡 warning, fit +- `plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:22` : "No criterion is checked without a bare `file:line`" contradicts Trace (:13) and the template (`assets/review-template.md:21`), where a `not-applicable` criterion is checked with a why and no `file:line` → except `not-applicable` in the test line — 🟡 warning, rot +- `scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:263` : the scope rule itself, one `Out of the diff:` line with the plan's phase name and criteria count (`actions/02-review-functional.md:13`, `assets/review-template.md:22`), has no guard: deleting either leaves the suite 12/12 green → assert both, each with its mutation — 🟡 warning, code +- `plugins/aidd-dev/skills/05-review/assets/review-template.md:3` : header lines `Rounds` and `Started` sit outside `review-validator.yml`, the declared closed set, so their shape is checked nowhere → declare a header in the validator or name it in SKILL.md's Sections rule — 🟢 minor, conform + +## Round 9 · r-1936 + +- Date: 2026-10-08 +- By: Baptiste LAFOURCADE +- Diff: `origin/next...HEAD` plus the working tree +- Axes: code, functional, relevancy +- Verdict: changes-requested +- Score: 6/6 met, 0 unmet, 0 out of the diff + +### Criteria + +- [x] Each round and scope assertion reddens alone under the mutation of the rule it names. — not-applicable, a mutation run is runtime, no diff can show it +- [x] With a round 1 claiming every criterion met on files the diff never touched, a second run appends a round scored from the diff alone and leaves round 1 untouched. — plugins/aidd-dev/skills/05-review/SKILL.md:30 +- [x] A phase outside the diff gets one `Out of the diff:` line, named as the plan names it, in plan order, holding no box. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:13 +- [x] Each score and status assertion reddens alone under the mutation of the rule it names. — not-applicable, a mutation run is runtime, no diff can show it +- [x] A plan of 6 criteria whose diff touches one phase of 2, with 1 met, reads `1/6 met, 1 unmet, 4 out of the diff`; with no plan it reads `not scored`. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:21 +- [x] A criterion on runtime timing is checked `not-applicable` and counted as met; a round with no criterion in scope does not read `approve`; a three-axis run writes findings keyed on a `file:line`, each with its fix. — plugins/aidd-dev/skills/05-review/references/review-rubric.md:15 + +### Findings + +- `plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:6` : the diff is "a git ref range", defaulting to the default branch (`main` here, while work targets `next`); a two-dot `git diff origin/next` on a branch one commit behind also shows #971 reverted (88 files against 13), so two runs on one change trace different diffs, against #858's one-report goal → resolve the diff from the merge base (`...HEAD` plus the working tree) in the Input of all three actions, matching the template's three-dot `Diff:` field — 🟡 warning, fit +- `plugins/aidd-dev/skills/05-review/SKILL.md:30` : "its id drawn once so two rounds appended in parallel never collide" holds for the id only; two parallel runs both write `Round n+1` and both set `Rounds` to n+1, so the header undercounts → say the id, not the number, is what stays unique in parallel, or derive `Rounds` from the count of round sections — 🟢 minor, rot +- `scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:61` : the Trace-step extraction regex is repeated at lines 61, 67, 90, 95 (and the Re-run one at 30, 107) → one `step(file, pattern)` helper beside `read()` — 🟢 minor, code-health + +## Round 10 · r-bf43 + +- Date: 2026-10-08 +- By: Baptiste LAFOURCADE +- Diff: `origin/next...HEAD` plus the working tree +- Axes: code, functional, relevancy +- Verdict: changes-requested +- Score: 6/6 met, 0 unmet, 0 out of the diff + +### Criteria + +- [x] Each round and scope assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:33 +- [x] With a round 1 claiming every criterion met on files the diff never touched, a second run appends a round scored from the diff alone and leaves round 1 untouched. — plugins/aidd-dev/skills/05-review/SKILL.md:31 +- [x] A phase outside the diff gets one `Out of the diff:` line, named as the plan names it, in plan order, holding no box. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:13 +- [x] Each score and status assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:125 +- [x] A plan of 6 criteria whose diff touches one phase of 2, with 1 met, reads `1/6 met, 1 unmet, 4 out of the diff`; with no plan it reads `not scored`. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:21 +- [x] A criterion on runtime timing is checked `not-applicable` and counted as met; a round with no criterion in scope does not read `approve`; a three-axis run writes findings keyed on a `file:line`, each with its fix. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:14 + +### Findings + +- `plugins/aidd-dev/skills/05-review/references/review-rubric.md:15` : a single-axis code or relevancy round, which `SKILL.md:20` allows and `SKILL.md:32` verdicts on the axes run only, has no criterion in scope, so a clean run reads `changes-requested` with nothing to change → scope "a round with no criterion in scope never approves" to rounds where functional ran; a round without functional takes its verdict from its findings — 🟡 warning, rot +- `plugins/aidd-dev/skills/05-review/references/review-rubric.md:17` : `blocked` now comes only from a critical finding, and an unmet criterion never becomes one, so an unmet critical criterion can no longer block, a drop the plan's Decisions do not record → state whether an unmet critical criterion yields `blocked`, in the rubric — 🟢 minor, fit +- `plugins/aidd-dev/skills/05-review/SKILL.md:31` : two rounds appended in parallel both take "one past the last", so they share a number and `Rounds` undercounts the sections; the id tells them apart, the header stays false → derive the number and `Rounds` from the sections present at write time, or drop the parallel claim — 🟢 minor, rot + +## Round 11 · r-2fd8 + +- Date: 2026-10-08 +- By: Baptiste LAFOURCADE +- Diff: `origin/next...HEAD` +- Axes: code, functional, relevancy +- Verdict: changes-requested +- Score: 4/6 met, 2 unmet, 0 out of the diff + +### Criteria + +- [ ] Each round and scope assertion reddens alone under the mutation of the rule it names. — renaming the `Criteria` list in `plugins/aidd-dev/skills/05-review/assets/review-validator.yml:13` keeps 13/13 green: the test asserts the validator's `fields`, never its `lists` +- [x] With a round 1 claiming every criterion met on files the diff never touched, a second run appends a round scored from the diff alone and leaves round 1 untouched. — plugins/aidd-dev/skills/05-review/SKILL.md:31 +- [x] A phase outside the diff gets one `Out of the diff:` line, named as the plan names it, in plan order, holding no box. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:13 +- [ ] Each score and status assertion reddens alone under the mutation of the rule it names. — each mutation keeps 13/13 green: dropping `or no criterion in scope` from `references/review-rubric.md:16`, dropping `counts as met` from `actions/02-review-functional.md:14`, dropping `"not scored"` from `SKILL.md:30`, re-adding `fixed` to `references/review-rubric.md:15` +- [x] A plan of 6 criteria whose diff touches one phase of 2, with 1 met, reads `1/6 met, 1 unmet, 4 out of the diff`; with no plan it reads `not scored`. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:14 +- [x] A criterion on runtime timing is checked `not-applicable` and counted as met; a round with no criterion in scope does not read `approve`; a three-axis run writes findings keyed on a `file:line`, each with its fix. — plugins/aidd-dev/skills/05-review/references/review-rubric.md:16 + +### Findings + +- `scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:89` : named "a round that scored nothing never approves" but asserts the opposite rule, that a round without functional takes its verdict from its findings alone, so a code-only round with no finding approves → rename it to the rule it asserts and give the never-approve rule (`no criterion in scope`) its own test — 🟡 warning, code +- `plugins/aidd-dev/skills/05-review/SKILL.md:26` : the base falls back to "the branch's pull request target" only; the actions' former default (the repository default branch) is gone, so a branch with no pull request and no argument has no defined base and two models may pick two → name a last fallback, e.g. the repository default branch — 🟡 warning, fit +- `plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:12` : "Read no earlier round" restates the `SKILL.md:31` re-run rule, and its removal reddens no test → drop it and let `SKILL.md` own the rule — 🟢 minor, rot + +## Round 12 · r-5ca4 + +- Date: 2026-10-08 +- By: Baptiste LAFOURCADE +- Diff: `origin/next...HEAD` +- Axes: code, functional, relevancy +- Verdict: approve +- Score: 6/6 met, 0 unmet, 0 out of the diff + +### Criteria + +- [x] Each round and scope assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:33 +- [x] With a round 1 claiming every criterion met on files the diff never touched, a second run appends a round scored from the diff alone and leaves round 1 untouched. — plugins/aidd-dev/skills/05-review/SKILL.md:31 +- [x] A phase outside the diff gets one `Out of the diff:` line, named as the plan names it, in plan order, holding no box. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:13 +- [x] Each score and status assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:152 +- [x] A plan of 6 criteria whose diff touches one phase of 2, with 1 met, reads `1/6 met, 1 unmet, 4 out of the diff`; with no plan it reads `not scored`. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:14 +- [x] A criterion on runtime timing is checked `not-applicable` and counted as met; a round with no criterion in scope does not read `approve`; a three-axis run writes findings keyed on a `file:line`, each with its fix. — plugins/aidd-dev/skills/05-review/references/review-rubric.md:16 + +### Findings + +- `plugins/aidd-dev/skills/05-review/assets/review-template.md:6` : the round-level HTML comment no longer says to drop it (the old one did), while SKILL.md:28 allows "boxes and finding lines only, no prose", so a filler may copy it into every review.md → append "drop this comment" or move the guidance into SKILL.md — 🟢 minor, code +- `plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:5` : the four section headings lost their blank line, unlike 01-review-code.md:5 and 03-review-relevancy.md:5 and every other action file → restore the blank line under each heading — 🟢 minor, rot + +## Round 13 · r-6db1 + +- Date: 2026-10-08 +- By: Baptiste LAFOURCADE +- Diff: `origin/next...HEAD` +- Axes: code, functional, relevancy +- Verdict: changes-requested +- Score: 5/6 met, 1 unmet, 0 out of the diff + +### Criteria + +- [x] Each round and scope assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:33 +- [ ] With a round 1 claiming every criterion met on files the diff never touched, a second run appends a round scored from the diff alone and leaves round 1 untouched. — only the rule is in the diff (SKILL.md:31); no trap fixture and no recorded run on it, the 9/9 in plan.md Decisions has no artifact +- [x] A phase outside the diff gets one `Out of the diff:` line, named as the plan names it, in plan order, holding no box. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:16 +- [x] Each score and status assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:152 +- [x] A plan of 6 criteria whose diff touches one phase of 2, with 1 met, reads `1/6 met, 1 unmet, 4 out of the diff`; with no plan it reads `not scored`. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:17 +- [x] A criterion on runtime timing is checked `not-applicable` and counted as met; a round with no criterion in scope does not read `approve`; a three-axis run writes findings keyed on a `file:line`, each with its fix. — aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/review.md:308 + +### Findings + +- `plugins/aidd-dev/skills/05-review/references/review-rubric.md:17` : `blocked` hinges on "an unchecked criterion the plan words as critical", but the plan template gives a criterion no severity, so two models can split `blocked` from `changes-requested` on one diff, the divergence #858 closes → name the signal (e.g. a criterion the plan marks 🔴) or drop the clause — 🟢 minor, fit +- `aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/plan.md:1` : the plan has no frontmatter, so no `objective` and no `status`, which `01-plan/assets/plan-template.md:1` and `01-plan/references/plan-status.md:15` require for `implemented → reviewed` → add `objective` and `status: implemented` frontmatter — 🟢 minor, conform + +## Round 14 · r-a675 + +- Date: 2026-10-09 +- By: Baptiste LAFOURCADE +- Diff: `origin/next...HEAD` +- Axes: code, functional, relevancy +- Verdict: approve +- Score: 6/6 met, 0 unmet, 0 out of the diff + +### Criteria + +- [x] Each round and scope assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:33 +- [x] Runtime, no diff can show it: with a round 1 claiming every criterion met on files the diff never touched, a second run appends a round scored from the diff alone and leaves round 1 untouched. — not-applicable, runtime behavior of a model run, no diff can show it +- [x] A phase outside the diff gets one `Out of the diff:` line, named as the plan names it, in plan order, holding no box. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:16 +- [x] Each score and status assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:152 +- [x] A plan of 6 criteria whose diff touches one phase of 2, with 1 met, reads `1/6 met, 1 unmet, 4 out of the diff`; with no plan it reads `not scored`. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:25 +- [x] A criterion on runtime timing is checked `not-applicable` and counted as met; a round with no criterion in scope does not read `approve`; a three-axis run writes findings keyed on a `file:line`, each with its fix. — plugins/aidd-dev/skills/05-review/references/review-rubric.md:16 + +### Findings + +- `plugins/aidd-dev/skills/05-review/references/review-rubric.md:15` : `approve` requires only "no critical finding", so a round with warnings matches both `approve` and `changes-requested` → write "no critical or warning finding" — 🟢 minor, code +- `plugins/aidd-dev/skills/05-review/SKILL.md:31` : "Never read ... an earlier round" contradicts reading the sections present to number the new one → say "count the `## Round` headings, never read their bodies" — 🟢 minor, code +- `plugins/aidd-dev/skills/05-review/SKILL.md:26` : the Diff rule and the diff resolution dropped from `01-review-code.md` and `03-review-relevancy.md` trace to no task or decision of the plan → record it in `plan.md` Decisions or a phase task — 🟢 minor, fit +- `aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/phase-2.md:9` : the projection omits `SKILL.md` and the guard test though tasks 1 and 2.3 change them, and the projection is what now decides scope → add both to the projection — 🟢 minor, conform + +## Round 15 · r-fa80 + +- Date: 2026-10-09 +- By: Baptiste LAFOURCADE +- Diff: `origin/next...HEAD` +- Axes: code, functional, relevancy +- Verdict: approve +- Score: 6/6 met, 0 unmet, 0 out of the diff + +### Criteria + +- [x] Each round and scope assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:36, scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:64, scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:69 +- [x] Runtime, no diff can show it: with a round 1 claiming every criterion met on files the diff never touched, a second run appends a round scored from the diff alone and leaves round 1 untouched. — not-applicable, a property of a model's run, not of the diff +- [x] A phase outside the diff gets one `Out of the diff:` line, named as the plan names it, in plan order, holding no box. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:16, plugins/aidd-dev/skills/05-review/assets/review-template.md:22 +- [x] Each score and status assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:86, scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:109, scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:154 +- [x] A plan of 6 criteria whose diff touches one phase of 2, with 1 met, reads `1/6 met, 1 unmet, 4 out of the diff`; with no plan it reads `not scored`. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:25, plugins/aidd-dev/skills/05-review/SKILL.md:30 +- [x] A criterion on runtime timing is checked `not-applicable` and counted as met; a round with no criterion in scope does not read `approve`; a three-axis run writes findings keyed on a `file:line`, each with its fix. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:17, plugins/aidd-dev/skills/05-review/references/review-rubric.md:16, plugins/aidd-dev/skills/05-review/assets/review-template.md:26 + +### Findings + +- `plugins/aidd-dev/skills/05-review/assets/review-template.md:12` : the `Diff` field renders `{{base}}...{{head}}` while `SKILL.md:26` reviews that range plus the working tree, so a round over uncommitted edits records a range that does not reproduce its own diff → render the working tree in the field, e.g. `{{base}}...{{head}}` + `working tree` when it held changes — 🟢 minor, rot +- `scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:58` : the header assertion runs one way, from the validator's `header` to the template, so deleting `Started` from `review-validator.yml` keeps the suite green (mutated in a copy: 16/16 pass) and the declared header can shrink unseen → pin the list, `assert.deepEqual(headerItems, ["Rounds", "Started"])`, as line 61 pins `fields` — 🟢 minor, code +- `aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/phase-2.md:16` : the projection nests `../../../../scripts/__tests__` under the skill folder while `phase-1.md:14` places `scripts/__tests__` at the root, two shapes for one path in the address the functional axis reads scope from → draw it as a root sibling, as phase 1 does — 🟢 minor, rot + +## Round 16 · r-6242 + +- Date: 2026-10-09 +- By: Baptiste LAFOURCADE +- Diff: `origin/next...HEAD` plus the working tree +- Axes: code, functional, relevancy +- Verdict: changes-requested +- Score: 6/6 met, 0 unmet, 0 out of the diff + +### Criteria + +- [x] Each round and scope assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:33 +- [x] Runtime, no diff can show it: with a round 1 claiming every criterion met on files the diff never touched, a second run appends a round scored from the diff alone and leaves round 1 untouched. — not-applicable, a runtime property of a model run that no diff can show +- [x] A phase outside the diff gets one `Out of the diff:` line, named as the plan names it, in plan order, holding no box. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:16 +- [x] Each score and status assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:154 +- [x] A plan of 6 criteria whose diff touches one phase of 2, with 1 met, reads `1/6 met, 1 unmet, 4 out of the diff`; with no plan it reads `not scored`. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:25 +- [x] A criterion on runtime timing is checked `not-applicable` and counted as met; a round with no criterion in scope does not read `approve`; a three-axis run writes findings keyed on a `file:line`, each with its fix. — plugins/aidd-dev/skills/05-review/references/review-rubric.md:16 + +### Findings + +- `plugins/aidd-dev/skills/05-review/SKILL.md:31` : the re-run rule forbids reading an earlier round's body, yet when the feature folder's own `review.md` is in the diff (here +358 lines) the code and relevancy axes are told to read every changed line, earlier rounds included → exclude the feature folder's `review.md` from the reviewed diff in the `Diff` rule — 🟡 warning, rot +- `plugins/aidd-dev/skills/05-review/SKILL.md:26` : "plus the working tree" does not say whether untracked files count, and `git diff` omits them, so a new file not yet added falls outside the review silently → name tracked and untracked changes explicitly — 🟢 minor, code +- `plugins/aidd-dev/skills/05-review/assets/review-template.md:26` : `{{severity}}` leaves the form open while the rubric spells it `🔴 critical`, so two runs can write `warning` and `🟡 warning` for one finding, against the plan's one-report goal → render it as the rubric's emoji and word — 🟢 minor, fit +- `plugins/aidd-dev/skills/05-review/references/review-rubric.md:17` : `blocked` now comes from findings only, so an unmet criterion can no longer block whatever its weight, a change no plan decision records → record the choice in `plan.md`'s Decisions, or let a critical criterion block — 🟢 minor, fit +- `scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:96` : the negative guard matches one phrasing (`is a … line`, `appears as a … row`), so "an unmet criterion goes to `Findings`" passes it → match any skill sentence naming an unmet criterion together with `Findings` — 🟢 minor, code +- `plugins/aidd-dev/skills/05-review/assets/review-validator.yml:1` : the comment says a round carries "exactly these" while `header` lists items above the rounds, not in one → scope the comment to `fields` and `lists` — 🟢 minor, rot + +## Round 17 · r-ab67 + +- Date: 2026-10-09 +- By: Baptiste LAFOURCADE +- Diff: `origin/next...c9c182f2` plus the working tree +- Axes: code, functional, relevancy +- Verdict: changes-requested +- Score: 6/6 met, 0 unmet, 0 out of the diff + +### Criteria + +- [x] Each round and scope assertion reddens alone under the mutation of the rule it names. — not-applicable, a mutation's outcome is runtime; one assertion per rule sits at `scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:33` +- [x] Runtime, no diff can show it: with a round 1 claiming every criterion met on files the diff never touched, a second run appends a round scored from the diff alone and leaves round 1 untouched. — not-applicable, runtime by the plan's own wording +- [x] A phase outside the diff gets one `Out of the diff:` line, named as the plan names it, in plan order, holding no box. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:16 +- [x] Each score and status assertion reddens alone under the mutation of the rule it names. — not-applicable, a mutation's outcome is runtime; one assertion per rule sits at `scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:156` +- [x] A plan of 6 criteria whose diff touches one phase of 2, with 1 met, reads `1/6 met, 1 unmet, 4 out of the diff`; with no plan it reads `not scored`. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:17 +- [x] A criterion on runtime timing is checked `not-applicable` and counted as met; a round with no criterion in scope does not read `approve`; a three-axis run writes findings keyed on a `file:line`, each with its fix. — plugins/aidd-dev/skills/05-review/references/review-rubric.md:16 + +### Findings + +- `plugins/aidd-dev/skills/05-review/references/review-rubric.md:16` : `changes-requested` takes "a fixable critical" while `blocked` takes "a critical finding that must not merge", and the severity scale defines every 🔴 as "must not merge as-is", so one critical can yield either verdict, the split the plan's "Only a finding blocks" decision set out to remove → give the two verdicts one test: either every critical blocks (drop "a fixable critical") or name what makes a critical fixable — 🟡 warning, rot +- `.hermes.md:1` : untracked tool scaffold ("Auto-scaffolded by tokenade") in the reviewed tree, outside the need → keep it out of the commit or ignore it locally — 🟢 minor, fit + +## Round 18 · r-c732 + +- Date: 2026-10-09 +- By: Baptiste LAFOURCADE +- Diff: `origin/next...HEAD` plus the working tree +- Axes: code, functional, relevancy +- Verdict: approve +- Score: 6/6 met, 0 unmet, 0 out of the diff + +### Criteria + +- [x] Each round and scope assertion reddens alone under the mutation of the rule it names. — not-applicable, a mutation run is runtime; the assertions sit at scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:33 +- [x] Runtime, no diff can show it: with a round 1 claiming every criterion met on files the diff never touched, a second run appends a round scored from the diff alone and leaves round 1 untouched. — not-applicable, a second run is runtime; the rule sits at plugins/aidd-dev/skills/05-review/SKILL.md:31 +- [x] A phase outside the diff gets one `Out of the diff:` line, named as the plan names it, in plan order, holding no box. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:16 +- [x] Each score and status assertion reddens alone under the mutation of the rule it names. — not-applicable, a mutation run is runtime; the assertions sit at scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:86 +- [x] A plan of 6 criteria whose diff touches one phase of 2, with 1 met, reads `1/6 met, 1 unmet, 4 out of the diff`; with no plan it reads `not scored`. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:25 +- [x] A criterion on runtime timing is checked `not-applicable` and counted as met; a round with no criterion in scope does not read `approve`; a three-axis run writes findings keyed on a `file:line`, each with its fix. — plugins/aidd-dev/skills/05-review/references/review-rubric.md:16 + +### Findings + +- `aidd_docs/tasks/2026_10/2026_10_07_review-phase-scope/backlog-link.json:1` : no final newline, against `.editorconfig` `insert_final_newline = true` and the 9 other tracked `backlog-link.json` → end the file with a newline — 🟢 minor, conform +- `plugins/aidd-dev/skills/05-review/SKILL.md:31` : the Re-run rule says where `Rounds` comes from but no rule owns the header's `Started`, so an appending run may rewrite it → state that round 1 writes `Started` and later rounds keep it — 🟢 minor, code + +## Round 19 · r-7e85 + +- Date: 2026-10-09 +- By: Baptiste LAFOURCADE +- Diff: `origin/next...HEAD` plus the working tree +- Axes: code, functional, relevancy +- Verdict: approve +- Score: 6/6 met, 0 unmet, 0 out of the diff + +### Criteria + +- [x] Each round and scope assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:45 +- [x] Runtime, no diff can show it: with a round 1 claiming every criterion met on files the diff never touched, a second run appends a round scored from the diff alone and leaves round 1 untouched. — not-applicable, a model's run over a seeded fixture is runtime behaviour no diff holds +- [x] A phase outside the diff gets one `Out of the diff:` line, named as the plan names it, in plan order, holding no box. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:19 +- [x] Each score and status assertion reddens alone under the mutation of the rule it names. — scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:121 +- [x] A plan of 6 criteria whose diff touches one phase of 2, with 1 met, reads `1/6 met, 1 unmet, 4 out of the diff`; with no plan it reads `not scored`. — plugins/aidd-dev/skills/05-review/actions/02-review-functional.md:24 +- [x] A criterion on runtime timing is checked `not-applicable` and counted as met; a round with no criterion in scope does not read `approve`; a three-axis run writes findings keyed on a `file:line`, each with its fix. — plugins/aidd-dev/skills/05-review/references/review-rubric.md:16 + +### Findings + +- `scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js:130` : the lookahead `(?![^.]*\bno\b)` exempts any sentence holding the word "no" before its period, so "an unmet criterion goes to `Findings` with no severity" passes the one-home scan → anchor the exemption on the negated phrase itself, e.g. `no \`Findings\`` — 🟢 minor, code +- `plugins/aidd-dev/skills/05-review/SKILL.md:19` : the flow draws a later run only from `changes`, while a `blocked` round is re-reviewed after its fix too → add `blocked -.->|a later run| all` — 🟢 minor, rot diff --git a/plugins/aidd-dev/CATALOG.md b/plugins/aidd-dev/CATALOG.md index c54c4de54..d2077d3a9 100644 --- a/plugins/aidd-dev/CATALOG.md +++ b/plugins/aidd-dev/CATALOG.md @@ -89,12 +89,15 @@ Auto-generated index of skills, agents, references and assets shipped by the `ai | Group | File | Description | |-------|------|---| -| `actions` | [01-review-code.md](skills/05-review/actions/01-review-code.md) | - | -| `actions` | [02-review-functional.md](skills/05-review/actions/02-review-functional.md) | - | -| `actions` | [03-review-relevancy.md](skills/05-review/actions/03-review-relevancy.md) | - | +| `actions` | [01-prepare.md](skills/05-review/actions/01-prepare.md) | - | +| `actions` | [02-review-code.md](skills/05-review/actions/02-review-code.md) | - | +| `actions` | [03-review-functional.md](skills/05-review/actions/03-review-functional.md) | - | +| `actions` | [04-review-relevancy.md](skills/05-review/actions/04-review-relevancy.md) | - | +| `actions` | [05-finalize.md](skills/05-review/actions/05-finalize.md) | - | | `assets` | [review-template.md](skills/05-review/assets/review-template.md) | - | +| `references` | [report-contract.md](skills/05-review/references/report-contract.md) | - | | `references` | [review-rubric.md](skills/05-review/references/review-rubric.md) | - | -| `-` | [SKILL.md](skills/05-review/SKILL.md) | `Review a diff read-only on three axes, code, behavior versus the plan, and relevancy, into one verdict report. Use before shipping a change. Not for fixing findings or auditing a codebase.` | +| `-` | [SKILL.md](skills/05-review/SKILL.md) | `Reviews a change or a diff and says whether it is ready to ship, naming every problem with its fix. Use when the user wants a change checked before it ships. Not for fixing what the review finds, and not for auditing a whole codebase.` | #### `skills/06-test` diff --git a/plugins/aidd-dev/skills/05-review/SKILL.md b/plugins/aidd-dev/skills/05-review/SKILL.md index 4da8ad889..84d8edbae 100644 --- a/plugins/aidd-dev/skills/05-review/SKILL.md +++ b/plugins/aidd-dev/skills/05-review/SKILL.md @@ -1,40 +1,37 @@ --- name: 05-review -description: Review a diff read-only on three axes, code, behavior versus the plan, and relevancy, into one verdict report. Use before shipping a change. Not for fixing findings or auditing a codebase. +description: Reviews a change or a diff and says whether it is ready to ship, naming every problem with its fix. Use when the user wants a change checked before it ships. Not for fixing what the review finds, and not for auditing a whole codebase. argument-hint: diff | plan model: opus --- -# Skill: review +# Review -Read-only review of a diff along three axes, code quality, feature behavior, and relevancy, composed into one report. +```mermaid +flowchart LR + all([all axes]) --> prepare + one([one axis named]) --> prepare + prepare --> review-code & review-functional & review-relevancy --> finalize + finalize --> approve([approve]) + finalize --> changes([changes-requested]) + finalize --> blocked([blocked]) + changes -.->|a later run| all + blocked -.->|a later run| all +``` ## Actions -| # | Action | Axis | -| --- | ------------------- | ---------------------------------------------------------------- | -| 01 | `review-code` | Clean-code quality on the changed lines | -| 02 | `review-functional` | The diff against the plan's phases and their acceptance criteria | -| 03 | `review-relevancy` | Does the change belong: fit to the need, rule conformance, no rot | +Run the flow above, reading only the next action file. -Run all three by default, composing one report. Run a single axis only when the caller names it; if it is unclear whether they want all or one, ask. Before running an action, read its file in `actions/`, not only the table or assets. +| Action | Does | +| --- | --- | +| prepare | resolve the diff and open the round | +| review-code | grade the changed lines on clean code | +| review-functional | trace the diff against the plan's criteria | +| review-relevancy | judge whether the change belongs | +| finalize | judge the round and check its shape | ## Transversal rules -- Read-only: surface each finding with its fix described, never patch. -- Output: always write `review.md` to disk; the file is the deliverable, never an inline-only verdict. -- Folder: write into the reviewed work's feature folder (`aidd_docs/tasks//_/`, beside `plan.md`), or one resolved from the change when it has none. -- Report: fill `review.md` from `assets/review-template.md`. One shared `Findings` table for every axis: functional writes the `Phases` boxes and the `Verification` table, and appends a `functional` row to `Findings` for each unmet criterion tagged `fix`; code and relevancy append rows to `Findings` under their `Kind`. Tables and boxes, no prose, no per-axis sections. The Phase column ties a finding to the plan when one is in scope, `-` otherwise. -- Sections: the report has exactly the sections in `assets/review-validator.yml`. Before returning, verify against it and remove any section not listed. -- Not run: every required section always exists. The header `Axes run` lists which axes ran, so a skipped code or relevancy axis is visible even though `Findings` is shared. An axis that did not run marks the sections it owns "Not run" (functional owns `Phases` and `Verification`); it never leaves a placeholder or invents data. -- Re-run: overwrite `review.md` with the current review. It is a snapshot of the current diff, not a history; a later review of the same work replaces the earlier one. -- Verdict: one overall verdict, the strictest across the axes run, per `references/review-rubric.md`. - -## References - -- `references/review-rubric.md`: the severity scale, the verdict rule, the code categories, and the relevancy lenses. - -## Assets - -- `assets/review-template.md`: the single report the three axes fill. -- `assets/review-validator.yml`: the closed set of report sections. +- Ask which entry the caller wants when the request does not say. +- Review statically: never run the app. diff --git a/plugins/aidd-dev/skills/05-review/actions/01-prepare.md b/plugins/aidd-dev/skills/05-review/actions/01-prepare.md new file mode 100644 index 000000000..d5454a8b7 --- /dev/null +++ b/plugins/aidd-dev/skills/05-review/actions/01-prepare.md @@ -0,0 +1,25 @@ +# 01 - Prepare + +Resolve what is reviewed and where it lands, then open this round. + +## Input + +The arguments, and the plan when one is named. + +## Output + +A round appended to the report, and the diff the axes read. + +## Process + +1. **Resolve.** Resolve the diff and the report per [report-contract.md](../references/report-contract.md). +2. **Open.** Append a round from the template `report-contract.md` names, filling what is known before the axes run. +3. **Hand over.** Name the diff and the round the axes write into. + +## Test + +| Case | Pass | +| --- | --- | +| A fresh report | round 1 appended, its date as the start date | +| A report holding two rounds | a third appended, the header counting 3, the first two unchanged | +| A run naming one axis | the round names that axis alone | diff --git a/plugins/aidd-dev/skills/05-review/actions/01-review-code.md b/plugins/aidd-dev/skills/05-review/actions/01-review-code.md deleted file mode 100644 index 37de2cae9..000000000 --- a/plugins/aidd-dev/skills/05-review/actions/01-review-code.md +++ /dev/null @@ -1,23 +0,0 @@ -# 01 - Review Code - -Grade the diff against clean-code principles and record the findings in the review report. - -## Input - -The diff to review, a git ref range or path, from the arguments; defaults to the diff against the repository default branch. The plan, when in scope, to tag each finding's phase. - -## Output - -Rows in the `Findings` table of the feature folder's `review.md`, kind `code`, each rated and citing a changed `file:line`. - -## Process - -1. **Resolve.** Take the diff from the arguments, otherwise the diff against the repository default branch. -2. **Review.** Read every changed line for clean-code: naming, structure, complexity, smells, error handling. No runtime checks. Rule conformance is the relevancy lens, not this one. -3. **Rate.** One row per issue, rated per [review-rubric.md](../references/review-rubric.md), citing a `file:line`. -4. **Record.** Append one row per finding to the `Findings` table: kind `code`, Phase the plan phase the file falls in or `-`. Write "None." when the run found nothing. - -## Test - -- Each code finding is a `Findings` row, kind `code`, rated and citing a changed `file:line`. -- No code is patched. diff --git a/plugins/aidd-dev/skills/05-review/actions/02-review-code.md b/plugins/aidd-dev/skills/05-review/actions/02-review-code.md new file mode 100644 index 000000000..99a42bcd8 --- /dev/null +++ b/plugins/aidd-dev/skills/05-review/actions/02-review-code.md @@ -0,0 +1,24 @@ +# 02 - Review Code + +Grade the changed lines on clean code. + +## Input + +The changed lines prepare resolved. + +## Output + +The round's code findings, each rated and keyed on a changed line. + +## Process + +1. **Read.** Read every changed line against each kind: `standards`, `architecture`, `code-health`, `security`, `error-handling`, `performance`, `frontend`, `backend`. + - Rule conformance belongs to the relevancy lens. +2. **Rate.** Record one finding per issue, under its kind, rated per [review-rubric.md](../references/review-rubric.md). + +## Test + +| Case | Pass | +| --- | --- | +| An issue is found | one finding, rated, under a listed kind, keyed on a changed line | +| The diff is clean | no finding | diff --git a/plugins/aidd-dev/skills/05-review/actions/02-review-functional.md b/plugins/aidd-dev/skills/05-review/actions/02-review-functional.md deleted file mode 100644 index 3c7ab48e7..000000000 --- a/plugins/aidd-dev/skills/05-review/actions/02-review-functional.md +++ /dev/null @@ -1,26 +0,0 @@ -# 02 - Review Functional - -Trace the diff against the plan's phases and their acceptance criteria, recording each as a checked or unchecked box. - -## Input - -The plan path holding the phases and their acceptance criteria, from the arguments or the prompt, and the diff to trace (a git ref range; defaults to the diff against the repository default branch). - -## Output - -The `Phases` and `Verification` sections of the feature folder's `review.md` (one block per plan phase with a box per acceptance criterion, and the verification summary), plus a `functional` row in `Findings` for each unmet criterion tagged `fix`. - -## Process - -1. **Read.** Take the plan from the arguments; if absent, ask for the acceptance criteria, and mark the `Phases` and `Verification` sections "Not run" when none are available. Static review only, no app execution or browser. -2. **Trace.** Fetch the diff, then walk each phase in plan order. For every acceptance criterion, check the box `[x]` only when the diff shows evidence, citing the `file:line`; leave it `[ ]` when unmet or partial, naming the gap. -3. **Summarize.** Fill `Verification`: the percent and count of checked criteria, the files checked, every unchecked criterion tagged `fix`, `not-applicable`, or `fixed`, and any unplanned change in the diff that traces to no criterion. Write "none" for an empty list. -4. **Count.** For each unmet criterion tagged `fix`, append a `functional` row to the `Findings` table with every column filled: Phase, Location (the phase's target file, or `-`), Issue (the unmet criterion and its gap), Fix (what to implement), and a severity that matches the criterion (🔴 critical when the criterion itself is critical, otherwise 🟡 warning, never 🟢 minor), so the header count matches the verdict. A `not-applicable` or `fixed` criterion adds no row. -5. **Record.** Write the phase blocks, the functional findings, and the verification summary into `review.md`. No prose paragraphs, boxes and the summary only. - -## Test - -- The `Phases` section holds one block per plan phase, every acceptance criterion a checked or unchecked box citing evidence or a gap. -- Every unmet criterion tagged `fix` has a matching `functional` row in `Findings`; `not-applicable` criteria have none. -- The `Verification` section reports the verified percent, the files checked, a tag on each unchecked criterion, and the unplanned changes (or "none"). -- No code is patched. diff --git a/plugins/aidd-dev/skills/05-review/actions/03-review-functional.md b/plugins/aidd-dev/skills/05-review/actions/03-review-functional.md new file mode 100644 index 000000000..4d459dc52 --- /dev/null +++ b/plugins/aidd-dev/skills/05-review/actions/03-review-functional.md @@ -0,0 +1,33 @@ +# 03 - Review Functional + +Trace the diff against the plan's acceptance criteria. + +## Input + +The plan, its phase files, and the diff prepare resolved. + +## Output + +The round's criteria and score. + +## Process + +1. **Read.** Take the plan, or ask for the acceptance criteria. + - None to be had: not scored. +2. **Trace.** Walk the plan's phases in order, one line per criterion. + - A phase whose `## Architecture projection` files the diff leaves untouched: out of the diff, no box. A phase projecting no file is traced, never out of the diff. + - Met by the files as the diff leaves them, an added line or a context line: checked. + - Unmet, partial, its subject absent, or a name echoing it with no behaviour behind: unchecked, never a finding. + - Its subject there, its property one no diff could ever show: checked, not-applicable. +3. **Score.** Count met + unmet + out of the diff = the plan's whole criteria total. + - A not-applicable criterion counts as met. + +## Test + +| Case | Pass | +| --- | --- | +| A plan of 6 criteria, the diff touching one phase of 2 with 1 met | `1/6 met, 1 unmet, 4 out of the diff`, the untouched phases out of the diff in plan order | +| A criterion unmet, its subject absent, or a name echoing it with no behaviour behind | unchecked, naming the gap, never a finding | +| A criterion whose property no diff could show | checked, not-applicable, counted as met | +| No plan and no criteria given | not scored, and no criteria | +| A second round over the same plan | every box follows the current diff, none carried from the earlier round | diff --git a/plugins/aidd-dev/skills/05-review/actions/03-review-relevancy.md b/plugins/aidd-dev/skills/05-review/actions/03-review-relevancy.md deleted file mode 100644 index 88108dba3..000000000 --- a/plugins/aidd-dev/skills/05-review/actions/03-review-relevancy.md +++ /dev/null @@ -1,24 +0,0 @@ -# 03 - Review Relevancy - -Judge whether the diff belongs (serves the need, conforms to the rules, no rot) and record the misfits in the review report. - -## Input - -The diff to review (a git ref range or path; defaults to the diff against the repository default branch), the need it serves (the plan objective or the ticket), the project's declared rules discovered at runtime, and the plan when in scope. - -## Output - -Misfit rows in the `Findings` table of the feature folder's `review.md`, each under a lens (`fit`, `conform`, `rot`), tied to evidence. - -## Process - -1. **Gather.** Resolve the diff, otherwise the diff against the repository default branch. Capture the need from the plan objective or the ticket. Discover the declared rules at runtime, never hardcoded. Fall back cleanly when a source is absent. -2. **Fit.** Does the change serve the real intent end to end, not only the literal criteria? Flag drift. Lens `fit`. -3. **Conform.** Does it hold to the declared rules and conventions? Flag each violation with the rule it breaks. Lens `conform`. -4. **Rot.** Scan for duplication, over-engineering, and incoherence. Cite the site. Lens `rot`. -5. **Record.** Append each misfit to the `Findings` table: kind the lens (`fit`, `conform`, `rot`), Phase the plan phase the file falls in or `-`. A bare opinion is not a finding: tie each to a rule, a duplication site, a smell, or a named need-gap. Write "None." when clean. - -## Test - -- Each misfit is a `Findings` row, kind `fit`, `conform`, or `rot`, tied to a `file:line`, a rule, a duplication site, or a need-gap. -- No finding is a bare opinion, and no code is patched. diff --git a/plugins/aidd-dev/skills/05-review/actions/04-review-relevancy.md b/plugins/aidd-dev/skills/05-review/actions/04-review-relevancy.md new file mode 100644 index 000000000..9fb090b8b --- /dev/null +++ b/plugins/aidd-dev/skills/05-review/actions/04-review-relevancy.md @@ -0,0 +1,26 @@ +# 04 - Review Relevancy + +Judge whether the change belongs. + +## Input + +The diff prepare resolved, and the need it serves: the plan or the ticket. + +## Output + +The round's misfit findings, each with its lens as kind. + +## Process + +1. **Gather.** Capture the need, and read the project's declared rules, never assuming one. +2. **Flag.** Record each misfit under its lens, tied to a rule, a site or the need; a bare opinion is no finding. + - `fit`: drift from the real intent, end to end, not only the literal criteria. + - `conform`: a violation of a declared rule, naming it. + - `rot`: duplication, over-engineering or incoherence, citing the site. + +## Test + +| Case | Pass | +| --- | --- | +| A misfit is found | one finding, its lens as kind, tied to a rule, a site or the need | +| The change fits | no finding | diff --git a/plugins/aidd-dev/skills/05-review/actions/05-finalize.md b/plugins/aidd-dev/skills/05-review/actions/05-finalize.md new file mode 100644 index 000000000..b3a3c201f --- /dev/null +++ b/plugins/aidd-dev/skills/05-review/actions/05-finalize.md @@ -0,0 +1,23 @@ +# 05 - Finalize + +Judge the round, and check it before it ships. + +## Input + +The round the axes filled. + +## Output + +The round's verdict, and the round in its declared shape. + +## Process + +1. **Judge.** Set the verdict per [review-rubric.md](../references/review-rubric.md). +2. **Check.** Verify the round against the shape [report-contract.md](../references/report-contract.md) names, and fix what breaks. + +## Test + +| Case | Pass | +| --- | --- | +| Two axes ran, one major between them | the verdict reads `changes-requested` | +| A field or a list is missing | it is filled, or the run says why it cannot be | diff --git a/plugins/aidd-dev/skills/05-review/assets/review-template.md b/plugins/aidd-dev/skills/05-review/assets/review-template.md index 133e31f1f..b3a51292d 100644 --- a/plugins/aidd-dev/skills/05-review/assets/review-template.md +++ b/plugins/aidd-dev/skills/05-review/assets/review-template.md @@ -1,36 +1,26 @@ - - # Review: {{feature}} -- **Verdict**: {{approve | changes-requested | blocked}} -- **Diff**: `{{base}}...{{head}}` -- **Axes run**: {{the axes that ran, from code, functional, relevancy}} -- **Date**: {{yyyy_mm_dd}} -- **Findings**: {{n_critical}} critical, {{n_warning}} warning, {{n_minor}} minor +- Rounds: {{the count of round sections}} +- Started: {{round 1's date, never changed}} -## Phases +## Round {{the count of round headings, this one included}} · r-{{4 hex characters, drawn once}} - +- Date: {{yyyy-mm-dd}} +- By: {{git config user.name}} +- Diff: `{{base}}...{{head}}`{{ plus the working tree, when it held changes}} +- Axes: {{the axes that ran, from code, functional, relevancy}} +- Verdict: {{verdict}} +- Score: {{n_met}}/{{n_plan}} met, {{n_unmet}} unmet, {{n_out}} out of the diff, or not scored -### Phase {{n}} — {{phase-name}} +### Criteria - [x] {{criterion met}} — {{file:line}} - [ ] {{criterion unmet}} — {{gap}} +- [x] {{criterion no diff could show}} — not-applicable, {{why}} +- Out of the diff: {{phase-name}} ({{n}} criteria) -## Findings - - - -| Sev | Kind | Phase | Location | Issue | Fix | -| --- | ---- | ----- | -------- | ----- | --- | - -## Verification +### Findings - +- `{{file:line}}` : {{issue}} → {{fix}} — {{🔴 critical | 🟡 major | 🟢 minor}}, {{kind}} -| Metric | Value | -| ------------- | ------------------------------------------------- | -| Verified | {{pct}}% ({{n_checked}}/{{n_total}}) | -| Files checked | {{files, comma-separated}} | -| Unchecked | {{criterion — fix / not-applicable / fixed}}, or none | -| Unplanned | {{change tracing to no criterion}}, or none | + diff --git a/plugins/aidd-dev/skills/05-review/assets/review-validator.yml b/plugins/aidd-dev/skills/05-review/assets/review-validator.yml index 0f4489ead..244064dd3 100644 --- a/plugins/aidd-dev/skills/05-review/assets/review-validator.yml +++ b/plugins/aidd-dev/skills/05-review/assets/review-validator.yml @@ -1,5 +1,14 @@ -# review.md has exactly these sections. Any other is invalid: remove it, never add or rename. -sections: - - Phases # required: one block per plan phase, a box per criterion - - Findings # required: one table, all axes, by Kind - - Verification # required: the summary table +# Each round is one `## Round · r-` section, appended, carrying exactly these fields and lists. +header: # above the rounds, once + - Rounds + - Started +fields: # a round's, in this order, one per line + - Date + - By + - Diff + - Axes + - Verdict + - Score +lists: + - Criteria # the plan's criteria. Absent when functional did not run, or ran with none + - Findings # defects no criterion covers. Absent when none was found diff --git a/plugins/aidd-dev/skills/05-review/references/report-contract.md b/plugins/aidd-dev/skills/05-review/references/report-contract.md new file mode 100644 index 000000000..7b7b96682 --- /dev/null +++ b/plugins/aidd-dev/skills/05-review/references/report-contract.md @@ -0,0 +1,27 @@ +# Review report + +Where a review lands, what it reviews, and how a round joins it. + +## The file + +| Question | Answer | +| --- | --- | +| What | one `review.md`, appended to, never rewritten | +| Where | the reviewed work's feature folder, `aidd_docs/tasks//_/`, beside `plan.md` | +| When the work has no folder | resolve one from the change | +| Form | [review-template.md](../assets/review-template.md), in the shape [review-validator.yml](../assets/review-validator.yml) declares | + +## The diff + +| Question | Answer | +| --- | --- | +| Range | `...HEAD` plus the working tree, tracked and untracked | +| Base | from the arguments, else the branch's pull request target, else the repository default branch | +| Left out | the feature folder's own `review.md` | + +## A round + +| Rule | Why | +| --- | --- | +| Append a round, derived from the plan and the current diff alone | a round states what this diff shows, and nothing it inherited | +| Count the `## Round` headings, never read their bodies | an earlier round's boxes describe a diff that no longer exists, and copying one is the defect | diff --git a/plugins/aidd-dev/skills/05-review/references/review-rubric.md b/plugins/aidd-dev/skills/05-review/references/review-rubric.md index 400f62591..ef44734e9 100644 --- a/plugins/aidd-dev/skills/05-review/references/review-rubric.md +++ b/plugins/aidd-dev/skills/05-review/references/review-rubric.md @@ -1,29 +1,24 @@ # Review rubric -Shared definitions for the three review axes. The actions and the report template draw from here, so the scales live in one place. +The scales the axes share, so a finding reads the same whoever wrote it. ## Severity -- 🔴 critical: must not merge as-is. -- 🟡 warning: should fix. +- 🔴 critical: would harm production, its users, their data or what depends on it; must not merge as-is. +- 🟡 major: should fix. - 🟢 minor: nit. ## Verdict -One overall verdict, the strictest across the axes run: +One overall verdict, the strictest across the axes run. -- `approve`: no critical finding, and no criterion left unchecked and tagged `fix`, ship it. A `not-applicable` or `fixed` criterion does not block. -- `changes-requested`: warnings, a fixable critical, or any unchecked criterion tagged `fix`. -- `blocked`: a critical that must not merge, or an unchecked critical criterion tagged `fix`. A `not-applicable` or `fixed` criterion never blocks, whatever its severity. +| Verdict | When | +| --- | --- | +| `approve` | no finding above minor, and, where functional ran, at least one criterion in scope and none unchecked | +| `changes-requested` | a major, an unchecked criterion, or no criterion in scope | +| `blocked` | a critical finding | -An unchecked criterion tagged `fix` is a functional finding: it appears as a `functional` row in `Findings` (so it counts) and cannot yield `approve`. A `not-applicable` criterion is neither a finding nor a blocker. - -## Code categories - -`standards`, `architecture`, `code-health`, `security`, `error-handling`, `performance`, `frontend`, `backend`. - -## Relevancy lenses - -- `fit`: serves the real need, not only the literal criteria. -- `conform`: the project's declared rules and the surrounding conventions. -- `rot`: duplication, over-engineering, incoherence (naming, docs versus code). +| Case | Counts as | +| --- | --- | +| A criterion out of the diff | neither unchecked nor blocking | +| A round without functional | a verdict from its findings alone | diff --git a/scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js b/scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js new file mode 100644 index 000000000..d32b5c939 --- /dev/null +++ b/scripts/__tests__/a-review-appends-a-round-and-scores-the-whole-plan.test.js @@ -0,0 +1,328 @@ +const assert = require("node:assert/strict"); +const fs = require("node:fs"); +const path = require("node:path"); +const { describe, it } = require("node:test"); + +const SKILL = path.resolve(__dirname, "../../plugins/aidd-dev/skills/05-review"); + +/** + * A review is appended to, one section per round, so a past round sits in the file while the next + * one is written: the rules that forbid reading it are the only thing between the two. The field + * set and the template are the two sides of a round's shape, an unmet criterion has one home, and + * the score counts over the whole plan so a covered third cannot read as a finished plan. + */ + +function read(relative) { + return fs.readFileSync(path.join(SKILL, relative), "utf8"); +} + +/** The router's transversal rules, whatever bullet each one sits on. */ +function rules() { + const router = read("SKILL.md"); + return router.slice(router.indexOf("## Transversal rules")); +} + +/** One `## Process` step with the sub-items that qualify it. */ +function step(relative, label) { + const body = read(relative); + const opens = new RegExp(`^\\d+\\. \\*\\*${label}\\.\\*\\*`, "mu").exec(body); + if (!opens) return ""; + const rest = body.slice(opens.index); + const ends = /^(?:\d+\. |## )/mu.exec(rest.slice(1)); + return ends ? rest.slice(0, ends.index + 1) : rest; +} + +function everyMarkdown(directory, into) { + for (const entry of fs.readdirSync(directory, { withFileTypes: true })) { + const full = path.join(directory, entry.name); + if (entry.isDirectory()) everyMarkdown(full, into); + else if (entry.name.endsWith(".md")) into.push(full); + } + return into; +} + +/** A path under the skill, spelled the way the documents spell it, on every platform. */ +function inSkill(file) { + return path.relative(SKILL, file).split(path.sep).join("/"); +} + +describe("a review appends a round and scores the whole plan", () => { + it("a round is appended, and an earlier one is never read back", () => { + const reference = read("references/report-contract.md"); + assert.match(reference, /Append a round, derived from the plan and the current diff alone/u); + assert.match( + reference, + /Count the `## Round` headings, never read their bodies/u, + "and reads no earlier round", + ); + }); + + it("the report's address, its diff and its rounds have one home", () => { + const homes = everyMarkdown(SKILL, []) + .filter((file) => + /Count the `## Round` headings|aidd_docs\/tasks\/|branch's pull request target/u.test( + fs.readFileSync(file, "utf8"), + ), + ) + .map(inSkill); + assert.deepEqual(homes, ["references/report-contract.md"]); + }); + + it("the artifact is named, and no file of the skill can pass for it", () => { + assert.match(read("references/report-contract.md"), /one `review\.md`, appended to, never rewritten/u); + const lookalikes = fs + .readdirSync(path.join(SKILL, "references")) + .concat(fs.readdirSync(path.join(SKILL, "assets"))) + .filter((file) => /^review[-_]/u.test(file) && !/template|validator|rubric/u.test(file)); + assert.deepEqual(lookalikes, [], "a sibling named review-* reads as the artifact"); + }); + + it("the report reference is read by prepare and finalize, not by an axis", () => { + const cites = fs + .readdirSync(path.join(SKILL, "actions")) + .filter((action) => /report-contract\.md/u.test(read(path.join("actions", action)))); + assert.deepEqual(cites, ["01-prepare.md", "05-finalize.md"]); + for (const action of cites) { + const links = read(path.join("actions", action)).match( + /\[report-contract\.md\]\(\.\.\/references\/report-contract\.md\)/gu, + ); + assert.equal(links?.length, 1, `${action} cites it once as a link`); + } + }); + + it("the header says how it stays true as rounds are added", () => { + const template = read("assets/review-template.md"); + assert.match(template, /^- Rounds: \{\{the count of round sections\}\}$/mu); + assert.match(template, /^- Started: \{\{round 1's date, never changed\}\}$/mu); + }); + + it("a round's number and id come from the sections already present", () => { + const heading = read("assets/review-template.md").match(/^## Round .+$/mu)?.[0] ?? ""; + assert.match(heading, /\{\{the count of round headings, this one included\}\}/u); + assert.match(heading, /r-\{\{4 hex characters, drawn once\}\}/u, "two rounds appended in parallel still differ"); + }); + + it("a finding carries no box, so no state of it can be copied", () => { + const template = read("assets/review-template.md"); + assert.doesNotMatch(template.slice(template.indexOf("### Findings")), /^- \[[ x]\]/mu); + }); + + it("finalize checks the round against its declared shape before it ships", () => { + assert.match(step("actions/05-finalize.md", "Check"), /Verify the round against the shape/u); + }); + + it("the template refuses a placeholder and an empty list", () => { + assert.match( + read("assets/review-template.md"), + /Every placeholder is replaced by observed data, and a list no axis of this round owns is left out/u, + ); + }); + + it("a phase's projected files have one address", () => { + const homes = everyMarkdown(SKILL, []) + .filter((file) => /## Architecture projection/u.test(fs.readFileSync(file, "utf8"))) + .map(inSkill); + assert.deepEqual(homes, ["actions/03-review-functional.md"]); + }); + + it("the closed field set and the template render the same round", () => { + const validator = read("assets/review-validator.yml"); + const template = read("assets/review-template.md"); + const declared = validator.slice(validator.indexOf("fields:"), validator.indexOf("lists:")); + const fields = [...declared.matchAll(/^ {2}- (\w+)/gmu)].map((match) => match[1]); + assert.deepEqual(fields, ["Date", "By", "Diff", "Axes", "Verdict", "Score"]); + const head = validator.slice(validator.indexOf("header:"), validator.indexOf("fields:")); + const headerItems = [...head.matchAll(/^ {2}- (\w+)/gmu)].map((match) => match[1]); + assert.deepEqual(headerItems, ["Rounds", "Started"]); + for (const line of headerItems) { + assert.match(template, new RegExp(`^- ${line}: `, "mu"), "the header renders what it declares"); + } + const round = template.slice(template.indexOf("## Round")); + const rendered = [...round.matchAll(/^- (\w+):/gmu)].map((match) => match[1]); + assert.deepEqual(rendered, fields, "the round renders them all, in order, and nothing else"); + }); + + it("the lists a round declares are the lists the template renders", () => { + const validator = read("assets/review-validator.yml"); + const lists = [...validator.slice(validator.indexOf("lists:")).matchAll(/^ {2}- (\w+)/gmu)].map( + (match) => match[1], + ); + assert.deepEqual(lists, ["Criteria", "Findings"]); + for (const list of lists) { + assert.match(read("assets/review-template.md"), new RegExp(`^### ${list}$`, "mu")); + } + }); + + it("a phase out of the diff has one line, and the template renders it", () => { + assert.match( + step("actions/03-review-functional.md", "Trace"), + /untouched: out of the diff, no box/u, + ); + assert.match( + read("assets/review-template.md"), + /^- Out of the diff: \{\{phase-name\}\} \(\{\{n\}\} criteria\)$/mu, + ); + }); + + it("a phase projecting no file is traced, not called out of the diff", () => { + assert.match( + step("actions/03-review-functional.md", "Trace"), + /A phase projecting no file is traced, never out of the diff/u, + ); + }); + + it("an unmet criterion has one home, and it is not Findings", () => { + assert.match(step("actions/03-review-functional.md", "Trace"), /unchecked, never a finding/u); + const homes = everyMarkdown(SKILL, []) + .filter((file) => + /(?:unmet|unchecked) criterion(?![^.]*no `Findings`)[^.]*`Findings`/u.test( + fs.readFileSync(file, "utf8"), + ), + ) + .map(inSkill); + assert.deepEqual(homes, []); + }); + + it("a context line of the diff is evidence, not only an added one", () => { + assert.match( + step("actions/03-review-functional.md", "Trace"), + /Met by the files as the diff leaves them, an added line or a context line: checked/u, + ); + }); + + it("a label echoing the criterion is not evidence", () => { + const body = read("actions/03-review-functional.md"); + assert.match( + body.slice(body.indexOf("## Test")), + /a name echoing it with no behaviour behind \| unchecked, naming the gap/u, + ); + }); + + it("a criterion whose subject is absent is a gap", () => { + assert.match( + step("actions/03-review-functional.md", "Trace"), + /its subject absent[^:\n]*: unchecked/u, + ); + }); + + it("a criterion no diff could show is not-applicable, not a gap", () => { + assert.match( + step("actions/03-review-functional.md", "Trace"), + /Its subject there, its property one no diff could ever show: checked, not-applicable/u, + ); + }); + + it("the severity scale is one scale: critical, major, minor", () => { + assert.match(read("references/review-rubric.md"), /- 🟡 major: should fix\./u); + assert.match( + read("assets/review-template.md"), + /\{\{🔴 critical \| 🟡 major \| 🟢 minor\}\}/u, + "the template renders the scale the rubric names", + ); + const strays = everyMarkdown(SKILL, []) + .filter((file) => /\bwarning\b/iu.test(fs.readFileSync(file, "utf8"))) + .map(inSkill); + assert.deepEqual(strays, [], "a severity the rubric does not name"); + }); + + it("a critical is decided by the harm a merge would do, never by how strictly a rule is worded", () => { + assert.match( + read("references/review-rubric.md"), + /^- 🔴 critical: would harm production, its users, their data or what depends on it; must not merge as-is\.$/mu, + ); + }); + + it("a round without functional is judged on its findings alone", () => { + assert.match( + read("references/review-rubric.md"), + /A round without functional \| a verdict from its findings alone/u, + ); + }); + + it("a round with nothing in scope asks for changes, and never says approve", () => { + const rubric = read("references/review-rubric.md"); + assert.match(rubric, /^\| `changes-requested` \| .*no criterion in scope \|$/mu); + assert.match(rubric, /where functional ran, at least one criterion in scope and none unchecked/u); + assert.doesNotMatch(rubric, /fixable critical/u, "one critical has one verdict"); + }); + + it("no verdict of the skill is fixed: a round records no state of its own", () => { + const homes = everyMarkdown(SKILL, []) + .filter((file) => /`fixed`|\/ fixed\b/u.test(fs.readFileSync(file, "utf8"))) + .map(inSkill); + assert.deepEqual(homes, []); + }); + + it("each axis owns its own vocabulary", () => { + const owner = (pattern) => + everyMarkdown(SKILL, []) + .filter((file) => pattern.test(fs.readFileSync(file, "utf8"))) + .map(inSkill); + assert.deepEqual(owner(/`error-handling`/u), ["actions/02-review-code.md"]); + assert.deepEqual(owner(/`rot`:/u), ["actions/04-review-relevancy.md"]); + }); + + it("the template renders every form a round can hold", () => { + const template = read("assets/review-template.md"); + assert.match(template, /or not scored/u); + assert.match(template, /not-applicable, \{\{why\}\}/u); + }); + + it("the score counts over the whole plan, and the template holds its form", () => { + const score = step("actions/03-review-functional.md", "Score"); + assert.match(score, /whole criteria total/u, "the count is over the plan, not over what is in scope"); + assert.match(score, /met \+ unmet \+ out of the diff = the plan's whole criteria total/u, "and the three numbers are checkable against it"); + assert.match(score, /counts as met/u, "and a not-applicable criterion is counted"); + assert.doesNotMatch(score, /\{\{/u, "a rule names no template placeholder"); + assert.match( + step("actions/03-review-functional.md", "Read"), + /None to be had: not scored/u, + "and a run with no criteria says so instead of scoring", + ); + assert.match( + read("assets/review-template.md"), + /\{\{n_met\}\}\/\{\{n_plan\}\} met, \{\{n_unmet\}\} unmet, \{\{n_out\}\} out of the diff/u, + "the form is rendered by the template, not spelled out in a rule", + ); + }); + + it("the router routes: it carries the flow, the actions and the rules, and nothing else", () => { + const router = read("SKILL.md"); + const headings = [...router.matchAll(/^## (.+)$/gmu)].map((match) => match[1]); + assert.deepEqual(headings, ["Actions", "Transversal rules"]); + assert.match(router, /^\| Action \| Does \|$/mu, "the action table is the contract's own"); + const intro = router.slice(router.indexOf("## Actions") + 10, router.indexOf("| Action |")).trim(); + assert.equal(intro.split(/(?<=\.)\s/u).length, 1, "one sentence above the table, nothing more"); + for (const slug of ["prepare", "review-code", "review-functional", "review-relevancy", "finalize"]) { + assert.match(router, new RegExp(`^\\| ${slug} \\| [a-z]`, "mu"), "each row is a bare slug"); + } + }); + + it("the flow draws every path a run can take", () => { + const router = read("SKILL.md"); + const flow = router.slice(router.indexOf("```mermaid"), router.indexOf("## Actions")); + for (const slug of ["prepare", "review-code", "review-functional", "review-relevancy", "finalize"]) { + assert.match(flow, new RegExp(slug, "u"), "every action is a node"); + } + for (const outcome of ["approve", "changes", "blocked"]) { + assert.match(flow, new RegExp(`${outcome}\\(\\[`, "u"), "every verdict is a terminal node"); + } + for (const outcome of ["changes", "blocked"]) { + assert.match(flow, new RegExp(`^ ${outcome} -\\.->`, "mu"), "a verdict asking for work loops back"); + } + }); + + it("every action carries the contract's sections, and its test is a table", () => { + for (const action of fs.readdirSync(path.join(SKILL, "actions"))) { + const body = read(path.join("actions", action)); + const headings = [...body.matchAll(/^## (.+)$/gmu)].map((match) => match[1]); + assert.deepEqual(headings, ["Input", "Output", "Process", "Test"], action); + assert.match(body.slice(body.indexOf("## Test")), /^\| Case \| Pass \|$/mu, action); + } + }); + + it("a review reads the code and never runs or fixes it", () => { + assert.match(rules(), /^- Review statically: never run the app\.$/mu); + assert.match(read("SKILL.md"), /Not for fixing what the review finds/u); + }); +});