feat: the findings loop — review → fix → re-review in the engine, roles untouched

CHANGELOG.md

@@ -46,6 +46,33 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   ignore the plugin dirs, as they already ignore pack subagents.
   (docs/pack-loading.md §3a)
 
+- **The findings loop: review → fix → re-review, in the engine, with the roles
+  left exactly as they are** (docs/findings-loop.md). A read-only review phase
+  had no way to get its findings acted on: the pipeline had no backward edge,
+  so the only exits were "approve anyway" or "re-run the reviewer, who still
+  cannot edit" — and the model would ask to be re-run under a write-capable
+  role, the one thing the role model exists to prevent. A phase now declares
+  `findings: { fixWith: <write-capable role>, blocking?, maxRounds?, gate? }`
+  and the engine does the rest: the reviewer's report must end with a fenced
+  `findings` block (JSON; a missing block is a format failure with one bounded
+  retry); any blocking finding runs a fix leg on the same session under
+  `fixWith`'s role, whose report must end with a `dispositions` block — every
+  blocking finding answered as fixed / not_a_finding / declined / deferred, a
+  reason required unless fixed, validated by the engine, never trusted from
+  prose; an optional fix gate (`tests_pass`) runs on the fix leg; then the
+  reviewer runs again with the ledger and names what remains open. Bounded by
+  `maxRounds`; blocking findings left open fail the boundary with the ledger.
+  `review`-kind gates finally have a real verdict (S4), the fix leg gets its
+  own model binding through the usual rungs, every leg is one persisted engine
+  step (restart-safe), and the pipeline wire / `codeoid pipeline status` show
+  the counts and the ledger. `maxRounds: 0` makes a pure audit phase. On a
+  findings phase `onFail: retry` means another fix loop (fresh fix budget),
+  a failing fix gate repairs the same fix leg once before the phase's onFail
+  applies, the loop's own legs skip the phase's entry gate, and the loader
+  refuses shapes that cannot mean what they say (a read-only fixer,
+  `skipWhenSatisfied`, a review-kind entry or fix gate). Findings loops
+  require a pack; an explicit-`phases` plan cannot declare one.
+
 ### Fixed
 
 - **Two installed packs declaring the same skill or gate id overwrote each

docs/findings-loop.md

