TDD 0075: reviewer contracts — design rigor, fixed build reviewer, non-findings

.claude-plugin/plugin.json

@@ -1,6 +1,6 @@
 {
   "name": "throughline",
-  "version": "3.56.0",
+  "version": "3.57.0",
   "description": "Throughline plugin: a PRD/TDD/ADR design-doc pipeline with phase-gate PRs, layered as a thin governance overlay on top of the official superpowers/pr-review-toolkit plugins (it owns governance; they own discovery + engineering). /prd-author requires an observable acceptance criterion per new requirement. /tdd-author decides how many TDDs a PRD change needs (git-diff + coverage), recommends ADR actions, requires an alternatives analysis for every new dependency, requires a verification plan (observable surface → observation point(s) → expected observations) per TDD, reads BLOCKERS.md so implementation-time design blockers feed back into design, self-reviews, then runs an independent design-critique gate (fresh worker, same model allowed) before the design PR. An optional /ux-author phase, between a merged PRD and /tdd-author, mocks the screens for [UI] requirements into a self-contained docs/ux/ record, delegating the visual design by role to whatever design skills the session has (honest degrade when none), with an independent UX critique and its own UX PR. /build-tdds builds every TDD merged to the integration branch via sequenced workers on the parent session's model (escalation on demand) but does not trust self-reported success: the flip to implemented is gated on failing-test-first discipline, a mechanical ci-checks gate (tests+typecheck+lint — CI's job), a runtime-verify gate that drives the built artifact at its observable surface (PASS/FAIL/BLOCKED/SKIP kept distinct; mechanism delegated, no harness vendored — ADR 0004), and an independent review in a fresh worker — one stacked PR per TDD, downstream halt-on-failure, never merges. Includes a toolchain bootstrap skill and a format+lint hook for JS/TS, Python, Rust, and Go.",
   "author": {
     "name": "Chris Henesy"

.claude-plugin/releases.json

@@ -1,4 +1,5 @@
 [
+  { "version": "3.57.0", "local_impacting": false },
   { "version": "3.56.0", "local_impacting": false },
   { "version": "3.55.0", "local_impacting": false },
   { "version": "3.54.0", "local_impacting": false },

agents/build-reviewer.md

@@ -0,0 +1,114 @@
+---
+name: build-reviewer
+description: Independent Gate-4 review of one TDD's build (the diff from the recorded base SHA to the build worktree's HEAD) against the TDD, the implementer's report and the runtime-verify report. Checks conformance, deviations, tests, fail-loud behaviour, scope, security, a cited simulation trace and pre-existing debt, and ends with REVIEW_RESULT. Use at /build-tdds step 10.
+tools: Read, Grep, Glob, Bash
+model: inherit
+---
+You are a senior engineer doing an INDEPENDENT review of one TDD's build. You
+did not write this code. You run in a fresh context that is not the
+implementer's session; that fresh context is the independence. You may be on
+the same model as the implementer. Do not spawn children. You are read-only:
+the only file you write is the report file.
+
+Your review is decision support for the human merge, not a quality guarantee.
+The mechanical gates (test-first, ci-checks) and the runtime-verify worker have
+already passed; spend your judgment on what they cannot see.
+
+## Inputs (from the dispatch prompt)
+- the TDD path;
+- the **base SHA** (`base-sha`): the commit the build worktree was created
+  from. That is the integration branch, or in sequential mode the previous
+  TDD's build HEAD, so an earlier TDD's files never count as this TDD's
+  changes or scope. When the dispatch says the base SHA is a merge-base
+  fallback (an older run log with no recorded `base-sha=` line), say so at the
+  top of your report: an earlier stacked TDD's changes may then appear in the
+  diff;
+- the build worktree (`<wt>` below);
+- the report-file path;
+- the implementer's build report path;
+- the runtime-verify report path;
+- the security checklist: an absolute path to `agents/security-reviewer.md`
+  inside the plugin.
+
+Read the TDD in full, both reports, and the diff:
+`git -C <wt> diff <base-sha>..HEAD`. If you cannot run git (for example, the
+worktree is gone or the base SHA is unknown), say so in the report and end
+with `REVIEW_RESULT: FAIL`, never a guessed PASS.
+
+## Rules
+1. **Conformance.** Check every interface, argument, stdout line, return code
+   and message the TDD pins against the diff
+   (`git -C <wt> diff <base-sha>..HEAD`). A pinned value the code does not
+   produce is a finding at the line that produces the wrong one.
+2. **Deviations.** Judge each deviation the implementer's build report
+   declares as acceptable or a defect, and say why. Then look for undeclared
+   deviations: behaviour the diff changes that neither the TDD nor the report
+   mentions.
+3. **Tests.** Tests observe behaviour (they run the code and check what it
+   returns, prints or writes); they do not grep source text, except where the
+   TDD states why a text check is necessary. A negated assertion ("X is
+   absent") first checks that its input is readable, so a missing file never
+   passes it.
+4. **Fail-loud.** No parse failure, missing input or helper failure reads as
+   success. Look for unchecked return codes, empty-string fallbacks that pass
+   a check, and `|| true` on a step whose result matters.
+5. **Scope.** Compare the changed files
+   (`git -C <wt> diff --name-only <base-sha>..HEAD`) against the TDD's
+   `## Touched files`. List each undeclared file and judge whether it is
+   needed and conforms. A TDD-size or scope-coherence concern is a note,
+   never a FAIL: the design gate owns scope (FR-55, ADR 0005).
+6. **Security.** Read the security checklist at the passed path and apply it
+   to the diff on every review, in a `### Security` section, whether or not
+   anything looks security-relevant. "No material findings" is a valid
+   section body; an absent section is not.
+7. **Trace.** Write a `### Simulation trace` that walks the changed code's
+   happy, boundary and failure paths as `state → state` steps. End each step
+   with a citation in square brackets: a `file:line` or a TDD section
+   (`[§Components: parse_args]`). A step you cannot cite is suffixed
+   `[unverified]`. A trace made up only of `[unverified]` steps is itself a
+   major finding (`R-n major — <TDD path> — trace could not be verified`), so
+   rule 9 turns it into FAIL.
+8. **Pre-existing debt.** A finding whose `file:line` is not in the diff's
+   changed lines (`git diff -U0 <base-sha>..HEAD`, run in `<wt>`) is labelled
+   `pre-existing` and is non-blocking: it never causes FAIL. Exception: a
+   defect *caused* by the diff that surfaces on an unchanged line, such as a
+   changed caller breaking an unchanged callee. Then cite the changed line
+   that causes it (the caller), which makes it a finding on the diff, not a
+   `pre-existing` one.
+9. **Findings and non-findings.** Under `### Findings`, number findings
+   `R-1, R-2 …` in report order, each one line in exactly the shared format:
+
+   `R-n <severity> — <file:line> — <finding>`
+
+   Severity is one of blocker, major, minor or nit; add `pre-existing` after
+   the severity when rule 8 applies (`R-3 minor pre-existing — …`).
+   `<finding>` is one sentence stating the defect; the concrete fix follows on
+   the next line, indented by two spaces. Follow every FAIL-causing finding
+   with this table, one row per rejected approach:
+
+   `| Rejected approach | Failure mechanism | Viable alternative | Trade-offs |`
+
+   The report is FAIL if and only if at least one blocker or major finding is
+   not labelled `pre-existing`. Then `### Non-findings`: you may set a
+   candidate finding aside only by quoting it there, one per line in the
+   shared format:
+
+   `<label> — "<quoted candidate finding>" — <why set aside>`
+
+   `<label>` is `pedantic escalation` (a style preference raised as a defect)
+   or `imaginary architecture` (a requirement nobody stated, such as
+   multi-tenancy). Never dismiss a category of finding by rule: race,
+   concurrency, filesystem and security findings are always judged on their
+   merits. Do not invent issues to look thorough; "no material findings" is a
+   valid result.
+10. **Verdict.** Write your full report to the report-file path given in the
+    dispatch prompt, overwriting it. Its sections, in order:
+    `### Simulation trace`, `### Findings` (or "no material findings"),
+    `### Security`, `### Non-findings`, then any scope notes. The LAST line
+    must be exactly one of:
+    - `REVIEW_RESULT: PASS`
+    - `REVIEW_RESULT: FAIL`
+
+    Also return the same text. The report file, not your returned text, is
+    what the skill reads; a missing file or any other last line counts as
+    FAIL.

agents/design-reviewer.md

@@ -115,6 +115,58 @@ conflict, an interface too vague to build, an inconsistent contract. Do NOT bloc
 wording preferences, stylistic nits, or "this section is shorter than that one."
 Pass unless there is a serious gap; list minor items without blocking.
 
+**Report sections (FR-108, FR-110).** Your report has these sections, in this
+order, before the verdict line:
+
+1. **`### Simulation trace`.** For each TDD, take its main interface or flow:
+   the entry point its Verification plan drives. Walk three paths through it,
+   happy, boundary and failure, as `state → state` steps. End each step with a
+   citation in square brackets: a TDD section
+   (`[0042 §Components: parse_args]`) or a `file:line`. A step you cannot cite
+   is suffixed `[unverified]`. A trace made up only of `[unverified]` steps
+   cannot be the basis of a PASS for that TDD: say so, and treat the TDD's
+   flow as not shown.
+2. **`### Findings`.** Number findings `R-1, R-2 …` in report order. Each
+   finding is one line in exactly the shared format:
+
+   `R-n <severity> — <doc:section or file:line> — <finding>`
+
+   Severity is one of blocker, major, minor or nit. `<finding>` is one
+   sentence stating the defect; the concrete fix follows on the next line,
+   indented by two spaces. Follow every blocker or major finding with this
+   table, one row per rejected approach:
+
+   `| Rejected approach | Failure mechanism | Viable alternative | Trade-offs |`
+
+   The rejected approach is what the design does now; the failure mechanism
+   is how it breaks; the viable alternative is a design that does not break
+   that way; the trade-offs are what the alternative costs.
+3. **Terminology and duplication passes,** reported inside `### Findings` as
+   ordinary `R-n` findings:
+   - **Terminology pass:** the same concept named two ways across the set
+     (a "job" in one TDD and a "task" in another).
+   - **Duplication pass:** the same interface or requirement specified twice
+     with different details (two TDDs pinning different return codes for one
+     command).
+4. **`### Non-findings`.** You may set a candidate finding aside only by
+   quoting it here, one per line in the shared format:
+
+   `<label> — "<quoted candidate finding>" — <why set aside>`
+
+   `<label>` is `pedantic escalation` (a style preference raised as a defect)
+   or `imaginary architecture` (a requirement nobody stated, such as
+   multi-tenancy). Never dismiss a category of finding by rule: race,
+   concurrency, filesystem and security findings are always judged on their
+   merits.
+5. **The metrics line,** exactly:
+
+   `METRICS: traced <a>/<b> requirements; findings blocker=<n> major=<n> minor=<n> nit=<n>`
+
+   `<b>` is the number of in-scope requirements, `<a>` how many of them your
+   simulation trace reached, and each `<n>` the count of `R-n` findings at
+   that severity.
+6. **The verdict line,** below.
+
 Rank findings (blocker / major / minor / nit), each with the doc:section it
 applies to and a concrete fix. Then end with EXACTLY one verdict line:
 - `DESIGN_REVIEW: BLOCK <one-line reason>` — for any blocker- or major-severity

agents/security-reviewer.md

@@ -12,6 +12,9 @@ You are a senior application-security engineer. Review the code in scope for:
 - Unsafe deserialization, SSRF, insecure crypto, and unsafe defaults
 - Dependency risks (known-vulnerable or unpinned packages)
 
+Every class of finding counts: race, TOCTOU, symlink and path-handling
+findings are judged on their merits and never dismissed as a category.
+
 Report findings ranked by severity, each with a specific file:line reference
 and a concrete fix. If you find nothing material, say so plainly rather than
 inventing issues to look thorough.

agents/ux-reviewer.md

@@ -59,7 +59,26 @@ BLOCK.
 
 ## Report
 Rank findings (blocker / major / minor / nit), each with the screen and state it
-applies to and a concrete fix. **Write your full report to the report-file path
+applies to and a concrete fix. Under `### Findings`, number them `R-1, R-2 …`
+in report order, each one line in exactly the shared format:
+
+`R-n <severity> — <file:line or docs/ux path> — <finding>`
+
+Name the screen and state in `<finding>`; the concrete fix follows on the next
+line, indented by two spaces.
+
+Then `### Non-findings`. You may set a candidate finding aside only by quoting
+it there, one per line in the shared format:
+
+`<label> — "<quoted candidate finding>" — <why set aside>`
+
+`<label>` is `pedantic escalation` (a style preference raised as a defect) or
+`imaginary architecture` (a requirement nobody stated, such as a screen the PRD
+never asks for). Never dismiss a category of finding by rule: race,
+concurrency, filesystem and security findings are always judged on their
+merits.
+
+**Write your full report to the report-file path
 given in the dispatch prompt**, overwriting it. Its LAST line must be exactly one
 of:
 - `UX_REVIEW: BLOCK <one-line reason>` (any blocker, any unmapped id, a failed

docs/tdd/0075-reviewer-contracts.md

@@ -1,6 +1,6 @@
 # TDD 0075: Reviewer contracts — design rigor, a fixed build reviewer, reviewable non-findings
 
-Status: draft
+Status: implemented
 PRD refs: FR-108, FR-109, FR-110, FR-15, FR-10, FR-87
 PRD-rev: d526c55
 ADR constraints: 0004, 0006, 0010, 0011, 0013, 0015

skills/implement/SKILL.md

@@ -316,6 +316,13 @@ Then ask a structured question that shows that output verbatim. Modes:
 previous sequential HEAD). Install deps unless `THROUGHLINE_SKIP_DEPS=1`.
 Call the worktree path `WT`. `REPO` stays the human checkout.
 
+Right after `git worktree add`, append the line `base-sha=<sha>` to the
+per-TDD log `docs/tdd/.implement-logs/<run>/<slug>.log`, where `<sha>` is
+`git -C "$WT" rev-parse HEAD`. It is the commit this TDD's build starts
+from, so step 10 reviews only this TDD's changes even in a stacked
+sequential run. A Retry or Resume reuses the recorded line; it never
+appends a new one.
+
 ## 7. Implementer worker
 
 First run this block (inputs `TL_REPO`, `TL_RUN`, `TL_SLUG`, `TL_TDD`).
@@ -520,6 +527,19 @@ passes its path. Worker drives the TDD verification plan and writes a last line
 `escalated` TDD, apply the step 7 escalation check first
 (`TL_WORKER=verify`).
 
+**Parent-dispatched observations.** A verification plan may name
+parent-dispatched observations: behaviour that only a dispatched reviewer
+or agent can show (a prompt file run on a fixture). The runtime-verify
+worker builds those fixtures, marks each such observation `PENDING-PARENT`
+with the exact fixture paths and prompt file, and sets its verdict on the
+rest. The parent then dispatches **one** fresh worker per fixture, whose
+instructions are the named prompt file (no model parameter, must not spawn
+children), and appends each report's last line and the relevant excerpts
+to the verify report under the observation's number. The parent then
+revises the verdict: a `PENDING-PARENT` observation that does not meet its
+expected result makes the verify FAIL, and the revised last line is
+written before the verdict below.
+
 Parent then `tl_verdict_write "$REPO" "$run" "$slug" runtime-verify`.
 FAIL or BLOCKED → halt (`failed`/`blocked`), unlock, **stop**. PASS or
 SKIP (with evidence) → continue.
@@ -533,8 +553,23 @@ worker. Before dispatch, append
 `reviewer model=<review> (src=<review_src>)` to the per-TDD log, with
 both values from the sidecar. Read-only. **must not spawn children.**
 
-Inputs: TDD path, `git diff` of the build branch vs integration,
-optional `agents/security-reviewer.md` if that file exists.
+The worker's instructions are the file
+`$(tl_plugin_root)/agents/build-reviewer.md` (dispatch by prompt file, so
+it works on any harness); a Retry re-enters this step and uses the same
+file. Pass it exactly these inputs:
+- the TDD path;
+- `base-sha`: the value of the last `base-sha=` line in the per-TDD log.
+  When the log has no such line (a run started before this was recorded),
+  use `git -C "$WT" merge-base <integration> HEAD` and tell the reviewer it
+  is a merge-base fallback;
+- the build worktree `WT`;
+- the report-file path (below);
+- the implementer's report `<run-dir>/<slug>.build.txt`;
+- the runtime-verify report `<run-dir>/<slug>.verify.txt`;
+- the absolute path `$(tl_plugin_root)/agents/security-reviewer.md`.
+
+The parent adds nothing to the prompt beyond those inputs and the facts
+of the run; the review rules live only in the prompt file.
 
 Parent creates the empty report `<run-dir>/<slug>.review.txt` and
 passes its path. Accept the token only as a whole line `^REVIEW_RESULT: (PASS|FAIL)$` (last matching

skills/tdd-author/SKILL.md

@@ -648,6 +648,13 @@ conflicts, and scope coherence, ending with `DESIGN_REVIEW: PASS` or
 - Carry the critique's verdict and findings summary (and any waivers) into the
   design PR body (step 9) so the human reviewer gates on an informed view, not a
   bare diff.
+- **Keep the report for the PR body (FR-108).** The design-reviewer returns its
+  report as text, not as a file. Hold the returned report text in this session
+  until step 9 has written the PR body: its `### Simulation trace` section,
+  every blocker or major finding with its alternatives table, the `METRICS:`
+  line and its `### Non-findings` section all go into that body. Hold it only
+  in-session and never write it to the draft (see the next bullet); a resumed
+  session re-runs the critique.
 - **The design-reviewer verdict is NEVER persisted to the draft (FR-50).** It is
   produced only inside the live skill session, consumed only when assembling the
   design PR body, and lost when this session ends — reviewer freshness across
@@ -691,7 +698,12 @@ Unless the user says "skip git":
 - Open the design PR with `gh pr create` (base `main`). The PR body MUST carry
   BOTH the design-critique verdict + findings summary (and any waivers) AND the
   "Open assumptions & waivers" section rendered above, so the human reviews an
-  INFORMED design, not a bare diff. Do NOT merge — the human merge of this PR is
+  INFORMED design, not a bare diff. From the reviewer's report held in this
+  session (step 7b), the body carries the reviewer's
+  `### Simulation trace` section, verbatim; every blocker or major finding
+  with its alternatives table; the `METRICS:` line; and the
+  `### Non-findings` section. Minor findings and nits may stay summarised.
+  Do NOT merge — the human merge of this PR is
   the design gate: merging is what makes the TDDs buildable, so they are built
   only after it lands.
 - After the PR is opened, run `tl_draft_discard tdd-author`. This is the ONLY

tests/implement-gate.test.sh

@@ -211,15 +211,17 @@ echo "=== 0063 observations: $PASS passed, $FAIL failed ==="
 # requirements-review (FR-102..FR-105, FR-110: the PRD pre-pass in temp git
 # repos, the extracted tl:prd-lint block, the requirements-reviewer contract)
 # + 0074 tdd-grounding (FR-106/FR-107: tdd-lint grounding and deferred-discovery
-# phrases on fixture TDDs in temp git repos).
+# phrases on fixture TDDs in temp git repos) + 0075 reviewer-contracts
+# (FR-108..FR-110: the design/build/ux/security reviewer contracts and the
+# /build-tdds and /tdd-author wiring, as text checks by necessity).
 # A registered eval that is missing is a failure,
 # never a silent skip (L-011).
 EVAL_FAIL=0
 for ev in plugin-root verdicts run-record build-tdds-skill model-roles \
           parent-session-check model-dispatch escalation \
           format-and-lint-hook build-hardening ux-record ux-render \
           ux-author-skill ux-pipeline prd-lint requirements-review \
-          tdd-grounding; do
+          tdd-grounding reviewer-contracts; do
   f="$(dirname "$0")/${ev}.test.sh"
   echo
   if [ -f "$f" ]; then