From 32c040937a1c16ffcf89f73c8f45a28b4c6e6ea1 Mon Sep 17 00:00:00 2001 From: Yash Datta Date: Tue, 15 Sep 2026 23:18:24 +0800 Subject: [PATCH 1/2] =?UTF-8?q?feat:=20the=20findings=20loop=20=E2=80=94?= =?UTF-8?q?=20review=20=E2=86=92=20fix=20=E2=86=92=20re-review=20in=20the?= =?UTF-8?q?=20engine,=20roles=20untouched?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A read-only review phase (reviewer / adversary / verifier) had no way to get its findings acted on. The pipeline had no backward edge: onFail was halt, retry-the-same-phase, or abort, and Revise re-ran the same phase. So the only exits from a review with findings were "approve anyway" or "re-run the reviewer, who still cannot edit" — and in practice the model in the review phase asked to be re-run under a write-capable role, which is the one thing the role model exists to prevent. The obvious fixes (give reviewers write access; hand-author fix phases in every pack) each lose what the review rounds are for: findings get reported, not silently patched, and someone verifies the fix. A phase now declares that it produces findings and who resolves them: findings: { fixWith: implementer, blocking: [critical, high], maxRounds: 2, gate: tests_pass } and the engine runs the loop, one persisted step per leg: - Review leg, under the phase's own read-only role, with a contract appended: end the report with a fenced ```findings block (JSON: id, severity, title, location, detail, confidence; empty when clean). A missing or malformed block is a FORMAT failure — one bounded re-run with the exact gap, on the engine's own channel, then onFail. - Fix leg, on the same bound session under `fixWith`'s role (the runner swaps the role per leg exactly as per phase), driving the built-in `findings-fix` skill with the findings and a disposition contract. The engine validates the answer — every blocking finding has exactly one disposition (fixed / not_a_finding / declined / deferred), a reason unless fixed, no unknown ids, no duplicates, no bare deferrals — one bounded retry, then the phase halts with the ledger. - Optional fix gate (e.g. tests_pass) on the fix leg: a fixer that broke the build halts the phase with that reason. - Re-review under the reviewer's role with the ledger: verify each fix, accept or re-raise the rejected ones, report new findings, and list ONLY what remains open — the latest round IS the open set, so the reviewer decides convergence and the engine enforces it. - Bounded by maxRounds (0 = audit only). Blocking findings left open fail the exit boundary with the ledger; `review`-kind gates therefore have a real verdict at last (S4), and a findings phase with no gate gets the same verdict from the engine directly. The fix leg gets its own model binding through the six usual rungs with the same skip rules, persisted on the def (review on a reasoning tier, fix on a mechanical one). Load refuses a read-only or unknown fix role. The wire carries counts + a markdown ledger per phase (additive), and `codeoid pipeline status` prints them. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Yash Datta --- CHANGELOG.md | 21 ++ docs/findings-loop.md | 158 ++++++++++ docs/pipeline-run.md | 9 +- packages/protocol/src/types.ts | 18 ++ src/daemon/pipeline/builtin.ts | 17 +- src/daemon/pipeline/engine.ts | 241 +++++++++++++-- src/daemon/pipeline/findings-loop.test.ts | 237 ++++++++++++++ src/daemon/pipeline/findings-pack.test.ts | 174 +++++++++++ src/daemon/pipeline/findings.test.ts | 153 +++++++++ src/daemon/pipeline/findings.ts | 360 ++++++++++++++++++++++ src/daemon/pipeline/interface.ts | 15 + src/daemon/pipeline/manager.ts | 75 ++++- src/daemon/pipeline/pack.ts | 98 +++++- src/daemon/pipeline/skill-kind.ts | 16 +- src/daemon/session-manager.ts | 14 + src/terminal/pipeline-format.ts | 9 + 16 files changed, 1573 insertions(+), 42 deletions(-) create mode 100644 docs/findings-loop.md create mode 100644 src/daemon/pipeline/findings-loop.test.ts create mode 100644 src/daemon/pipeline/findings-pack.test.ts create mode 100644 src/daemon/pipeline/findings.test.ts create mode 100644 src/daemon/pipeline/findings.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index f3cd62d8..db11837f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,27 @@ 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: , 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 `pipeline.pack.list` / `codeoid pipeline status` + show the counts. `maxRounds: 0` makes a pure audit phase. + ### Fixed - **Two installed packs declaring the same skill or gate id overwrote each diff --git a/docs/findings-loop.md b/docs/findings-loop.md new file mode 100644 index 00000000..75474aaf --- /dev/null +++ b/docs/findings-loop.md @@ -0,0 +1,158 @@ +# 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 retry; a gap after + that halts the phase with the ledger. +4. **Fix gate.** If `fixWith.gate` is set, it is evaluated on the fix leg; a + fixer that broke the build halts the phase with that reason rather than + handing the reviewer a red tree. +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 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 fix budget is per phase, not per revise. +- **Reject** fails the run. + +## 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. diff --git a/docs/pipeline-run.md b/docs/pipeline-run.md index 11f08b85..c770a9e3 100644 --- a/docs/pipeline-run.md +++ b/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 diff --git a/packages/protocol/src/types.ts b/packages/protocol/src/types.ts index 1f5f0382..4edfc756 100644 --- a/packages/protocol/src/types.ts +++ b/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). */ diff --git a/src/daemon/pipeline/builtin.ts b/src/daemon/pipeline/builtin.ts index ec75b4f4..fd98e416 100644 --- a/src/daemon/pipeline/builtin.ts +++ b/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); } diff --git a/src/daemon/pipeline/engine.ts b/src/daemon/pipeline/engine.ts index 1b1c7179..7e318955 100644 --- a/src/daemon/pipeline/engine.ts +++ b/src/daemon/pipeline/engine.ts @@ -9,10 +9,15 @@ * Persistence, identity, worker sessions, and frontend surfacing are NOT here — * they compose around this in PipelineManager (and later slices). Keeping the * transition rules side-effect-free is exactly what makes them unit-testable. + * + * A phase that declares `def.findings` runs the FINDINGS LOOP (findings.ts): + * its review legs and fix legs are each one `step()` — one model turn — so the + * loop persists between legs and resumes after a restart exactly where it was. */ import type { GateVerdict, + PhaseCtx, PhaseDef, PhaseFailAction, PhaseRunResult, @@ -22,6 +27,23 @@ import type { } from "./interface"; import { isTerminal } from "./interface"; import { errMessage } from "./errors"; +import { + FINDINGS_FIX_SKILL_ID, + FORMAT_RETRIES, + type FindingsLoopState, + type FindingsSpec, + blockingOf, + findingsContract, + fixContract, + newLoopState, + openBlocking, + parseDispositions, + parseFindings, + renderLedger, + rereviewContract, + summarizeLoop, + validateDispositions, +} from "./findings"; import { resolveScoped } from "./scoped"; /** Defensive cap against a mis-authored retry loop (each retry is one step). */ @@ -95,23 +117,30 @@ export class PipelineEngine { if (!v.pass) return applyFail(s, phase, v, attempts, "entry"); } + // ── Findings loop: a pending FIX LEG runs instead of the phase's own kind. + const spec = phase.def.findings; + if (spec && !phase.findings) phase.findings = newLoopState(); + const loop = spec ? phase.findings : undefined; + if (spec && loop && loop.next === "fix") { + return this.#fixLeg(s, phase, spec, loop, attempts); + } + // Run the phase kind. A throwing plugin must not crash the run and leave // the pipeline stuck "running" in the store (→ a restart crash-loop); a // throw is treated as a phase failure, then handled by the onFail policy. - const kind = this.#registries.phases.resolve(phase.def.kind); - let res: PhaseRunResult; - if (!kind) { - res = { outcome: "failed", reason: `unknown phase kind "${phase.def.kind}"` }; - } else { - try { - // Hand plugins a clone — a buggy/hostile kind mutating our working - // state must not corrupt the engine's transition (the "returns a NEW - // state" guarantee). - res = await kind.run({ pipeline: clone(s), phase: phase.def, registries: this.#registries }); - } catch (err) { - res = { outcome: "failed", reason: `phase kind "${phase.def.kind}" threw: ${errMessage(err)}` }; - } - } + // For a findings phase this is a REVIEW LEG: the first one carries the + // findings contract, every later one the ledger + re-review contract, and + // a format retry carries the exact gap on top. + const reviewAppend = + spec && loop + ? [ + loop.rounds.length === 0 ? findingsContract(spec) : rereviewContract(loop, spec), + ...(loop.formatFeedback + ? [`\nYour previous report did not satisfy the contract — ${loop.formatFeedback}. Report again.`] + : []), + ].join("\n") + : undefined; + const res = await this.#runKind(s, phase.def, reviewAppend ? { promptAppend: reviewAppend } : {}); if (res.outcome === "halted") { phase.state = { @@ -132,37 +161,84 @@ export class PipelineEngine { // halted state itself doesn't carry a summary). if (res.summary !== undefined) phase.lastSummary = res.summary; + // ── Findings loop: record the round; dispatch a fix leg if anything blocks. + if (spec && loop) { + const parsed = parseFindings(res.summary ?? ""); + if (!parsed.ok) { + // A report without a valid block is a FORMAT failure, not a verdict: + // one bounded re-run with the exact gap (the engine's own feedback + // channel, never the human's revise notes), then the onFail policy. + if (loop.formatRetries < FORMAT_RETRIES) { + loop.formatRetries += 1; + loop.formatFeedback = parsed.reason; + return touch(s); + } + loop.formatRetries = 0; + loop.formatFeedback = undefined; + return applyFail( + s, + phase, + { pass: false, reason: `phase "${phase.def.id}" produced no valid findings block: ${parsed.reason}` }, + attempts, + "exit", + ); + } + loop.formatRetries = 0; + loop.formatFeedback = undefined; + loop.rounds.push({ findings: parsed.value }); + if (blockingOf(parsed.value, spec.blocking).length > 0 && loop.fixLegs < spec.maxRounds) { + loop.next = "fix"; + return touch(s); + } + // Converged (clean) or the fix budget is spent: fall through to the exit + // boundary. A later human revise re-enters as a review leg with the ledger. + loop.next = "review"; + } + // Exit boundary — two DISTINCT things happen here: // // 1. An optional automated CHECK. A `command` gate produces a real pass/ - // fail verdict. A failing check still honors onFail:retry (machine loop - // within budget) or onFail:abort (hard fail); with the default halt it - // just carries its reason to the human. `skill`/`review`/`self` gates - // carry no automated verdict yet — they pass, and the human is the - // reviewer (S4 may add subagent verdicts). See pack.ts. + // fail verdict; a `review` gate's verdict is "no blocking finding is + // open" once the phase runs the findings loop (pack.ts). A failing + // check still honors onFail:retry (machine loop within budget) or + // onFail:abort (hard fail); with the default halt it just carries its + // reason to the human. `skill`/`self` gates carry no automated verdict + // yet — they pass, and the human is the reviewer. // 2. The phase then HALTS for a human decision (Approve / Revise / Reject). // This boundary halt is UNIVERSAL: a run never rolls into the next phase // on its own — the human always decides (docs/pipeline-run.md). let gateReason: string | undefined; - if (phase.def.gate) { - const v = await this.#gate(phase.def.gate, s, phase.def, "exit"); - if (!v.pass) { - const onFail = phase.def.onFail ?? { action: "halt" }; - // A machine retry/abort short-circuits the human boundary. - if (onFail.action === "retry" || onFail.action === "abort") { - return applyFail(s, phase, v, attempts, "exit"); - } - gateReason = v.reason ?? "gate check failed"; + let verdict: GateVerdict = { pass: true }; + if (phase.def.gate) verdict = await this.#gate(phase.def.gate, s, phase.def, "exit"); + if (verdict.pass && spec && loop) { + // No gate (or a passing one) but blocking findings are still open — the + // loop's own verdict fails the boundary, ledger attached, whether or not + // the pack declared a `review` gate. + const open = openBlocking(loop, spec); + if (open.length > 0) { + verdict = { + pass: false, + reason: `${open.length} blocking finding${open.length === 1 ? "" : "s"} still open after ${loop.fixLegs} fix leg${loop.fixLegs === 1 ? "" : "s"}:\n${renderLedger(loop, spec)}`, + }; + } + } + if (!verdict.pass) { + const onFail = phase.def.onFail ?? { action: "halt" }; + // A machine retry/abort short-circuits the human boundary. + if (onFail.action === "retry" || onFail.action === "abort") { + return applyFail(s, phase, verdict, attempts, "exit"); } + gateReason = verdict.reason ?? "gate check failed"; } // The phase's work is done (kept in lastSummary); halt for the human. + const ledger = spec && loop ? ` — ${summarizeLoop(loop, spec)}` : ""; phase.state = { status: "halted", requestId: `exit:${phase.def.id}`, reason: gateReason ? `phase "${phase.def.id}" complete — gate not satisfied: ${gateReason}` - : `phase "${phase.def.id}" complete — review and approve`, + : `phase "${phase.def.id}" complete${ledger} — review and approve`, }; s.status = "halted"; return touch(s); @@ -194,6 +270,113 @@ export class PipelineEngine { return s; } + /** + * One FIX LEG of the findings loop: run the built-in `findings-fix` skill on + * the same bound session under the phase's `fixWith` role (the runner swaps + * the role per leg exactly as it swaps it per phase), parse + validate the + * dispositions the engine demanded, run the optional fix gate, and hand the + * phase back to a review leg. Format gaps get one bounded retry with the + * exact gap; a gap after that halts the phase with the ledger. + */ + async #fixLeg( + s: PipelineState, + phase: PipelinePhase, + spec: FindingsSpec, + loop: FindingsLoopState, + attempts: number, + ): Promise { + const round = loop.rounds[loop.rounds.length - 1]; + if (!round) { + // Unreachable by construction (next="fix" is set right after a round is + // pushed); recover rather than wedge — treat as "review next". + loop.next = "review"; + return touch(s); + } + const legDef: PhaseDef = { + id: `${phase.def.id}#fix${loop.fixLegs + 1}`, + kind: "skill", + skill: FINDINGS_FIX_SKILL_ID, + role: spec.fixWith.role, + ...(spec.fixWith.provider !== undefined ? { provider: spec.fixWith.provider } : {}), + ...(spec.fixWith.model !== undefined ? { model: spec.fixWith.model } : {}), + ...(spec.fixWith.resolvedFrom !== undefined ? { resolvedFrom: spec.fixWith.resolvedFrom } : {}), + }; + const res = await this.#runKind(s, legDef, { + promptAppend: fixContract(round, spec, loop.formatFeedback), + freshPrompt: true, + }); + if (res.outcome === "halted") { + phase.state = { status: "halted", requestId: res.requestId, reason: res.reason, questions: res.questions }; + s.status = "halted"; + return touch(s); + } + if (res.outcome === "failed") { + return applyFail(s, phase, { pass: false, reason: `fix leg "${legDef.id}" failed: ${res.reason}` }, attempts, "kind"); + } + const parsed = parseDispositions(res.summary ?? ""); + const check = parsed.ok ? validateDispositions(round.findings, spec.blocking, parsed.value) : parsed; + if (!check.ok) { + if (loop.formatRetries < FORMAT_RETRIES) { + loop.formatRetries += 1; + loop.formatFeedback = check.reason; + return touch(s); // still running; next stays "fix" + } + loop.formatRetries = 0; + loop.formatFeedback = undefined; + loop.next = "review"; + return applyFail( + s, + phase, + { + pass: false, + reason: `fix leg "${legDef.id}" did not resolve the findings: ${check.reason}\n${renderLedger(loop, spec)}`, + }, + attempts, + "exit", + ); + } + round.dispositions = parsed.ok ? parsed.value : []; + round.fixSummary = res.summary; + loop.fixLegs += 1; + loop.formatRetries = 0; + loop.formatFeedback = undefined; + loop.next = "review"; + // The fix gate (e.g. tests_pass): a fixer that broke the build halts the + // phase with that reason rather than handing the reviewer a red tree. + if (spec.gate) { + const v = await this.#gate(spec.gate, s, legDef, "exit"); + if (!v.pass) { + return applyFail( + s, + phase, + { pass: false, reason: `fix leg "${legDef.id}" failed gate "${spec.gate}": ${v.reason ?? "check failed"}` }, + attempts, + "exit", + ); + } + } + return touch(s); + } + + /** Run a phase kind for `def` — the phase's own def or a synthetic fix-leg + * def — with the engine's prompt extras. A throw is a failed result. */ + async #runKind( + s: PipelineState, + def: PhaseDef, + extra: Pick, + ): Promise { + const kind = this.#registries.phases.resolve(def.kind); + if (!kind) return { outcome: "failed", reason: `unknown phase kind "${def.kind}"` }; + try { + // Hand plugins a clone — a buggy/hostile kind mutating our working + // state must not corrupt the engine's transition (the "returns a NEW + // state" guarantee). + return await kind.run({ pipeline: clone(s), phase: def, registries: this.#registries, ...extra }); + } catch (err) { + return { outcome: "failed", reason: `phase kind "${def.kind}" threw: ${errMessage(err)}` }; + } + } + async #gate( id: string, pipeline: PipelineState, diff --git a/src/daemon/pipeline/findings-loop.test.ts b/src/daemon/pipeline/findings-loop.test.ts new file mode 100644 index 00000000..726d41e1 --- /dev/null +++ b/src/daemon/pipeline/findings-loop.test.ts @@ -0,0 +1,237 @@ +/** + * The findings loop through the engine (engine.ts + findings.ts): review leg → + * fix leg under the writer role → re-review, with a scripted runner so every + * prompt, role swap, and state transition is observable. No backend. + */ + +import { describe, expect, test } from "bun:test"; +import { registerBuiltins } from "./builtin"; +import { PipelineEngine } from "./engine"; +import type { FindingsSpec } from "./findings"; +import type { PhaseDef, PipelineRegistries, PipelineState } from "./interface"; +import { createRegistries } from "./registry"; +import type { PhaseRunner, PhaseRunRequest } from "./runner"; +import { makeSkillPhaseKind } from "./skill-kind"; + +const fb = (v: unknown): string => `review report\n\n\`\`\`findings\n${JSON.stringify(v)}\n\`\`\``; +const db = (v: unknown): string => `fix report\n\n\`\`\`dispositions\n${JSON.stringify(v)}\n\`\`\``; + +const F1 = { id: "F1", severity: "high", title: "nil deref", location: "a.go:12" }; +const F2 = { id: "F2", severity: "low", title: "naming" }; + +function scripted(outputs: string[]): { runner: PhaseRunner; calls: PhaseRunRequest[] } { + const calls: PhaseRunRequest[] = []; + let i = 0; + return { + calls, + runner: { + async runPrompt(req) { + calls.push(req); + const out = outputs[i++]; + if (out === undefined) throw new Error(`scripted runner exhausted at call ${i}`); + return { summary: out }; + }, + }, + }; +} + +function regs(runner: PhaseRunner): PipelineRegistries { + const r = createRegistries(); + registerBuiltins(r); + r.phases.register(makeSkillPhaseKind(runner)); + r.skills.register({ id: "review", kind: "prompt", template: "Review the change." }); + return r; +} + +function pipeline(phases: PhaseDef[]): PipelineState { + return { + id: "p", + name: "p", + spec: "ship feature X", + phases: phases.map((def) => ({ def, state: { status: "pending" } })), + cursor: 0, + status: "draft", + accountId: "a", + projectId: "p", + createdBy: "u", + createdAt: 1, + updatedAt: 1, + }; +} + +const spec = (over: Partial = {}): FindingsSpec => ({ + fixWith: { role: "implementer", model: "claude-sonnet-5", resolvedFrom: "config-tier" }, + blocking: ["critical", "high"], + maxRounds: 2, + ...over, +}); + +const reviewPhase = (findings: FindingsSpec, extra: Partial = {}): PhaseDef => ({ + id: "review", + kind: "skill", + skill: "review", + role: "reviewer", + findings, + ...extra, +}); + +describe("findings loop — review → fix → re-review", () => { + test("a blocking finding runs a fix leg under the writer role, then the reviewer verifies; clean → boundary halt", async () => { + const { runner, calls } = scripted([ + fb([F1, F2]), // round 1: one blocking, one not + db([{ id: "F1", disposition: "fixed", evidence: "guard added; tests green" }]), // fix leg 1 + fb([F2]), // round 2: F1 verified fixed; F2 stays open but is non-blocking + ]); + const out = await new PipelineEngine(regs(runner)).run(pipeline([reviewPhase(spec())])); + + expect(calls).toHaveLength(3); + // Review leg 1: the reviewer's own role and the findings contract. + expect(calls[0]!.phase.role).toBe("reviewer"); + expect(calls[0]!.prompt).toContain("Review the change."); + expect(calls[0]!.prompt).toContain("```findings"); + // Fix leg: the WRITER role, its own persisted binding, a fresh prompt with + // the findings + disposition contract, and the run's goal as context. + expect(calls[1]!.phase.id).toBe("review#fix1"); + expect(calls[1]!.phase.role).toBe("implementer"); + expect(calls[1]!.model).toBe("claude-sonnet-5"); + expect(calls[1]!.phase.resolvedFrom).toBe("config-tier"); + expect(calls[1]!.prompt).toContain("implementer for this pipeline run"); + expect(calls[1]!.prompt).toContain("**F1** [high, blocking] nil deref — a.go:12"); + expect(calls[1]!.prompt).toContain("```dispositions"); + expect(calls[1]!.prompt).toContain("ship feature X"); + expect(calls[1]!.prompt).not.toContain("Your previous output for this phase"); + // Re-review: back under the reviewer, carrying the ledger. + expect(calls[2]!.phase.role).toBe("reviewer"); + expect(calls[2]!.prompt).toContain("Review round 2"); + expect(calls[2]!.prompt).toContain("FIXED (guard added; tests green)"); + + expect(out.status).toBe("halted"); + const ph = out.phases[0]!; + expect(ph.state.status).toBe("halted"); + if (ph.state.status === "halted") { + expect(ph.state.requestId).toBe("exit:review"); + expect(ph.state.reason).toContain("1 finding open (0 blocking), 1 fixed — 2 review rounds, 1 fix leg"); + expect(ph.state.reason).toContain("review and approve"); + } + expect(ph.findings?.rounds).toHaveLength(2); + expect(ph.findings?.fixLegs).toBe(1); + expect(ph.findings?.rounds[0]!.dispositions).toEqual([{ id: "F1", disposition: "fixed", evidence: "guard added; tests green" }]); + expect(ph.lastSummary).toContain("review report"); // the LAST review leg's output + }); + + test("a clean first report needs no fix leg", async () => { + const { runner, calls } = scripted([fb([])]); + const out = await new PipelineEngine(regs(runner)).run(pipeline([reviewPhase(spec())])); + expect(calls).toHaveLength(1); + expect(out.status).toBe("halted"); + const st = out.phases[0]!.state; + if (st.status === "halted") expect(st.reason).toContain("0 findings open (0 blocking), 0 fixed — 1 review round, 0 fix legs"); + }); + + test("blocking findings still open when the fix budget is spent fail the boundary with the ledger (halt by default, abort if asked)", async () => { + const script = [ + fb([F1]), + db([{ id: "F1", disposition: "declined", reason: "by design" }]), + fb([F1]), // reviewer does not accept the reason: still open + ]; + const halted = await new PipelineEngine(regs(scripted(script).runner)).run( + pipeline([reviewPhase(spec({ maxRounds: 1 }))]), + ); + expect(halted.status).toBe("halted"); + const st = halted.phases[0]!.state; + if (st.status === "halted") { + expect(st.reason).toContain("gate not satisfied"); + expect(st.reason).toContain("1 blocking finding still open after 1 fix leg"); + expect(st.reason).toContain("DECLINED — by design"); + } + const aborted = await new PipelineEngine(regs(scripted(script).runner)).run( + pipeline([reviewPhase(spec({ maxRounds: 1 }), { onFail: { action: "abort" } })]), + ); + expect(aborted.status).toBe("failed"); + }); + + test("maxRounds: 0 is an audit-only phase — findings are reported, never fixed", async () => { + const { runner, calls } = scripted([fb([F1])]); + const out = await new PipelineEngine(regs(runner)).run(pipeline([reviewPhase(spec({ maxRounds: 0 }))])); + expect(calls).toHaveLength(1); + const st = out.phases[0]!.state; + if (st.status === "halted") expect(st.reason).toContain("still open after 0 fix legs"); + }); + + test("a fix leg that omits or botches its dispositions gets ONE retry with the exact gap, then the phase halts with the ledger", async () => { + // Retry succeeds. + const ok = scripted([fb([F1]), "I fixed it, trust me", db([{ id: "F1", disposition: "fixed" }]), fb([])]); + const out = await new PipelineEngine(regs(ok.runner)).run(pipeline([reviewPhase(spec())])); + expect(ok.calls).toHaveLength(4); + expect(ok.calls[2]!.phase.id).toBe("review#fix1"); // same leg, retried + expect(ok.calls[2]!.prompt).toContain("did not satisfy the contract — no ```dispositions block"); + expect(out.status).toBe("halted"); + expect(out.phases[0]!.findings?.fixLegs).toBe(1); + + // Retry also fails → halt (default onFail) carrying the gap + ledger. + const bad = scripted([fb([F1]), db([{ id: "F1", disposition: "deferred" }]), db([{ id: "F1", disposition: "deferred" }])]); + const halted = await new PipelineEngine(regs(bad.runner)).run(pipeline([reviewPhase(spec())])); + expect(bad.calls).toHaveLength(3); + expect(bad.calls[2]!.prompt).toContain('"F1" is deferred without a reason'); + expect(halted.status).toBe("halted"); + const st = halted.phases[0]!.state; + if (st.status === "halted") { + expect(st.requestId).toBe("exit:review"); + expect(st.reason).toContain("did not resolve the findings"); + expect(st.reason).toContain("| F1 | high |"); + } + }); + + test("a review leg without a valid findings block gets ONE retry with the gap, then onFail", async () => { + const ok = scripted(["looks fine to me", fb([])]); + const out = await new PipelineEngine(regs(ok.runner)).run(pipeline([reviewPhase(spec())])); + expect(ok.calls).toHaveLength(2); + expect(ok.calls[1]!.prompt).toContain("did not satisfy the contract — no ```findings block"); + expect(ok.calls[1]!.phase.role).toBe("reviewer"); + expect(out.status).toBe("halted"); + expect(out.phases[0]!.findings?.rounds).toHaveLength(1); + expect(out.phases[0]!.feedback).toBeUndefined(); // the human's revise channel is untouched + + const bad = scripted(["looks fine", "still no block"]); + const halted = await new PipelineEngine(regs(bad.runner)).run(pipeline([reviewPhase(spec())])); + expect(halted.status).toBe("halted"); + const st = halted.phases[0]!.state; + if (st.status === "halted") expect(st.reason).toContain("produced no valid findings block"); + }); + + test("a fix gate (e.g. tests) that fails after the fix leg halts the phase with that reason", async () => { + const { runner, calls } = scripted([fb([F1]), db([{ id: "F1", disposition: "fixed" }])]); + const out = await new PipelineEngine(regs(runner)).run(pipeline([reviewPhase(spec({ gate: "manual" }))])); + expect(calls).toHaveLength(2); // no re-review: the fixer broke the gate + expect(out.status).toBe("halted"); + const st = out.phases[0]!.state; + if (st.status === "halted") expect(st.reason).toContain('fix leg "review#fix1" failed gate "manual"'); + }); + + test("each leg is one step, and the loop survives a serialize/parse round-trip between legs (restart-safe)", async () => { + const { runner, calls } = scripted([fb([F1]), db([{ id: "F1", disposition: "fixed" }]), fb([])]); + const engine = new PipelineEngine(regs(runner)); + let s = await engine.step(pipeline([reviewPhase(spec())])); + expect(s.status).toBe("running"); + expect(s.phases[0]!.findings?.next).toBe("fix"); + s = JSON.parse(JSON.stringify(s)); // what the store does between steps + s = await engine.step(s); + expect(s.phases[0]!.findings?.next).toBe("review"); + expect(s.phases[0]!.findings?.fixLegs).toBe(1); + s = JSON.parse(JSON.stringify(s)); + s = await engine.step(s); + expect(s.status).toBe("halted"); + expect(calls.map((c) => c.phase.id)).toEqual(["review", "review#fix1", "review"]); + }); + + test("a phase without `findings` is untouched by the loop", async () => { + const { runner, calls } = scripted(["done"]); + const out = await new PipelineEngine(regs(runner)).run( + pipeline([{ id: "impl", kind: "skill", skill: "review", role: "implementer" }]), + ); + expect(calls).toHaveLength(1); + expect(calls[0]!.prompt).not.toContain("```findings"); + expect(out.phases[0]!.findings).toBeUndefined(); + expect(out.status).toBe("halted"); + }); +}); diff --git a/src/daemon/pipeline/findings-pack.test.ts b/src/daemon/pipeline/findings-pack.test.ts new file mode 100644 index 00000000..2ac72293 --- /dev/null +++ b/src/daemon/pipeline/findings-pack.test.ts @@ -0,0 +1,174 @@ +/** + * The findings loop as pack DATA: `findings:` on a phase compiles to a + * FindingsSpec with defaults, refuses a read-only or unknown fix role, the + * `review` gate kind produces a real verdict from the loop state, and create + * binds the fix leg's model through the same rungs as a phase. + */ + +import { Database } from "bun:sqlite"; +import { afterEach, describe, expect, test } from "bun:test"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { newLoopState } from "./findings"; +import type { PipelineState } from "./interface"; +import { PipelineManager } from "./manager"; +import { loadPack } from "./pack"; +import { createRegistries } from "./registry"; +import { PipelineStore } from "./store"; + +const dirs: string[] = []; +afterEach(() => { + for (const d of dirs.splice(0)) rmSync(d, { recursive: true, force: true }); +}); + +function writePack(manifest: string): string { + const dir = mkdtempSync(join(tmpdir(), "findings-pack-")); + dirs.push(dir); + mkdirSync(join(dir, "roles")); + writeFileSync(join(dir, "roles", "reviewer.yaml"), "name: reviewer\nwrite: false\nnetwork: read-only\nenvelope: [read, grep, glob, bash]\n"); + writeFileSync( + join(dir, "roles", "implementer.yaml"), + "name: implementer\ntier: mechanical\nwrite: true\nnetwork: read-only\nenvelope: all\n", + ); + writeFileSync(join(dir, "pack.yaml"), manifest); + return dir; +} + +const MANIFEST = (phase: string) => `schema: codeoid/pack@v1 +id: fp +name: Findings Pack +version: 0.1.0 +roles: [./roles/reviewer.yaml, ./roles/implementer.yaml] +skills: + - { id: review, kind: prompt, template: "Review it." } +gates: + - { id: bench_clear, kind: review } + - { id: tests_pass, kind: command, run: "true" } +phases: +${phase} +`; + +const tenant = { accountId: "a", projectId: "p", createdBy: "u" }; + +function stateFor(pack: ReturnType): PipelineState { + return { + id: "run", + name: "run", + packId: pack.id, + phases: pack.pipeline.map((def) => ({ def, state: { status: "running", startedAt: 1, attempts: 0 } })), + cursor: 0, + status: "running", + accountId: "a", + projectId: "p", + createdBy: "u", + createdAt: 1, + updatedAt: 1, + }; +} + +describe("findings: on a pack phase", () => { + test("compiles with defaults (blocking critical+high, 2 rounds) and accepts the object form with pins", () => { + const pack = loadPack( + writePack( + MANIFEST(` - { id: review, skill: review, role: reviewer, gate: bench_clear, findings: { fixWith: implementer } } + - id: adversary + skill: review + role: reviewer + findings: + fixWith: { role: implementer, model: claude-fable-5 } + blocking: [critical] + maxRounds: 3 + gate: tests_pass`), + ), + ); + expect(pack.pipeline[0]!.findings).toEqual({ + fixWith: { role: "implementer" }, + blocking: ["critical", "high"], + maxRounds: 2, + }); + expect(pack.pipeline[1]!.findings).toEqual({ + fixWith: { role: "implementer", model: "claude-fable-5" }, + blocking: ["critical"], + maxRounds: 3, + gate: "tests_pass", + }); + }); + + test("refuses a read-only fix role and an unknown one at load", () => { + expect(() => + loadPack(writePack(MANIFEST(" - { id: review, skill: review, role: reviewer, findings: { fixWith: reviewer } }"))), + ).toThrow(/fixWith role "reviewer" is read-only/); + expect(() => + loadPack(writePack(MANIFEST(" - { id: review, skill: review, role: reviewer, findings: { fixWith: ghost } }"))), + ).toThrow(/unknown role "ghost"/); + }); + + test("the `review` gate kind is the loop's verdict: fails while a blocking finding is open, passes when clean or absent", async () => { + const pack = loadPack( + writePack(MANIFEST(" - { id: review, skill: review, role: reviewer, gate: bench_clear, findings: { fixWith: implementer } }")), + ); + const r = createRegistries(); + pack.register(r); + const gate = r.gates.resolve("fp/bench_clear")!; + const s = stateFor(pack); + const phase = s.phases[0]!; + // No loop state yet (never ran) → pass, the human is the reviewer. + expect((await gate.evaluate({ pipeline: s, phase: phase.def })).pass).toBe(true); + // Open blocking finding → fail, with the ledger in the reason. + phase.findings = newLoopState(); + phase.findings.rounds.push({ findings: [{ id: "F1", severity: "critical", title: "boom" }] }); + const v = await gate.evaluate({ pipeline: s, phase: phase.def }); + expect(v.pass).toBe(false); + expect(v.reason).toContain("1 blocking finding still open after 0 fix legs"); + expect(v.reason).toContain("| F1 | critical | boom |"); + // Only non-blocking left → pass. + phase.findings.rounds.push({ findings: [{ id: "F2", severity: "low", title: "nit" }] }); + expect((await gate.evaluate({ pipeline: s, phase: phase.def })).pass).toBe(true); + }); + + test("create validates the fix gate and binds the fix leg's model through the role's tier", () => { + const pack = loadPack( + writePack( + MANIFEST(" - { id: review, skill: review, role: reviewer, findings: { fixWith: implementer, gate: tests_pass } }"), + ), + ); + const mgr = new PipelineManager(new PipelineStore(new Database(":memory:"))); + mgr.installPack(pack); + const warnings: string[] = []; + const run = mgr.create({ + ...tenant, + name: "r", + pack: "fp", + sessionProvider: "claude", + modelConfig: { modelTiers: { mechanical: { provider: "claude", model: "claude-fable-5" } }, modelRoles: {} }, + warn: (m) => warnings.push(m), + }); + const fw = run.phases[0]!.def.findings?.fixWith; + expect(fw).toEqual({ role: "implementer", resolvedFrom: "config-tier", provider: "claude", model: "claude-fable-5" }); + // The reviewer phase itself has no tier → stays unbound; no warnings. + expect(run.phases[0]!.def.model).toBeUndefined(); + expect(warnings).toEqual([]); + + // An unknown fix gate is a create-time error, like any other gate. + const broken = loadPack( + writePack(MANIFEST(" - { id: review, skill: review, role: reviewer, findings: { fixWith: implementer, gate: nope } }")), + ); + const mgr2 = new PipelineManager(new PipelineStore(new Database(":memory:"))); + mgr2.installPack(broken); + expect(() => mgr2.create({ ...tenant, name: "r", pack: "fp" })).toThrow(/unknown findings fix gate "nope"/); + }); + + test("a cross-provider fix binding is skipped with a warning, never persisted", () => { + const pack = loadPack( + writePack(MANIFEST(" - { id: review, skill: review, role: reviewer, findings: { fixWith: { role: implementer, provider: codex } } }")), + ); + const mgr = new PipelineManager(new PipelineStore(new Database(":memory:"))); + mgr.installPack(pack); + const warnings: string[] = []; + const run = mgr.create({ ...tenant, name: "r", pack: "fp", sessionProvider: "claude", warn: (m) => warnings.push(m) }); + expect(run.phases[0]!.def.findings?.fixWith).toEqual({ role: "implementer" }); + expect(warnings.join("\n")).toContain('fix leg (role "implementer")'); + expect(warnings.join("\n")).toContain('targets provider "codex"'); + }); +}); diff --git a/src/daemon/pipeline/findings.test.ts b/src/daemon/pipeline/findings.test.ts new file mode 100644 index 00000000..be5f0c62 --- /dev/null +++ b/src/daemon/pipeline/findings.test.ts @@ -0,0 +1,153 @@ +/** + * The findings loop's pure parts (findings.ts): block extraction, schema + * parsing, the disposition contract the engine enforces, and the ledger. + */ + +import { describe, expect, test } from "bun:test"; +import { + type Finding, + type FindingsSpec, + extractLastBlock, + findingsContract, + fixContract, + newLoopState, + openBlocking, + parseDispositions, + parseFindings, + renderLedger, + rereviewContract, + summarizeLoop, + validateDispositions, +} from "./findings"; + +const SPEC: FindingsSpec = { fixWith: { role: "implementer" }, blocking: ["critical", "high"], maxRounds: 2 }; + +const block = (tag: string, v: unknown): string => `some prose\n\n\`\`\`${tag}\n${JSON.stringify(v, null, 2)}\n\`\`\``; + +const F1: Finding = { id: "F1", severity: "high", title: "nil deref on empty input", location: "a.go:12" }; +const F2: Finding = { id: "F2", severity: "low", title: "naming" }; + +describe("extractLastBlock", () => { + test("returns the LAST block with the tag, tolerating CRLF and trailing spaces", () => { + const text = "```findings\n[1]\n```\nlater\n```findings \r\n[2]\r\n```\n```other\n[3]\n```"; + expect(extractLastBlock(text, "findings")?.trim()).toBe("[2]"); + expect(extractLastBlock(text, "other")?.trim()).toBe("[3]"); + expect(extractLastBlock(text, "nope")).toBeUndefined(); + }); +}); + +describe("parseFindings", () => { + test("parses a valid block", () => { + const p = parseFindings(block("findings", [F1, F2])); + expect(p.ok).toBe(true); + if (p.ok) expect(p.value.map((f) => f.id)).toEqual(["F1", "F2"]); + }); + + test("an empty array is a clean report", () => { + const p = parseFindings(block("findings", [])); + expect(p).toEqual({ ok: true, value: [] }); + }); + + test("names the gap: missing block, bad JSON, schema, duplicate ids", () => { + expect(parseFindings("no block here")).toMatchObject({ ok: false, reason: expect.stringContaining("no ```findings block") }); + expect(parseFindings("```findings\n{oops\n```")).toMatchObject({ ok: false, reason: expect.stringContaining("not valid JSON") }); + expect(parseFindings(block("findings", [{ id: "F1", severity: "urgent", title: "x" }]))).toMatchObject({ + ok: false, + reason: expect.stringContaining("severity"), + }); + expect(parseFindings(block("findings", [F1, { ...F2, id: "F1" }]))).toMatchObject({ + ok: false, + reason: expect.stringContaining('duplicate finding id "F1"'), + }); + }); +}); + +describe("validateDispositions (the contract the engine enforces)", () => { + test("every blocking finding needs a disposition; non-blocking ones may be left alone", () => { + expect(validateDispositions([F1, F2], SPEC.blocking, [{ id: "F1", disposition: "fixed" }])).toEqual({ ok: true }); + const missing = validateDispositions([F1, F2], SPEC.blocking, []); + expect(missing).toMatchObject({ ok: false, reason: expect.stringContaining('blocking finding "F1" (high) has no disposition') }); + }); + + test("anything but `fixed` needs a reason — no bare deferrals", () => { + for (const disposition of ["not_a_finding", "declined", "deferred"] as const) { + const r = validateDispositions([F1], SPEC.blocking, [{ id: "F1", disposition }]); + expect(r).toMatchObject({ ok: false, reason: expect.stringContaining(`"F1" is ${disposition} without a reason`) }); + expect(validateDispositions([F1], SPEC.blocking, [{ id: "F1", disposition, reason: "because" }])).toEqual({ ok: true }); + } + }); + + test("unknown ids and duplicate dispositions are refused", () => { + expect(validateDispositions([F1], SPEC.blocking, [{ id: "F1", disposition: "fixed" }, { id: "F9", disposition: "fixed" }])).toMatchObject({ + ok: false, + reason: expect.stringContaining('"F9" is not a reported finding'), + }); + expect( + validateDispositions([F1], SPEC.blocking, [ + { id: "F1", disposition: "fixed" }, + { id: "F1", disposition: "declined", reason: "r" }, + ]), + ).toMatchObject({ ok: false, reason: expect.stringContaining('"F1" has more than one disposition') }); + }); + + test("parseDispositions rejects an unknown disposition kind", () => { + expect(parseDispositions(block("dispositions", [{ id: "F1", disposition: "wontfix" }]))).toMatchObject({ ok: false }); + expect(parseDispositions(block("dispositions", [{ id: "F1", disposition: "declined", reason: "r" }]))).toMatchObject({ ok: true }); + }); +}); + +describe("loop state helpers + ledger", () => { + test("openBlocking reads the LATEST round only", () => { + const loop = newLoopState(); + loop.rounds.push({ findings: [F1, F2], dispositions: [{ id: "F1", disposition: "fixed" }] }); + loop.fixLegs = 1; + expect(openBlocking(loop, SPEC).map((f) => f.id)).toEqual(["F1"]); // reviewer hasn't re-reviewed yet + loop.rounds.push({ findings: [F2] }); + expect(openBlocking(loop, SPEC)).toEqual([]); + expect(summarizeLoop(loop, SPEC)).toBe("1 finding open (0 blocking), 1 fixed — 2 review rounds, 1 fix leg"); + }); + + test("renderLedger shows every round, dispositions with reasons, and open markers", () => { + const loop = newLoopState(); + loop.rounds.push({ + findings: [F1, F2], + dispositions: [ + { id: "F1", disposition: "fixed", evidence: "guard added; go test ./... green" }, + { id: "F2", disposition: "declined", reason: "matches house style" }, + ], + }); + loop.rounds.push({ findings: [{ id: "F3", severity: "critical", title: "regression | pipe in title" }] }); + const md = renderLedger(loop, SPEC); + expect(md).toContain("### Review round 1"); + expect(md).toContain("| F1 | high | nil deref on empty input | a.go:12 | FIXED (guard added; go test ./... green) |"); + expect(md).toContain("DECLINED — matches house style"); + expect(md).toContain("### Review round 2 (open set)"); + expect(md).toContain("regression \\| pipe in title"); // table-safe + expect(md).toContain("| open |"); + }); +}); + +describe("contracts", () => { + test("the review contract names the blocking severities and the block shape", () => { + const c = findingsContract(SPEC); + expect(c).toContain("critical and high BLOCK"); + expect(c).toContain("```findings"); + }); + + test("the re-review contract carries the ledger and asks for the OPEN set", () => { + const loop = newLoopState(); + loop.rounds.push({ findings: [F1], dispositions: [{ id: "F1", disposition: "fixed" }] }); + const c = rereviewContract(loop, SPEC); + expect(c).toContain("Review round 2"); + expect(c).toContain("| F1 | high |"); + expect(c).toContain("ONLY what remains open"); + }); + + test("the fix contract lists findings, marks blocking, and carries format feedback on a retry", () => { + const c = fixContract({ findings: [F1, F2] }, SPEC, 'blocking finding "F1" (high) has no disposition'); + expect(c).toContain("**F1** [high, blocking]"); + expect(c).toContain("**F2** [low]"); + expect(c).toContain("```dispositions"); + expect(c).toContain("did not satisfy the contract"); + }); +}); diff --git a/src/daemon/pipeline/findings.ts b/src/daemon/pipeline/findings.ts new file mode 100644 index 00000000..8aee4631 --- /dev/null +++ b/src/daemon/pipeline/findings.ts @@ -0,0 +1,360 @@ +/** + * The findings loop — review → fix → re-review, in the engine, with the + * capability roles left exactly as they are. + * + * The problem it solves: a read-only review phase (reviewer / adversary / + * verifier) produces findings but the pipeline has no backward edge, so the + * only exits were "human approves anyway" or "human re-runs the reviewer, who + * still cannot edit". Every workaround on offer — giving reviewers write + * access, or hand-authoring `fix` phases in every pack — either destroys the + * reason the review rounds exist (findings get reported, not silently patched) + * or pushes engine work into pack prose. + * + * What the engine does instead, for a phase that declares `findings:`: + * + * 1. Runs the phase under its own (read-only) role with a FINDINGS CONTRACT + * appended: the report must end with a fenced ```findings block — a JSON + * array of {id, severity, title, …}. Empty array = clean. A missing or + * malformed block is a format failure: one bounded re-run with the exact + * gap as feedback, then the phase's onFail policy. + * 2. If any finding is BLOCKING (severity in `blocking`, default critical + + * high) and fix rounds remain, runs a FIX LEG on the same bound session + * under `fixWith.role` (a pack role that MUST be write-capable — checked + * at load). The fixer gets the findings and a DISPOSITION CONTRACT: every + * blocking finding needs a disposition — fixed, not_a_finding, declined, + * deferred — and the last three need a reason. The engine validates this + * itself (no bare deferrals, no silently dropped ids); a gap is fed back + * for one bounded retry, then the phase halts with the ledger. + * 3. Optionally evaluates `fixWith.gate` (e.g. `tests_pass`) on the fix leg — + * a fixer that broke the build halts the phase with that reason. + * 4. RE-RUNS THE REVIEWER with the ledger: verify each fixed finding is really + * fixed and didn't regress, accept or re-raise the rejected ones, report + * anything new, and list what remains OPEN. The latest round's block IS + * the open set — the reviewer decides convergence, the engine enforces it. + * 5. Loops until no blocking finding is open or `maxRounds` fix legs have + * run. Then the normal exit boundary: a `review`-kind gate now has a real + * verdict (no open blocking findings), and the human halt carries the + * ledger either way. + * + * Every leg is one engine step (one model turn), so the loop persists between + * legs and survives a daemon restart mid-round. Everything here is pure: the + * contracts, the parsers, the validators, the ledger renderer. + */ + +import { z } from "zod"; +import type { ResolvedFrom } from "./binding"; + +export const SEVERITIES = ["critical", "high", "medium", "low"] as const; +export type Severity = (typeof SEVERITIES)[number]; + +export const DISPOSITIONS = ["fixed", "not_a_finding", "declined", "deferred"] as const; +export type DispositionKind = (typeof DISPOSITIONS)[number]; + +/** Default blocking severities when a pack doesn't say. */ +export const DEFAULT_BLOCKING: readonly Severity[] = ["critical", "high"]; +/** Default fix rounds when a pack doesn't say. */ +export const DEFAULT_MAX_ROUNDS = 2; +/** Bounded re-runs on a malformed / missing block before onFail applies. */ +export const FORMAT_RETRIES = 1; + +/** The built-in prompt skill the fix leg drives (registered by builtin.ts). */ +export const FINDINGS_FIX_SKILL_ID = "findings-fix"; + +const idField = z + .string() + .min(1) + .max(32) + .regex(/^[A-Za-z0-9][A-Za-z0-9._-]*$/, "finding ids are short alphanumeric tokens (e.g. F1)"); + +export const findingSchema = z.object({ + id: idField, + severity: z.enum(SEVERITIES), + title: z.string().min(1).max(300), + location: z.string().max(300).optional(), + detail: z.string().max(4000).optional(), + confidence: z.enum(["confirmed", "plausible"]).optional(), +}); +export type Finding = z.infer; + +export const dispositionSchema = z.object({ + id: idField, + disposition: z.enum(DISPOSITIONS), + reason: z.string().max(2000).optional(), + evidence: z.string().max(2000).optional(), +}); +export type Disposition = z.infer; + +/** The loop's declaration on a phase (PhaseDef.findings) — compiled from the + * pack manifest by loadPack, with defaults applied. */ +export interface FindingsSpec { + /** The write-capable pack role the fix legs run under, plus its persisted + * model binding (resolved at create like a phase's own — + * docs/role-model-binding.md §3). */ + fixWith: { role: string; provider?: string; model?: string; resolvedFrom?: ResolvedFrom }; + /** Severities that must be resolved before the phase can pass. */ + blocking: Severity[]; + /** Maximum fix legs. 0 = report only, never fix (a pure audit phase). */ + maxRounds: number; + /** Optional gate evaluated after each fix leg (e.g. `tests_pass`). */ + gate?: string; +} + +export interface FindingsRound { + /** What the reviewer reported this round — for round ≥ 2, the OPEN set. */ + findings: Finding[]; + /** The fix leg's answer to this round's findings (absent when none was needed). */ + dispositions?: Disposition[]; + fixSummary?: string; +} + +/** Persisted per phase (PipelinePhase.findings) so each leg is one engine step + * and the loop resumes exactly where it stopped. */ +export interface FindingsLoopState { + rounds: FindingsRound[]; + /** What the next engine step runs for this phase. */ + next: "review" | "fix"; + /** Fix legs completed (the `maxRounds` budget). */ + fixLegs: number; + /** Format retries consumed by the leg currently pending, and the gap to feed back. */ + formatRetries: number; + formatFeedback?: string; +} + +export function newLoopState(): FindingsLoopState { + return { rounds: [], next: "review", fixLegs: 0, formatRetries: 0 }; +} + +// ── Parsing ────────────────────────────────────────────────────────────────── + +/** The LAST fenced block tagged `tag` in `text` (a model may quote an earlier + * draft; the final one is authoritative). */ +export function extractLastBlock(text: string, tag: string): string | undefined { + const re = new RegExp(`\`\`\`${tag}[ \\t]*\\r?\\n([\\s\\S]*?)\\r?\\n[ \\t]*\`\`\``, "g"); + let last: string | undefined; + for (const m of text.matchAll(re)) last = m[1]; + return last; +} + +export type Parsed = { ok: true; value: T } | { ok: false; reason: string }; + +function parseBlock(text: string, tag: string, schema: z.ZodType): Parsed { + const raw = extractLastBlock(text, tag); + if (raw === undefined) return { ok: false, reason: `no \`\`\`${tag} block found at the end of the report` }; + let json: unknown; + try { + json = JSON.parse(raw); + } catch (e) { + return { ok: false, reason: `the \`\`\`${tag} block is not valid JSON (${e instanceof Error ? e.message : String(e)})` }; + } + const parsed = schema.safeParse(json); + if (!parsed.success) { + const first = parsed.error.issues[0]; + const path = first?.path.length ? ` at ${first.path.join(".")}` : ""; + return { ok: false, reason: `the \`\`\`${tag} block does not match the schema${path}: ${first?.message ?? "schema error"}` }; + } + return { ok: true, value: parsed.data }; +} + +export function parseFindings(text: string): Parsed { + const p = parseBlock(text, "findings", z.array(findingSchema).max(200)); + if (!p.ok) return p; + const seen = new Set(); + for (const f of p.value) { + if (seen.has(f.id)) return { ok: false, reason: `duplicate finding id "${f.id}"` }; + seen.add(f.id); + } + return p; +} + +export function parseDispositions(text: string): Parsed { + return parseBlock(text, "dispositions", z.array(dispositionSchema).max(200)); +} + +// ── Validation ─────────────────────────────────────────────────────────────── + +export const isBlocking = (f: Finding, blocking: readonly Severity[]): boolean => blocking.includes(f.severity); + +export const blockingOf = (findings: readonly Finding[], blocking: readonly Severity[]): Finding[] => + findings.filter((f) => isBlocking(f, blocking)); + +/** The open set: the latest review round's findings (empty before any round). */ +export function openFindings(loop: FindingsLoopState | undefined): Finding[] { + const last = loop?.rounds[loop.rounds.length - 1]; + return last ? last.findings : []; +} + +export const openBlocking = (loop: FindingsLoopState | undefined, spec: FindingsSpec): Finding[] => + blockingOf(openFindings(loop), spec.blocking); + +/** + * The disposition contract, enforced by the engine rather than trusted from + * prose: every blocking finding has exactly one disposition; a disposition + * names a finding that exists; anything other than `fixed` carries a reason. + * A `deferred` without a reason is the "bare deferral" every review loop + * eventually rots into — refused here. + */ +export function validateDispositions( + findings: readonly Finding[], + blocking: readonly Severity[], + dispositions: readonly Disposition[], +): { ok: true } | { ok: false; reason: string } { + const ids = new Set(findings.map((f) => f.id)); + const seen = new Map(); + const problems: string[] = []; + for (const d of dispositions) { + if (!ids.has(d.id)) problems.push(`"${d.id}" is not a reported finding`); + if (seen.has(d.id)) problems.push(`"${d.id}" has more than one disposition`); + seen.set(d.id, d); + if (d.disposition !== "fixed" && !(d.reason ?? "").trim()) { + problems.push(`"${d.id}" is ${d.disposition} without a reason`); + } + } + for (const f of blockingOf(findings, blocking)) { + if (!seen.has(f.id)) problems.push(`blocking finding "${f.id}" (${f.severity}) has no disposition`); + } + return problems.length === 0 ? { ok: true } : { ok: false, reason: problems.join("; ") }; +} + +// ── Rendering ──────────────────────────────────────────────────────────────── + +const dispositionLabel: Record = { + fixed: "FIXED", + not_a_finding: "NOT A FINDING", + declined: "DECLINED", + deferred: "DEFERRED", +}; + +/** A markdown ledger of every round: findings, dispositions, what is open. */ +export function renderLedger(loop: FindingsLoopState, spec: FindingsSpec): string { + const out: string[] = []; + loop.rounds.forEach((round, i) => { + out.push(`### Review round ${i + 1}${i === loop.rounds.length - 1 ? " (open set)" : ""}`); + if (round.findings.length === 0) { + out.push("_no findings_"); + } else { + out.push("| id | severity | finding | location | disposition |", "|---|---|---|---|---|"); + for (const f of round.findings) { + const d = round.dispositions?.find((x) => x.id === f.id); + const disp = d + ? `${dispositionLabel[d.disposition]}${d.reason ? ` — ${d.reason}` : ""}${d.evidence ? ` (${d.evidence})` : ""}` + : isBlocking(f, spec.blocking) + ? "open" + : "open (non-blocking)"; + out.push(`| ${f.id} | ${f.severity} | ${cell(f.title)} | ${cell(f.location ?? "")} | ${cell(disp)} |`); + } + } + out.push(""); + }); + return out.join("\n").trimEnd(); +} + +const cell = (s: string): string => s.replace(/\|/g, "\\|").replace(/\s+/g, " ").trim(); + +/** One line for halt reasons and status rails. */ +export function summarizeLoop(loop: FindingsLoopState, spec: FindingsSpec): string { + const open = openFindings(loop); + const blocking = blockingOf(open, spec.blocking); + const fixed = loop.rounds.reduce( + (n, r) => n + (r.dispositions?.filter((d) => d.disposition === "fixed").length ?? 0), + 0, + ); + const rounds = `${loop.rounds.length} review round${loop.rounds.length === 1 ? "" : "s"}, ${loop.fixLegs} fix leg${loop.fixLegs === 1 ? "" : "s"}`; + return `${open.length} finding${open.length === 1 ? "" : "s"} open (${blocking.length} blocking), ${fixed} fixed — ${rounds}`; +} + +// ── Prompt contracts ───────────────────────────────────────────────────────── + +const FINDINGS_BLOCK_SPEC = [ + "```findings", + "[", + ' { "id": "F1", "severity": "critical|high|medium|low", "title": "one-line claim",', + ' "location": "path/file.ext:line (optional)", "detail": "trigger → wrong outcome (optional)",', + ' "confidence": "confirmed|plausible (optional)" }', + "]", + "```", +].join("\n"); + +/** Appended to the FIRST review leg of a findings phase. */ +export function findingsContract(spec: FindingsSpec): string { + return [ + "", + "---", + "## Reporting findings (engine contract)", + "This phase's findings drive an automated fix-and-re-review loop, so they must be", + "machine-readable. End your report with exactly one fenced block tagged `findings`", + "containing a JSON array — an EMPTY array `[]` when the work is clean:", + "", + FINDINGS_BLOCK_SPEC, + "", + `Severities ${spec.blocking.join(" and ")} BLOCK the phase: a writer will be asked to resolve each`, + "one and you will review the result. Report only what you verified or can point at;", + "keep ids short and stable (F1, F2, …). Put the block last, before the completion marker.", + ].join("\n"); +} + +/** Appended to every review leg after a fix leg — the reviewer sees the ledger + * and decides what remains open. */ +export function rereviewContract(loop: FindingsLoopState, spec: FindingsSpec): string { + return [ + "", + "---", + `## Review round ${loop.rounds.length + 1} (engine contract)`, + "A writer responded to your previous findings. The ledger so far:", + "", + renderLedger(loop, spec), + "", + "Now re-review the current state of the work:", + "- For each finding marked FIXED: verify the fix is real and did not regress anything", + " you previously validated. Re-raise it (same id) if it is not actually fixed.", + "- For each NOT A FINDING / DECLINED: accept the reason, or re-raise it with the same id", + " and say why the reason does not hold.", + "- DEFERRED findings stay open unless the work now resolves them.", + "- Report anything NEW with a new id.", + "", + "End with the `findings` block listing ONLY what remains open now (carry unresolved ids", + "forward; omit what is resolved). An empty array `[]` means the work is clean:", + "", + FINDINGS_BLOCK_SPEC, + ].join("\n"); +} + +/** The fix leg's whole brief (appended to the built-in `findings-fix` skill). */ +export function fixContract(round: FindingsRound, spec: FindingsSpec, formatFeedback?: string): string { + const blocking = blockingOf(round.findings, spec.blocking); + const rows = round.findings.map( + (f) => + `- **${f.id}** [${f.severity}${isBlocking(f, spec.blocking) ? ", blocking" : ""}] ${f.title}${f.location ? ` — ${f.location}` : ""}${f.detail ? `\n ${f.detail}` : ""}`, + ); + return [ + "", + "---", + "## Findings to resolve (engine contract)", + `The reviewer reported ${round.findings.length} finding${round.findings.length === 1 ? "" : "s"}, ${blocking.length} blocking:`, + "", + ...rows, + "", + "For EVERY blocking finding (and any other you choose to act on), decide and act:", + "- `fixed` — you changed the code; say what changed and what you ran to prove it.", + "- `not_a_finding` — the claim is wrong; give the reason (the reviewer will check it).", + "- `declined` — real but deliberately not done; give the reason.", + "- `deferred` — real and tracked elsewhere; give where and why. Never a bare deferral.", + "", + "Run the project's tests before finishing. Do not silently drop a finding, and do not", + "widen the change beyond what the findings need. End your report with exactly one", + "fenced block tagged `dispositions`, listing every blocking finding id:", + "", + "```dispositions", + "[", + ' { "id": "F1", "disposition": "fixed|not_a_finding|declined|deferred",', + ' "reason": "required unless fixed", "evidence": "what changed / what ran (optional)" }', + "]", + "```", + ...(formatFeedback + ? [ + "", + `Your previous attempt did not satisfy the contract — ${formatFeedback}. Fix that and`, + "report again; the block must be last, before the completion marker.", + ] + : []), + ].join("\n"); +} diff --git a/src/daemon/pipeline/interface.ts b/src/daemon/pipeline/interface.ts index f33eb4b4..ac34c69e 100644 --- a/src/daemon/pipeline/interface.ts +++ b/src/daemon/pipeline/interface.ts @@ -9,6 +9,7 @@ */ import type { ResolvedFrom, RoleModelSource } from "./binding"; +import type { FindingsLoopState, FindingsSpec } from "./findings"; // ── Phase definition (the static plan) ──────────────────────────────────── @@ -71,6 +72,11 @@ export interface PhaseDef { writes?: string; /** failure policy for this phase. Defaults to `{ action: "halt" }`. */ onFail?: PhaseFailAction; + /** This phase produces FINDINGS and the engine runs the fix-and-re-review + * loop for them (findings.ts): a fix leg under `findings.fixWith.role` + * answers each blocking finding with an engine-validated disposition, then + * this phase re-runs to verify. The phase's own role stays read-only. */ + findings?: FindingsSpec; } // ── Phase + pipeline runtime state ──────────────────────────────────────── @@ -114,6 +120,9 @@ export interface PipelinePhase { /** The phase's most recent run output — kept so a revise re-run can show the * agent its prior attempt (a halt otherwise drops the summary). */ lastSummary?: string; + /** The findings loop's persisted state (rounds, dispositions, what runs + * next) — present only for a phase that declares `def.findings`. */ + findings?: FindingsLoopState; } /** The full, daemon-owned pipeline state — the source of truth persisted per @@ -172,6 +181,12 @@ export interface PhaseCtx { pipeline: PipelineState; phase: PhaseDef; registries: PipelineRegistries; + /** Engine-supplied text appended to the composed prompt — the findings + * loop's contracts (findings.ts). Absent for an ordinary phase run. */ + promptAppend?: string; + /** When true, the prompt omits the phase's prior output + human revise notes + * (a fix leg is a different actor from the reviewer whose phase it serves). */ + freshPrompt?: boolean; } export type PhaseRunResult = diff --git a/src/daemon/pipeline/manager.ts b/src/daemon/pipeline/manager.ts index a4bb45f5..4fe68fbf 100644 --- a/src/daemon/pipeline/manager.ts +++ b/src/daemon/pipeline/manager.ts @@ -29,6 +29,72 @@ import type { PipelineStore } from "./store"; /** Max pipelines re-driven concurrently on boot (each may spawn a worker turn). */ const RESUME_CONCURRENCY = 4; +/** + * Resolve the model binding for a findings phase's FIX LEGS (findings.ts) — + * the `fixWith` role's tier / config-role / CLI / pin rungs, with exactly the + * skip rules a phase gets (cross-provider → skipped; a model the session's + * backend can't run → skipped; both with a warning naming the rung). Persisted + * on `def.findings.fixWith` so resume and retry keep the same binding. + */ +function bindFixWith( + def: PhaseDef, + pack: Pack | undefined, + bindings: ReadonlyMap, + opts: CreatePipelineOpts, + warn: (m: string) => void, + warnedTiers: Set, +): PhaseDef { + const spec = def.findings; + if (!spec) return def; + const fw = spec.fixWith; + const role = pack?.roles?.[fw.role]; + const hasPin = fw.provider !== undefined || fw.model !== undefined; + const resolved = resolveBinding({ + packId: pack?.id, + roleName: fw.role, + role, + cliBinding: bindings.get(fw.role.toLowerCase()), + phasePin: hasPin ? { provider: fw.provider, model: fw.model } : undefined, + config: opts.modelConfig, + }); + const label = `phase "${def.id}" fix leg (role "${fw.role}")`; + const unbound = (): PhaseDef => ({ ...def, findings: { ...spec, fixWith: { role: fw.role } } }); + if (resolved.resolvedFrom === "default") { + if (role?.tier !== undefined && !warnedTiers.has(role.tier)) { + warnedTiers.add(role.tier); + warn(`role "${fw.role}" declares tier "${role.tier}" but no modelTiers mapping exists — using the provider default`); + } + return unbound(); + } + if (resolved.provider !== undefined && opts.sessionProvider !== undefined && resolved.provider !== opts.sessionProvider) { + warn( + `${label}: the ${resolved.resolvedFrom} binding targets provider "${resolved.provider}" but this run's session is bound to "${opts.sessionProvider}" — a run drives one session on one backend; skipping the binding (using the session's model)`, + ); + return unbound(); + } + if (resolved.model !== undefined) { + const target = resolved.provider ?? opts.sessionProvider; + if (resolveModelIdForProvider(resolved.model, target) === null) { + warn( + `${label}: the ${resolved.resolvedFrom} binding's model "${resolved.model}" is not valid for provider "${target ?? "claude"}" — skipping the binding (using the session's model)`, + ); + return unbound(); + } + } + return { + ...def, + findings: { + ...spec, + fixWith: { + role: fw.role, + resolvedFrom: resolved.resolvedFrom, + ...(resolved.provider !== undefined ? { provider: resolved.provider } : {}), + ...(resolved.model !== undefined ? { model: resolved.model } : {}), + }, + }, + }; +} + export interface CreatePipelineOpts { name: string; /** Explicit phase plan, OR provide `pack` to use an installed pack's pipeline. */ @@ -364,7 +430,7 @@ export class PipelineManager { delete clean.resolvedFrom; return clean; }; - return phases.map((def) => { + const bound = phases.map((def) => { const role = def.role !== undefined ? pack?.roles?.[def.role] : undefined; const hasPin = def.provider !== undefined || def.model !== undefined; const resolved = resolveBinding({ @@ -426,6 +492,10 @@ export class PipelineManager { else delete bound.model; return bound; }); + // The findings loop's fix legs run under their own role (findings.ts), so + // they get their own binding through the same six rungs and the same skip + // rules — resolved here, once, and persisted on the def like the phase's. + return bound.map((def) => bindFixWith(def, pack, bindings, opts, warn, warnedTiers)); } /** `packId` scopes gate/skill lookups to the pack the plan came from @@ -455,6 +525,9 @@ export class PipelineManager { if (p.entryGate && !hasScoped(this.#registries.gates, packId, p.entryGate)) { throw new Error(`phase "${p.id}": ${unknown("entry gate", this.#registries.gates, p.entryGate)}`); } + if (p.findings?.gate && !hasScoped(this.#registries.gates, packId, p.findings.gate)) { + throw new Error(`phase "${p.id}": ${unknown("findings fix gate", this.#registries.gates, p.findings.gate)}`); + } if (p.kind === "skill") { if (!p.skill) throw new Error(`phase "${p.id}": kind "skill" requires a skill id`); if (!hasScoped(this.#registries.skills, packId, p.skill)) { diff --git a/src/daemon/pipeline/pack.ts b/src/daemon/pipeline/pack.ts index 801d9992..051f5d97 100644 --- a/src/daemon/pipeline/pack.ts +++ b/src/daemon/pipeline/pack.ts @@ -10,6 +10,14 @@ import { readFileSync, realpathSync, statSync } from "node:fs"; import { isAbsolute, join, resolve, sep } from "node:path"; import { z } from "zod"; +import { + DEFAULT_BLOCKING, + DEFAULT_MAX_ROUNDS, + type FindingsSpec, + openBlocking, + renderLedger, + SEVERITIES, +} from "./findings"; import { buildProbeGate, probePathEscapes, type ProbeSpec } from "./gate-probes"; import type { GatePlugin, @@ -107,6 +115,24 @@ const gateSchema = z.union([ z.object({ id: idField, kind: z.literal("review"), role: z.string().max(64).optional(), at: gateAt }), ]); +/** The findings loop declaration on a phase (findings.ts). `fixWith` is a pack + * role name — or `{ role, provider?, model? }` to pin the fix legs' backend — + * and MUST be write-capable (checked at load: a read-only fixer is a + * misconfiguration, not a policy). */ +const findingsSchema = z.object({ + fixWith: z.union([ + z.string().min(1).max(64), + z.object({ + role: z.string().min(1).max(64), + provider: z.string().max(64).optional(), + model: z.string().max(256).optional(), + }), + ]), + blocking: z.array(z.enum(SEVERITIES)).min(1).max(SEVERITIES.length).optional(), + maxRounds: z.number().int().min(0).max(10).optional(), + gate: idField.optional(), +}); + const phaseSchema = z.object({ id: idField, name: z.string().max(128).optional(), @@ -122,6 +148,8 @@ const phaseSchema = z.object({ * OFF by default — no phase auto-skips unless a pack opts in. */ skipWhenSatisfied: z.boolean().optional(), onFail: onFailSchema, + /** This phase reports findings; the engine runs the fix-and-re-review loop. */ + findings: findingsSchema.optional(), }); export const packManifestSchema = z.object({ @@ -289,6 +317,7 @@ export function loadPack(dir: string, opts: LoadPackOptions = {}): LoadedPack { if (p.skipWhenSatisfied) def.skipWhenSatisfied = true; const onFail = toOnFail(p.onFail); if (onFail) def.onFail = onFail; + if (p.findings) def.findings = toFindingsSpec(m.id, p.id, p.findings, roles); return def; }); @@ -313,6 +342,39 @@ export function loadPack(dir: string, opts: LoadPackOptions = {}): LoadedPack { }; } +/** Compile a phase's `findings:` block. The fix role must exist in the pack and + * be write-capable — the loop's whole point is that the reviewer stays + * read-only and someone else writes; a read-only fixer would just re-run the + * problem this feature exists to remove. */ +function toFindingsSpec( + packId: string, + phaseId: string, + f: NonNullable, + roles: Record, +): FindingsSpec { + const fw = typeof f.fixWith === "string" ? { role: f.fixWith } : f.fixWith; + const role = roles[fw.role]; + if (!role) { + throw new Error(`pack "${packId}": phase "${phaseId}" findings.fixWith references unknown role "${fw.role}"`); + } + if (!role.write) { + throw new Error( + `pack "${packId}": phase "${phaseId}" findings.fixWith role "${fw.role}" is read-only (write: false) — the fix leg must run under a write-capable role`, + ); + } + const spec: FindingsSpec = { + fixWith: { + role: fw.role, + ...(fw.provider !== undefined ? { provider: fw.provider } : {}), + ...(fw.model !== undefined ? { model: fw.model } : {}), + }, + blocking: [...(f.blocking ?? DEFAULT_BLOCKING)], + maxRounds: f.maxRounds ?? DEFAULT_MAX_ROUNDS, + }; + if (f.gate) spec.gate = f.gate; + return spec; +} + function toOnFail(v: PackManifest["phases"][number]["onFail"]): PhaseFailAction | undefined { if (v === undefined) return undefined; if (v === "halt") return { action: "halt" }; @@ -376,18 +438,44 @@ function buildGate(g: PackManifest["gates"][number], dir: string, trusted: boole }, }; } - // self / skill / review gates carry no AUTOMATED verdict yet. They no longer - // fail closed (that surfaced a confusing "not yet enforced" halt): every phase + if (g.kind === "review") { + // S4 for review gates: a REAL verdict when the phase runs the findings loop + // — pass iff no blocking finding is open in the latest review round. On a + // phase without `findings:` it behaves as before (human is the reviewer). + return findingsReviewGate(g.id, at); + } + // self / skill gates carry no AUTOMATED verdict yet. They no longer fail + // closed (that surfaced a confusing "not yet enforced" halt): every phase // already halts at its boundary for a human decision (see engine.ts), so these - // gates simply pass and defer to that human review. S4 may turn them into real - // subagent verdicts shown alongside the human decision. + // gates simply pass and defer to that human review. return humanReviewGate(g.id, at); } /** A gate with no automated verdict — it passes, deferring acceptance to the * universal human boundary halt. Distinct from failClosedGate: this is not a * silent success that skips review, because the phase halts for the human - * regardless (engine.ts). Used for self/skill/review gate kinds. */ + * regardless (engine.ts). Used for self/skill gate kinds. */ function humanReviewGate(id: string, at: "entry" | "exit"): GatePlugin { return { id, at, async evaluate() { return { pass: true }; } }; } + +/** The `review` gate kind: the findings loop's verdict (findings.ts). Reads the + * phase under the cursor — the gate is evaluated at that phase's exit, after + * its last review leg — and fails while a blocking finding is still open. */ +function findingsReviewGate(id: string, at: "entry" | "exit"): GatePlugin { + return { + id, + at, + async evaluate(ctx) { + const phase = ctx.pipeline.phases[ctx.pipeline.cursor]; + const spec = phase?.def.findings; + if (!phase || !spec || !phase.findings) return { pass: true }; + const open = openBlocking(phase.findings, spec); + if (open.length === 0) return { pass: true }; + return { + pass: false, + reason: `${open.length} blocking finding${open.length === 1 ? "" : "s"} still open after ${phase.findings.fixLegs} fix leg${phase.findings.fixLegs === 1 ? "" : "s"}:\n${renderLedger(phase.findings, spec)}`, + }; + }, + }; +} diff --git a/src/daemon/pipeline/skill-kind.ts b/src/daemon/pipeline/skill-kind.ts index ee1d03dd..28c4a32b 100644 --- a/src/daemon/pipeline/skill-kind.ts +++ b/src/daemon/pipeline/skill-kind.ts @@ -13,7 +13,12 @@ import { resolveScoped } from "./scoped"; /** Compose a phase's prompt: the skill command/template, the run's goal, and — * on a revise re-run — the phase's prior output + the accumulated human * feedback so the agent re-iterates on the same phase (docs/pipeline-run.md). */ -function composePhasePrompt(base: string, spec: string | undefined, phase: PipelinePhase | undefined): string { +function composePhasePrompt( + base: string, + spec: string | undefined, + phase: PipelinePhase | undefined, + append?: string, +): string { const parts = [base]; if (spec) { // Frame the overall goal as CONTEXT, and scope the model to THIS phase's @@ -32,6 +37,10 @@ function composePhasePrompt(base: string, spec: string | undefined, phase: Pipel if (phase?.lastSummary) parts.push(`## Your previous output for this phase\n${phase.lastSummary}`); parts.push(`## Reviewer feedback — revise this phase accordingly\n${feedback.map((f, i) => `${i + 1}. ${f}`).join("\n")}`); } + // Engine contracts (the findings loop) go LAST so they sit right above the + // completion marker the host appends — the model reads them as the final + // instruction on how to end its report. + if (append) parts.push(append); return parts.join("\n\n"); } @@ -74,7 +83,10 @@ async function runSkill( }; } const base = skill.kind === "slash" ? skill.command : skill.template; - const prompt = composePhasePrompt(base, ctx.pipeline.spec, ctx.pipeline.phases[ctx.pipeline.cursor]); + // A fix leg (freshPrompt) is a different actor from the phase's reviewer: it + // must not inherit the reviewer's prior output or the human's revise notes. + const current = ctx.freshPrompt ? undefined : ctx.pipeline.phases[ctx.pipeline.cursor]; + const prompt = composePhasePrompt(base, ctx.pipeline.spec, current, ctx.promptAppend); const res = await runner.runPrompt({ prompt, provider: ctx.phase.provider, diff --git a/src/daemon/session-manager.ts b/src/daemon/session-manager.ts index 0e9141b7..88a46edc 100644 --- a/src/daemon/session-manager.ts +++ b/src/daemon/session-manager.ts @@ -130,6 +130,7 @@ import type { } from "../protocol/types.js"; import type { Scope } from "../protocol/scopes.js"; import type { PipelineState } from "./pipeline/interface.js"; +import { blockingOf, openFindings, renderLedger } from "./pipeline/findings.js"; /** Per-phase autonomous turn budget for a pipeline run. A phase runs the model * to completion within its role (the human gate is the phase boundary, not each @@ -3523,6 +3524,19 @@ mcpHub: this.#mcpHub, if (p.lastSummary) w.summary = p.lastSummary; } if (p.feedback && p.feedback.length > 0) w.feedback = p.feedback; + // The findings loop (findings.ts): counts + the ledger, so a client can + // render "2 open (1 blocking), 1 fix leg" and the per-round table. + if (p.def.findings && p.findings) { + const open = openFindings(p.findings); + w.findings = { + rounds: p.findings.rounds.length, + fixLegs: p.findings.fixLegs, + open: open.length, + blocking: blockingOf(open, p.def.findings.blocking).length, + next: p.findings.next, + ledger: renderLedger(p.findings, p.def.findings), + }; + } return w; }), }; diff --git a/src/terminal/pipeline-format.ts b/src/terminal/pipeline-format.ts index b9a98488..b1a696f7 100644 --- a/src/terminal/pipeline-format.ts +++ b/src/terminal/pipeline-format.ts @@ -49,6 +49,15 @@ export function formatPipeline(p: PipelineWire): string[] { if (ph.feedback && ph.feedback.length > 0) { out.push(` revisions: ${ph.feedback.length}`); } + // The findings loop (review → fix → re-review): where it stands, and what + // the engine runs next while the phase is still in motion. + if (ph.findings) { + const f = ph.findings; + const next = ph.status === "running" ? `, next: ${f.next === "fix" ? "fix leg" : "review leg"}` : ""; + out.push( + ` findings: ${f.open} open (${f.blocking} blocking) after ${f.rounds} review round${f.rounds === 1 ? "" : "s"}, ${f.fixLegs} fix leg${f.fixLegs === 1 ? "" : "s"}${next}`, + ); + } } const cur = p.phases[p.cursor]; if (cur && cur.status === "halted") { From 3991a982bab0c91022dce638f747e29f93d61202 Mon Sep 17 00:00:00 2001 From: Yash Datta Date: Tue, 15 Sep 2026 23:35:32 +0800 Subject: [PATCH 2/2] fix: harden the findings loop after an adversarial audit of #337 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An independent review of the first commit found one dead end and a set of semantic gaps; all are closed here, with tests. **A failing fix gate on the last budgeted leg was a dead end.** The leg was counted and handed back to the reviewer BEFORE `fixWith.gate` ran, so a fixer that broke the build consumed the budget, every Revise re-ran the read-only reviewer against the red tree, and the human was left with "approve a red tree or reject". The gate now runs before the leg commits: a failure repairs the SAME leg once with the gate's reason fed back, and only a repair that also fails reaches onFail — the reviewer never sees a red tree, the leg never counts. **`onFail: retry` on a findings phase re-ran the reviewer and pasted the ledger into the human's revise notes.** The generic retry channel appends the failure reason to `phase.feedback`, which is rendered as revision history and re-pasted into every later prompt. A findings phase now handles its own retries: a retry is another fix loop — fresh fix budget, straight to a fix leg when blockers are open — with the reason carried as the engine's note to that leg. **A bare-marker turn after a nudge dropped the findings block.** The phase summary was only the model's LAST message; a reviewer that writes its report, rests without the marker, is nudged, and answers with just the marker returned an empty summary and burned the format retry. The summary is now the whole of what the model said across the leg's turns. **Explicit-`phases` plans accepted `findings` with no role check.** No pack means no `fixWith` role and no write-capable check, so reviewer and fixer would both have run with no role at all. Rejected at create. **Revise could not reach the fixer.** The human's notes now travel to every fix leg ("fix F3 with a guard clause" is for the writer). Also: the loop's own legs no longer re-evaluate the phase's entry gate (it grounds a phase run, not every turn); the loader refuses `skipWhenSatisfied`, a review-kind entry gate, and a non-deterministic fix gate on a findings phase; the gate-failure halt keeps the loop summary; the re-review contract no longer claims "a writer responded" when none did; "fixed" counts a re-raised-then-fixed finding once; the web cockpit keeps the ledger's line breaks and shows the counts; the CHANGELOG names the pipeline wire, not `pack.list`. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Yash Datta --- CHANGELOG.md | 10 +- docs/findings-loop.md | 37 ++++- src/daemon/pipeline/engine.ts | 172 +++++++++++++++------- src/daemon/pipeline/findings-loop.test.ts | 103 ++++++++++++- src/daemon/pipeline/findings-pack.test.ts | 48 ++++++ src/daemon/pipeline/findings.test.ts | 25 +++- src/daemon/pipeline/findings.ts | 48 ++++-- src/daemon/pipeline/manager.ts | 8 + src/daemon/pipeline/pack.ts | 28 +++- src/daemon/session-manager.ts | 32 +++- src/tests/pipeline-runner.test.ts | 42 ++++++ web/src/components/PipelineRunner.tsx | 14 +- 12 files changed, 472 insertions(+), 95 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index db11837f..8083eac6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,8 +64,14 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). `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 `pipeline.pack.list` / `codeoid pipeline status` - show the counts. `maxRounds: 0` makes a pure audit phase. + 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 diff --git a/docs/findings-loop.md b/docs/findings-loop.md index 75474aaf..d6ed3a02 100644 --- a/docs/findings-loop.md +++ b/docs/findings-loop.md @@ -77,11 +77,15 @@ review leg (role: reviewer) ──findings block──► any blocking? ─ 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 retry; a gap after - that halts the phase with the ledger. -4. **Fix gate.** If `fixWith.gate` is set, it is evaluated on the fix leg; a - fixer that broke the build halts the phase with that reason rather than - handing the reviewer a red tree. + 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 @@ -126,15 +130,34 @@ 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 semantics +## 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 fix budget is per phase, not per revise. + 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 diff --git a/src/daemon/pipeline/engine.ts b/src/daemon/pipeline/engine.ts index 7e318955..bc5fe9d8 100644 --- a/src/daemon/pipeline/engine.ts +++ b/src/daemon/pipeline/engine.ts @@ -111,16 +111,22 @@ export class PipelineEngine { } } + // The findings loop's state (findings.ts), when this phase runs it. + const spec = phase.def.findings; + if (spec && !phase.findings) phase.findings = newLoopState(); + const loop = spec ? phase.findings : undefined; + // A leg the LOOP dispatched (a fix leg, the re-review right after it, or a + // format retry) is not "the phase acting" again — the entry gate is a + // grounding probe for a phase run, not for every turn inside the loop. + const loopLeg = loop !== undefined && (loop.next === "fix" || loop.rereview === true || loop.formatRetries > 0); + // Entry (grounding) gate — read-only probe before the phase acts (§5a.3). - if (phase.def.entryGate) { + if (phase.def.entryGate && !loopLeg) { const v = await this.#gate(phase.def.entryGate, s, phase.def, "entry"); if (!v.pass) return applyFail(s, phase, v, attempts, "entry"); } // ── Findings loop: a pending FIX LEG runs instead of the phase's own kind. - const spec = phase.def.findings; - if (spec && !phase.findings) phase.findings = newLoopState(); - const loop = spec ? phase.findings : undefined; if (spec && loop && loop.next === "fix") { return this.#fixLeg(s, phase, spec, loop, attempts); } @@ -130,14 +136,12 @@ export class PipelineEngine { // throw is treated as a phase failure, then handled by the onFail policy. // For a findings phase this is a REVIEW LEG: the first one carries the // findings contract, every later one the ledger + re-review contract, and - // a format retry carries the exact gap on top. + // a retry carries the engine's note on the previous attempt on top. const reviewAppend = spec && loop ? [ loop.rounds.length === 0 ? findingsContract(spec) : rereviewContract(loop, spec), - ...(loop.formatFeedback - ? [`\nYour previous report did not satisfy the contract — ${loop.formatFeedback}. Report again.`] - : []), + ...(loop.formatFeedback ? [`\nEngine note on your previous attempt: ${loop.formatFeedback}\nAddress it and report again.`] : []), ].join("\n") : undefined; const res = await this.#runKind(s, phase.def, reviewAppend ? { promptAppend: reviewAppend } : {}); @@ -163,6 +167,7 @@ export class PipelineEngine { // ── Findings loop: record the round; dispatch a fix leg if anything blocks. if (spec && loop) { + loop.rereview = false; // this leg consumed the post-fix re-review const parsed = parseFindings(res.summary ?? ""); if (!parsed.ok) { // A report without a valid block is a FORMAT failure, not a verdict: @@ -173,14 +178,13 @@ export class PipelineEngine { loop.formatFeedback = parsed.reason; return touch(s); } - loop.formatRetries = 0; - loop.formatFeedback = undefined; - return applyFail( + return this.#findingsFail( s, phase, - { pass: false, reason: `phase "${phase.def.id}" produced no valid findings block: ${parsed.reason}` }, + spec, + loop, attempts, - "exit", + `phase "${phase.def.id}" produced no valid findings block: ${parsed.reason}`, ); } loop.formatRetries = 0; @@ -210,20 +214,25 @@ export class PipelineEngine { let gateReason: string | undefined; let verdict: GateVerdict = { pass: true }; if (phase.def.gate) verdict = await this.#gate(phase.def.gate, s, phase.def, "exit"); - if (verdict.pass && spec && loop) { + const openBlockers = spec && loop ? openBlocking(loop, spec) : []; + if (verdict.pass && spec && loop && openBlockers.length > 0) { // No gate (or a passing one) but blocking findings are still open — the // loop's own verdict fails the boundary, ledger attached, whether or not // the pack declared a `review` gate. - const open = openBlocking(loop, spec); - if (open.length > 0) { - verdict = { - pass: false, - reason: `${open.length} blocking finding${open.length === 1 ? "" : "s"} still open after ${loop.fixLegs} fix leg${loop.fixLegs === 1 ? "" : "s"}:\n${renderLedger(loop, spec)}`, - }; - } + verdict = { + pass: false, + reason: `${openBlockers.length} blocking finding${openBlockers.length === 1 ? "" : "s"} still open after ${loop.fixLegs} fix leg${loop.fixLegs === 1 ? "" : "s"}:\n${renderLedger(loop, spec)}`, + }; } if (!verdict.pass) { const onFail = phase.def.onFail ?? { action: "halt" }; + // A findings phase whose boundary fails on OPEN BLOCKERS: `retry` means + // "another fix loop" (fresh fix budget, straight to a fix leg — the + // reviewer already spoke), never "re-run the read-only reviewer with the + // ledger pasted into its revise notes". `abort` fails as usual. + if (spec && loop && openBlockers.length > 0 && (onFail.action === "retry" || onFail.action === "abort")) { + return this.#findingsFail(s, phase, spec, loop, attempts, verdict.reason ?? "blocking findings open"); + } // A machine retry/abort short-circuits the human boundary. if (onFail.action === "retry" || onFail.action === "abort") { return applyFail(s, phase, verdict, attempts, "exit"); @@ -231,13 +240,14 @@ export class PipelineEngine { gateReason = verdict.reason ?? "gate check failed"; } - // The phase's work is done (kept in lastSummary); halt for the human. + // The phase's work is done (kept in lastSummary); halt for the human. A + // findings phase carries the loop's one-line summary in both branches. const ledger = spec && loop ? ` — ${summarizeLoop(loop, spec)}` : ""; phase.state = { status: "halted", requestId: `exit:${phase.def.id}`, reason: gateReason - ? `phase "${phase.def.id}" complete — gate not satisfied: ${gateReason}` + ? `phase "${phase.def.id}" complete${ledger} — gate not satisfied: ${gateReason}` : `phase "${phase.def.id}" complete${ledger} — review and approve`, }; s.status = "halted"; @@ -274,9 +284,11 @@ export class PipelineEngine { * One FIX LEG of the findings loop: run the built-in `findings-fix` skill on * the same bound session under the phase's `fixWith` role (the runner swaps * the role per leg exactly as it swaps it per phase), parse + validate the - * dispositions the engine demanded, run the optional fix gate, and hand the - * phase back to a review leg. Format gaps get one bounded retry with the - * exact gap; a gap after that halts the phase with the ledger. + * dispositions the engine demanded, run the optional fix gate, and only THEN + * commit the leg and hand the phase back to a review leg. A format gap or a + * failing fix gate gets one bounded repair of the SAME leg with the exact + * problem fed back; a problem after that goes to the phase's onFail policy + * with the ledger — never to the reviewer with a red tree. */ async #fixLeg( s: PipelineState, @@ -301,8 +313,10 @@ export class PipelineEngine { ...(spec.fixWith.model !== undefined ? { model: spec.fixWith.model } : {}), ...(spec.fixWith.resolvedFrom !== undefined ? { resolvedFrom: spec.fixWith.resolvedFrom } : {}), }; + // The human's revise notes reach the fixer too — "fix F3 this way" is for + // the writer, and a note left while a fix leg was pending must not be lost. const res = await this.#runKind(s, legDef, { - promptAppend: fixContract(round, spec, loop.formatFeedback), + promptAppend: fixContract(round, spec, loop.formatFeedback, phase.feedback), freshPrompt: true, }); if (res.outcome === "halted") { @@ -313,48 +327,94 @@ export class PipelineEngine { if (res.outcome === "failed") { return applyFail(s, phase, { pass: false, reason: `fix leg "${legDef.id}" failed: ${res.reason}` }, attempts, "kind"); } - const parsed = parseDispositions(res.summary ?? ""); - const check = parsed.ok ? validateDispositions(round.findings, spec.blocking, parsed.value) : parsed; - if (!check.ok) { + // The same-leg repair path: one bounded retry with the exact problem. + const repair = (problem: string): PipelineState | null => { if (loop.formatRetries < FORMAT_RETRIES) { loop.formatRetries += 1; - loop.formatFeedback = check.reason; + loop.formatFeedback = problem; return touch(s); // still running; next stays "fix" } - loop.formatRetries = 0; - loop.formatFeedback = undefined; - loop.next = "review"; - return applyFail( - s, - phase, - { - pass: false, - reason: `fix leg "${legDef.id}" did not resolve the findings: ${check.reason}\n${renderLedger(loop, spec)}`, - }, - attempts, - "exit", + return null; + }; + const parsed = parseDispositions(res.summary ?? ""); + const check = parsed.ok ? validateDispositions(round.findings, spec.blocking, parsed.value) : parsed; + if (!check.ok) { + return ( + repair(check.reason) ?? + this.#findingsFail( + s, + phase, + spec, + loop, + attempts, + `fix leg "${legDef.id}" did not resolve the findings: ${check.reason}\n${renderLedger(loop, spec)}`, + ) ); } + // The fix gate (e.g. tests_pass) — evaluated BEFORE the leg counts, so a + // fixer that broke the build repairs its own leg instead of handing the + // reviewer a red tree (and, on the last budgeted leg, a dead end). + if (spec.gate) { + const v = await this.#gate(spec.gate, s, legDef, "exit"); + if (!v.pass) { + const problem = `fix gate "${spec.gate}" failed: ${v.reason ?? "check failed"}`; + return ( + repair(problem) ?? + this.#findingsFail(s, phase, spec, loop, attempts, `fix leg "${legDef.id}" — ${problem}\n${renderLedger(loop, spec)}`) + ); + } + } round.dispositions = parsed.ok ? parsed.value : []; round.fixSummary = res.summary; loop.fixLegs += 1; loop.formatRetries = 0; loop.formatFeedback = undefined; loop.next = "review"; - // The fix gate (e.g. tests_pass): a fixer that broke the build halts the - // phase with that reason rather than handing the reviewer a red tree. - if (spec.gate) { - const v = await this.#gate(spec.gate, s, legDef, "exit"); - if (!v.pass) { - return applyFail( - s, - phase, - { pass: false, reason: `fix leg "${legDef.id}" failed gate "${spec.gate}": ${v.reason ?? "check failed"}` }, - attempts, - "exit", - ); - } + loop.rereview = true; + return touch(s); + } + + /** + * A findings phase failed at its loop (a leg exhausted its repair, or + * blocking findings stayed open past the budget): apply the phase's onFail + * policy WITHOUT the generic retry channel. `retry` on a findings phase is + * another fix loop — fresh fix budget, straight to a fix leg when blockers + * are open — with the reason carried as the engine's note to the next leg, + * never appended to the human's revise notes (where it would be rendered as + * revision history and re-pasted into every later prompt). + */ + #findingsFail( + s: PipelineState, + phase: PipelinePhase, + spec: FindingsSpec, + loop: FindingsLoopState, + attempts: number, + reason: string, + ): PipelineState { + const onFail: PhaseFailAction = phase.def.onFail ?? { action: "halt" }; + loop.formatRetries = 0; + loop.formatFeedback = undefined; + loop.rereview = false; + const blockersOpen = openBlocking(loop, spec).length > 0; + loop.next = blockersOpen ? "fix" : "review"; + const nextAttempts = attempts + 1; + if (onFail.action === "retry" && nextAttempts < onFail.max) { + loop.fixLegs = 0; + loop.formatFeedback = reason; + phase.state = { status: "running", startedAt: now(), attempts: nextAttempts }; + s.status = "running"; + return touch(s); + } + if (onFail.action === "halt") { + // A human Revise re-enters as a review leg (the reviewer speaks first); + // the ledger in the reason is what they decide on. + loop.next = "review"; + phase.state = { status: "halted", requestId: `exit:${phase.def.id}`, reason }; + s.status = "halted"; + return touch(s); } + phase.state = { status: "failed", reason, attempts: nextAttempts }; + s.status = "failed"; return touch(s); } diff --git a/src/daemon/pipeline/findings-loop.test.ts b/src/daemon/pipeline/findings-loop.test.ts index 726d41e1..c55ddf82 100644 --- a/src/daemon/pipeline/findings-loop.test.ts +++ b/src/daemon/pipeline/findings-loop.test.ts @@ -164,7 +164,7 @@ describe("findings loop — review → fix → re-review", () => { const out = await new PipelineEngine(regs(ok.runner)).run(pipeline([reviewPhase(spec())])); expect(ok.calls).toHaveLength(4); expect(ok.calls[2]!.phase.id).toBe("review#fix1"); // same leg, retried - expect(ok.calls[2]!.prompt).toContain("did not satisfy the contract — no ```dispositions block"); + expect(ok.calls[2]!.prompt).toContain("Engine note on your previous attempt: no ```dispositions block"); expect(out.status).toBe("halted"); expect(out.phases[0]!.findings?.fixLegs).toBe(1); @@ -186,7 +186,7 @@ describe("findings loop — review → fix → re-review", () => { const ok = scripted(["looks fine to me", fb([])]); const out = await new PipelineEngine(regs(ok.runner)).run(pipeline([reviewPhase(spec())])); expect(ok.calls).toHaveLength(2); - expect(ok.calls[1]!.prompt).toContain("did not satisfy the contract — no ```findings block"); + expect(ok.calls[1]!.prompt).toContain("Engine note on your previous attempt: no ```findings block"); expect(ok.calls[1]!.phase.role).toBe("reviewer"); expect(out.status).toBe("halted"); expect(out.phases[0]!.findings?.rounds).toHaveLength(1); @@ -199,13 +199,104 @@ describe("findings loop — review → fix → re-review", () => { if (st.status === "halted") expect(st.reason).toContain("produced no valid findings block"); }); - test("a fix gate (e.g. tests) that fails after the fix leg halts the phase with that reason", async () => { - const { runner, calls } = scripted([fb([F1]), db([{ id: "F1", disposition: "fixed" }])]); + test("a failing fix gate repairs the SAME fix leg once (the leg does not count), then halts with the ledger", async () => { + // `manual` never passes, so the repair also fails → halt. The leg never + // committed: fixLegs stays 0 and the reviewer is never handed a red tree. + const { runner, calls } = scripted([fb([F1]), db([{ id: "F1", disposition: "fixed" }]), db([{ id: "F1", disposition: "fixed" }])]); const out = await new PipelineEngine(regs(runner)).run(pipeline([reviewPhase(spec({ gate: "manual" }))])); - expect(calls).toHaveLength(2); // no re-review: the fixer broke the gate + expect(calls).toHaveLength(3); + expect(calls[2]!.phase.id).toBe("review#fix1"); // the same leg, repaired + expect(calls[2]!.prompt).toContain('Engine note on your previous attempt: fix gate "manual" failed'); + expect(out.status).toBe("halted"); + const ph = out.phases[0]!; + expect(ph.findings?.fixLegs).toBe(0); + expect(ph.findings?.rounds[0]!.dispositions).toBeUndefined(); + expect(ph.findings?.next).toBe("review"); // a human Revise re-enters at the reviewer + if (ph.state.status === "halted") { + expect(ph.state.reason).toContain('fix leg "review#fix1" — fix gate "manual" failed'); + expect(ph.state.reason).toContain("| F1 | high |"); + } + }); + + test("a failing fix gate that the repair fixes counts the leg once and continues to the re-review", async () => { + // A gate that fails the first evaluation and passes the second. + let evals = 0; + const flaky = { + id: "flaky", + at: "exit" as const, + async evaluate() { + evals += 1; + return evals === 1 ? { pass: false, reason: "2 tests failed" } : { pass: true }; + }, + }; + const { runner, calls } = scripted([fb([F1]), db([{ id: "F1", disposition: "fixed" }]), db([{ id: "F1", disposition: "fixed", evidence: "green now" }]), fb([])]); + const r = regs(runner); + r.gates.register(flaky); + const out = await new PipelineEngine(r).run(pipeline([reviewPhase(spec({ gate: "flaky" }))])); + expect(calls.map((c) => c.phase.id)).toEqual(["review", "review#fix1", "review#fix1", "review"]); + expect(out.status).toBe("halted"); + expect(out.phases[0]!.findings?.fixLegs).toBe(1); + expect(out.phases[0]!.findings?.rounds[0]!.dispositions?.[0]?.evidence).toBe("green now"); + }); + + test("onFail: retry on a findings phase is ANOTHER FIX LOOP — fresh budget, straight to a fix leg, nothing pasted into the human's revise notes", async () => { + const { runner, calls } = scripted([ + fb([F1]), // round 1 + db([{ id: "F1", disposition: "declined", reason: "by design" }]), // fix leg 1 (budget 1) + fb([F1]), // round 2: still open → budget spent → boundary fails → retry + db([{ id: "F1", disposition: "fixed" }]), // retry = fresh loop: fix leg first, not the reviewer + fb([]), // round 3: clean + ]); + const out = await new PipelineEngine(regs(runner)).run( + pipeline([reviewPhase(spec({ maxRounds: 1 }), { onFail: { action: "retry", max: 2 } })]), + ); + expect(calls.map((c) => c.phase.id)).toEqual(["review", "review#fix1", "review", "review#fix1", "review"]); + expect(calls[3]!.prompt).toContain("Engine note on your previous attempt: 1 blocking finding still open"); + expect(out.status).toBe("halted"); + const ph = out.phases[0]!; + expect(ph.feedback).toBeUndefined(); // the revise channel is the human's + if (ph.state.status === "halted") expect(ph.state.reason).toContain("review and approve"); + // Budget exhausted with blockers still open → failed, as retry semantics demand. + const stuck = scripted([fb([F1]), db([{ id: "F1", disposition: "declined", reason: "no" }]), fb([F1]), db([{ id: "F1", disposition: "declined", reason: "no" }]), fb([F1])]); + const failed = await new PipelineEngine(regs(stuck.runner)).run( + pipeline([reviewPhase(spec({ maxRounds: 1 }), { onFail: { action: "retry", max: 2 } })]), + ); + expect(failed.status).toBe("failed"); + }); + + test("the human's revise notes reach the fixer, and the gate-failure halt keeps the loop summary", async () => { + const { runner, calls } = scripted([fb([F1]), db([{ id: "F1", disposition: "fixed" }]), fb([])]); + const p = pipeline([reviewPhase(spec(), { gate: "manual" })]); + p.phases[0]!.feedback = ["fix F1 with a guard clause, not a try/catch"]; + const out = await new PipelineEngine(regs(runner)).run(p); + expect(calls[1]!.prompt).toContain("## Notes from the human (revise)"); + expect(calls[1]!.prompt).toContain("guard clause"); + // Loop clean, but the phase's own `manual` gate fails → halt, and the + // reason still carries the loop's one-line summary. expect(out.status).toBe("halted"); const st = out.phases[0]!.state; - if (st.status === "halted") expect(st.reason).toContain('fix leg "review#fix1" failed gate "manual"'); + if (st.status === "halted") { + expect(st.reason).toContain("0 findings open (0 blocking), 1 fixed"); + expect(st.reason).toContain("gate not satisfied"); + } + }); + + test("the phase's entry gate grounds the phase run once, not every leg of the loop", async () => { + let entries = 0; + const counting = { + id: "counting", + at: "entry" as const, + async evaluate() { + entries += 1; + return { pass: true }; + }, + }; + const { runner } = scripted(["no block", fb([F1]), "no dispositions", db([{ id: "F1", disposition: "fixed" }]), fb([])]); + const r = regs(runner); + r.gates.register(counting); + const out = await new PipelineEngine(r).run(pipeline([reviewPhase(spec(), { entryGate: "counting" })])); + expect(out.status).toBe("halted"); + expect(entries).toBe(1); // review retry, fix leg, fix repair, re-review: all loop legs }); test("each leg is one step, and the loop survives a serialize/parse round-trip between legs (restart-safe)", async () => { diff --git a/src/daemon/pipeline/findings-pack.test.ts b/src/daemon/pipeline/findings-pack.test.ts index 2ac72293..1abeab71 100644 --- a/src/daemon/pipeline/findings-pack.test.ts +++ b/src/daemon/pipeline/findings-pack.test.ts @@ -171,4 +171,52 @@ describe("findings: on a pack phase", () => { expect(warnings.join("\n")).toContain('fix leg (role "implementer")'); expect(warnings.join("\n")).toContain('targets provider "codex"'); }); + + test("shapes that cannot mean what they say are refused at load", () => { + // A findings phase always runs — skipWhenSatisfied would skip the review. + expect(() => + loadPack( + writePack( + MANIFEST( + " - { id: review, skill: review, role: reviewer, gate: bench_clear, skipWhenSatisfied: true, findings: { fixWith: implementer } }", + ), + ), + ), + ).toThrow(/skipWhenSatisfied is not allowed/); + // A review gate is the loop's EXIT verdict; at entry it would block every revise. + expect(() => + loadPack( + writePack( + MANIFEST(" - { id: review, skill: review, role: reviewer, entryGate: bench_clear, findings: { fixWith: implementer } }"), + ), + ), + ).toThrow(/entryGate "bench_clear" is a review gate/); + // The fix gate must be deterministic — a review-kind fix gate always fails. + expect(() => + loadPack( + writePack( + MANIFEST(" - { id: review, skill: review, role: reviewer, findings: { fixWith: implementer, gate: bench_clear } }"), + ), + ), + ).toThrow(/findings.gate "bench_clear" is a review gate/); + }); + + test("an explicit-phases plan cannot declare a findings loop (no pack, no role check)", () => { + const mgr = new PipelineManager(new PipelineStore(new Database(":memory:"))); + mgr.registries.skills.register({ id: "review", kind: "prompt", template: "x" }); + expect(() => + mgr.create({ + ...tenant, + name: "r", + phases: [ + { + id: "review", + kind: "skill", + skill: "review", + findings: { fixWith: { role: "anything" }, blocking: [], maxRounds: 1_000_000 }, + }, + ], + }), + ).toThrow(/findings loops require a pack/); + }); }); diff --git a/src/daemon/pipeline/findings.test.ts b/src/daemon/pipeline/findings.test.ts index be5f0c62..cea7eaa2 100644 --- a/src/daemon/pipeline/findings.test.ts +++ b/src/daemon/pipeline/findings.test.ts @@ -139,8 +139,31 @@ describe("contracts", () => { loop.rounds.push({ findings: [F1], dispositions: [{ id: "F1", disposition: "fixed" }] }); const c = rereviewContract(loop, SPEC); expect(c).toContain("Review round 2"); + expect(c).toContain("A writer responded to your previous findings"); expect(c).toContain("| F1 | high |"); expect(c).toContain("ONLY what remains open"); + // No fix leg ran (audit-only, budget spent, a leg that never committed): + // say so rather than claiming a writer responded. + const audit = newLoopState(); + audit.rounds.push({ findings: [F1] }); + expect(rereviewContract(audit, SPEC)).toContain("No fix leg ran since your previous findings"); + }); + + test("summarizeLoop counts a finding fixed, re-raised, and fixed again once — and never while it is still open", () => { + const loop = newLoopState(); + loop.rounds.push({ findings: [F1], dispositions: [{ id: "F1", disposition: "fixed" }] }); + loop.rounds.push({ findings: [F1], dispositions: [{ id: "F1", disposition: "fixed" }] }); // re-raised, fixed again + loop.fixLegs = 2; + expect(summarizeLoop(loop, SPEC)).toContain("1 finding open (1 blocking), 0 fixed"); // still open + loop.rounds.push({ findings: [] }); + expect(summarizeLoop(loop, SPEC)).toContain("0 findings open (0 blocking), 1 fixed"); + }); + + test("the fix contract carries the human's revise notes", () => { + const c = fixContract({ findings: [F1] }, SPEC, undefined, ["use a guard clause", "no new deps"]); + expect(c).toContain("## Notes from the human (revise)"); + expect(c).toContain("1. use a guard clause"); + expect(c).toContain("2. no new deps"); }); test("the fix contract lists findings, marks blocking, and carries format feedback on a retry", () => { @@ -148,6 +171,6 @@ describe("contracts", () => { expect(c).toContain("**F1** [high, blocking]"); expect(c).toContain("**F2** [low]"); expect(c).toContain("```dispositions"); - expect(c).toContain("did not satisfy the contract"); + expect(c).toContain('Engine note on your previous attempt: blocking finding "F1" (high) has no disposition'); }); }); diff --git a/src/daemon/pipeline/findings.ts b/src/daemon/pipeline/findings.ts index 8aee4631..c42446e7 100644 --- a/src/daemon/pipeline/findings.ts +++ b/src/daemon/pipeline/findings.ts @@ -115,9 +115,14 @@ export interface FindingsLoopState { next: "review" | "fix"; /** Fix legs completed (the `maxRounds` budget). */ fixLegs: number; - /** Format retries consumed by the leg currently pending, and the gap to feed back. */ + /** Repair retries consumed by the leg currently pending (a malformed block, + * a failing fix gate), and the engine's note to feed back to it. */ formatRetries: number; formatFeedback?: string; + /** True between a committed fix leg and the review leg that verifies it — + * that review is the loop's own leg, not the phase acting again (so the + * phase's entry gate is not re-evaluated for it). */ + rereview?: boolean; } export function newLoopState(): FindingsLoopState { @@ -250,16 +255,19 @@ export function renderLedger(loop: FindingsLoopState, spec: FindingsSpec): strin const cell = (s: string): string => s.replace(/\|/g, "\\|").replace(/\s+/g, " ").trim(); -/** One line for halt reasons and status rails. */ +/** One line for halt reasons and status rails. "fixed" counts distinct finding + * ids a fix leg marked fixed that are NOT still open — a finding fixed, re- + * raised, and fixed again is one resolved finding, not two. */ export function summarizeLoop(loop: FindingsLoopState, spec: FindingsSpec): string { const open = openFindings(loop); const blocking = blockingOf(open, spec.blocking); - const fixed = loop.rounds.reduce( - (n, r) => n + (r.dispositions?.filter((d) => d.disposition === "fixed").length ?? 0), - 0, - ); + const openIds = new Set(open.map((f) => f.id)); + const fixedIds = new Set(); + for (const r of loop.rounds) { + for (const d of r.dispositions ?? []) if (d.disposition === "fixed" && !openIds.has(d.id)) fixedIds.add(d.id); + } const rounds = `${loop.rounds.length} review round${loop.rounds.length === 1 ? "" : "s"}, ${loop.fixLegs} fix leg${loop.fixLegs === 1 ? "" : "s"}`; - return `${open.length} finding${open.length === 1 ? "" : "s"} open (${blocking.length} blocking), ${fixed} fixed — ${rounds}`; + return `${open.length} finding${open.length === 1 ? "" : "s"} open (${blocking.length} blocking), ${fixedIds.size} fixed — ${rounds}`; } // ── Prompt contracts ───────────────────────────────────────────────────────── @@ -295,11 +303,17 @@ export function findingsContract(spec: FindingsSpec): string { /** Appended to every review leg after a fix leg — the reviewer sees the ledger * and decides what remains open. */ export function rereviewContract(loop: FindingsLoopState, spec: FindingsSpec): string { + // Say what actually happened: a fix leg answered the last round, or none ran + // (audit-only, budget spent, a leg that never committed) and the findings stand. + const last = loop.rounds[loop.rounds.length - 1]; + const intro = last?.dispositions + ? "A writer responded to your previous findings. The ledger so far:" + : "No fix leg ran since your previous findings — they stand as reported. The ledger so far:"; return [ "", "---", `## Review round ${loop.rounds.length + 1} (engine contract)`, - "A writer responded to your previous findings. The ledger so far:", + intro, "", renderLedger(loop, spec), "", @@ -318,8 +332,15 @@ export function rereviewContract(loop: FindingsLoopState, spec: FindingsSpec): s ].join("\n"); } -/** The fix leg's whole brief (appended to the built-in `findings-fix` skill). */ -export function fixContract(round: FindingsRound, spec: FindingsSpec, formatFeedback?: string): string { +/** The fix leg's whole brief (appended to the built-in `findings-fix` skill). + * `humanNotes` are the phase's revise notes — a "fix F3 this way" is for the + * writer, so the fixer sees them too. */ +export function fixContract( + round: FindingsRound, + spec: FindingsSpec, + formatFeedback?: string, + humanNotes?: readonly string[], +): string { const blocking = blockingOf(round.findings, spec.blocking); const rows = round.findings.map( (f) => @@ -349,11 +370,14 @@ export function fixContract(round: FindingsRound, spec: FindingsSpec, formatFeed ' "reason": "required unless fixed", "evidence": "what changed / what ran (optional)" }', "]", "```", + ...(humanNotes && humanNotes.length > 0 + ? ["", "## Notes from the human (revise)", ...humanNotes.map((n, i) => `${i + 1}. ${n}`)] + : []), ...(formatFeedback ? [ "", - `Your previous attempt did not satisfy the contract — ${formatFeedback}. Fix that and`, - "report again; the block must be last, before the completion marker.", + `Engine note on your previous attempt: ${formatFeedback}`, + "Address it and report again; the block must be last, before the completion marker.", ] : []), ].join("\n"); diff --git a/src/daemon/pipeline/manager.ts b/src/daemon/pipeline/manager.ts index 4fe68fbf..734bc7c7 100644 --- a/src/daemon/pipeline/manager.ts +++ b/src/daemon/pipeline/manager.ts @@ -525,6 +525,14 @@ export class PipelineManager { if (p.entryGate && !hasScoped(this.#registries.gates, packId, p.entryGate)) { throw new Error(`phase "${p.id}": ${unknown("entry gate", this.#registries.gates, p.entryGate)}`); } + if (p.findings && !packId) { + // The loop's fix legs run under a PACK role (fixWith) and the loader is + // what checks that role is write-capable; an explicit plan has neither, + // so its `findings` would run reviewer and fixer with no role at all. + throw new Error( + `phase "${p.id}": findings loops require a pack (fixWith names a pack role) — create the run with \`pack\``, + ); + } if (p.findings?.gate && !hasScoped(this.#registries.gates, packId, p.findings.gate)) { throw new Error(`phase "${p.id}": ${unknown("findings fix gate", this.#registries.gates, p.findings.gate)}`); } diff --git a/src/daemon/pipeline/pack.ts b/src/daemon/pipeline/pack.ts index 051f5d97..0e6dff2d 100644 --- a/src/daemon/pipeline/pack.ts +++ b/src/daemon/pipeline/pack.ts @@ -293,6 +293,7 @@ export function loadPack(dir: string, opts: LoadPackOptions = {}): LoadedPack { const skillIds = new Set(skills.map((s) => s.id)); const gates: GatePlugin[] = m.gates.map((g) => buildGate(g, dir, opts.trusted ?? false)); + const gateKinds = new Map(m.gates.map((g) => [g.id, g.kind] as const)); const seen = new Set(); const pipeline: PhaseDef[] = m.phases.map((p) => { @@ -317,7 +318,7 @@ export function loadPack(dir: string, opts: LoadPackOptions = {}): LoadedPack { if (p.skipWhenSatisfied) def.skipWhenSatisfied = true; const onFail = toOnFail(p.onFail); if (onFail) def.onFail = onFail; - if (p.findings) def.findings = toFindingsSpec(m.id, p.id, p.findings, roles); + if (p.findings) def.findings = toFindingsSpec(m.id, p, roles, gateKinds); return def; }); @@ -348,10 +349,12 @@ export function loadPack(dir: string, opts: LoadPackOptions = {}): LoadedPack { * problem this feature exists to remove. */ function toFindingsSpec( packId: string, - phaseId: string, - f: NonNullable, + p: PackManifest["phases"][number], roles: Record, + gateKinds: ReadonlyMap, ): FindingsSpec { + const phaseId = p.id; + const f = p.findings!; const fw = typeof f.fixWith === "string" ? { role: f.fixWith } : f.fixWith; const role = roles[fw.role]; if (!role) { @@ -362,6 +365,25 @@ function toFindingsSpec( `pack "${packId}": phase "${phaseId}" findings.fixWith role "${fw.role}" is read-only (write: false) — the fix leg must run under a write-capable role`, ); } + // Shapes that cannot mean what they say on a findings phase — refused at + // load rather than discovered as a run that skips its review or can never + // be revised out of a halt. + if (p.skipWhenSatisfied) { + throw new Error(`pack "${packId}": phase "${phaseId}" declares findings — it always runs; skipWhenSatisfied is not allowed`); + } + if (p.entryGate && gateKinds.get(p.entryGate) === "review") { + throw new Error( + `pack "${packId}": phase "${phaseId}" entryGate "${p.entryGate}" is a review gate — a review gate is the findings loop's EXIT verdict and cannot ground entry`, + ); + } + if (f.gate) { + const kind = gateKinds.get(f.gate); + if (kind !== undefined && kind !== "command" && kind !== "probe") { + throw new Error( + `pack "${packId}": phase "${phaseId}" findings.gate "${f.gate}" is a ${kind} gate — the fix gate must be deterministic (command or probe)`, + ); + } + } const spec: FindingsSpec = { fixWith: { role: fw.role, diff --git a/src/daemon/session-manager.ts b/src/daemon/session-manager.ts index 88a46edc..75be0c6f 100644 --- a/src/daemon/session-manager.ts +++ b/src/daemon/session-manager.ts @@ -3327,6 +3327,20 @@ mcpHub: this.#mcpHub, // behind a tools-only turn whose own text is empty). Without this, phase N // "completes" instantly by reading phase N-1's marker. let textAtSend = session.lastAssistantText ?? ""; + // Every NEW assistant text this phase produced, in order. The phase's + // summary is the whole of what the model said across its turns, not only + // the last one: a model that writes its report, rests without the marker, + // is nudged, and answers with the bare marker would otherwise hand back an + // empty summary — and a findings phase would lose its findings block. + const produced: string[] = []; + let lastProduced = textAtSend; + const summary = (finalText: string): string => { + if (produced.length === 0) return finalText; + return produced + .map((t) => stripPhaseCompleteMarker(stripNeedInputMarker(t))) + .filter((t) => t.trim().length > 0) + .join("\n\n"); + }; let nudges = 0; let spurious = 0; try { @@ -3344,11 +3358,11 @@ mcpHub: this.#mcpHub, const finalStatus = await done; const text = session.lastAssistantText ?? ""; // A non-idle rest (error / budget-exhausted) is a real phase failure. - if (finalStatus !== "idle") return { finalStatus, text }; + if (finalStatus !== "idle") return { finalStatus, text: summary(text) }; // The user interrupted (Stop) — an interrupt leaves the session idle, // so without this we'd re-drive a nudge over the stop. Hand the partial // to the review boundary instead. - if (session.turnInterrupted) return { finalStatus: "idle", text }; + if (session.turnInterrupted) return { finalStatus: "idle", text: summary(text) }; // The model hasn't COMMITTED a turn in response to our prompt yet — the // last canonical turn is still our own USER prompt/nudge, which is where a // transient query-loop rebuild idle rests BEFORE the real turn runs. Don't @@ -3357,15 +3371,19 @@ mcpHub: this.#mcpHub, // reaches the human boundary. Immune to history-length / user-turn commit // timing — the reason the earlier length watermark missed the rebuild idle. if (session.lastTurnRole !== "assistant") { - if (++spurious > MAX_SPURIOUS_RESTS) return { finalStatus: "idle", text }; + if (++spurious > MAX_SPURIOUS_RESTS) return { finalStatus: "idle", text: summary(text) }; continue; } spurious = 0; + if (text !== lastProduced) { + produced.push(text); + lastProduced = text; + } // Deliverable complete → done, marker stripped. Guarded on NEW text: a // phase can't "complete" by reading the PRIOR phase's marker still sitting // in lastAssistantText (behind a tools-only turn) — only its OWN output. if (text !== textAtSend && isPhaseComplete(text)) { - return { finalStatus: "idle", text: stripPhaseCompleteMarker(text) }; + return { finalStatus: "idle", text: summary(text) }; } // The model needs the user's input. Surface the question as an input // dialog and feed the answer back as the next turn — a REAL answer is a @@ -3380,13 +3398,13 @@ mcpHub: this.#mcpHub, message: stripNeedInputMarker(text), placeholder: "Type your answer…", }); - if (session.turnInterrupted) return { finalStatus: "idle", text }; + if (session.turnInterrupted) return { finalStatus: "idle", text: summary(text) }; if (!resp.cancelled && resp.value && resp.value.trim().length > 0) { pendingSend = resp.value; nudges = 0; continue; } - if (nudges >= MAX_PHASE_NUDGES) return { finalStatus: "idle", text }; + if (nudges >= MAX_PHASE_NUDGES) return { finalStatus: "idle", text: summary(text) }; nudges += 1; pendingSend = PHASE_NO_INPUT_NUDGE; continue; @@ -3394,7 +3412,7 @@ mcpHub: this.#mcpHub, // Rested with new output but no marker (an intermediate pause). Nudge to // continue, bounded; after the cap, hand what it has to the human review // boundary so a never-completing model still reaches Approve/Reject. - if (nudges >= MAX_PHASE_NUDGES) return { finalStatus: "idle", text }; + if (nudges >= MAX_PHASE_NUDGES) return { finalStatus: "idle", text: summary(text) }; nudges += 1; pendingSend = PHASE_CONTINUE_NUDGE; } diff --git a/src/tests/pipeline-runner.test.ts b/src/tests/pipeline-runner.test.ts index 626546ec..51742c73 100644 --- a/src/tests/pipeline-runner.test.ts +++ b/src/tests/pipeline-runner.test.ts @@ -303,6 +303,48 @@ describe("pipeline runtime (real SessionManager + mock backend)", () => { const ph = out.pipeline.phases[0]; expect(ph.status).toBe("halted"); expect(ph.summary).toContain("implemented the feature"); + // The summary is the WHOLE of what the model said across the leg's turns, + // not only the last one — the pre-nudge report is kept too. + expect(ph.summary).toContain("Here's my plan"); + expect(ph.summary ?? "").not.toContain(PHASE_COMPLETE_MARKER); + await m2.drain(3_000); + }); + + test("a report followed by a bare-marker turn after the nudge keeps the report as the summary", async () => { + // The findings loop depends on this: a reviewer writes its report (with the + // ```findings block), rests without the marker, is nudged, and answers with + // nothing but the marker. The block must survive into the phase summary. + const store2 = new Store(join(tmp, "codeoid-bare-marker.db")); + const m2 = new SessionManager(store2, transcript, undefined, undefined, undefined, { + config: mkConfig(join(tmp, "codeoid-bare-marker.db"), true), + _testProviderFactory: () => + new MockSessionProvider("mock", [ + sayTurn('review report\n\n```findings\n[{"id":"F1","severity":"high","title":"nil deref"}]\n```'), + sayTurn(PHASE_COMPLETE_MARKER), + ]), + }); + const pm = m2.pipelines; + expect(pm).toBeDefined(); + if (!pm) return; + pm.registries.skills.register({ id: "impl", kind: "slash", command: "/impl" }); + const created = await m2.handle( + { + type: "pipeline.create", + id: "1", + name: "R", + workdir: join(tmp, "repo"), + phases: [{ id: "impl", kind: "skill", skill: "impl" }], + }, + AUTH, + CLIENT, + ); + if (created.type !== "pipeline.snapshot") throw new Error(`create failed: ${JSON.stringify(created)}`); + const out = await m2.handle({ type: "pipeline.advance", id: "2", pipelineId: created.pipeline.id }, AUTH, CLIENT); + if (out.type !== "pipeline.snapshot") throw new Error(`advance failed: ${JSON.stringify(out)}`); + expect(out.pipeline.status).toBe("halted"); + const ph = out.pipeline.phases[0]; + expect(ph.summary).toContain("```findings"); + expect(ph.summary).toContain('"id":"F1"'); expect(ph.summary ?? "").not.toContain(PHASE_COMPLETE_MARKER); await m2.drain(3_000); }); diff --git a/web/src/components/PipelineRunner.tsx b/web/src/components/PipelineRunner.tsx index 539a0a88..484cb785 100644 --- a/web/src/components/PipelineRunner.tsx +++ b/web/src/components/PipelineRunner.tsx @@ -385,8 +385,20 @@ const HaltCard: Component<{ + {/* A findings-loop halt reason carries a multi-line markdown ledger — + keep its line breaks so the table doesn't collapse into one run-on + line of pipes. */} -

{props.phase.reason}

+

{props.phase.reason}

+ + + + {(f) => ( +

+ findings: {f().open} open ({f().blocking} blocking) · {f().rounds} review round + {f().rounds === 1 ? "" : "s"} · {f().fixLegs} fix leg{f().fixLegs === 1 ? "" : "s"} +

+ )}