diff --git a/plugins/flow/mcp-server/dist/cli.js b/plugins/flow/mcp-server/dist/cli.js index a195f25f..52547cc2 100644 --- a/plugins/flow/mcp-server/dist/cli.js +++ b/plugins/flow/mcp-server/dist/cli.js @@ -28151,7 +28151,17 @@ var ExecutionManifestSchema = external_exports.object({ // Distinct from 'verdict-failed' (which implies a quality problem) and from // 'reviewer-no-session-result' (file-absent skipped-first-call path). Carries // the FR45 guidance so the operator knows how to resolve the setup issue. - "review-could-not-run" + "review-could-not-run", + // Story native:01KVVJNMHDWZZS366S1BY188CG: the VERDICT step itself could not + // produce a readable result — processReviewerTranscript errored (e.g. an + // unrecognised recommendedVerdict token / non-JSON reviewer-result file → + // ReviewerResultFileMalformedError → MCP isError with no `next`) or its + // courier relay garbled (seam _parseError). A green, finished story whose + // verdict step cannot produce a result is a re-runnable SETUP problem, NOT a + // quality failure — so it is held distinctly from 'verdict-failed' (a quality + // problem) and, mirroring 'review-could-not-run' but naming the VERDICT step, + // distinctly from a review that could not RUN (AC3). + "verdict-could-not-run" ]).optional(), /** * Structured violation list for manifests blocked by `planning-discipline`. diff --git a/plugins/flow/mcp-server/dist/index.js b/plugins/flow/mcp-server/dist/index.js index e801de2a..e3802f09 100644 --- a/plugins/flow/mcp-server/dist/index.js +++ b/plugins/flow/mcp-server/dist/index.js @@ -38130,7 +38130,17 @@ var ExecutionManifestSchema = external_exports.object({ // Distinct from 'verdict-failed' (which implies a quality problem) and from // 'reviewer-no-session-result' (file-absent skipped-first-call path). Carries // the FR45 guidance so the operator knows how to resolve the setup issue. - "review-could-not-run" + "review-could-not-run", + // Story native:01KVVJNMHDWZZS366S1BY188CG: the VERDICT step itself could not + // produce a readable result — processReviewerTranscript errored (e.g. an + // unrecognised recommendedVerdict token / non-JSON reviewer-result file → + // ReviewerResultFileMalformedError → MCP isError with no `next`) or its + // courier relay garbled (seam _parseError). A green, finished story whose + // verdict step cannot produce a result is a re-runnable SETUP problem, NOT a + // quality failure — so it is held distinctly from 'verdict-failed' (a quality + // problem) and, mirroring 'review-could-not-run' but naming the VERDICT step, + // distinctly from a review that could not RUN (AC3). + "verdict-could-not-run" ]).optional(), /** * Structured violation list for manifests blocked by `planning-discipline`. diff --git a/plugins/flow/mcp-server/src/schemas/execution-manifest.ts b/plugins/flow/mcp-server/src/schemas/execution-manifest.ts index 0058e3e7..6a33dd63 100644 --- a/plugins/flow/mcp-server/src/schemas/execution-manifest.ts +++ b/plugins/flow/mcp-server/src/schemas/execution-manifest.ts @@ -283,6 +283,16 @@ export const ExecutionManifestSchema = z // 'reviewer-no-session-result' (file-absent skipped-first-call path). Carries // the FR45 guidance so the operator knows how to resolve the setup issue. "review-could-not-run", + // Story native:01KVVJNMHDWZZS366S1BY188CG: the VERDICT step itself could not + // produce a readable result — processReviewerTranscript errored (e.g. an + // unrecognised recommendedVerdict token / non-JSON reviewer-result file → + // ReviewerResultFileMalformedError → MCP isError with no `next`) or its + // courier relay garbled (seam _parseError). A green, finished story whose + // verdict step cannot produce a result is a re-runnable SETUP problem, NOT a + // quality failure — so it is held distinctly from 'verdict-failed' (a quality + // problem) and, mirroring 'review-could-not-run' but naming the VERDICT step, + // distinctly from a review that could not RUN (AC3). + "verdict-could-not-run", ]) .optional(), diff --git a/plugins/flow/mcp-server/src/tools/__tests__/run-concurrent-dispatch.test.ts b/plugins/flow/mcp-server/src/tools/__tests__/run-concurrent-dispatch.test.ts index 499d3e7f..d3e2d981 100644 --- a/plugins/flow/mcp-server/src/tools/__tests__/run-concurrent-dispatch.test.ts +++ b/plugins/flow/mcp-server/src/tools/__tests__/run-concurrent-dispatch.test.ts @@ -185,7 +185,13 @@ async function runRun(opts: { if (label.startsWith("verdict:")) { const ref = refFromLabel(label, "verdict:"); const o = outcomes[ref]!; - if (o.kind === "verdict-blocked") return { next: "blocked-something" }; + // A GENUINE reviewer block (recommendedVerdict === BLOCKED). The run + // classifies this recognised verdict to the real `reviewer-verdict-blocked` + // quality-block reason (native:01KVVJNMHDWZZS366S1BY188CG split the verdict + // catch-all). Previously this harness used a stand-in `blocked-something`; + // that token is now (correctly) reclassified as a verdict-step setup error, + // so we inject the real verdict value the production code recognises. + if (o.kind === "verdict-blocked") return { next: "done-blocked-reviewer-blocked" }; if (o.kind === "needs-changes-then-merge") { const round = (verdictRound[ref] ??= 0); verdictRound[ref] = round + 1; @@ -360,7 +366,7 @@ describe("run concurrent dispatch (Story 8.22)", () => { // Each blocked entry preserved its reason verbatim. const block4 = n.blocked.find((b: any) => b.ref === "s:4"); const block5 = n.blocked.find((b: any) => b.ref === "s:5"); - expect(block4.blocked_by).toBe("blocked-something"); + expect(block4.blocked_by).toBe("reviewer-verdict-blocked"); expect(block5.blocked_by).toBe("dev-no-handoff"); }); diff --git a/plugins/flow/mcp-server/src/tools/__tests__/run-fault-injection.test.ts b/plugins/flow/mcp-server/src/tools/__tests__/run-fault-injection.test.ts index 3a847883..ceb132ce 100644 --- a/plugins/flow/mcp-server/src/tools/__tests__/run-fault-injection.test.ts +++ b/plugins/flow/mcp-server/src/tools/__tests__/run-fault-injection.test.ts @@ -100,6 +100,14 @@ interface RunOpts { throwSeam?: { ref?: string; prefix: string }; /** Refs that should drive the needs-human-decision path, → their verbatim question. */ needsHuman?: Record; + /** + * Override the `verdict:` seam's `next` value PER REF. Lets a case drive a + * GENUINE reviewer verdict (e.g. `done-blocked-reviewer-blocked`) through the + * real run.workflow.js verdict-classification branch, distinct from a garbled + * relay (which yields a _parseError, not a recognised `next`). Refs absent from + * the map use the default `done-ready-for-merge`. + */ + verdictNext?: Record; /** * What the `claim:` seam returns once the queue is exhausted. Default * `"queue-emptied"` (the happy terminal). Set `"waiting-on-in-progress"` to @@ -205,7 +213,11 @@ async function runRun(opts: RunOpts = {}): Promise<{ reviewerPrompt: "REV-PERSONA", }; } - if (label.startsWith("verdict:")) return { next: "done-ready-for-merge" }; + if (label.startsWith("verdict:")) { + const ref = refOfLabel(label); + const overridden = ref ? (opts.verdictNext ?? {})[ref] : undefined; + return { next: overridden ?? "done-ready-for-merge" }; + } if (label.startsWith("gate:")) { return { decision: "pause-needs-human", reason: "no-agreement-history" }; } @@ -469,6 +481,91 @@ describe("run fault-injection — honest reporting under misbehaving seams (nati assertHonestyInvariant(result, thrown, [REF_A, REF_B]); }); + // ── native:01KVVJNMHDWZZS366S1BY188CG — verdict-step error vs genuine quality block ── + // + // A green, FINISHED story whose VERDICT STEP errors or returns an unreadable / + // unrecognised result must be surfaced as a re-runnable SETUP problem + // (`verdict-could-not-run`), NEVER dumped in the blocked pile as a false + // `verdict-failed`. A GENUINE reviewer block (`done-blocked-reviewer-blocked`) + // must still be held back as a real quality decision (`reviewer-verdict-blocked`), + // exactly as today. These two pin BOTH directions in the real run.workflow.js body. + + it("AC1/AC3: a verdict step that returns an UNREADABLE result on a green story routes to 'verdict-could-not-run' (a re-runnable setup problem) — never 'verdict-failed'", async () => { + // The injected garble feeds the verdict seam a non-JSON relay, so seam() yields + // a _parseError and the workflow's `v` (verdict.next) is undefined — exactly the + // shape processReviewerTranscript produces when readReviewerResultFile throws + // ReviewerResultFileMalformedError (an unrecognised recommendedVerdict token or a + // non-JSON reviewer-result file) and surfaces as an MCP isError with no `next`. + const queue = [ + { ref: REF_A, title: "Story A (green; verdict step returns unreadable result)" }, + { ref: REF_B, title: "Story B (clean sibling)" }, + ]; + const { result, thrown } = await runRun({ + queue, + maxConcurrency: 2, + garble: { ref: REF_A, prefix: "verdict:" }, + }); + + expect(thrown).toBeUndefined(); + + // A is surfaced as a re-runnable SETUP problem naming the VERDICT step — NOT the + // false-failure 'verdict-failed', and distinct from 'review-could-not-run' (AC3). + const a = result.blocked.find((b: any) => b.ref === REF_A); + expect(a, "story A should be surfaced as a setup problem").toBeDefined(); + expect(a.blocked_by).toBe("verdict-could-not-run"); + expect(a.blocked_by).not.toBe("verdict-failed"); + expect(a.blocked_by).not.toBe("review-could-not-run"); + // It never reached the green-verdict completion / merge path. + expect(result.completed).not.toContain(REF_A); + expect(result.merged.some((m: any) => m.ref === REF_A)).toBe(false); + + // The blockStory give-up move stamped the SAME setup reason on the manifest move + // (the block-story seam fires with that label/reason), confirming the manifest is + // pulled out of in-progress/ as a setup problem, not silently swallowed. + // (A green sibling still finishes normally below.) + expect(result.pausedForHuman.some((p: any) => p.ref === REF_A)).toBe(false); + + // The run kept going and finished the OTHER story cleanly. + expect(result.completed).toContain(REF_B); + expect(result.pausedForHuman.some((p: any) => p.ref === REF_B)).toBe(true); + + expect(result.runReason).toBe("queue-emptied"); + assertHonestyInvariant(result, thrown, [REF_A, REF_B]); + }); + + it("AC2: a GENUINE reviewer BLOCK verdict still lands as the real quality decision 'reviewer-verdict-blocked' (setup-problem handling does NOT apply)", async () => { + // The verdict seam returns a recognised `done-blocked-reviewer-blocked` — a real + // quality verdict the reviewer made (recommendedVerdict === BLOCKED). This must be + // held back EXACTLY as today; the new verdict-could-not-run handling applies ONLY + // when NO real verdict could be produced. + const queue = [ + { ref: REF_A, title: "Story A (reviewer genuinely BLOCKS)" }, + { ref: REF_B, title: "Story B (clean sibling)" }, + ]; + const { result, thrown } = await runRun({ + queue, + maxConcurrency: 2, + verdictNext: { [REF_A]: "done-blocked-reviewer-blocked" }, + }); + + expect(thrown).toBeUndefined(); + + // A is held back as a REAL quality block — its existing reason, NOT the new + // setup-problem reason. + const a = result.blocked.find((b: any) => b.ref === REF_A); + expect(a, "story A should be held back as a quality block").toBeDefined(); + expect(a.blocked_by).toBe("reviewer-verdict-blocked"); + expect(a.blocked_by).not.toBe("verdict-could-not-run"); + expect(result.completed).not.toContain(REF_A); + expect(result.merged.some((m: any) => m.ref === REF_A)).toBe(false); + + // The sibling still finishes normally. + expect(result.completed).toContain(REF_B); + + expect(result.runReason).toBe("queue-emptied"); + assertHonestyInvariant(result, thrown, [REF_A, REF_B]); + }); + it("AC2: a garbled GATE relay (after a green verdict) buckets that story in BLOCKED — a story whose merge gate cannot be confirmed never reaches done/, run keeps going", async () => { // fix/run-isolation-coordination-honesty: a garbled gate relay means the // gate decision (incl. the CI-green check) could NOT be confirmed. Under the diff --git a/plugins/flow/workflows/internal/run.workflow.js b/plugins/flow/workflows/internal/run.workflow.js index 0f376ddb..d50f20ed 100644 --- a/plugins/flow/workflows/internal/run.workflow.js +++ b/plugins/flow/workflows/internal/run.workflow.js @@ -714,8 +714,26 @@ async function processStory({ ref, title, manifestPath, resumeAtReview = false, await blockStoryGiveUp(ref, 'review-could-not-run') return } - blocked.push({ ref, blocked_by: v || verdict?._parseError || 'verdict-failed' }) - await blockStoryGiveUp(ref, v === 'done-blocked-reviewer-blocked' ? 'reviewer-verdict-blocked' : 'verdict-failed') + // A GENUINE quality block from the reviewer (recommendedVerdict === BLOCKED) — + // still held back as a real quality decision (AC2, unchanged behaviour). + if (v === 'done-blocked-reviewer-blocked') { + blocked.push({ ref, blocked_by: 'reviewer-verdict-blocked' }) + await blockStoryGiveUp(ref, 'reviewer-verdict-blocked') + return + } + // No RECOGNISED verdict reached this point: the verdict STEP itself errored or + // returned an unreadable/unrecognised result — e.g. processReviewerTranscript + // threw ReviewerResultFileMalformedError (an unrecognised recommendedVerdict + // token or non-JSON file) and surfaced as an MCP isError response with no + // `next`, or the courier relay garbled and seam() returned a _parseError. That + // is a SETUP problem, NOT a quality verdict: a green, finished story must NEVER + // be dumped into the blocked pile as 'verdict-failed' for the operator to rescue + // by hand (the false-failure this story fixes). Route it to a distinct, + // re-runnable setup-error outcome whose reason names the VERDICT step, so it + // reads distinctly from 'review-could-not-run' (a review that could not RUN) — + // satisfying AC3's requirement to tell the two apart. + blocked.push({ ref, blocked_by: 'verdict-could-not-run' }) + await blockStoryGiveUp(ref, 'verdict-could-not-run') return }