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

+ )}