@@ -0,0 +1,181 @@
+# The findings loop — review → fix → re-review, in the engine
+
+> Status: shipped (this document describes the implemented behaviour).
+> Code: `src/daemon/pipeline/findings.ts` (contracts, parsers, validators, ledger),
+> `engine.ts` (the loop), `pack.ts` (`findings:` schema + the `review` gate verdict),
+> `manager.ts` (fix-leg model binding), `builtin.ts` (the `findings-fix` skill).
+
+## 1. The problem
+
+A governed pipeline runs its review phases under read-only capability roles —
+`reviewer`, `adversary`, `verifier` — on purpose: findings get *reported*, not
+silently patched, which is what makes a second review round and a closeout
+phase mean anything.
+
+But the pipeline had no backward edge. `onFail` was halt, retry-the-same-phase,
+or abort; Revise re-ran the same phase. So when a reviewer produced findings the
+only exits were "the human approves anyway" or "the human re-runs the reviewer,
+who still cannot edit". In practice the model in the review phase ended up
+asking to be re-run under a write-capable role, which is the one thing the role
+model exists to prevent.
+
+The two obvious fixes both lose something real:
+
+- **Give reviewers write access.** A reviewer that can fix its own findings stops
+  writing them down; the adversary stops refuting and starts tidying; the
+  closeout phase has nothing to verify. Every strong harness converged on the
+  opposite (separate fix rounds, forced disposition of each finding).
+- **Hand-author `fix` phases in every pack.** Pushes engine work into pack prose,
+  with no validation that a finding was actually answered, and no way for the
+  reviewer to verify the fix.
+
+## 2. What the engine does
+
+A phase declares that it produces findings and who resolves them:
+
+```yaml
+gates:
+  - { id: bench_clear, kind: review }        # now a REAL verdict (§4)
+  - { id: tests_pass,  kind: command, run: "make test" }
+
+phases:
+  - id: review
+    skill: review                            # /review — runs under the read-only role
+    role: reviewer
+    gate: bench_clear
+    findings:
+      fixWith: implementer                   # a write-capable pack role (checked at load)
+      blocking: [critical, high]             # default
+      maxRounds: 2                           # fix legs before the human sees the ledger; default 2
+      gate: tests_pass                       # optional: evaluated after every fix leg
+```
+
+Each leg is one engine step (one model turn on the run's bound session), so the
+loop persists between legs and resumes after a daemon restart exactly where it
+stopped.
+
+```
+review leg (role: reviewer)  ──findings block──►  any blocking?  ──no──► exit boundary
+        ▲                                              │ yes, budget left
+        │                                              ▼
+        └── ledger ◄── dispositions block ◄── fix leg (role: implementer) ◄── findings
+                                                  │
+                                                  └── fixWith.gate (e.g. tests_pass)
+```
+
+1. **Review leg.** The phase's own skill runs under its own role, with the
+   *findings contract* appended: end the report with a fenced ```findings block —
+   a JSON array of `{ id, severity, title, location?, detail?, confidence? }`,
+   empty when clean. A missing or malformed block is a *format* failure, not a
+   verdict: one bounded re-run with the exact gap fed back (the engine's own
+   channel, never the human's revise notes), then the phase's `onFail` policy.
+2. **Fix leg.** If any finding is blocking and fix legs remain, the engine runs
+   the built-in `findings-fix` skill on the same session under `fixWith.role`
+   (the runner swaps the role per leg exactly as it swaps it per phase), with a
+   fresh prompt: the findings and the *disposition contract*. The fix leg gets
+   its own model binding through the same six rungs as a phase (§5).
+3. **Dispositions are validated by the engine.** Every blocking finding needs
+   exactly one disposition — `fixed`, `not_a_finding`, `declined`, `deferred` —
+   and anything but `fixed` needs a reason. Unknown ids, duplicates, and bare
+   deferrals are refused. A gap is fed back for one bounded repair of the same
+   leg; a gap after that goes to the phase's `onFail` policy with the ledger.
+4. **Fix gate.** If `fixWith.gate` is set, it is evaluated on the fix leg
+   *before the leg counts*: a fixer that broke the build repairs its own leg
+   once (the gate's reason fed back), and only a repair that also fails goes to
+   `onFail`. The reviewer is never handed a red tree, and on the last budgeted
+   leg the human is never left with only "approve a red tree or reject".
+   The fix gate must be deterministic (`command` or `probe`); the loader
+   refuses a `review`/`skill`/`self` kind here.
+5. **Re-review.** The reviewer runs again with the ledger: verify each `fixed`
+   finding is real and did not regress anything, accept or re-raise the rejected
+   ones (same id), keep `deferred` ones open, report anything new, and end with
+   the block listing **only what remains open**. The latest round's block *is*
+   the open set — the reviewer decides convergence, the engine enforces it.
+6. **Exit.** The loop ends when no blocking finding is open or `maxRounds` fix
+   legs have run. Then the normal exit boundary: a `review`-kind gate has a real
+   verdict, and the universal human halt carries a one-line summary plus the
+   ledger either way. `maxRounds: 0` is an audit-only phase: findings are
+   reported, never fixed.
+
+## 3. What is enforced, and where
+
+| Rule | Enforced by |
+| --- | --- |
+| The reviewer cannot write | the capability role, unchanged (`write: false` → tool deny on claude; advisory elsewhere) |
+| The fixer is a *different, write-capable* role | `loadPack` refuses a read-only or unknown `fixWith` role |
+| Findings are structured | `parseFindings` (zod) — format retry, then `onFail` |
+| Every blocking finding is answered, with a reason unless fixed | `validateDispositions` — format retry, then halt with ledger |
+| The fix did not break the build | `fixWith.gate` on the fix leg |
+| The fix is real | the re-review leg, under the reviewer's role |
+| The loop terminates | `maxRounds`; blocking findings left open fail the boundary |
+| The human sees what happened | halt reason + `PipelinePhaseWire.findings` (counts + markdown ledger); `codeoid pipeline status` prints the counts |
+
+## 4. The `review` gate kind, finally
+
+`kind: review` gates used to pass unconditionally and defer to the human halt.
+On a phase with `findings:` they now return the loop's verdict: fail while a
+blocking finding is open in the latest round (reason = the ledger), pass when
+clean. On a phase without `findings:` they behave as before. A findings phase
+without any declared gate gets the same verdict from the engine directly, so the
+boundary never reads "complete — review and approve" over open blockers.
+
+## 5. Model binding for fix legs
+
+`fixWith` accepts a role name or `{ role, provider?, model? }`. At create the
+fix leg's binding is resolved through the same rungs as a phase — CLI `--role`,
+`modelRoles`, the pin, the role's `model`, `modelTiers` via the role's `tier`,
+provider default — with the same skip rules (cross-provider bindings and models
+the session's backend cannot run are skipped with a warning naming the rung).
+The result is persisted on `def.findings.fixWith`, so resume and retry keep the
+same binding. A pack can therefore review on one tier and fix on another, e.g.
+review under a `reasoning-max` role and fix under a `mechanical` one.
+
+## 6. Human and policy semantics
+
+- **Approve** at the boundary accepts the phase as-is (open non-blocking
+  findings are recorded, not lost). Approving over open *blocking* findings is
+  the same deliberate override it always was for a failing gate.
+- **Revise** re-runs the phase as a review leg with the human's notes *and* the
+  ledger; if the reviewer reports blocking findings and fix legs remain, the
+  loop continues. The human's notes also reach every fix leg ("fix F3 with a
+  guard clause" is for the writer). The fix budget is per phase, not per
+  revise.
+- **Reject** fails the run.
+- **`onFail: retry`** on a findings phase means *another fix loop*: a fresh fix
+  budget and, when blockers are open, straight to a fix leg — the reviewer
+  already spoke. The reason travels as the engine's note to that leg, never
+  into the human's revise notes (which are rendered as revision history and
+  re-pasted into every later prompt). **`onFail: abort`** fails the run.
+- **Entry gates** ground a phase *run*; the loop's own legs (fix legs, the
+  re-review right after one, a format repair) do not re-evaluate them.
+- The loader refuses shapes that cannot mean what they say on a findings
+  phase: a read-only or unknown `fixWith`, `skipWhenSatisfied` (the review
+  would be skipped), a `review`-kind entry gate (it would block every revise),
+  and a non-deterministic fix gate. Findings loops require a pack; an
+  explicit-`phases` plan has no pack role to run the fix leg under and is
+  rejected at create.
+- **Phase summaries are the whole of what the model said** across the turns of
+  a leg, not only its last message — so a reviewer that writes its report,
+  rests without the completion marker, is nudged, and answers with the bare
+  marker still hands the engine its findings block.
+
+## 7. Why this is different
+
+| | codeoid findings loop | typical harnesses |
+| --- | --- | --- |
+| Who fixes | a different role, write-capable, on the same session | the reviewer, or a hand-written "fix" node |
+| Disposition of each finding | required and *validated by the engine* | prose convention, if at all |
+| Re-verification | the same reviewer role re-runs with the ledger and names the open set | none, or a fresh review with no memory of round 1 |
+| Termination | bounded fix legs; open blockers fail the boundary | prose sentinels / loop counters |
+| Governance | roles untouched; fix legs get their own model binding | roles loosened to make the loop work |
+| Restart safety | every leg is one persisted step | run-local |
+
+## 8. Authoring notes
+
+- Keep `blocking` honest: a `medium` finding that should block the phase is a
+  `high`. Non-blocking findings ride along in the ledger for the human.
+- The fix leg is told to run the project's tests; set `fixWith.gate` when the
+  pack has a deterministic one, so a green claim is checked, not trusted.
+- A reviewer that keeps re-raising a `declined` finding is doing its job; the
+  human decides at the boundary with the whole ledger in front of them.
+- `maxRounds: 0` turns any findings phase into a pure audit.

docs/pipeline-run.md

@@ -70,9 +70,10 @@ The only thing it saves is one auto-attach, which the client does for free.
 - **Phase boundaries are the human checkpoint.**
   With you present and Approving every boundary, the honest model is: *you* are the reviewer.
   The misleading "gate … is not yet enforced" halt goes away — a boundary reads "phase *N* ready — review the chat above."
-- **`skill` / `review` gates become optional automated verdicts (later slice).**
-  When we do implement them, a gate runs a validation subagent that returns pass/fail (e.g. a spec-completeness check, a reviewer pass), shown alongside the human decision — an *assist*, never a silent pass.
-  Until then they are simply absent, not fake.
+- **`review` gates have a real verdict on a phase that declares `findings:`** — the findings loop (docs/findings-loop.md): the reviewer's structured findings drive a fix leg under a write-capable role and a re-review, and the gate fails while a blocking finding is still open, with the ledger as its reason.
+  On a phase without `findings:` a `review` gate behaves as before (passes; the human is the reviewer).
+- **`skill` gates remain optional automated verdicts (later slice).**
+  Until implemented they are simply absent, not fake.
 
 ---
 
@@ -137,7 +138,7 @@ Full hard enforcement on the other backends (mapping their native tool names, or
    `/pipeline` opens the extended create-session dialog (name · workdir · provider · **goal** · **installed pack**); on submit, focus the run-session.
    Chat-primary layout, a non-modal collapsible cockpit dock over the run's chat.
    (Retires the `#217` bespoke "Start panel".)
-4. **S4 (optional, later) — automated skill/review gate verdicts** via validation subagents, shown as an assist.
+4. **S4 — automated review gate verdicts.** ✅ for `review` gates, via the findings loop (docs/findings-loop.md): structured findings → validated dispositions from a write-capable fix leg → re-review; the gate's verdict is "no blocking finding open". `skill` gates remain future work.
 
 ## Open questions
 

packages/protocol/src/types.ts

@@ -2215,6 +2215,24 @@ export interface PipelinePhaseWire {
   /** Human revise notes accumulated on this phase (newest last) — the client
    *  renders the revision history. */
   feedback?: string[];
+  /** The findings loop's state for a phase that declares `findings:` (the
+   *  engine's review → fix → re-review loop): counts plus a markdown ledger of
+   *  every round. Optional (additive) — absent on phases without the loop and
+   *  from older daemons. */
+  findings?: {
+    /** Review legs completed so far. */
+    rounds: number;
+    /** Fix legs completed so far. */
+    fixLegs: number;
+    /** Findings open after the latest review leg. */
+    open: number;
+    /** Of those, how many block the phase. */
+    blocking: number;
+    /** What the engine runs next for this phase while it is still running. */
+    next: "review" | "fix";
+    /** Markdown ledger of every round (findings + dispositions). */
+    ledger: string;
+  };
 }
 
 /** A pipeline projected for the wire (serializable subset of PipelineState). */

src/daemon/pipeline/builtin.ts

@@ -5,7 +5,8 @@
  * ships as separate packs in later slices; nothing here encodes an SDLC.
  */
 
-import type { GatePlugin, PhaseKind, PipelineRegistries } from "./interface";
+import { FINDINGS_FIX_SKILL_ID } from "./findings";
+import type { GatePlugin, PhaseKind, PipelineRegistries, SkillPlugin } from "./interface";
 
 /** A phase kind that does nothing and immediately passes — the minimal runnable
  *  phase. Lets a pipeline advance end to end so the engine, store, and restart
@@ -37,9 +38,23 @@ export const manualGate: GatePlugin = {
   },
 };
 
+/** The fix leg of the findings loop (findings.ts). Content-free on purpose: the
+ *  findings, the disposition contract, and any format feedback are appended by
+ *  the engine per leg (PhaseCtx.promptAppend); this template only names the
+ *  actor. Runs under the phase's `findings.fixWith.role` — a write-capable pack
+ *  role — never under the reviewer's. */
+export const findingsFixSkill: SkillPlugin = {
+  id: FINDINGS_FIX_SKILL_ID,
+  kind: "prompt",
+  template:
+    "You are the implementer for this pipeline run. A review phase has reported findings " +
+    "against the current work; resolve them as instructed below, in the working tree of this run.",
+};
+
 /** Register the built-in plugins into a set of registries. */
 export function registerBuiltins(r: PipelineRegistries): void {
   r.phases.register(noopPhaseKind);
   r.gates.register(alwaysGate);
   r.gates.register(manualGate);
+  r.skills.register(findingsFixSkill);
 }