diff --git a/.changes/planning/feat-8335-plan-time-design-checks-9d4c1e62.md b/.changes/planning/feat-8335-plan-time-design-checks-9d4c1e62.md new file mode 100644 index 00000000000..237bd32d4d5 --- /dev/null +++ b/.changes/planning/feat-8335-plan-time-design-checks-9d4c1e62.md @@ -0,0 +1,13 @@ +--- +bump: minor +--- + +### Added + +- `/planning:design-handoff`: the gate requires a principles thread and a lifecycle-and-reversibility thread, each resolved, directional or tagged-deferred (a prototype names both as deferred); a design missing either fails. +- `/planning:plan`: Step 2's design-default checklist audits the same two threads and confirms the Brief carries the `Quality bar:` line a phase-boundary review rates at; the plan template says to tag `Review: code-design` on any phase that adds or changes a contract, type, module, or a fact or rule several files read, Markdown included, and maps each tag to its `/review:quality-gate` mode (`concurrency` to `slice concurrency`). +- `planning:plan-reviewer`: Design alignment flags a plan that places one fact or rule in several files or rebuilds what existing code already does, and a missing principle or reversibility thread. + +### Fixed + +- `/planning:design-handoff`: a failed gate opens its reply with the `Gate: FAIL` verdict and the coverage table before routing back to `/planning:design`, so the design round no longer replaces the verdict. diff --git a/plugins/planning/evals/dh-missing-reversibility-thread-fails/case.yaml b/plugins/planning/evals/dh-missing-reversibility-thread-fails/case.yaml new file mode 100644 index 00000000000..0e4d9c34219 --- /dev/null +++ b/plugins/planning/evals/dh-missing-reversibility-thread-fails/case.yaml @@ -0,0 +1,4 @@ +schema_version: "1.1" +name: dh-missing-reversibility-thread-fails +context: + scaffold_script: scaffold.sh diff --git a/plugins/planning/evals/dh-missing-reversibility-thread-fails/graders/names-missing-reversibility.md b/plugins/planning/evals/dh-missing-reversibility-thread-fails/graders/names-missing-reversibility.md new file mode 100644 index 00000000000..938a773da5c --- /dev/null +++ b/plugins/planning/evals/dh-missing-reversibility-thread-fails/graders/names-missing-reversibility.md @@ -0,0 +1,5 @@ +--- +type: regex +pattern: '^(?![^\n]*\b(?:no|not|nothing|none|never)(?:\s+\w+){0,3}\s+(?:missing|absent|lacks?)\b)(?![^\n]*n''t(?:\s+\w+){0,2}\s+(?:missing|absent)\b)(?=[^\n]*\b(?:reversib\w*|lifecycle|doors?)\b)[^\n]*?(?:\b(?:reversib\w*|lifecycle|doors?)\b[^\n.;]{0,80}\b(?:missing|absent|not (?:recorded|present|found))\b|\b(?:missing|absent|lacks?|lacking|without)\b[^\n.;]{0,60}\b(?:reversib\w*|lifecycle|doors?)\b|\bno\s+(?:[\w\x60*-]+\s+){0,2}(?:threads?\b|doors?\b|calls?\b|\x60?Door:))' +flags: mi +--- diff --git a/plugins/planning/evals/dh-missing-reversibility-thread-fails/graders/no-plan-edit.md b/plugins/planning/evals/dh-missing-reversibility-thread-fails/graders/no-plan-edit.md new file mode 100644 index 00000000000..ccc433edd1f --- /dev/null +++ b/plugins/planning/evals/dh-missing-reversibility-thread-fails/graders/no-plan-edit.md @@ -0,0 +1,7 @@ +--- +type: tool_used +tool: Edit +input_match: 'PLAN\.md' +min: 0 +max: 0 +--- diff --git a/plugins/planning/evals/dh-missing-reversibility-thread-fails/graders/no-plan-write.md b/plugins/planning/evals/dh-missing-reversibility-thread-fails/graders/no-plan-write.md new file mode 100644 index 00000000000..8671bc5b660 --- /dev/null +++ b/plugins/planning/evals/dh-missing-reversibility-thread-fails/graders/no-plan-write.md @@ -0,0 +1,7 @@ +--- +type: tool_used +tool: Write +input_match: 'PLAN\.md' +min: 0 +max: 0 +--- diff --git a/plugins/planning/evals/dh-missing-reversibility-thread-fails/graders/verdict-fail.md b/plugins/planning/evals/dh-missing-reversibility-thread-fails/graders/verdict-fail.md new file mode 100644 index 00000000000..5af145a9fd5 --- /dev/null +++ b/plugins/planning/evals/dh-missing-reversibility-thread-fails/graders/verdict-fail.md @@ -0,0 +1,4 @@ +--- +type: regex +pattern: '(?.work/export-archive/PLAN.md <<'MD' +# Account data export + +## Brief + +### Goal +Let a customer download every record the product holds about their account as one archive. + +### Acceptance criteria +- A customer can request an export from account settings and download it within an hour. +- The archive holds every project, comment and attachment the account owns. + +### Captured assumptions +- Quality bar: production +- Intent: second variant = speculative; existing structure: one Django app per product area; upstream: none +MD + +cat >"$slice/design-threads.md" <<'MD' +# Design threads: account data export + +## T1. Principles + +- Status: RESOLVED +- Decision: keep the exporter specific to accounts; no generic export framework. +- Rationale: the Brief's `Intent:` line says a second export variant is speculative, so the reuse + default waits for a third occurrence; SRP keeps archive building apart from delivery. + +## T2. Archive format + +- Options: one ZIP of JSON files per entity type; a single newline-delimited JSON file +- Status: RESOLVED +- Decision: one ZIP holding a JSON file per entity type plus the attachment files. +- Rationale: customers open ZIP files without tooling, and per-type files let them find their + comments without parsing projects. + +## T3. Delivery + +- Options: email the archive; a signed download URL valid for 24 hours +- Status: RESOLVED +- Decision: a signed download URL at /exports//download, valid for 24 hours. +- Rationale: archives with attachments exceed mail size limits, and a signed URL expires on its own. + +## T4. Job runner + +- Options: build the archive in the request; the existing Celery worker +- Status: RESOLVED +- Decision: the existing Celery worker builds the archive. +- Rationale: large accounts take minutes to export, longer than the request timeout, and the + worker already runs the product's other long jobs. +MD diff --git a/plugins/planning/evals/plan-reviewer-flags-spread-fact/case.yaml b/plugins/planning/evals/plan-reviewer-flags-spread-fact/case.yaml new file mode 100644 index 00000000000..ae8750f74fb --- /dev/null +++ b/plugins/planning/evals/plan-reviewer-flags-spread-fact/case.yaml @@ -0,0 +1,4 @@ +schema_version: "1.1" +name: plan-reviewer-flags-spread-fact +context: + scaffold_script: scaffold.sh diff --git a/plugins/planning/evals/plan-reviewer-flags-spread-fact/graders/names-spread-fact.md b/plugins/planning/evals/plan-reviewer-flags-spread-fact/graders/names-spread-fact.md new file mode 100644 index 00000000000..18599860517 --- /dev/null +++ b/plugins/planning/evals/plan-reviewer-flags-spread-fact/graders/names-spread-fact.md @@ -0,0 +1,5 @@ +--- +type: regex +pattern: '^(?![^\n]*(?:\b(?:no|not|never|without|none)|n''t)\s+(?:\w+\s+){0,2}(?:(?:duplicate\w*|duplicating|duplications?)|repeat\w*|spread|cop(?:y|ies)|(?:restate\w*|restating|restatements?)|hard-?cod\w*)\b)(?![^\n]*No plan gaps found)(?=[^\n]*(?:\b30\b|retention))[^\n]*\b(?:(?:duplicate\w*|duplicating|duplications?)|repeat\w*|spread\w*|cop(?:y|ies)|(?:restate\w*|restating|restatements?)|hard-?cod\w*|(?:several|multiple|three|3|separate)\s+(?:\w+\s+)?(?:files|places|surfaces|locations|sites))\b' +flags: mi +--- diff --git a/plugins/planning/evals/plan-reviewer-flags-spread-fact/graders/no-code-edit.md b/plugins/planning/evals/plan-reviewer-flags-spread-fact/graders/no-code-edit.md new file mode 100644 index 00000000000..f43c4c8332e --- /dev/null +++ b/plugins/planning/evals/plan-reviewer-flags-spread-fact/graders/no-code-edit.md @@ -0,0 +1,7 @@ +--- +type: tool_used +tool: Edit +input_match: '\.py"' +min: 0 +max: 0 +--- diff --git a/plugins/planning/evals/plan-reviewer-flags-spread-fact/graders/no-code-write.md b/plugins/planning/evals/plan-reviewer-flags-spread-fact/graders/no-code-write.md new file mode 100644 index 00000000000..3076a7bea97 --- /dev/null +++ b/plugins/planning/evals/plan-reviewer-flags-spread-fact/graders/no-code-write.md @@ -0,0 +1,7 @@ +--- +type: tool_used +tool: Write +input_match: '\.py"' +min: 0 +max: 0 +--- diff --git a/plugins/planning/evals/plan-reviewer-flags-spread-fact/graders/spread-fact-judge.md b/plugins/planning/evals/plan-reviewer-flags-spread-fact/graders/spread-fact-judge.md new file mode 100644 index 00000000000..682752fb470 --- /dev/null +++ b/plugins/planning/evals/plan-reviewer-flags-spread-fact/graders/spread-fact-judge.md @@ -0,0 +1,14 @@ +--- +type: llm +--- + +The draft plan writes a 30-day audit log retention window as three separate literals: a constant +in app/cleanup/job.py, a `30` in the app/api/settings_view.py response, and a sentence in +docs/operations.md. The repo already keeps shared limits in app/settings.py. + +PASS if the reported plan-review findings include one that the retention value is placed in more +than one file with no single source, and recommend giving it one home (for example a setting in +app/settings.py that the job and the API read, with the docs pointing at it or generated from it). + +FAIL if no finding addresses where the 30-day value lives, if the repetition is mentioned only to +accept it, or if the reply reports no plan gaps. diff --git a/plugins/planning/evals/plan-reviewer-flags-spread-fact/prompt.md b/plugins/planning/evals/plan-reviewer-flags-spread-fact/prompt.md new file mode 100644 index 00000000000..432f00d15e9 --- /dev/null +++ b/plugins/planning/evals/plan-reviewer-flags-spread-fact/prompt.md @@ -0,0 +1,13 @@ +--- +description: "Outcome case: a draft plan writes one 30-day retention window into the cleanup job, the admin API view and the operator docs as three literals, while the repo keeps shared limits in app/settings.py. The plan stress-test passes when its findings flag the spread fact" +tags: [plan, plan-reviewer, outcome, principles, dry, scaffold] +runs: 3 +max_turns: 30 +timeout_seconds: 900 +allowed_tools: [Read, Glob, Grep, Skill, Bash, Agent, Write, Edit] +expected_outcome: "The reported plan-review findings flag that the 30-day retention value is placed in several files (app/cleanup/job.py, app/api/settings_view.py, docs/operations.md) with no single home, and point at one source such as app/settings.py. No .py file is written or edited" +--- + +/planning:plan + +The draft plan for topic `log-retention` is at `.work/log-retention/PLAN.md`. Run its Step 3 plan stress-test on that draft and show me the findings, then stop. Don't edit any file. diff --git a/plugins/planning/evals/plan-reviewer-flags-spread-fact/samples/names-spread-fact.json b/plugins/planning/evals/plan-reviewer-flags-spread-fact/samples/names-spread-fact.json new file mode 100644 index 00000000000..3b9474667cd --- /dev/null +++ b/plugins/planning/evals/plan-reviewer-flags-spread-fact/samples/names-spread-fact.json @@ -0,0 +1,38 @@ +{ + "pass": [ + { + "answer": "| 1 | IMPORTANT | Design alignment | The 30-day retention window is written into three files (app/cleanup/job.py, app/api/settings_view.py, docs/operations.md) with no shared source | Define it once in app/settings.py and read it everywhere |\n", + "why": "findings table row" + }, + { + "answer": "The plan hardcodes `30` for retention in the job, the API view and the docs.\n", + "why": "hardcoded in each surface" + }, + { + "answer": "- Retention days are duplicated across job.py and settings_view.py; a later change to 30 would diverge.\n", + "why": "duplicated" + } + ], + "fail": [ + { + "answer": "", + "why": "empty" + }, + { + "answer": "No plan gaps found.\n", + "why": "clean pass" + }, + { + "answer": "No duplicated retention value: the plan keeps 30 days in one setting.\n", + "why": "negated" + }, + { + "answer": "The plan doesn't repeat the 30-day retention anywhere.\n", + "why": "negated with n't" + }, + { + "answer": "| 1 | SUGGESTION | Plan mechanics | Phase 1's Sanity Check names tests/test_cleanup.py, which does not exist yet for the 30-day retention job | Create it in the phase |\n", + "why": "a different finding about retention" + } + ] +} diff --git a/plugins/planning/evals/plan-reviewer-flags-spread-fact/samples/no-code-edit.json b/plugins/planning/evals/plan-reviewer-flags-spread-fact/samples/no-code-edit.json new file mode 100644 index 00000000000..7930b70aa58 --- /dev/null +++ b/plugins/planning/evals/plan-reviewer-flags-spread-fact/samples/no-code-edit.json @@ -0,0 +1,36 @@ +{ + "pass": [ + { + "answer": [ + { + "tool": "Edit", + "input": { + "file_path": ".work/log-retention/PLAN.md", + "old_string": "## Plan\n", + "new_string": "## Plan\n\n\n" + } + } + ], + "why": "editing the plan is allowed" + }, + { + "answer": [], + "why": "no tool call" + } + ], + "fail": [ + { + "answer": [ + { + "tool": "Edit", + "input": { + "file_path": "app/cleanup/job.py", + "old_string": "def ", + "new_string": "def changed_" + } + } + ], + "why": "edited existing source code" + } + ] +} diff --git a/plugins/planning/evals/plan-reviewer-flags-spread-fact/samples/no-code-write.json b/plugins/planning/evals/plan-reviewer-flags-spread-fact/samples/no-code-write.json new file mode 100644 index 00000000000..7eeba0da50f --- /dev/null +++ b/plugins/planning/evals/plan-reviewer-flags-spread-fact/samples/no-code-write.json @@ -0,0 +1,22 @@ +{ + "pass": [ + { + "answer": [], + "why": "no tool call" + } + ], + "fail": [ + { + "answer": [ + { + "tool": "Write", + "input": { + "file_path": "app/settings.py", + "content": "AUDIT_RETENTION_DAYS = 30\n" + } + } + ], + "why": "wrote code during a review" + } + ] +} diff --git a/plugins/planning/evals/plan-reviewer-flags-spread-fact/samples/spread-fact-judge.json b/plugins/planning/evals/plan-reviewer-flags-spread-fact/samples/spread-fact-judge.json new file mode 100644 index 00000000000..d33a77c52dd --- /dev/null +++ b/plugins/planning/evals/plan-reviewer-flags-spread-fact/samples/spread-fact-judge.json @@ -0,0 +1,18 @@ +{ + "pass": [ + { + "answer": "## Plan review: audit log retention\n\n| # | Severity | Category | Finding | Action |\n|---|---|---|---|---|\n| 1 | IMPORTANT | Design alignment | The 30-day window is written separately into app/cleanup/job.py, the get_limits() response and docs/operations.md, so a later change to the window updates one and leaves the others wrong | Add AUDIT_RETENTION_DAYS to app/settings.py beside SESSION_TIMEOUT_MINUTES; the job and the API read it, and the docs point at it |\n", + "why": "flags the spread fact and names one home" + } + ], + "fail": [ + { + "answer": "## Plan review: audit log retention\n\n| # | Severity | Category | Finding | Action |\n|---|---|---|---|---|\n| 1 | SUGGESTION | Plan mechanics | tests/test_cleanup.py does not exist yet | Create it in Phase 1 |\n", + "why": "no finding about where the value lives" + }, + { + "answer": "No plan gaps found.", + "why": "clean pass" + } + ] +} diff --git a/plugins/planning/evals/plan-reviewer-flags-spread-fact/scaffold.sh b/plugins/planning/evals/plan-reviewer-flags-spread-fact/scaffold.sh new file mode 100755 index 00000000000..4fd57443568 --- /dev/null +++ b/plugins/planning/evals/plan-reviewer-flags-spread-fact/scaffold.sh @@ -0,0 +1,87 @@ +#!/usr/bin/env bash +# A draft plan for a 30-day log retention window whose file list writes the 30 into the cleanup +# job, the admin API view and the operator docs, each as its own literal, while the repo already +# keeps shared limits in app/settings.py. The plan reviewer should flag the spread fact. +set -euo pipefail + +git init -q -b main +git config user.name eval +git config user.email eval@example.invalid +mkdir -p app/cleanup app/api docs .work/log-retention + +cat >app/__init__.py <<'PY' +PY + +cat >app/settings.py <<'PY' +MAX_UPLOAD_BYTES = 25 * 1024 * 1024 +SESSION_TIMEOUT_MINUTES = 60 +PY + +cat >app/cleanup/__init__.py <<'PY' +PY + +cat >app/cleanup/job.py <<'PY' +from app.settings import SESSION_TIMEOUT_MINUTES + + +def purge_sessions(store, now): + store.delete_older_than(now, minutes=SESSION_TIMEOUT_MINUTES) +PY + +cat >app/api/__init__.py <<'PY' +PY + +cat >app/api/settings_view.py <<'PY' +from app.settings import MAX_UPLOAD_BYTES + + +def get_limits(): + return {"max_upload_bytes": MAX_UPLOAD_BYTES} +PY + +cat >docs/operations.md <<'MD' +# Operations + +Sessions expire after the timeout in `app/settings.py`. +MD + +cat >.work/log-retention/PLAN.md <<'MD' +# Audit log retention + +## Brief + +### Goal +Delete audit log entries older than 30 days, show the window to admins, and document it for operators. + +### Acceptance criteria +- A nightly job deletes audit log entries older than 30 days. +- GET /admin/limits reports the retention window. +- docs/operations.md states the retention window. + +### Captured assumptions +- Quality bar: production + +## Plan + +**Goal.** Enforce, expose and document a 30-day audit log retention window. + +### Phase 1: Retention job, API and docs +- `app/cleanup/job.py`: add `AUDIT_RETENTION_DAYS = 30` and `purge_audit_log(store, now)`, which + deletes entries older than `AUDIT_RETENTION_DAYS`. +- `app/api/settings_view.py`: add `"audit_retention_days": 30` to the `get_limits()` response. +- `docs/operations.md`: add "Audit log entries are deleted after 30 days." +- **Sanity Check:** `pytest tests/test_cleanup.py` exits 0. + +### Files affected +| File | Action | +|---|---| +| app/cleanup/job.py | modify: new constant and purge function | +| app/api/settings_view.py | modify: new response field | +| docs/operations.md | modify: new sentence | + +### Test strategy +Unit test `purge_audit_log` with entries 29 and 31 days old. +MD + +git add . +git commit -q -m "feat: settings and session cleanup" diff --git a/plugins/planning/skills/design-handoff/SKILL.md b/plugins/planning/skills/design-handoff/SKILL.md index 4d8741c4d33..4ec00534d8b 100644 --- a/plugins/planning/skills/design-handoff/SKILL.md +++ b/plugins/planning/skills/design-handoff/SKILL.md @@ -45,9 +45,24 @@ Read `design-threads.md` in the topic's design slice and confirm, thread by thre - **directional**. Direction agreed AND the remaining detail carries a research tag, or - **TAGGED-DEFERRED**. An explicit research tag naming the external investigation needed. -A thread that is unresolved AND untagged is a silent gap → **FAIL**: list the offending -thread(s), route back by invoking `/planning:design` via the Skill tool (its design-threads and -discussion rounds) to resolve or tag, and do NOT hand off. Read the verdict off +Two threads are required, each in one of those states: + +- **Principles**: which design principles, or consumer overrides recorded as + `Overrides bundled default per `, shaped the design, and how + generalize-now-or-wait was settled (citing the Brief's `Intent:` line counts). +- **Lifecycle and reversibility**: the door calls `/planning:design` "Lifecycle and + reversibility" requires at the Brief's quality bar, in a thread of that name or as `Door:` lines + in the decision threads. + +At quality bar `prototype` both are skipped and named: a thread reading `deferred (prototype)` +counts as TAGGED-DEFERRED. Stumble: the gate judged only the threads present, so a design that +never weighed principles or reversibility passed. + +A thread that is unresolved AND untagged, or a required thread that is missing, is a silent gap → +**FAIL**: open the reply with the verdict sentence (`Gate: FAIL`, naming each offending or missing +thread) and the coverage table, then route back by invoking `/planning:design` via the Skill tool +(its design-threads and discussion rounds) to resolve or tag, and do NOT hand off. Stumble: routing +first let the design round replace the verdict, so the reply never said the gate failed. Read the verdict off `design-threads.md`, never from a "did we cover enough?" recap of your own: a producing model rubber-stamps its own recap. @@ -59,7 +74,7 @@ FAIL: no design evidence; route back by invoking `/planning:design` via the Skil ## Coverage report (advisory) -After the verdict sentence, and on a FAIL after the routing sentence too, emit one table reporting +After the verdict sentence (on a FAIL, before routing back), emit one table reporting coverage across the six dimensions a design answers: what, how, where, who, when, why. Emit it on PASS and on FAIL alike. It never contributes to the pass/fail determination, it never blocks a handoff, no flag makes it block, and the table is never written to disk. @@ -143,7 +158,8 @@ Hand off by invoking `/planning:plan` via the Skill tool. Sourced from the artif - Deferred research items with tags - Design artifacts produced, and that PLAN.md's `## Design` section now holds them - Dependency order for implementation (which decisions block others) -- Extension / config / observability threads **RESOLVED** or **TAGGED-DEFERRED**. +- Extension / config / observability, principle and reversibility threads **RESOLVED** or + **TAGGED-DEFERRED**. `/planning:plan` next walks its design-default checklist against the plan - **Review-routing notes**. When the consuming project declares review checklists (architecture, code-design, security, multi-tenancy, messaging, and the like), list which apply to this slice diff --git a/plugins/planning/skills/design-handoff/evals/evals.json b/plugins/planning/skills/design-handoff/evals/evals.json index f91b4efce59..909bd1af21c 100644 --- a/plugins/planning/skills/design-handoff/evals/evals.json +++ b/plugins/planning/skills/design-handoff/evals/evals.json @@ -11,7 +11,8 @@ "Output evaluates each thread against the artifact and reports a PASS", "Output emits a plan-ready handoff summary sourced from the artifact (resolved decisions with rationale, deferred research tags)", "The summary is drawn from the artifact content, not recalled from conversation memory", - "Output emits a resume prompt so a fresh session can pick up at /planning:plan" + "Output emits a resume prompt so a fresh session can pick up at /planning:plan", + "The required Principles and Lifecycle and reversibility threads are present and RESOLVED, so neither is reported missing" ] }, { @@ -75,7 +76,8 @@ "Each row names the thread from the fixture that addresses that dimension and that thread's status", "The directional threads (runtime placement; timing, sequencing and lifecycle) count as covering their dimensions, reported with status directional", "No row reads `none`", - "The coverage table follows the PASS verdict and does not change it" + "The coverage table follows the PASS verdict and does not change it", + "The required Principles and Lifecycle and reversibility threads are present and RESOLVED, so neither is reported missing" ] }, { @@ -89,7 +91,8 @@ "The coverage table reads `none` for the where row and for the who row", "The coverage gap does NOT downgrade the PASS or block the handoff, and is not promoted to a FAIL", "The handoff summary lists the uncovered dimensions (where, who) and the resume prompt carries them", - "The TAGGED-DEFERRED encoding thread is not treated as covering any dimension" + "The TAGGED-DEFERRED encoding thread is not treated as covering any dimension", + "The required Principles and Lifecycle and reversibility threads are present and RESOLVED, so neither is reported missing" ] }, { @@ -117,7 +120,9 @@ "The ADR candidates list holds exactly one candidate: the tunnel exposure boundary decision", "That candidate shows pass or fail on each of the three criteria with one line of evidence each, including the rejected alternative (direct exposure with per-request token checks) taken from the artifact", "The data-folder-per-session and log line format decisions are not listed as candidates and are named as failing, each naming the criterion it fails (costly to undo and/or not obvious from the code)", - "Any offer to invoke /architecture:record-decision applies only to the listed candidate, not to the two that failed" + "Any offer to invoke /architecture:record-decision applies only to the listed candidate, not to the two that failed", + "The required Principles and Lifecycle and reversibility threads are present and RESOLVED, so neither is reported missing", + "The Principles and Lifecycle and reversibility threads are not listed as ADR candidates" ] }, { @@ -130,7 +135,9 @@ "Output reports a PASS and emits the handoff summary", "The ADR candidates entry says None and lists no candidate", "No decision from the artifact (file naming, defaults-file bridge, page size) is offered to /architecture:record-decision", - "Each of the three decisions is named on a separate not-candidates line with the criterion it fails and one line of evidence" + "Each of the three decisions is named on a separate not-candidates line with the criterion it fails and one line of evidence", + "The required Principles and Lifecycle and reversibility threads are present and RESOLVED, so neither is reported missing", + "The Principles and Lifecycle and reversibility threads are not listed as ADR candidates" ] } ] diff --git a/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-all-resolved.md b/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-all-resolved.md index d1a8b9b3266..8371d8ffd22 100644 --- a/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-all-resolved.md +++ b/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-all-resolved.md @@ -25,3 +25,18 @@ limits before finalizing the policy thresholds]. [RESEARCH: evaluate whether the existing telemetry sink can carry per-attempt charge outcomes, or whether a new structured event is needed]. + +## Thread 5: Principles. RESOLVED + +Principles applied: DIP puts the domain in charge of the `IPaymentGateway` port; information +hiding keeps provider refund semantics inside each adapter. +Generalize now or wait: the Brief's `Intent: second variant = known` line (a third provider is +contracted) settles it: generalize now, isolating what varies behind the port. +Rationale: the Intent line is the user's answer, so the port is built now rather than on the +third occurrence. + +## Thread 6: Lifecycle and reversibility. RESOLVED + +Door: Thread 1 one-way (every adapter builds on the port's shape); Thread 2 one-way (stored +idempotency keys must keep matching across a deploy). The ADR prompt was offered for both. +Rationale: changing either needs a coordinated release, so both were designed deliberately. diff --git a/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-coverage-all-six.md b/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-coverage-all-six.md index 0d8206ebc90..0ddd45fdcc2 100644 --- a/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-coverage-all-six.md +++ b/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-coverage-all-six.md @@ -39,3 +39,16 @@ Decision: keep the existing outbox rather than adopting the provider's own broad Rationale: the outbox already gives us at-least-once delivery and a replayable audit trail, and the provider's broadcast API offers neither; the recorded tradeoff is one extra hop for durability we would otherwise have to rebuild. + +## Thread 7: Principles. RESOLVED + +Principles applied: SRP keeps per-channel formatting out of the fanout service; reuse of the +existing outbox over a new delivery path. +Generalize now or wait: the Brief's `Intent: second variant = category` line settles it: channels +vary, so the per-channel step is isolated behind one interface now. +Rationale: the Intent line is the user's answer; every new channel is a known variation. + +## Thread 8: Lifecycle and reversibility. RESOLVED + +Door: Thread 1 one-way (callers build on the event contract); Threads 2, 4 and 6 two-way. +Rationale: only the event contract crosses a team boundary; the rest changes in code alone. diff --git a/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-coverage-gaps.md b/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-coverage-gaps.md index d15e0b99c04..81064e71939 100644 --- a/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-coverage-gaps.md +++ b/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-coverage-gaps.md @@ -29,3 +29,16 @@ that join is the whole point of those reports; the recorded tradeoff is one more [RESEARCH: confirm which of the three consumers still require a BOM-prefixed UTF-8 file before fixing the encoding]. + +## Thread 6: Principles. RESOLVED + +Principles applied: SRP keeps the query apart from the writer; YAGNI rules out a pluggable format +layer. +Generalize now or wait: the Brief's `Intent: second variant = speculative` line settles it: wait +for the third occurrence and keep the CSV writer specific. +Rationale: no second export format is planned, so an abstraction now would have one implementation. + +## Thread 7: Lifecycle and reversibility. RESOLVED + +Door: Thread 1 one-way (the column layout is a published contract); Threads 2, 3 and 4 two-way. +Rationale: only the column layout outlives a code change; the rest is replaced by editing code. diff --git a/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-no-adr-worthy.md b/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-no-adr-worthy.md index aae52a8ffd8..a940fc743ca 100644 --- a/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-no-adr-worthy.md +++ b/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-no-adr-worthy.md @@ -20,3 +20,15 @@ Decision: stream exports in pages of 500 rows. Rationale: the constant lives in one place and can be tuned from measurements without any data or contract change. Loading the whole result set into memory was rejected because of peak memory on large reports. + +## Thread 4: Principles. RESOLVED + +Principles applied: YAGNI keeps options in one defaults file until the settings service lands. +Generalize now or wait: the Brief's `Intent: second variant = speculative` line settles it: wait +for the third occurrence. +Rationale: the Intent line is the user's answer. + +## Thread 5: Lifecycle and reversibility. RESOLVED + +Door: Threads 1, 2 and 3 two-way. +Rationale: each changes by editing code, with no data or contract change. diff --git a/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-one-adr-worthy.md b/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-one-adr-worthy.md index 5c31183d776..b9fd8b3e1ae 100644 --- a/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-one-adr-worthy.md +++ b/plugins/planning/skills/design-handoff/evals/fixtures/design-threads-one-adr-worthy.md @@ -24,3 +24,15 @@ Decision: log one JSON object per line. Rationale: the existing log reader already parses JSON lines, and the format can be changed by editing one formatter. Plain text lines were rejected because they would need a new parser. + +## Thread 4: Principles. RESOLVED + +Principles applied: information hiding keeps the trust decision in the proxy, out of the handlers. +Generalize now or wait: the Brief's `Intent: not applicable (confirmed)` line settles it: no +variation question. +Rationale: the Intent line is the user's answer. + +## Thread 5: Lifecycle and reversibility. RESOLVED + +Door: Thread 1 one-way (integrators build on the exposure); Threads 2 and 3 two-way. +Rationale: Thread 1 is the only call that integrators build on. diff --git a/plugins/planning/skills/plan/SKILL.md b/plugins/planning/skills/plan/SKILL.md index 867486deb5d..b472ef1ad3f 100644 --- a/plugins/planning/skills/plan/SKILL.md +++ b/plugins/planning/skills/plan/SKILL.md @@ -40,7 +40,7 @@ For multi-step planning (almost always steps 1-5), copy `templates/checklist.md` **Skip when:** the one-sentence skip (Purpose) applied, so no plan is written. On a mid-flight `review` replan, append a dated scope-change note and revise the PLAN phases instead; do not spawn a second checklist file. -For **non-trivial / multi-layer** plans, Step 2 walks its design-default checklist **against the plan**: confirm the plan carries the resolved threads in PLAN.md's `## Design` section (and the design artifacts behind it) rather than re-deriving the axes. Configurability, extension points, observability, and testability are design threads owned by `/planning:design`'s "Design defaults"; type-collaboration shape is its type modeling. Audit that the plan carries their resolutions, don't re-open them. Magic-literal hygiene stays plan's own review check against the consuming project's review conventions when it declares them. +For **non-trivial / multi-layer** plans, Step 2 walks its design-default checklist **against the plan**: confirm the plan carries the resolved threads in PLAN.md's `## Design` section (and the design artifacts behind it) rather than re-deriving the axes. Configurability, extension points, observability, and testability are design threads owned by `/planning:design`'s "Design defaults"; type-collaboration shape is its type modeling. The principle and lifecycle-and-reversibility threads `/planning:design-handoff` requires are audited the same way: resolved, directional or tagged-deferred (at quality bar `prototype`, named as deferred); a missing one routes back through `/planning:design-handoff`, except under an early-exit `design-resolution.md`, which carries neither. Audit that the plan carries their resolutions, don't re-open them. When a phase is tagged for a phase-boundary review, confirm the Brief carries a `Quality bar:` line: that review rates at it ([context/plan-template.md](context/plan-template.md) "Phase Review tags"). Magic-literal hygiene stays plan's own review check against the consuming project's review conventions when it declares them. ## Action Router diff --git a/plugins/planning/skills/plan/context/plan-reviewer.md b/plugins/planning/skills/plan/context/plan-reviewer.md index 7de0047f770..d01c03e846c 100644 --- a/plugins/planning/skills/plan/context/plan-reviewer.md +++ b/plugins/planning/skills/plan/context/plan-reviewer.md @@ -51,6 +51,8 @@ the finding; a tool behavior you did not probe is an assumption, and the finding - Testable by design — logic separated from orchestration; injectable boundaries for planned handlers/services - Bug-fix plans — does the test strategy name the regression test (level + rough assertion), or document an explicit carve-out? - Names reveal responsibility — vague role-suffixes (Manager, Helper, Util, Processor) on planned types/files? +- Does the planned file list or design place one fact or rule in several files, or build what existing code already does? +- Principle and reversibility threads: the Design section or design artifacts record them resolved, directional or tagged-deferred (a prototype names them deferred); a missing one is a finding, except under an early-exit `design-resolution.md`, which carries neither - Variation verdicts: every external system the plan touches has a verdict in the Design section of abstract now or contain now, and a `leave inline` verdict on an external system is a finding (leave inline is for internal one-offs only); no provider name reaches domain code, storage, or a public contract under a contain verdict; no planned abstraction exposes implementation types in its signatures ### Plan mechanics diff --git a/plugins/planning/skills/plan/context/plan-template.md b/plugins/planning/skills/plan/context/plan-template.md index 8b25f34843d..ef07ab014bb 100644 --- a/plugins/planning/skills/plan/context/plan-template.md +++ b/plugins/planning/skills/plan/context/plan-template.md @@ -359,9 +359,13 @@ When a phase touches a review concern that should be caught at the phase boundar Review: architecture # structural integrity, dependency direction Review: security # auth, input handling, secrets Review: concurrency # races, shared state -Review: code-design # configurability, testability, observability +Review: code-design # repeated facts, cascades, configurability, testability, observability ``` +Each tag maps to a `/review:quality-gate` mode: `architecture`, `security` and `code-design` to the mode of the same name, `concurrency` to `/review:quality-gate slice concurrency`. The review rates at the Brief's `Quality bar:` line, which implementation reads from the plan and passes with the phase diff; a phase never repeats it. + +Tag `Review: code-design` on any phase that adds or changes a contract, type, module, or a fact or rule several files read, skill and instruction Markdown included; omit it only for trivial edits. Stumble: phases that only changed Markdown or a shared value went untagged, so no phase-boundary review saw the copies they spread. + A phase that changes what users see or do can also carry one optional evidence value, written as a plain description of the evidence: ```markdown @@ -370,8 +374,6 @@ Review: screenshots of each changed screen, plus a short recording of the change Leave out the recording when no interaction changes. No reviewer runs on this value at the phase boundary: the evidence is gathered when the pull request is marked ready (`/source-control:pull-request ready`, when it is among the available skills; otherwise attach it by hand) and linked from the pull request. A phase that must leave its screens unchanged uses [Visual parity](#visual-parity-optional-per-phase) instead. -Omit `Review:` when the phase is docs-only or trivial with no new types/contracts. - ## Planned breakage (optional per phase) When a phase knowingly leaves the build, the tests or the lint red until a later phase lands (stub signatures before their callers, a schema change before its readers), declare it in that phase so implementation tells the planned failure from a regression: