FN-7642: emit diagnostic output for dispatch/infra failures in optional-group and CE gate nodes
Fixes the code-review/plan-review/CE gate workflow node failing with a blank "(no feedback captured)" message when a dispatch or infra exception (not a reviewer verdict) causes the step to fail. - WorkflowGraphExecutor now synthesizes a non-blank WorkflowStepResult.output when an enabled optional-group (code-review, plan-review, browser-verification) or CE source:"node" skill-gate template node fails via dispatch/infra exception - Diagnostic output is derived from the node:<id>:error context-patch key, falling back to the failure value, then a stable sentinel - status, verdict extraction, edge routing, and self-healing's latestFailedPreMergeStep selection are unchanged - Added regression test coverage: workflow-graph-optional-group-no-feedback.test.ts - Added changeset (patch) documenting the fix for Runfusion/Fusion#1946 Files changed: .changeset/fn-7642-code-review-no-feedback-diagnostic.md | 7 + packages/engine/src/__tests__/workflow-graph-optional-group-no-feedback.test.ts | 246 +++++++++++++++++++++ packages/engine/src/workflow-graph-executor.ts | 104 ++++++++- 3 files changed, 355 insertions(+), 2 deletions(-) Fusion-Task-Id: FN-7642 Fusion-Task-Lineage: 1329e907-652f-4230-a945-5a9d7040ae69 Co-authored-by: Fusion (runfusion.ai) <noreply@runfusion.ai>
This commit is contained in:
gsxdsm
7
.changeset/fn-7642-code-review-no-feedback-diagnostic.md
Normal file
7
.changeset/fn-7642-code-review-no-feedback-diagnostic.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
summary: Code Review/Plan Review/CE gate failures now record a diagnostic instead of "(no feedback captured)".
|
||||
category: fix
|
||||
dev: When an enabled optional-group (`code-review`, `plan-review`, `browser-verification`) or CE `source:"node"` skill-gate template node fails via a dispatch/infra exception rather than a reviewer verdict, `WorkflowGraphExecutor` now synthesizes a non-blank `WorkflowStepResult.output` from the underlying `node:<id>:error` context-patch key (falling back to the failure `value`, then a stable sentinel) instead of leaving `output`/`notes` field-absent. Fixes Runfusion/Fusion#1946. `status`, verdict extraction, edge routing, and self-healing's `latestFailedPreMergeStep` selection are unchanged.
|
||||
@@ -0,0 +1,246 @@
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import type { TaskDetail, WorkflowIr } from "@fusion/core";
|
||||
|
||||
import { WorkflowGraphExecutor, type WorkflowNodeHandler } from "../workflow-graph-executor.js";
|
||||
|
||||
/*
|
||||
FNXC:WorkflowStepResults 2026-07-07-00:00:
|
||||
Regression coverage for Runfusion/Fusion#1946: a non-verdict optional-group /
|
||||
`source:"node"` failure (dispatch/infra exception, not a reviewer verdict) must
|
||||
never be recorded with `status:"failed"` and an absent `output` — the
|
||||
`(no feedback captured)` signature that stranded cards in `in-review`. These
|
||||
tests drive `WorkflowGraphExecutor` with a recorder-fake (mirrors
|
||||
`workflow-graph-optional-group.test.ts` / `builtin-coding-workflow-step-results.test.ts`)
|
||||
and assert the synthesized diagnostic `output`, while control cases prove
|
||||
genuine verdicts and disabled groups are byte-inert.
|
||||
*/
|
||||
|
||||
const settingsOn = () => ({ experimentalFeatures: { workflowGraphExecutor: true } });
|
||||
|
||||
function taskWith(enabled: string[] | undefined): TaskDetail {
|
||||
return { id: "FN-NFC", enabledWorkflowSteps: enabled } as TaskDetail;
|
||||
}
|
||||
|
||||
/** A single-node `code-review` optional-group between start/end, configurable phase. */
|
||||
function codeReviewGroupIr(options: { phase?: "pre-merge" | "post-merge" } = {}): WorkflowIr {
|
||||
return {
|
||||
version: "v2",
|
||||
name: "code-review-no-feedback-test",
|
||||
columns: [{ id: "work", name: "Work", traits: [] }],
|
||||
nodes: [
|
||||
{ id: "start", kind: "start" },
|
||||
{
|
||||
id: "code-review",
|
||||
kind: "optional-group",
|
||||
config: {
|
||||
name: "Code Review",
|
||||
defaultOn: true,
|
||||
phase: options.phase,
|
||||
template: {
|
||||
nodes: [{ id: "review", kind: "prompt", config: { prompt: "review" } }],
|
||||
edges: [],
|
||||
},
|
||||
},
|
||||
},
|
||||
{ id: "after", kind: "prompt", config: { prompt: "after" } },
|
||||
{ id: "end", kind: "end" },
|
||||
],
|
||||
edges: [
|
||||
{ from: "start", to: "code-review" },
|
||||
{ from: "code-review", to: "after", condition: "success" },
|
||||
{ from: "code-review", to: "end", condition: "failure" },
|
||||
{ from: "after", to: "end" },
|
||||
],
|
||||
};
|
||||
}
|
||||
|
||||
/** A single top-level `gate` node (CE `source:"node"` skill gate) — no optional-group wrapper. */
|
||||
function nodeGateIr(): WorkflowIr {
|
||||
return {
|
||||
version: "v2",
|
||||
name: "node-gate-no-feedback-test",
|
||||
columns: [{ id: "work", name: "Work", traits: [] }],
|
||||
nodes: [
|
||||
{ id: "start", kind: "start" },
|
||||
{ id: "gatecheck", kind: "gate", config: { prompt: "check", skillName: "security-gate" } },
|
||||
{ id: "end", kind: "end" },
|
||||
],
|
||||
edges: [
|
||||
{ from: "start", to: "gatecheck" },
|
||||
{ from: "gatecheck", to: "end" },
|
||||
],
|
||||
};
|
||||
}
|
||||
|
||||
describe("workflow-graph-executor: non-verdict failure diagnostic (Runfusion/Fusion#1946)", () => {
|
||||
it("SYMPTOM: a code-review dispatch exception records a non-empty diagnostic output, never (no feedback captured)", async () => {
|
||||
const records: Array<Record<string, unknown>> = [];
|
||||
const handler: WorkflowNodeHandler = async (node) => {
|
||||
if (node.id === "review") throw new Error("model provider dispatch failed");
|
||||
return { outcome: "success" };
|
||||
};
|
||||
const executor = new WorkflowGraphExecutor({
|
||||
handlers: { prompt: handler },
|
||||
maxRetriesPerNode: 3,
|
||||
recordWorkflowStepResult: async (_taskId, result) => { records.push(result as unknown as Record<string, unknown>); },
|
||||
});
|
||||
|
||||
await executor.run(taskWith(["code-review"]), settingsOn(), codeReviewGroupIr());
|
||||
|
||||
const terminal = records.find((r) => r.workflowStepId === "code-review" && r.status === "failed");
|
||||
expect(terminal).toBeDefined();
|
||||
expect(terminal?.verdict).toBeUndefined();
|
||||
expect(typeof terminal?.output).toBe("string");
|
||||
expect((terminal?.output as string).length).toBeGreaterThan(0);
|
||||
expect(terminal?.output).toContain("model provider dispatch failed");
|
||||
expect(terminal?.output).not.toBe("(no feedback captured)");
|
||||
});
|
||||
|
||||
it("SYMPTOM (post-merge phase): a post-merge optional-group dispatch exception also records a diagnostic output", async () => {
|
||||
const records: Array<Record<string, unknown>> = [];
|
||||
const handler: WorkflowNodeHandler = async (node) => {
|
||||
if (node.id === "review") throw new Error("session dispatch race");
|
||||
return { outcome: "success" };
|
||||
};
|
||||
const executor = new WorkflowGraphExecutor({
|
||||
handlers: { prompt: handler },
|
||||
maxRetriesPerNode: 3,
|
||||
recordWorkflowStepResult: async (_taskId, result) => { records.push(result as unknown as Record<string, unknown>); },
|
||||
});
|
||||
|
||||
await executor.run(taskWith(["code-review"]), settingsOn(), codeReviewGroupIr({ phase: "post-merge" }));
|
||||
|
||||
const terminal = records.find((r) => r.workflowStepId === "code-review" && r.status === "failed");
|
||||
expect(terminal).toBeDefined();
|
||||
expect(terminal?.phase).toBe("post-merge");
|
||||
expect(terminal?.output).toContain("session dispatch race");
|
||||
});
|
||||
|
||||
it("SYMPTOM (source:'node'): a CE skill-gate node exception records a diagnostic output, not a field-absent failure", async () => {
|
||||
const records: Array<Record<string, unknown>> = [];
|
||||
const handler: WorkflowNodeHandler = async () => {
|
||||
throw new Error("gate dispatch exploded");
|
||||
};
|
||||
const executor = new WorkflowGraphExecutor({
|
||||
handlers: { gate: handler },
|
||||
maxRetriesPerNode: 2,
|
||||
recordWorkflowStepResult: async (_taskId, result) => { records.push(result as unknown as Record<string, unknown>); },
|
||||
});
|
||||
|
||||
await executor.run(taskWith(undefined), settingsOn(), nodeGateIr());
|
||||
|
||||
const terminal = records.find((r) => r.workflowStepId === "gatecheck" && r.status === "failed");
|
||||
expect(terminal).toBeDefined();
|
||||
expect(terminal?.status).toBe("failed");
|
||||
expect(terminal?.source).toBe("node");
|
||||
expect(typeof terminal?.output).toBe("string");
|
||||
expect((terminal?.output as string).length).toBeGreaterThan(0);
|
||||
expect(terminal?.output).toContain("gate dispatch exploded");
|
||||
});
|
||||
|
||||
it("SURFACE: an 'aborted' failure value with no recoverable error text still yields a non-blank fallback output", async () => {
|
||||
const records: Array<Record<string, unknown>> = [];
|
||||
// The template node itself directly returns a failure with a bare `value` and
|
||||
// no `contextPatch` (the same shape `runOptionalGroup`/`executeNodeWithRetries`
|
||||
// produce for a mid-retry abort) — no recoverable `:error` text anywhere.
|
||||
const handler: WorkflowNodeHandler = async (node) =>
|
||||
node.id === "review" ? { outcome: "failure", value: "aborted" } : { outcome: "success" };
|
||||
const executor = new WorkflowGraphExecutor({
|
||||
handlers: { prompt: handler },
|
||||
recordWorkflowStepResult: async (_taskId, result) => { records.push(result as unknown as Record<string, unknown>); },
|
||||
});
|
||||
|
||||
await executor.run(taskWith(["code-review"]), settingsOn(), codeReviewGroupIr());
|
||||
|
||||
const terminal = records.find((r) => r.workflowStepId === "code-review" && r.status === "failed");
|
||||
expect(terminal).toBeDefined();
|
||||
expect(typeof terminal?.output).toBe("string");
|
||||
expect((terminal?.output as string).trim().length).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
it("CONTROL: a genuine REVISE verdict keeps its verdict + populated output unchanged (not overwritten by the diagnostic path)", async () => {
|
||||
const records: Array<Record<string, unknown>> = [];
|
||||
const handler: WorkflowNodeHandler = async (node) =>
|
||||
node.id === "review"
|
||||
? { outcome: "failure", value: "REVISE", contextPatch: { output: "Please add tests for the edge case" } }
|
||||
: { outcome: "success" };
|
||||
const executor = new WorkflowGraphExecutor({
|
||||
handlers: { prompt: handler },
|
||||
recordWorkflowStepResult: async (_taskId, result) => { records.push(result as unknown as Record<string, unknown>); },
|
||||
});
|
||||
|
||||
await executor.run(taskWith(["code-review"]), settingsOn(), codeReviewGroupIr());
|
||||
|
||||
const terminal = records.find((r) => r.workflowStepId === "code-review" && r.status === "failed");
|
||||
expect(terminal).toBeDefined();
|
||||
expect(terminal?.verdict).toBe("REVISE");
|
||||
expect(terminal?.output).toBe("Please add tests for the edge case");
|
||||
});
|
||||
|
||||
it("CONTROL: APPROVE and APPROVE_WITH_NOTES verdicts are unchanged", async () => {
|
||||
for (const verdict of ["APPROVE", "APPROVE_WITH_NOTES"] as const) {
|
||||
const records: Array<Record<string, unknown>> = [];
|
||||
const handler: WorkflowNodeHandler = async (node) =>
|
||||
node.id === "review"
|
||||
? { outcome: "success", value: verdict, contextPatch: { output: `${verdict} notes` } }
|
||||
: { outcome: "success" };
|
||||
const executor = new WorkflowGraphExecutor({
|
||||
handlers: { prompt: handler },
|
||||
recordWorkflowStepResult: async (_taskId, result) => { records.push(result as unknown as Record<string, unknown>); },
|
||||
});
|
||||
|
||||
await executor.run(taskWith(["code-review"]), settingsOn(), codeReviewGroupIr());
|
||||
|
||||
const terminal = records.find((r) => r.workflowStepId === "code-review" && r.status !== "pending");
|
||||
expect(terminal).toBeDefined();
|
||||
expect(terminal?.verdict).toBe(verdict);
|
||||
expect(terminal?.output).toBe(`${verdict} notes`);
|
||||
}
|
||||
});
|
||||
|
||||
it("CONTROL: a disabled code-review group records nothing (byte-inert)", async () => {
|
||||
const records: Array<Record<string, unknown>> = [];
|
||||
const handler: WorkflowNodeHandler = async (node) => {
|
||||
if (node.id === "review") throw new Error("should never run");
|
||||
return { outcome: "success" };
|
||||
};
|
||||
const executor = new WorkflowGraphExecutor({
|
||||
handlers: { prompt: handler },
|
||||
recordWorkflowStepResult: async (_taskId, result) => { records.push(result as unknown as Record<string, unknown>); },
|
||||
});
|
||||
|
||||
// defaultOn: true but not explicitly enabled — group defaults follow the
|
||||
// fixture's `enabledWorkflowSteps` gate the same way as the sibling suite.
|
||||
const ir = codeReviewGroupIr();
|
||||
(ir.nodes.find((n) => n.id === "code-review")!.config as { defaultOn?: boolean }).defaultOn = false;
|
||||
|
||||
const result = await executor.run(taskWith(undefined), settingsOn(), ir);
|
||||
|
||||
expect(records.filter((r) => r.workflowStepId === "code-review")).toHaveLength(0);
|
||||
expect(result.outcome).toBe("success");
|
||||
});
|
||||
|
||||
it("keeps status/verdict/edge-routing untouched so self-healing's status==='failed' selection is unaffected", async () => {
|
||||
const records: Array<Record<string, unknown>> = [];
|
||||
const calls: string[] = [];
|
||||
const handler: WorkflowNodeHandler = async (node) => {
|
||||
calls.push(node.id);
|
||||
if (node.id === "review") throw new Error("dispatch race");
|
||||
return { outcome: "success" };
|
||||
};
|
||||
const executor = new WorkflowGraphExecutor({
|
||||
handlers: { prompt: handler },
|
||||
maxRetriesPerNode: 2,
|
||||
recordWorkflowStepResult: async (_taskId, result) => { records.push(result as unknown as Record<string, unknown>); },
|
||||
});
|
||||
|
||||
await executor.run(taskWith(["code-review"]), settingsOn(), codeReviewGroupIr());
|
||||
|
||||
// The failure edge routes to `end`, not the success edge to `after` —
|
||||
// `self-healing.ts`'s `latestFailedPreMergeStep` relies on this same
|
||||
// `status:"failed"` signal; only `output` gained a diagnostic, nothing else.
|
||||
expect(calls).not.toContain("after");
|
||||
const terminal = records.find((r) => r.workflowStepId === "code-review" && r.status === "failed");
|
||||
expect(terminal?.status).toBe("failed");
|
||||
});
|
||||
});
|
||||
@@ -726,8 +726,32 @@ export class WorkflowGraphExecutor {
|
||||
else if (verdict === "REVISE") stepStatus = "advisory_failure";
|
||||
else stepStatus = "passed";
|
||||
const exitContextPatch = exitResult?.contextPatch;
|
||||
const stepOutput = typeof exitContextPatch?.output === "string" ? exitContextPatch.output : undefined;
|
||||
let stepOutput = typeof exitContextPatch?.output === "string" ? exitContextPatch.output : undefined;
|
||||
const stepNotes = typeof exitContextPatch?.notes === "string" ? exitContextPatch.notes : undefined;
|
||||
/*
|
||||
* FNXC:WorkflowStepResults 2026-07-07-00:00:
|
||||
* A non-verdict `stepStatus === "failed"` (dispatch/infra exception, not a
|
||||
* reviewer verdict) with no `output`/`notes` recovered from the exit
|
||||
* context-patch must never be recorded field-absent — that is the
|
||||
* `(no feedback captured)` signature from Runfusion/Fusion#1946. Synthesize
|
||||
* a diagnostic from the template node's `node:<id>:error` context-patch key
|
||||
* (derived from the last visited template node id) with a fallback to the
|
||||
* failure `value` (e.g. "exception", "aborted"). Genuine REVISE/APPROVE
|
||||
* records already carry `stepOutput`/`stepNotes` and are unaffected.
|
||||
*/
|
||||
if (stepStatus === "failed" && !verdict && stepOutput === undefined && stepNotes === undefined) {
|
||||
const lastVisited = groupResult.visitedNodeIds[groupResult.visitedNodeIds.length - 1];
|
||||
const templateNodeId = lastVisited?.includes("::") ? lastVisited.slice(lastVisited.indexOf("::") + 2) : undefined;
|
||||
stepOutput = this.synthesizeNonVerdictFailureOutput({
|
||||
stepLabel: groupName,
|
||||
contextPatch: exitContextPatch,
|
||||
templateNodeId,
|
||||
failureValue: verdictRaw,
|
||||
fallbackText: node.id === PLAN_REVIEW_GROUP_ID
|
||||
? "Plan Review failed before execution. Re-run triage to revise PROMPT.md before implementation continues."
|
||||
: undefined,
|
||||
});
|
||||
}
|
||||
await this.recordOptionalGroupStepResult(task.id, {
|
||||
workflowStepId: node.id,
|
||||
workflowStepName: groupName,
|
||||
@@ -1059,6 +1083,67 @@ export class WorkflowGraphExecutor {
|
||||
}
|
||||
}
|
||||
|
||||
/*
|
||||
* FNXC:WorkflowStepResults 2026-07-07-00:00:
|
||||
* A hard optional-group / node-gate failure that originates from a dispatch or
|
||||
* infra exception (rather than a reviewer verdict) must never be persisted as a
|
||||
* `WorkflowStepResult` with `verdict`/`output`/`notes` entirely absent — that is
|
||||
* the field-absent `(no feedback captured)` signature reported in
|
||||
* Runfusion/Fusion#1946 (3 confirmed instances: card stranded in `in-review`,
|
||||
* ~3s duration from `executeNodeWithRetries` exhausting fast dispatch retries,
|
||||
* no reviewer-agent error). `executeNodeWithRetries` stores the underlying error
|
||||
* text under a `node:<templateNodeId>:error` context-patch key (see the
|
||||
* `plugin-node-handler-error`/`exception` failure branches ~line 1140/1238), but
|
||||
* the terminal recorder previously derived `output`/`notes` only from
|
||||
* `contextPatch.output`/`.notes`, silently dropping that diagnostic. This helper
|
||||
* synthesizes a non-blank diagnostic `output` from the `:error`-suffixed patch
|
||||
* key (preferring the exact `node:<templateNodeId>:error` key when the template
|
||||
* node id is known) with a fallback to the failure `value` (e.g. `"exception"`,
|
||||
* `"aborted"`, `"plugin-node-handler-error"`) and finally a stable non-blank
|
||||
* fallback sentence — never an empty string. Shared by the optional-group
|
||||
* terminal recorder and `recordNodeProgressFinish` (CE `source:"node"` skill
|
||||
* gates) so both surfaces carry the identical guarantee. `status` (`"failed"`),
|
||||
* verdict extraction, edge routing, and `self-healing.ts`'s
|
||||
* `latestFailedPreMergeStep` (which filters on `status === "failed"`) are
|
||||
* untouched — this only adds `output` to an already-failed record.
|
||||
*/
|
||||
private synthesizeNonVerdictFailureOutput(params: {
|
||||
stepLabel: string;
|
||||
contextPatch?: Record<string, unknown>;
|
||||
templateNodeId?: string;
|
||||
failureValue?: string;
|
||||
/**
|
||||
* FNXC:WorkflowStepResults 2026-07-07-00:00:
|
||||
* `PLAN_REVIEW_GROUP_ID`'s existing hard-failure handoff (~line 802) already
|
||||
* has a dedicated, non-blank sentinel ("Plan Review failed before execution...")
|
||||
* for the fully-unrecoverable case (no `:error` key, no failure `value`). Pass
|
||||
* it through so the recorded `output` and the pre-merge fix `feedback` stay
|
||||
* byte-identical for that path when no real diagnostic exists to surface;
|
||||
* every other caller keeps the generic fallback sentence.
|
||||
*/
|
||||
fallbackText?: string;
|
||||
}): string {
|
||||
const { stepLabel, contextPatch, templateNodeId, failureValue, fallbackText } = params;
|
||||
let errorText: string | undefined;
|
||||
if (contextPatch) {
|
||||
if (templateNodeId) {
|
||||
const exact = contextPatch[`node:${templateNodeId}:error`];
|
||||
if (typeof exact === "string" && exact.trim()) errorText = exact.trim();
|
||||
}
|
||||
if (!errorText) {
|
||||
for (const [key, value] of Object.entries(contextPatch)) {
|
||||
if (key.endsWith(":error") && typeof value === "string" && value.trim()) {
|
||||
errorText = value.trim();
|
||||
break;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
const detail = errorText || (typeof failureValue === "string" && failureValue.trim() ? failureValue.trim() : undefined);
|
||||
if (detail) return `${stepLabel} failed before producing a verdict: ${detail}`;
|
||||
return fallbackText || "Workflow step failed before producing a verdict (no reviewer output captured).";
|
||||
}
|
||||
|
||||
/*
|
||||
* FNXC:WorkflowStepResults 2026-06-25-12:00:
|
||||
* Fail-soft forward to the `recordWorkflowStepResult` persistence sink (plan U2).
|
||||
@@ -1280,8 +1365,23 @@ export class WorkflowGraphExecutor {
|
||||
): Promise<void> {
|
||||
const status: WorkflowStepResult["status"] = nodeResult.outcome === "success" ? "passed" : "failed";
|
||||
const contextPatch = nodeResult.contextPatch ?? {};
|
||||
const output = typeof contextPatch.output === "string" ? contextPatch.output : undefined;
|
||||
let output = typeof contextPatch.output === "string" ? contextPatch.output : undefined;
|
||||
const notes = typeof contextPatch.notes === "string" ? contextPatch.notes : undefined;
|
||||
/*
|
||||
* FNXC:WorkflowStepResults 2026-07-07-00:00:
|
||||
* CE `source:"node"` skill-gate failures share the same `(no feedback
|
||||
* captured)` defect as the optional-group path (Runfusion/Fusion#1946): a
|
||||
* `failed` node with no `contextPatch.output`/`.notes` must still record a
|
||||
* non-blank diagnostic sourced from `node:<id>:error`.
|
||||
*/
|
||||
if (status === "failed" && output === undefined && notes === undefined) {
|
||||
output = this.synthesizeNonVerdictFailureOutput({
|
||||
stepLabel: this.workflowNodeProgressName(node),
|
||||
contextPatch,
|
||||
templateNodeId: node.id,
|
||||
failureValue: nodeResult.value,
|
||||
});
|
||||
}
|
||||
await this.recordOptionalGroupStepResult(taskId, {
|
||||
workflowStepId: node.id,
|
||||
workflowStepName: this.workflowNodeProgressName(node),
|
||||
|
||||
Reference in New Issue
Block a user