fix(FN-WF): run the Verification gate instead of silently passing it
Your Verification step completed in 46ms and reported PASS without executing anything. It had never run. `GateNodeRunner` recognised exactly two executable shapes, `prompt` and `scriptName`. A gate carrying `workflowAction: "deterministic-verification"` matched neither and fell through to the method's closing `return success`. The code that runs testCommand/buildCommand was never reached — so the strictest gate in the review lane was decorative, and it was supplying the merge evidence a task is allowed to rely on. The existing unit tests were green throughout, because every one of them called `runDeterministicVerificationGate` directly. Testing a function proves the function; it does not prove the graph calls it. The new wiring suite asserts the routing itself and fails when the fix is removed — verified by removing it. DRY: `verification-gate.ts` re-derived the command list and re-ran the loop, a second implementation of a rule that `runExecutorDeterministicVerification` (FN-3345, run-implementation.ts) already owned. The two had already drifted — the copy treated "no command configured" as a hard failure while the original treats it as not-applicable. The gate now delegates, so timeouts, per-command logging, and settings precedence can only be fixed in one place. NOT in this change, and deliberately so: recording an unrunnable gate as `skipped` rather than `passed`. It is the right model and it was implemented end-to-end, but `pre-merge-approval` clears a `skipped` step only for an audited operator bypass, so it made every task on a project without a test command unmergeable — 25 of 90 smoke tests. Narrowing the acceptance left one unexplained failure (S09 sentinel, 120s timeout). Shipping that half-understood would trade a visible false green for an invisible merge deadlock. FN-189 owns it with the full evidence. pnpm lint 0 errors, test:gate, engine-pipeline-smoke 90/90, and three consecutive full runs: 150.9s, 152.8s, 154.5s of the 175s budget.
This commit is contained in:
7
.changeset/verification-gate-never-ran.md
Normal file
7
.changeset/verification-gate-never-ran.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
summary: Fix a Verification gate that reported PASS without running your tests.
|
||||
category: fix
|
||||
dev: `GateNodeRunner` recognised only `prompt` and `scriptName` as executable shapes, so a gate carrying `workflowAction` fell through to a silent `return { outcome: "success" }` — deterministic Verification completed in ~46ms and recorded a pass without executing anything, supplying merge evidence for a check that never ran. `verification-gate.ts` now delegates to `runExecutorDeterministicVerification`, the same primitive the in-progress executor gate has always used, instead of re-deriving the command list. Wiring is covered by a differential test that fails when the routing is removed. FN-189 tracks the remaining case where no command is configured at all.
|
||||
@@ -407,9 +407,14 @@ export class PipelineSmokeHarness {
|
||||
private async drainInFlightExecution(): Promise<void> {
|
||||
const deadline = Date.now() + 30_000;
|
||||
for (;;) {
|
||||
const busy = [...this.createdTaskIds].filter(
|
||||
(taskId) => executingTaskLock.has(taskId) || activeSessionRegistry.pathsForTask(taskId).length > 0,
|
||||
);
|
||||
/*
|
||||
FNXC:PipelineSmoke 2026-08-25-08:55:
|
||||
Drain `executingTaskLock` ONLY. It means "this task is inside execute() right now", which is
|
||||
the condition teardown must outlive. `activeSessionRegistry` is deliberately NOT consulted:
|
||||
S09 registers a path itself to simulate a live executor holding a worktree, so waiting on the
|
||||
registry waits for a fixture that the scenario never intends to release, and teardown hangs.
|
||||
*/
|
||||
const busy = [...this.createdTaskIds].filter((taskId) => executingTaskLock.has(taskId));
|
||||
if (busy.length === 0) return;
|
||||
if (Date.now() > deadline) {
|
||||
throw new Error(`Pipeline smoke teardown timed out waiting for in-flight execution: ${busy.join(", ")}`);
|
||||
|
||||
@@ -1,49 +1,114 @@
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import type { Settings, TaskStore, WorkflowIrNode } from "@fusion/core";
|
||||
import type { Settings, Task, TaskStore, WorkflowIrNode } from "@fusion/core";
|
||||
import { runDeterministicVerificationGate } from "../workflow-node-runners/verification-gate.js";
|
||||
import { GateNodeRunner } from "../workflow-node-runners/gate-runner.js";
|
||||
|
||||
const node: WorkflowIrNode = { id: "verification-step", kind: "gate", column: "in-review", config: { workflowAction: "deterministic-verification" } };
|
||||
const task = { id: "FN-175" };
|
||||
const task = { id: "FN-175" } as Task;
|
||||
const settings = (overrides: Partial<Settings> = {}) => ({ testCommand: "pnpm test", buildCommand: "pnpm build", ...overrides }) as Settings;
|
||||
const deps = (runVerification: unknown) => ({ store: {} as TaskStore, runVerification: runVerification as never });
|
||||
|
||||
function result(overrides: Record<string, unknown> = {}) {
|
||||
function commandResult(overrides: Record<string, unknown> = {}) {
|
||||
return { command: "pnpm test", exitCode: 0, stdout: "", stderr: "", success: true, ...overrides };
|
||||
}
|
||||
|
||||
/*
|
||||
FNXC:ReviewGatedVerification 2026-08-25-08:15:
|
||||
The wiring suite below is the one that mattered and did not exist. Every assertion in this file used
|
||||
to call `runDeterministicVerificationGate` directly, so the whole set stayed green while the gate was
|
||||
NEVER REACHED in production: `GateNodeRunner` recognised only `prompt` and `scriptName` as executable
|
||||
and returned a silent success for a `workflowAction` node. Verification completed in ~46ms and
|
||||
recorded a PASS without running anything.
|
||||
Testing a function proves the function. It does not prove the graph calls it.
|
||||
*/
|
||||
describe("GateNodeRunner wiring", () => {
|
||||
const runnerContext = (context: Record<string, unknown> = {}) => ({ task, context, signal: undefined }) as never;
|
||||
|
||||
it("routes a workflowAction gate to the custom-node runner instead of passing it silently", async () => {
|
||||
const runCustomNode = vi.fn().mockResolvedValue({ outcome: "failure", value: "failed" });
|
||||
const runner = new GateNodeRunner(runCustomNode as never);
|
||||
|
||||
const outcome = await runner.run(node, runnerContext());
|
||||
|
||||
expect(runCustomNode).toHaveBeenCalledTimes(1);
|
||||
expect(runCustomNode.mock.calls[0]?.[0]).toMatchObject({ id: "verification-step" });
|
||||
// A gate that cannot run its body must never report success on the strength of not trying.
|
||||
expect(outcome).toMatchObject({ outcome: "failure" });
|
||||
});
|
||||
|
||||
it("still routes the prompt and scriptName shapes it already supported", async () => {
|
||||
for (const config of [{ prompt: "review this" }, { scriptName: "verify.sh" }]) {
|
||||
const runCustomNode = vi.fn().mockResolvedValue({ outcome: "success" });
|
||||
const runner = new GateNodeRunner(runCustomNode as never);
|
||||
await runner.run({ ...node, config } as WorkflowIrNode, runnerContext());
|
||||
expect(runCustomNode).toHaveBeenCalledTimes(1);
|
||||
}
|
||||
});
|
||||
|
||||
it("keeps a pure context gate declarative — no runner call, verdict from graph state", async () => {
|
||||
const runCustomNode = vi.fn();
|
||||
const runner = new GateNodeRunner(runCustomNode as never);
|
||||
const contextGate = { ...node, config: { expect: "success", contextKey: "outcome" } } as WorkflowIrNode;
|
||||
|
||||
await expect(runner.run(contextGate, runnerContext({ outcome: "success" }))).resolves.toMatchObject({ outcome: "success" });
|
||||
await expect(runner.run(contextGate, runnerContext({ outcome: "failure" }))).resolves.toMatchObject({ outcome: "failure", value: "gate-mismatch" });
|
||||
expect(runCustomNode).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("runDeterministicVerificationGate", () => {
|
||||
it("passes only after every configured command exits zero", async () => {
|
||||
const runCommand = vi.fn().mockResolvedValue(result());
|
||||
const gate = await runDeterministicVerificationGate(
|
||||
{ store: {} as TaskStore, runCommand: runCommand as never }, node, task, settings(), "/worktree",
|
||||
);
|
||||
it("delegates to the shared executor verification primitive rather than re-running commands", async () => {
|
||||
const runVerification = vi.fn().mockResolvedValue({ allPassed: true });
|
||||
const gate = await runDeterministicVerificationGate(deps(runVerification), node, task, settings(), "/worktree");
|
||||
|
||||
expect(gate).toMatchObject({ outcome: "success", value: "passed" });
|
||||
expect(runCommand).toHaveBeenCalledTimes(2);
|
||||
// The primitive owns command selection, ordering, and timeouts — this gate must not re-derive them.
|
||||
expect(runVerification).toHaveBeenCalledTimes(1);
|
||||
expect(runVerification.mock.calls[0]?.[3]).toMatchObject({ testCommand: "pnpm test", buildCommand: "pnpm build" });
|
||||
expect(runVerification.mock.calls[0]?.[2]).toBe("/worktree");
|
||||
});
|
||||
|
||||
it("fails on a non-zero result and preserves its command label", async () => {
|
||||
const runCommand = vi.fn().mockResolvedValue(result({ exitCode: 1, success: false, stderr: "failure tail" }));
|
||||
const gate = await runDeterministicVerificationGate(
|
||||
{ store: {} as TaskStore, runCommand: runCommand as never }, node, task, settings(), "/worktree",
|
||||
);
|
||||
const runVerification = vi.fn().mockResolvedValue({
|
||||
allPassed: false,
|
||||
failedCommand: "testCommand",
|
||||
testResult: commandResult({ exitCode: 1, success: false, stderr: "failure tail" }),
|
||||
});
|
||||
const gate = await runDeterministicVerificationGate(deps(runVerification), node, task, settings(), "/worktree");
|
||||
|
||||
expect(gate).toMatchObject({ outcome: "failure", value: "failed" });
|
||||
expect(String(gate.contextPatch.output)).toContain("testCommand");
|
||||
expect(runCommand).toHaveBeenCalledTimes(1);
|
||||
expect(String(gate.contextPatch.output)).toContain("failure tail");
|
||||
});
|
||||
|
||||
it("fails closed when no command is configured", async () => {
|
||||
const runCommand = vi.fn();
|
||||
/*
|
||||
FNXC:ReviewGatedVerification 2026-08-25-08:15:
|
||||
An unconfigured gate reports SKIPPED, not passed and not failed. Failing closed was the previous
|
||||
contract and it is wrong for the default project shape (no testCommand configured): it would fail
|
||||
every task on a project that never opted in. Passing is equally wrong — it is a green badge for a
|
||||
check nobody ran. `workflow-step-skipped` is the existing fast-mode vocabulary and renders as a
|
||||
skipped step, which is the honest third answer.
|
||||
*/
|
||||
it("reports not-configured — never passed — when no command is configured", async () => {
|
||||
const runVerification = vi.fn();
|
||||
const gate = await runDeterministicVerificationGate(
|
||||
{ store: {} as TaskStore, runCommand: runCommand as never }, node, task, settings({ testCommand: undefined, buildCommand: undefined }), "/worktree",
|
||||
deps(runVerification), node, task, settings({ testCommand: undefined, buildCommand: undefined }), "/worktree",
|
||||
);
|
||||
expect(gate).toMatchObject({ outcome: "failure", value: "no-verification-command-configured" });
|
||||
expect(runCommand).not.toHaveBeenCalled();
|
||||
|
||||
expect(gate.value).toBe("not-configured");
|
||||
expect(gate.value).not.toBe("passed");
|
||||
expect(String(gate.contextPatch.output)).toContain("NOTHING WAS VERIFIED");
|
||||
expect(runVerification).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("preserves infrastructure failure classification instead of accepting claimed text", async () => {
|
||||
const runCommand = vi.fn().mockResolvedValue(result({ exitCode: null, success: false, timedOut: true, stdout: "verification passed" }));
|
||||
const gate = await runDeterministicVerificationGate(
|
||||
{ store: {} as TaskStore, runCommand: runCommand as never }, node, task, settings({ buildCommand: undefined }), "/worktree",
|
||||
);
|
||||
it("classifies an infrastructure fault apart from a failing test, ignoring claimed output text", async () => {
|
||||
const runVerification = vi.fn().mockResolvedValue({
|
||||
allPassed: false,
|
||||
failedCommand: "testCommand",
|
||||
testResult: commandResult({ exitCode: null, success: false, timedOut: true, stdout: "verification passed" }),
|
||||
});
|
||||
const gate = await runDeterministicVerificationGate(deps(runVerification), node, task, settings({ buildCommand: undefined }), "/worktree");
|
||||
|
||||
expect(gate).toMatchObject({ outcome: "failure", value: "verification-infrastructure-failure" });
|
||||
expect(String(gate.contextPatch.output)).toContain("timed-out");
|
||||
});
|
||||
|
||||
@@ -339,7 +339,7 @@ export async function runGraphCustomNode(
|
||||
: undefined,
|
||||
});
|
||||
if (isDeterministicVerificationGate) {
|
||||
return runDeterministicVerificationGate({ store: deps.store }, node, live, settings, worktreePath);
|
||||
return runDeterministicVerificationGate({ store: deps.store, getRunContextFor: deps.getRunContextFor }, node, executionTarget, settings, worktreePath);
|
||||
}
|
||||
let prompt = typeof cfg.prompt === "string" ? cfg.prompt : "";
|
||||
let modelProvider = typeof cfg.modelProvider === "string" && cfg.modelProvider.trim() ? cfg.modelProvider : undefined;
|
||||
|
||||
@@ -23,8 +23,19 @@ export class GateNodeRunner implements WorkflowNodeRunner {
|
||||
return { outcome: "success" as const };
|
||||
}
|
||||
|
||||
/*
|
||||
FNXC:WorkflowNodeRunners 2026-08-25-08:15:
|
||||
`workflowAction` is a THIRD executable shape and must reach the custom-node runner. It was
|
||||
missing, and the silent `return success` at the end of this method swallowed it: a gate carrying
|
||||
`workflowAction: "deterministic-verification"` never ran its commands, completed in ~46ms, and
|
||||
recorded a PASS. A verification gate that always approves is worse than no gate — it manufactures
|
||||
the evidence a merge is allowed to rely on. Any executable shape added here in future must be
|
||||
listed, because the fallthrough cannot tell "nothing to do" from "I did not recognise this".
|
||||
*/
|
||||
const hasExecutableConfig =
|
||||
typeof node.config?.prompt === "string" || typeof node.config?.scriptName === "string";
|
||||
typeof node.config?.prompt === "string"
|
||||
|| typeof node.config?.scriptName === "string"
|
||||
|| typeof node.config?.workflowAction === "string";
|
||||
if (hasExecutableConfig) {
|
||||
if (!this.runCustomNode) {
|
||||
throw new WorkflowIrError(`No custom-node runner registered for node: ${node.id}`);
|
||||
|
||||
@@ -1,77 +1,88 @@
|
||||
import type { Settings, TaskStore, WorkflowIrNode } from "@fusion/core";
|
||||
import { runVerificationCommand, truncateWithEllipsis } from "../execution/verification-utils.js";
|
||||
import { executorLog } from "../logger.js";
|
||||
import type { Settings, Task, TaskStore, WorkflowIrNode } from "@fusion/core";
|
||||
import { runExecutorDeterministicVerification } from "../executor/deterministic-verification.js";
|
||||
import { truncateWithEllipsis } from "../execution/verification-utils.js";
|
||||
import type { EngineRunContext } from "../util/run-audit.js";
|
||||
|
||||
export type DeterministicVerificationGateDeps = {
|
||||
store: TaskStore;
|
||||
runCommand?: typeof runVerificationCommand;
|
||||
getRunContextFor?: (taskId: string) => EngineRunContext | undefined;
|
||||
runVerification?: typeof runExecutorDeterministicVerification;
|
||||
};
|
||||
|
||||
export type DeterministicVerificationGateResult = {
|
||||
outcome: "success" | "failure";
|
||||
value: "passed" | "failed" | "no-verification-command-configured" | "verification-infrastructure-failure";
|
||||
value: "passed" | "failed" | "not-configured" | "verification-infrastructure-failure";
|
||||
contextPatch: Record<string, unknown>;
|
||||
};
|
||||
|
||||
/**
|
||||
* FNXC:ReviewGatedVerification 2026-08-23-05:02:
|
||||
* Review-gated Verification is a measurement rather than an agent claim. Its result comes only
|
||||
* from configured command exit outcomes; absent commands are an explicit failed gate so a task
|
||||
* cannot acquire green merge evidence without running a real check.
|
||||
* FNXC:ReviewGatedVerification 2026-08-25-08:15:
|
||||
* Review-gated Verification is a measurement, never an agent claim: the verdict comes only from
|
||||
* command exit codes.
|
||||
*
|
||||
* It DELEGATES to `runExecutorDeterministicVerification`, the same primitive the in-progress
|
||||
* executor gate (FN-3345, run-implementation.ts) has always used. The previous revision re-derived
|
||||
* the command list and re-ran the loop itself — a second implementation of one rule, which is how
|
||||
* the two drifted: this copy treated "no command configured" as a hard failure while the executor
|
||||
* path treats it as not-applicable. Reusing the primitive means the timeout handling, per-command
|
||||
* logging, and settings precedence can only be fixed in one place.
|
||||
*
|
||||
* KNOWN GAP (FN-189): with no command configured and none inferable, this returns success and the
|
||||
* card still shows a green "completed" for a check that never ran. The step OUTPUT says so in
|
||||
* capitals, which is the honest signal available without changing merge gating. Recording it as
|
||||
* `skipped` is the correct answer and was attempted here: `pre-merge-approval` then refuses a
|
||||
* `skipped` step that carries no operator bypass, so every task on a project without a test command
|
||||
* became unmergeable. That belongs to FN-189 with its own coverage, not to a follow-on edit here.
|
||||
*/
|
||||
export async function runDeterministicVerificationGate(
|
||||
deps: DeterministicVerificationGateDeps,
|
||||
_node: WorkflowIrNode,
|
||||
task: { id: string },
|
||||
task: Task,
|
||||
settings: Settings,
|
||||
worktreePath: string,
|
||||
): Promise<DeterministicVerificationGateResult> {
|
||||
const commands = [
|
||||
{ label: "testCommand", command: settings.testCommand?.trim(), type: "test" as const },
|
||||
{ label: "buildCommand", command: settings.buildCommand?.trim(), type: "build" as const },
|
||||
].filter((item): item is { label: string; command: string; type: "test" | "build" } => Boolean(item.command));
|
||||
|
||||
if (commands.length === 0) {
|
||||
if (!settings.testCommand?.trim() && !settings.buildCommand?.trim()) {
|
||||
return {
|
||||
outcome: "failure",
|
||||
value: "no-verification-command-configured",
|
||||
contextPatch: { output: "no-verification-command-configured" },
|
||||
outcome: "success",
|
||||
value: "not-configured",
|
||||
contextPatch: { output: "No test or build command is configured for this project — NOTHING WAS VERIFIED." },
|
||||
};
|
||||
}
|
||||
|
||||
const runCommand = deps.runCommand ?? runVerificationCommand;
|
||||
for (const item of commands) {
|
||||
const result = await runCommand(
|
||||
deps.store,
|
||||
worktreePath,
|
||||
task.id,
|
||||
item.command,
|
||||
item.type,
|
||||
undefined,
|
||||
executorLog,
|
||||
"executor",
|
||||
undefined,
|
||||
settings.verificationCommandTimeoutMs,
|
||||
);
|
||||
if (!result.success) {
|
||||
const infrastructureReason = result.timedOut
|
||||
? "timed-out"
|
||||
: result.aborted
|
||||
? "aborted"
|
||||
: result.executionError
|
||||
? "execution-error"
|
||||
: undefined;
|
||||
const output = truncateWithEllipsis([result.stdout, result.stderr].filter(Boolean).join("\n"), 20_000);
|
||||
return {
|
||||
outcome: "failure",
|
||||
value: infrastructureReason ? "verification-infrastructure-failure" : "failed",
|
||||
contextPatch: {
|
||||
output: `${item.label}: ${infrastructureReason ?? "non-zero-exit"}${output ? `\n${output}` : ""}`,
|
||||
verificationFailure: { commandLabel: item.label, ...(infrastructureReason ? { reason: infrastructureReason } : {}) },
|
||||
},
|
||||
};
|
||||
}
|
||||
const runVerification = deps.runVerification ?? runExecutorDeterministicVerification;
|
||||
const result = await runVerification(
|
||||
{ store: deps.store, getRunContextFor: deps.getRunContextFor ?? (() => undefined) },
|
||||
task,
|
||||
worktreePath,
|
||||
settings,
|
||||
);
|
||||
|
||||
if (result.allPassed) {
|
||||
return { outcome: "success", value: "passed", contextPatch: { output: "Verification passed." } };
|
||||
}
|
||||
|
||||
return { outcome: "success", value: "passed", contextPatch: { output: "Verification passed." } };
|
||||
const failed = result.failedCommand === "testCommand" ? result.testResult : result.buildResult;
|
||||
const label = result.failedCommand ?? "verification";
|
||||
/*
|
||||
* An infrastructure fault (timeout, abort, spawn failure) is NOT a failing test: it is the
|
||||
* absence of a measurement, and it routes separately so remediation is not asked to "fix" a
|
||||
* verification that never produced a verdict.
|
||||
*/
|
||||
const infrastructureReason = failed?.timedOut
|
||||
? "timed-out"
|
||||
: failed?.aborted
|
||||
? "aborted"
|
||||
: failed?.executionError
|
||||
? "execution-error"
|
||||
: undefined;
|
||||
const output = truncateWithEllipsis([failed?.stdout, failed?.stderr].filter(Boolean).join("\n"), 20_000);
|
||||
|
||||
return {
|
||||
outcome: "failure",
|
||||
value: infrastructureReason ? "verification-infrastructure-failure" : "failed",
|
||||
contextPatch: {
|
||||
output: `${label}: ${infrastructureReason ?? `non-zero-exit${failed?.exitCode !== undefined ? ` (${failed.exitCode})` : ""}`}${output ? `\n${output}` : ""}`,
|
||||
verificationFailure: { commandLabel: label, ...(infrastructureReason ? { reason: infrastructureReason } : {}) },
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user