From 3167dbc839b78c5c5d4ec0c07dea347929510907 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 1 Jul 2026 22:31:27 -0700 Subject: [PATCH] fix: lenient review-verdict parsing + clear stale gate failures on retry Reviews no longer fail on formatting. Three changes to how reviewer/gate verdicts are parsed and how retries reset state: - Approval leniency: a review that clearly approves in prose passes even without a structured verdict (proseSignalsClearApproval, with a revise/reject/negated-approval guard so a rejection is never flipped). Any APPROVE*/APPROVAL verdict token classifies as approved. Shared by the reviewer/plan-review parser and the code-review/browser-verification gate. - Prose + trailing JSON: extractJsonObjectCandidates does a string-aware balanced-brace scan and prefers the last object, so a model that emits reasoning prose then a trailing {"verdict":...} payload parses correctly. An explicit "Verdict:" heading/line still takes precedence over an incidental/example JSON object. - Malformed handling: executeWorkflowStep retries the fallback model on malformed output (not just timeout); malformed gate output becomes a non-blocking advisory (a genuine parsed REVISE still blocks). - Retry clears prior terminal step failures (incl. optional gate nodes like code-review) after the task leaves the mergeable in-review column, so a retry starts clean without an auto-merge race. Fail-closed merge / PR-review / mission-verification gates are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/lenient-review-verdict-parsing.md | 7 + ...ar-terminal-workflow-step-failures.test.ts | 59 ++++++++ .../workflow-malformed-verdict-gate.test.ts | 25 ++-- .../workflow-step-verdict-parsing.test.ts | 141 ++++++++++++++++++ packages/engine/src/executor.ts | 109 +++++++++++--- packages/engine/src/reviewer.ts | 141 +++++++++++++++--- 6 files changed, 434 insertions(+), 48 deletions(-) create mode 100644 .changeset/lenient-review-verdict-parsing.md create mode 100644 packages/engine/src/__tests__/clear-terminal-workflow-step-failures.test.ts diff --git a/.changeset/lenient-review-verdict-parsing.md b/.changeset/lenient-review-verdict-parsing.md new file mode 100644 index 0000000000..e24071bcae --- /dev/null +++ b/.changeset/lenient-review-verdict-parsing.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Reviews stop failing on formatting: approvals pass, trailing-JSON verdicts parse, retries clear stale gate failures. +category: fix +dev: reviewer.ts adds shared `proseSignalsClearApproval` (approval prose with a revise/negated-approval guard), `extractJsonObjectCandidates` (string-aware balanced-brace scan, last-object preferred for prose→trailing-JSON), and `classifyReviewVerdictToken` (any APPROVE*/APPROVAL token → APPROVE). `extractVerdict` now prefers an explicit heading/line verdict over an incidental/example JSON object. Gate parser (`parseWorkflowStepVerdict`/`inferWorkflowStepVerdictFromProse`) shares the same logic. `executeWorkflowStep` retries the fallback model on malformed (not just timeout) and malformed gate output is a non-blocking advisory (relaxes FN-6582; genuine parsed REVISE still blocks). Retry paths clear prior terminal step failures (`clearTerminalWorkflowStepFailures`) only after the task leaves the mergeable in-review column (`clearTerminalStepFailuresForRetry` in the rerun bounce / resume path) to avoid an auto-merge race. Fail-closed merge/PR/mission-verification gates are unchanged. diff --git a/packages/engine/src/__tests__/clear-terminal-workflow-step-failures.test.ts b/packages/engine/src/__tests__/clear-terminal-workflow-step-failures.test.ts new file mode 100644 index 0000000000..08867b7a80 --- /dev/null +++ b/packages/engine/src/__tests__/clear-terminal-workflow-step-failures.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it } from "vitest"; +import type { WorkflowStepResult } from "@fusion/core"; +import { clearTerminalWorkflowStepFailures } from "../executor.js"; + +/* +FNXC:ReviewLeniency 2026-07-02-01:00: +Retrying a task must clear prior FAILURE states — including optional gate nodes +like code-review — while keeping passed/skipped/pending evidence so a +previously-passed Plan Review is not re-run. These pin the pure helper wired into +sendTaskBackForFix and routeGraphFailureToExecutionResume. +*/ + +function result(overrides: Partial & Pick): WorkflowStepResult { + return { + workflowStepName: overrides.workflowStepId, + phase: "pre-merge", + ...overrides, + } as WorkflowStepResult; +} + +describe("clearTerminalWorkflowStepFailures", () => { + it("drops failed and advisory_failure results (incl. optional gate nodes)", () => { + const input = [ + result({ workflowStepId: "plan-review", status: "passed" }), + result({ workflowStepId: "code-review", status: "failed" }), + result({ workflowStepId: "browser-verification", status: "advisory_failure" }), + ]; + expect(clearTerminalWorkflowStepFailures(input)).toEqual([ + result({ workflowStepId: "plan-review", status: "passed" }), + ]); + }); + + it("keeps passed / skipped / pending evidence untouched", () => { + const input = [ + result({ workflowStepId: "plan-review", status: "passed" }), + result({ workflowStepId: "code-review", status: "skipped" }), + result({ workflowStepId: "browser-verification", status: "pending" }), + ]; + expect(clearTerminalWorkflowStepFailures(input)).toEqual(input); + }); + + it("returns the SAME array reference when nothing was terminal (no-op write guard)", () => { + const input = [result({ workflowStepId: "plan-review", status: "passed" })]; + expect(clearTerminalWorkflowStepFailures(input)).toBe(input); + }); + + it("returns a new array when at least one failure is dropped", () => { + const input = [ + result({ workflowStepId: "plan-review", status: "passed" }), + result({ workflowStepId: "code-review", status: "failed" }), + ]; + expect(clearTerminalWorkflowStepFailures(input)).not.toBe(input); + }); + + it("handles undefined/empty input", () => { + expect(clearTerminalWorkflowStepFailures(undefined)).toEqual([]); + expect(clearTerminalWorkflowStepFailures([])).toEqual([]); + }); +}); diff --git a/packages/engine/src/__tests__/workflow-malformed-verdict-gate.test.ts b/packages/engine/src/__tests__/workflow-malformed-verdict-gate.test.ts index 85035cc6cf..d415174235 100644 --- a/packages/engine/src/__tests__/workflow-malformed-verdict-gate.test.ts +++ b/packages/engine/src/__tests__/workflow-malformed-verdict-gate.test.ts @@ -8,6 +8,9 @@ import { WorkflowGraphExecutor } from "../workflow-graph-executor.js"; /* FNXC:WorkflowGates 2026-06-17-18:27: FN-6582 requires malformed workflow-step verdicts to remain explicit failures for blocking gates while advisory gates may record a non-blocking advisory failure. These tests pin the shared imperative parser seam and the graph handler path so malformed output cannot be mistaken for APPROVE. + +FNXC:ReviewLeniency 2026-07-02-00:30: +POLICY CHANGE (operator request): malformed gate output (no parseable verdict, even after the executeWorkflowStep fallback-model retry) is now treated as a NON-BLOCKING advisory, relaxing the FN-6582 hard block. The real mapping lives in runGraphCustomNode (`outcome: success || !blocking || malformed ? "success" : "failure"`). A genuine PARSED non-pass verdict (REVISE) still blocks. The parser seam still classifies unparseable text as `malformed` (it is NOT silently promoted to APPROVE) — only the downstream blocking decision was relaxed. These handler/executor tests mock the node result to pin the graph PLUMBING for a genuine-failure verdict; they intentionally do not re-assert a malformed→block mapping that no longer exists. */ const task = { id: "FN-6582" } as TaskDetail; @@ -47,12 +50,13 @@ describe("workflow malformed-verdict gate", () => { expect(parseWorkflowStepOutput("native skill output", { requireVerdict: false })).toEqual({ output: "native skill output" }); }); - it("keeps a malformed blocking graph gate from producing a passing outcome", async () => { - const malformed = parseWorkflowStepOutput("lorem ipsum"); + it("keeps a blocking graph gate with a genuine REVISE verdict from passing", async () => { + // A PARSED non-pass verdict still blocks (only unparseable/malformed output + // was relaxed to a non-blocking advisory — see the ReviewLeniency note above). + const revise = parseWorkflowStepOutput("REQUEST REVISION\nfix the gate"); const runCustomNode = vi.fn(async () => ({ - outcome: malformed.malformed ? "failure" as const : "success" as const, - value: malformed.malformed ? "malformed" : malformed.verdict, - contextPatch: malformed.malformed ? { "workflow:gate:malformed": true } : undefined, + outcome: revise.verdict === "REVISE" ? "failure" as const : "success" as const, + value: revise.verdict, })); const handlers = createDefaultNodeHandlers(noopSeams(), runCustomNode); @@ -62,8 +66,7 @@ describe("workflow malformed-verdict gate", () => { ); expect(result.outcome).toBe("failure"); - expect(result.value).toBe("malformed"); - expect(result.contextPatch).toEqual({ "workflow:gate:malformed": true }); + expect(result.value).toBe("REVISE"); expect(runCustomNode).toHaveBeenCalledOnce(); }); @@ -85,12 +88,12 @@ describe("workflow malformed-verdict gate", () => { expect(result.contextPatch).toEqual({ "workflow:gate:malformed": true, "workflow:gate:advisory": true }); }); - it("terminates a graph run as failed when a malformed gate routes to failure", async () => { - const malformed = parseWorkflowStepOutput("lorem ipsum"); + it("terminates a graph run as failed when a blocking gate returns REVISE", async () => { + const revise = parseWorkflowStepOutput("REQUEST REVISION\nfix the gate"); const executor = new WorkflowGraphExecutor({ handlers: createDefaultNodeHandlers(noopSeams(), async () => ({ - outcome: malformed.malformed ? "failure" : "success", - value: malformed.malformed ? "malformed" : "APPROVE", + outcome: revise.verdict === "REVISE" ? "failure" : "success", + value: revise.verdict === "REVISE" ? "REVISE" : "APPROVE", })), runCustomNode: async () => ({ outcome: "success" }), }); diff --git a/packages/engine/src/__tests__/workflow-step-verdict-parsing.test.ts b/packages/engine/src/__tests__/workflow-step-verdict-parsing.test.ts index a0e06f1bca..67e2204af6 100644 --- a/packages/engine/src/__tests__/workflow-step-verdict-parsing.test.ts +++ b/packages/engine/src/__tests__/workflow-step-verdict-parsing.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from "vitest"; import { inferWorkflowStepVerdictFromProse, parseWorkflowStepVerdict } from "../executor.js"; +import { proseSignalsClearApproval, extractJsonObjectCandidates, classifyReviewVerdictToken } from "../reviewer.js"; describe("parseWorkflowStepVerdict", () => { it("parses plain JSON", () => { @@ -17,6 +18,40 @@ describe("parseWorkflowStepVerdict", () => { it("returns null for invalid verdict", () => { expect(parseWorkflowStepVerdict('{"verdict":"PASS"}')).toBeNull(); }); + + /* + FNXC:ReviewLeniency 2026-07-01-23:30: + Models often emit reasoning PROSE (sometimes containing braces) then a trailing + JSON verdict payload. The trailing payload must be extracted and preferred. + */ + it("extracts a trailing JSON payload after prose", () => { + const out = "I reviewed the diff and it meets the criteria.\n\n" + + '{"verdict":"APPROVE","notes":"clean"}'; + expect(parseWorkflowStepVerdict(out)).toEqual({ verdict: "APPROVE", notes: "clean" }); + }); + + it("extracts trailing JSON even when the prose itself contains braces", () => { + const out = "The change touches `render({ x: 1 })` and looks correct.\n" + + '{"verdict":"REVISE","notes":"tighten the type"}'; + expect(parseWorkflowStepVerdict(out)).toEqual({ verdict: "REVISE", notes: "tighten the type" }); + }); + + it("prefers the LAST JSON object when several appear", () => { + const out = 'Example format: {"verdict":"REVISE"}. My actual verdict follows.\n' + + '{"verdict":"APPROVE","notes":"ok"}'; + expect(parseWorkflowStepVerdict(out)).toEqual({ verdict: "APPROVE", notes: "ok" }); + }); + + // "Any approved" — approval-family verdict tokens all map to an approve pass. + it.each([ + ['{"verdict":"APPROVED"}', "APPROVE"], + ['{"verdict":"approve_with_verdict"}', "APPROVE"], + ['{"verdict":"APPROVE_WITH_NOTES","notes":"minor"}', "APPROVE_WITH_NOTES"], + ['{"verdict":"Approval"}', "APPROVE"], + ['{"verdict":"REJECT"}', "REVISE"], + ] as const)("classifies approval/revise family token %s", (input, expected) => { + expect(parseWorkflowStepVerdict(input)?.verdict).toBe(expected); + }); }); describe("inferWorkflowStepVerdictFromProse", () => { @@ -46,4 +81,110 @@ describe("inferWorkflowStepVerdictFromProse", () => { it("returns null for unrelated prose", () => { expect(inferWorkflowStepVerdictFromProse("lorem ipsum")).toBeNull(); }); + + /* + FNXC:ReviewLeniency 2026-07-01-22:15: + A review whose text clearly approves must pass even when not perfectly structured. + These broadened phrasings previously fell through to malformed → blocking gate. + */ + it.each([ + "Approving — nice work.", + "LGTM", + "ship it", + "All good, no blocking issues.", + "This is acceptable.", + "Good to merge.", + "Passes review.", + ])("infers approve from broadened approval phrasing: %s", (text) => { + expect(inferWorkflowStepVerdictFromProse(text)).toEqual({ verdict: "APPROVE", notes: "" }); + }); + + // Negation guard: a prose rejection must NOT be promoted to APPROVE. + it.each([ + "I do not approve this; please revise.", + "We can't approve — needs changes.", + "Rejecting this change.", + "Not approved.", + "Please revise the plan.", + ])("does not infer approve from a prose rejection: %s", (text) => { + expect(inferWorkflowStepVerdictFromProse(text)).toBeNull(); + }); +}); + +describe("proseSignalsClearApproval", () => { + it.each([ + "approve", + "approved", + "approving the work", + "LGTM", + "ship it", + "no blocking issues", + "no concerns", + "all good", + "acceptable", + "good to go", + "looks good", + ])("returns true for a clear approval: %s", (text) => { + expect(proseSignalsClearApproval(text)).toBe(true); + }); + + it.each([ + "", + "lorem ipsum", + "not approved", + "cannot approve this", + "do not approve", + "please revise", + "REVISE", + "reject", + "disapprove", + "needs revision before approval", + "The build passes.", + "This passes the unit tests but I want changes to the API.", + // Praise + change-request: an approval token is present but the review still + // requests changes, so it must NOT be promoted to APPROVE. + "The memory leak is out of scope for this PR, but we should still address the null check before merging.", + "I have no objections to the direction, but the race condition must be fixed.", + "The performance is acceptable. However, the API breaks compatibility and I want that changed.", + "It passes review of the happy path. That said, please fix the error-handling gap.", + ])("returns false for non-approval / rejection: %s", (text) => { + expect(proseSignalsClearApproval(text)).toBe(false); + }); +}); + +describe("extractJsonObjectCandidates", () => { + it("returns balanced top-level objects in document order", () => { + expect(extractJsonObjectCandidates('a {"x":1} b {"y":2} c')).toEqual(['{"x":1}', '{"y":2}']); + }); + + it("ignores braces inside string values", () => { + expect(extractJsonObjectCandidates('{"notes":"has } and { braces"}')).toEqual([ + '{"notes":"has } and { braces"}', + ]); + }); + + it("captures a nested object as one top-level candidate", () => { + expect(extractJsonObjectCandidates('prose {"a":{"b":2}} tail')).toEqual(['{"a":{"b":2}}']); + }); +}); + +describe("classifyReviewVerdictToken", () => { + it.each([ + ["APPROVE", "APPROVE"], + ["APPROVED", "APPROVE"], + ["APPROVE_WITH_NOTES", "APPROVE"], + ["approve_with_verdict", "APPROVE"], + ["Approval", "APPROVE"], + ["REVISE", "REVISE"], + ["REQUEST_REVISION", "REVISE"], + ["REJECT", "REVISE"], + ["RETHINK", "RETHINK"], + ] as const)("classifies %s", (token, expected) => { + expect(classifyReviewVerdictToken(token)).toBe(expected); + }); + + it("returns null for unknown tokens", () => { + expect(classifyReviewVerdictToken("PASS")).toBeNull(); + expect(classifyReviewVerdictToken("")).toBeNull(); + }); }); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 48af427225..bba6231f42 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -86,7 +86,7 @@ import { import { buildSessionSkillContext } from "./session-skill-context.js"; import type { SkillSelectionContext } from "./skill-resolver.js"; import { resolveMcpServersForStore } from "./mcp-resolution.js"; -import { reviewStep, type ReviewVerdict, type ReviewResult } from "./reviewer.js"; +import { reviewStep, proseSignalsClearApproval, extractJsonObjectCandidates, type ReviewVerdict, type ReviewResult } from "./reviewer.js"; import { buildUserCommentsPromptSection, selectUserCommentsForAgentContext } from "./agent-user-comments.js"; import { resolveSandboxBackend } from "./sandbox/index.js"; import type { SandboxBackend } from "./sandbox/types.js"; @@ -1139,8 +1139,6 @@ export type WorkflowStepResult = | { allPassed: false; revisionRequested: false; feedback: string; stepName: string } | { allPassed: false; revisionRequested: true; feedback: string; stepName: string }; -const WORKFLOW_STEP_VERDICTS = new Set(["APPROVE", "APPROVE_WITH_NOTES", "REVISE"] as const); - export function parseWorkflowStepVerdict(rawOutput: string): { verdict: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE"; notes: string } | null { const trimmed = rawOutput.trim(); const candidates: string[] = []; @@ -1148,19 +1146,30 @@ export function parseWorkflowStepVerdict(rawOutput: string): { verdict: "APPROVE for (const match of fencedMatches) { candidates.push(match[1].trim()); } - const jsonObjectMatches = trimmed.match(/\{[\s\S]*\}/g); - if (jsonObjectMatches) { - candidates.push(...jsonObjectMatches.map((value) => value.trim())); - } + /* + FNXC:ReviewLeniency 2026-07-01-23:30: + Prefer a balanced, string-aware object scan over a greedy `\{[\s\S]*\}` match: models that emit reasoning PROSE (which may itself contain braces) followed by a trailing `{"verdict":...}` payload broke the greedy span into invalid JSON. extractJsonObjectCandidates returns each top-level object in document order; iterating last→first prefers the trailing verdict payload. + */ + candidates.push(...extractJsonObjectCandidates(trimmed)); for (let i = candidates.length - 1; i >= 0; i -= 1) { try { - const parsed = JSON.parse(candidates[i]) as { verdict?: string; notes?: unknown }; - if (!parsed || typeof parsed.verdict !== "string" || !WORKFLOW_STEP_VERDICTS.has(parsed.verdict as "APPROVE")) { - continue; + const parsed = JSON.parse(candidates[i]) as { verdict?: unknown; notes?: unknown }; + if (!parsed || typeof parsed.verdict !== "string") continue; + /* + FNXC:ReviewLeniency 2026-07-01-23:30: + "Any approved" — accept approval-family verdict variants (APPROVE, APPROVED, APPROVE_WITH_NOTES, approve_with_verdict, …), not just the exact WORKFLOW_STEP_VERDICTS strings. A token starting with APPROVE maps to APPROVE_WITH_NOTES when it mentions notes, else APPROVE; REVISE-family → REVISE; anything else (e.g. "PASS") is not a verdict and the candidate is skipped. + */ + const token = parsed.verdict.trim().toUpperCase(); + let verdict: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE" | null = null; + if (token.startsWith("APPROVE") || token.startsWith("APPROVAL")) { + verdict = token.includes("NOTE") ? "APPROVE_WITH_NOTES" : "APPROVE"; + } else if (token.startsWith("REVISE") || token.startsWith("REQUEST_REVISION") || token.startsWith("REJECT")) { + verdict = "REVISE"; } + if (!verdict) continue; return { - verdict: parsed.verdict as "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE", + verdict, notes: typeof parsed.notes === "string" ? parsed.notes : "", }; } catch { @@ -1191,7 +1200,11 @@ export function inferWorkflowStepVerdictFromProse(rawOutput: string): { verdict: notes: "", }; } - if (/\b(approve|approved|looks good|no issues|out of scope)\b/i.test(trimmed)) { + /* + FNXC:ReviewLeniency 2026-07-01-22:15: + A gate review (code-review, browser-verification) whose text clearly approves must PASS even when it is not perfectly structured. Delegate to the shared proseSignalsClearApproval detector so this parser and the reviewer/plan-review parser agree on what "clearly approved" means, and so a prose rejection ("not approved", "please revise", "reject") is never promoted to APPROVE. Replaces the prior narrow approve/approved/looks good/no issues/out of scope regex (now a subset of the shared detector). + */ + if (proseSignalsClearApproval(trimmed)) { return { verdict: "APPROVE", notes: "" }; } return null; @@ -3427,6 +3440,19 @@ export class TaskExecutor { * this as a successful retry, since the original bounce may itself be * stuck. */ + /* + FNXC:ReviewLeniency 2026-07-02-02:10: + Clear prior terminal failure results (failed/advisory_failure — incl. optional gate nodes like code-review) so a retry starts clean. Call this ONLY once the task has left the mergeable in-review column (i.e. it is in `todo`): clearing while still in-review drops the merge blocker during the rerun-bounce window and could let a concurrent auto-merge sweep merge an empty-`steps` graph-native task with its gate failure unaddressed. `moveTask(in-review→todo)` already clears ALL results (applyReopenFieldClears), so this is chiefly for the in-progress→todo bounce path where the move does not. Passed/skipped/pending evidence is kept. + */ + private async clearTerminalStepFailuresForRetry(taskId: string): Promise { + const live = await this.store.getTask(taskId).catch(() => null); + if (!live) return; + const cleared = clearTerminalWorkflowStepFailures(live.workflowStepResults); + if (cleared !== live.workflowStepResults) { + await this.store.updateTask(taskId, { workflowStepResults: cleared }, this.getRunContextFor(taskId)); + } + } + private async performWorkflowRerunBounce( taskId: string, worktreePath: string, @@ -3508,6 +3534,8 @@ export class TaskExecutor { executorLog.log(`${taskId}: workflow rerun parked in todo — ${pauseLabelAfterTodo} became active during bounce`); return "deferred-paused"; } + // Now in `todo` (non-mergeable) — safe to clear prior gate failures. + await this.clearTerminalStepFailuresForRetry(taskId); await this.store.moveTask(taskId, "in-progress"); return "bounced"; } @@ -3519,6 +3547,8 @@ export class TaskExecutor { executorLog.log(`${taskId}: workflow rerun parked in todo — ${pauseLabelBeforeResume} became active before resume`); return "deferred-paused"; } + // Already in `todo` (non-mergeable) — safe to clear prior gate failures. + await this.clearTerminalStepFailuresForRetry(taskId); await this.store.moveTask(taskId, "in-progress"); return "bounced"; } @@ -7436,9 +7466,14 @@ export class TaskExecutor { * recovery synthesize a Plan Review REVISE even when no reviewer requested * one; `advisory_failure` preserves visibility without inventing feedback. */ - const advisoryFailureValue = (outcome as { malformed?: boolean }).malformed ? "advisory_failure" : "failed"; + const malformed = (outcome as { malformed?: boolean }).malformed === true; + const advisoryFailureValue = malformed ? "advisory_failure" : "failed"; + /* + FNXC:ReviewLeniency 2026-07-02-00:30: + Malformed review output (no parseable verdict, even after the fallback-model retry in executeWorkflowStep) is treated as a NON-BLOCKING advisory rather than a hard gate failure. Operators asked that an unparseable reviewer response not block a task in review — a genuine REVISE (parsed verdict) still blocks, and the advisory_failure value keeps the malformed result visible on the Workflow tab. Only `malformed` relaxes a gate; every parsed non-pass verdict continues to block exactly as before. + */ return { - outcome: outcome.success || !blocking ? "success" : "failure", + outcome: outcome.success || !blocking || malformed ? "success" : "failure", value: verdict ?? (outcome.success ? "passed" : advisoryFailureValue), ...(Object.keys(contextPatch).length > 0 ? { contextPatch } : {}), }; @@ -8561,6 +8596,12 @@ export class TaskExecutor { recoveryRehome: true, }); } + // FNXC:ReviewLeniency 2026-07-02-02:10: clear prior terminal failure results + // (incl. optional gate nodes like code-review) AFTER the task is in `todo` + // (non-mergeable) so the resumed run re-evaluates gates from a clean slate + // without dropping the in-review merge blocker mid-flight. (in-review→todo + // moveTask already clears all results; this covers the already-`todo` path.) + await this.clearTerminalStepFailuresForRetry(live.id); await this.persistTokenUsage(live.id); return true; } @@ -13704,7 +13745,14 @@ ${failureContext.output.slice(0, VERIFICATION_LOG_MAX_CHARS)} const updatedTask = await this.store.getTask(taskId); await this.reopenLastStepForRevision(taskId, updatedTask); - // 5. Clear error/status/session fields and reset workflow step retries + // 5. Clear error/status/session fields and reset workflow step retries. + // FNXC:ReviewLeniency 2026-07-02-02:10: prior terminal failure results + // (incl. optional gate nodes like code-review) are cleared by the rerun + // bounce AFTER the task leaves the mergeable in-review column (see + // clearTerminalStepFailuresForRetry), NOT here — clearing them while the + // task is still in-review would drop the merge blocker during the async + // bounce window and let a concurrent auto-merge sweep merge an + // empty-`steps` graph-native task with the gate failure unaddressed. await this.store.updateTask(taskId, { status: mergeVerificationFailure ? "merging-fix" : null, error: null, @@ -14750,9 +14798,12 @@ You have access to the file system to review changes.${inlineFixBlock}${verdictB } if (parsed.malformed) { + // FNXC:ReviewLeniency 2026-07-02-00:30: malformed output (after the + // fallback-model retry) is recorded as a NON-BLOCKING advisory, not a + // hard gate block — see runGraphCustomNode's outcome mapping. await this.store.logEntry( task.id, - `[pre-merge] Workflow step '${workflowStep.name}' produced malformed output — blocking gate success`, + `[pre-merge] Workflow step '${workflowStep.name}' produced malformed output (no parseable verdict) — recorded as non-blocking advisory`, ); if (workflowStep.requiresBrowser === true) { await logBrowserVerificationActivity(`[browser-verification] finished browser verification for task ${task.id}: malformed output`); @@ -14802,18 +14853,24 @@ You have access to the file system to review changes.${inlineFixBlock}${verdictB }; const primaryOutcome = await runOnce(primaryProvider, primaryModelId, "primary"); - if (!primaryOutcome.timedOut) return primaryOutcome; + /* + FNXC:ReviewLeniency 2026-07-02-00:30: + Retry the fallback model on a MALFORMED (unparseable-verdict) primary response, not only on a timeout. A single fumbled response — reasoning with no trailing verdict — should get one more attempt on the fallback model before the gate result is recorded, mirroring the reviewer path's UNAVAILABLE retry. If no fallback is configured the malformed primary is returned as-is (and is treated as a non-blocking advisory downstream, see runGraphCustomNode). + */ + const primaryMalformed = (primaryOutcome as { malformed?: boolean }).malformed === true; + if (!primaryOutcome.timedOut && !primaryMalformed) return primaryOutcome; if (!fallback) { - executorLog.warn(`${task.id}: workflow step '${workflowStep.name}' timed out and no fallback model is configured`); + const reason = primaryOutcome.timedOut ? "timed out" : "produced malformed output"; + executorLog.warn(`${task.id}: workflow step '${workflowStep.name}' ${reason} and no fallback model is configured`); await this.store.logEntry( task.id, - `Workflow step '${workflowStep.name}' timed out — no fallback model configured (set settings.validatorFallbackProvider/Id or fallbackProvider/Id)`, + `Workflow step '${workflowStep.name}' ${reason} — no fallback model configured (set settings.validatorFallbackProvider/Id or fallbackProvider/Id)`, ); return primaryOutcome; } - executorLog.log(`${task.id}: retrying workflow step '${workflowStep.name}' with fallback ${fallback.provider}/${fallback.modelId} (label=${fallback.label})`); + executorLog.log(`${task.id}: retrying workflow step '${workflowStep.name}' with fallback ${fallback.provider}/${fallback.modelId} (label=${fallback.label}) after primary ${primaryOutcome.timedOut ? "timeout" : "malformed output"}`); return runOnce(fallback.provider, fallback.modelId, "fallback"); } @@ -17830,6 +17887,18 @@ function hasNonTerminalWorkflowSteps(task: Pick): boolean { return task.steps.length > 0 && task.steps.some((step) => step.status !== "done" && step.status !== "skipped"); } +/* +FNXC:ReviewLeniency 2026-07-02-01:00: +Retrying a task must clear PRIOR FAILURE states so the retry starts clean — including on optional gate nodes like code-review / browser-verification. Results are upserted by node id, so a re-running node overwrites its own stale entry, but a send-back-for-fix leaves the failed entry in place until (and unless) that node re-runs; meanwhile self-healing's failed-pre-merge scan and the dashboard both see a stale failure, and a node that is skipped/relaxed on the retry never clears it. Drop every terminal failure result (`failed`/`advisory_failure`) on retry while keeping `passed`/`skipped`/`pending` evidence (so a previously-passed Plan Review is not re-run). Returns the same array reference when nothing changed so callers can skip a no-op write. +*/ +export function clearTerminalWorkflowStepFailures( + results: CoreWorkflowStepResult[] | undefined, +): CoreWorkflowStepResult[] { + const current = results ?? []; + const kept = current.filter((result) => result.status !== "failed" && result.status !== "advisory_failure"); + return kept.length === current.length ? current : kept; +} + function workflowStepResultPassed(task: Pick | undefined, workflowStepId: string): boolean { const results = task?.workflowStepResults ?? []; return results.some((result) => diff --git a/packages/engine/src/reviewer.ts b/packages/engine/src/reviewer.ts index 43956f33de..0d7c7fe5cd 100644 --- a/packages/engine/src/reviewer.ts +++ b/packages/engine/src/reviewer.ts @@ -878,32 +878,139 @@ function buildReviewRequest( return parts.join("\n"); } -function extractVerdict(review: string): ReviewVerdict { - // Strategy 1: Look for a JSON verdict block (structured output) - // Matches: ```json\n{"verdict": "APPROVE"}\n``` or inline {"verdict":"REVISE"} - const jsonMatch = review.match( - /\{\s*"verdict"\s*:\s*"(APPROVE|REVISE|RETHINK)"\s*\}/i, - ); - if (jsonMatch) { - reviewerLog.log(`Verdict extracted via JSON block: ${jsonMatch[1].toUpperCase()}`); - return jsonMatch[1].toUpperCase() as ReviewVerdict; - } +/* +FNXC:ReviewLeniency 2026-07-01-22:15: +Operators want a review whose text CLEARLY approves to PASS even when the output is not perfectly structured — no trailing JSON verdict block and no "Verdict:" line. This detects an explicit prose approval while refusing to flip a rejection: any REVISE/RETHINK/"request revision|changes"/reject/disapprove or negated-approval ("not/no/never/cannot/can't/don't/doesn't/won't/without approve") signal disqualifies the lenient pass, so a prose REJECTION is never silently promoted to APPROVE. The positive set is a superset of the historical approve/approved/looks good/no issues/out of scope keywords plus common approval phrasings (approving, approval, LGTM, ship it, passes review, all good, acceptable, good to go/merge, no blocking issues/concerns). - // Strategy 2: Look for verdict in a heading line (### Verdict: APPROVE, **Verdict: REVISE**) - // Only match lines that START with a verdict pattern to avoid matching keywords in body text +Shared by the reviewer/plan-review parser (extractVerdict, this file) and the code-review + browser-verification gate parser (inferWorkflowStepVerdictFromProse in executor.ts). Fail-closed merge / PR-review / mission-verification gates deliberately do NOT use this — leniently reading "approved" out of malformed output there could auto-merge on garbage. +*/ +export function proseSignalsClearApproval(rawOutput: string): boolean { + const text = rawOutput.trim(); + if (text.length === 0) return false; + // Revise/rethink/reject/needs-changes markers AND polite change-request + // phrasings ("must be fixed", "please fix", "should be corrected", "want X + // changed", "... before merging") disqualify leniency — a review that praises + // one aspect but requests a change is a REVISE, not an approval. "blocking" is + // intentionally NOT a marker: it appears in the approval phrase "no blocking + // issues". + const blockingSignal = new RegExp( + [ + /\bREVIS(?:E|ED|ES|ING|ION|IONS)\b/, + /\bRETHINK\b/, + /\bREQUEST(?:ING|ED)?\s+(?:REVISION|CHANGES?)\b/, + /\bNEEDS?\s+(?:REVISION|CHANGES?|WORK|FIXE?S?)\b/, + /\bMUST\s+(?:BE\s+)?(?:FIX|CHANG|CORRECT|ADDRESS|RESOLV|UPDAT)\w*/, + /\bSHOULD\s+(?:BE\s+)?(?:FIX|CHANG|CORRECT|ADDRESS|RESOLV|UPDAT|REVIS)\w*/, + /\bPLEASE\s+(?:FIX|CHANG|CORRECT|ADDRESS|UPDAT|REVIS)\w*/, + /\bWANTS?\s+(?:\w+\s+){0,3}?(?:CHANG|FIX|CORRECT|ADDRESS|REVIS)\w*/, + /\bBEFORE\s+MERG\w*/, + /\bREJECT(?:ED|ING|S)?\b/, + /\bDISAPPROVE\b/, + ].map((r) => r.source).join("|"), + "i", + ); + const negatedApproval = + /\b(?:not|no|never|cannot|can['’]?t|don['’]?t|doesn['’]?t|won['’]?t|without)\s+approv/i; + if (blockingSignal.test(text) || negatedApproval.test(text)) return false; + // NOTE: "passes" is anchored to "passes review" — a bare "pass"/"passes" + // matches unrelated "the tests/build pass", "pass on approving", etc., which + // are not review approvals. + const approvalSignal = + /\b(?:approv(?:e|ed|es|ing|al)|approve[_\s]with[_\s]notes|looks?\s+good|lgtm|ship\s+it|no\s+(?:blocking\s+)?(?:issues|concerns|problems|objections)|passes?\s+(?:the\s+)?review|all\s+good|acceptable|good\s+to\s+(?:go|merge)|out\s+of\s+scope)\b/i; + return approvalSignal.test(text); +} + +/* +FNXC:ReviewLeniency 2026-07-01-23:30: +Some models emit PROSE followed by a trailing JSON payload — e.g. a paragraph of reasoning, then `{"verdict":"APPROVE","notes":"..."}` at the very end. Extract balanced top-level `{...}` objects in document order, string/escape aware so a brace inside prose or a notes string does not miscount. Callers prefer the LAST candidate as the authoritative trailing verdict. Shared by extractVerdict (reviewer/plan-review) and parseWorkflowStepVerdict (code-review + browser-verification gate). +*/ +export function extractJsonObjectCandidates(text: string): string[] { + const out: string[] = []; + const starts: number[] = []; + let inString = false; + let escaped = false; + for (let i = 0; i < text.length; i += 1) { + const ch = text[i]; + if (inString) { + if (escaped) escaped = false; + else if (ch === "\\") escaped = true; + else if (ch === '"') inString = false; + continue; + } + if (ch === '"') inString = true; + else if (ch === "{") starts.push(i); + else if (ch === "}") { + const start = starts.pop(); + if (start !== undefined && starts.length === 0) out.push(text.slice(start, i + 1)); + } + } + return out; +} + +/* +FNXC:ReviewLeniency 2026-07-01-23:30: +"Any approved" — classify a verdict TOKEN leniently so approval-family variants all pass. Any token starting with APPROVE (APPROVE, APPROVED, APPROVE_WITH_NOTES, approve_with_verdict, …) → APPROVE; REVISE/REQUEST_REVISION/REJECT → REVISE; RETHINK → RETHINK. Unknown tokens (e.g. "PASS") → null so callers can fall through instead of misclassifying. +*/ +export function classifyReviewVerdictToken(raw: string): ReviewVerdict | null { + const v = raw.trim().toUpperCase(); + if (v.startsWith("APPROVE") || v.startsWith("APPROVAL")) return "APPROVE"; + if (v.startsWith("REVISE") || v.startsWith("REQUEST_REVISION") || v.startsWith("REJECT")) return "REVISE"; + if (v.startsWith("RETHINK")) return "RETHINK"; + return null; +} + +function extractVerdict(review: string): ReviewVerdict { + /* + FNXC:ReviewLeniency 2026-07-02-00:10: + An EXPLICIT verdict the reviewer wrote as a heading or "Verdict:" line takes precedence over any JSON object found in the body. A reviewer that writes `## Verdict: REVISE` and also pastes a format example ```json {"verdict":"APPROVE"}``` or quotes a prior reviewer's `{"verdict":"APPROVE"}` must NOT be read as APPROVE. The prose→trailing-JSON case the leniency targets has no such heading/line, so JSON is still reached and used. + */ + // Strategy 1: verdict in a heading line (### Verdict: APPROVE, **Verdict: REVISE**). + // Only match lines that START with a verdict pattern to avoid matching keywords in body text. + // Capture the whole token (e.g. APPROVE_WITH_NOTES) and classify leniently ("any approved"). const headingMatch = review.match( - /^[>\s]*(?:###?\s*|[*_]{1,2})Verdict[:\s]*[*_]{0,2}\s*(APPROVE|REVISE|RETHINK)\b/im, + /^[>\s]*(?:###?\s*|[*_]{1,2})Verdict[:\s]*[*_]{0,2}\s*([A-Za-z_]+)/im, ); if (headingMatch) { - return headingMatch[1].toUpperCase() as ReviewVerdict; + const classified = classifyReviewVerdictToken(headingMatch[1]); + if (classified) return classified; } - // Strategy 3: Standalone verdict line like "Verdict: APPROVE" or "Decision: REVISE" + // Strategy 2: Standalone verdict line like "Verdict: APPROVE" or "Decision: REVISE" const lineFallback = review.match( - /^[>\s]*(?:verdict|decision)\s*[-:]\s*(APPROVE|REVISE|RETHINK)\b/im, + /^[>\s]*(?:verdict|decision)\s*[-:]\s*([A-Za-z_]+)/im, ); if (lineFallback) { - return lineFallback[1].toUpperCase() as ReviewVerdict; + const classified = classifyReviewVerdictToken(lineFallback[1]); + if (classified) return classified; + } + + // Strategy 3: JSON verdict payload (structured output), tolerating prose before + // it, extra fields (notes), and approval-family verdict variants. Prefer the + // LAST balanced object — models emit the authoritative verdict as a trailing + // JSON payload after any reasoning prose. + const jsonCandidates = extractJsonObjectCandidates(review); + for (let i = jsonCandidates.length - 1; i >= 0; i -= 1) { + try { + const parsed = JSON.parse(jsonCandidates[i]) as { verdict?: unknown }; + if (typeof parsed?.verdict === "string") { + const classified = classifyReviewVerdictToken(parsed.verdict); + if (classified) { + reviewerLog.log(`Verdict extracted via JSON payload: ${classified}`); + return classified; + } + } + } catch { + // Not valid JSON — try the next candidate / fall through to prose strategies. + } + } + + // Strategy 4 (lenient): no structured verdict, but the prose clearly approves + // (and carries no revise/reject/negated-approval signal). Treat as APPROVE so + // an imperfectly-structured approval passes instead of collapsing to a + // synthetic UNAVAILABLE retry/block. See proseSignalsClearApproval. + if (proseSignalsClearApproval(review)) { + reviewerLog.log(`Verdict extracted via lenient prose approval (${review.length} chars) → APPROVE`); + return "APPROVE"; } reviewerLog.warn(`Could not extract verdict from review (${review.length} chars). Returning UNAVAILABLE.`);