root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

U7 PR3: the specification reaction acts on what finalize DID, not on the fact that planning stopped (#2506)

Browse Source 
Completes the pair started in #2498. That landed the outcome; this makes
the engine's reaction consume it.

## The bug

`onSpecifyComplete` fired on **every** finished specification, because
the seam announcing it fired unconditionally. So a card parked at the
manual plan-approval gate — finalize writes `status:
"awaiting-approval"` and **returns early**, before the release move —
was logged as `Specified X → todo` and had a Plan Review run armed for a
plan the operator had not approved.

#2491 stopped the **seeder** from acting on that, defensively, at the
seeder. This removes the reason it was ever asked. Both layers are
deliberate and neither is redundant:

- the seeder guard covers **every caller**, including self-healing's
re-seed;
- this one stops the engine doing work nobody asked for, and stops it
telling the operator something false about their own board.

`released` is the only outcome that licenses arming a run — the only one
meaning the card crossed into the hold column (or was already resting
there, plan-in-place) and is the graph's now. `parked` belongs to a
human; `withheld` belongs to the caller's retry budget.

## The event still fires on every outcome

Deliberately. Dropping the reaction for a non-release would also drop
the runtime's `recordActivity()` idle signal, and a reaction that
silently does not happen is harder to reason about than one that happens
with an accurate payload. R5's division of labour: **the seam announces,
the subscriber decides what a given outcome licenses.**

## Why there is a new extracted function

`reactToSpecificationComplete` is pulled out of the inline
`InProcessRuntime` callback for the same reason the continuation drain
was in #2491: the callback is built inside a class whose construction
attaches to the real central project registry, so no test could
distinguish *"the reaction respects the outcome"* from *"the reaction
ignores it"*.

**Revert proof:** with the outcome gate removed from the reaction, **5
of 8 fail**.

## Two call-site decisions worth naming

**`tryFinalizeExplicitDuplicateMarker` reports through a mutable ref,
not a widened return type.** Its boolean answers a *different* question
— "was this a duplicate marker at all?" — and 16 existing tests assert
it directly. I tried the widened return first and it turned all 16 red.
Expectation edits are exactly how a behavior change travels disguised as
churn, so I backed it out. **This diff touches zero existing test
expectations.**

**A duplicate-marker redirect reports `parked`**, which is accurate: it
deletes, flags, or clears the marker; it never releases the card into
the hold column.

## A fixture note — third of this shape on the program

My "task vanished between release and reaction" case passed `undefined`,
which triggered the harness **default parameter** and silently handed
the reaction a live task — making it a duplicate of the control rather
than the case it claimed to be. It now passes `null`, with a comment
saying why.

Running tally of near-false-greens on this unit, all the same family: a
fake that ignores its predicate (#2491), a stub that ignores its
callback (#2498), a default parameter that swallows the interesting
input (here). Each was caught by the test failing for the *wrong reason*
and being read rather than fixed.

## Verification

| Check | Result |
|---|---|
| new suite | 8/8 |
| 15 triage / planning / continuation suites | 361/361, **no expectation
edits** |
| `tsc --noEmit` (engine) | clean |
| `pnpm lint` | clean |
| `pnpm test:gate` | green (307 + 10 + 71) |
| `pnpm check:changesets` | clean |

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-07-28 17:15:29 -07:00
committed by GitHub
parent a2b4ca76ac
commit 8288e4a8ab
4 changed files with 242 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
.changeset/specify-complete-reaction.md Normal file
View File

@@ -0,0 +1,7 @@
---
"@runfusion/fusion": patch
---
summary: Plan review no longer starts for a task that is still waiting on your approval.
category: fix
dev: U7. `onSpecifyComplete` now carries the `PlanningHandoffReport` from finalize, and the engine's reaction (`reactToSpecificationComplete`, extracted from the inline `InProcessRuntime` callback so its gating is testable) arms the pre-release plan-review continuation only on `outcome === "released"`. Non-release outcomes log the real outcome instead of asserting `Specified X → todo`, and never reach the store. `recordActivity()` still fires for every outcome so idle detection is unaffected. Complements PR #2491's seeder-side guard, which stays as the defence covering every other caller.

133
packages/engine/src/__tests__/specify-complete-reaction.test.ts Normal file
View File

@@ -0,0 +1,133 @@
/*
FNXC:PlanningHandoffOutcome 2026-07-28-10:05 (U7 / R4, R5, R12 — workflow-owned lifecycle):
THE INVARIANT: the engine arms a pre-release Plan Review run only for a card that
was actually handed off to the graph.
Before this, the reaction fired on every finished specification, because the seam
that announces it fired unconditionally. A card parked at the manual plan-approval
gate — finalize writes `status: "awaiting-approval"` and RETURNS EARLY, before the
release move — was logged as `Specified X → todo` and had a Plan Review run armed
for a plan the operator had not approved.
PR #2491 stopped the SEEDER from acting on that, defensively, at the seeder. This
removes the reason it was ever asked. Both layers are deliberate and neither is
redundant: the seeder guard covers every caller including self-healing's re-seed,
while this one stops the engine from doing work nobody asked for and from telling
the operator something false about their own board.
WHY `released` IS THE ONLY LICENCE. It is the only outcome meaning the card crossed
into the hold column (or was already resting there, plan-in-place) and is the
graph's now. `parked` belongs to a human; `withheld` belongs to the caller's retry
budget. Arming a run for either is acting on a handoff that did not happen.
WHY THIS FILE EXISTS AT ALL. The reaction was an inline callback constructed inside
`InProcessRuntime`, whose construction attaches to the real central project
registry — so no test could tell "the reaction respects the outcome" from "the
reaction ignores it". Same extraction, same reason, as the continuation drain in
PR #2491. A guard that cannot be shown to fail is not a guard.
*/
import { describe, expect, it, vi } from "vitest";
import type { Task, WorkflowIr } from "@fusion/core";
import { reactToSpecificationComplete } from "../runtimes/in-process-runtime.js";
import type { PlanningHandoffOutcome } from "../triage.js";
const IR = { version: "v2", name: "wf", columns: [], nodes: [], edges: [] } as unknown as WorkflowIr;
// `null` means the row vanished. NOT `undefined`: that would trigger the default
// parameter and silently hand the reaction a live task, turning the
// vanished-task case into a duplicate of the control.
function harness(task: Task | null = { id: "FN-1", column: "todo" } as Task) {
const seeded: string[] = [];
const kicks: string[] = [];
const logs: string[] = [];
return {
seeded,
kicks,
logs,
run: (outcome: PlanningHandoffOutcome) => reactToSpecificationComplete({
taskId: "FN-1",
outcome,
getTask: async () => task ?? undefined,
resolveIr: async () => IR,
seed: async (t) => { seeded.push(t.id); return { seeded: true }; },
kick: () => { kicks.push("kick"); },
log: (m) => { logs.push(m); },
}),
};
}
describe("specification-complete reaction arms a plan review only on a real handoff", () => {
it("arms the run when the card was RELEASED (the control)", async () => {
const h = harness();
await h.run("released");
expect(h.seeded).toEqual(["FN-1"]);
expect(h.kicks).toEqual(["kick"]);
expect(h.logs).toEqual(["Specified FN-1 → todo"]);
});
for (const outcome of ["parked", "withheld"] as const) {
it(`arms NOTHING when finalize reported ${outcome}`, async () => {
const h = harness();
await h.run(outcome);
expect(h.seeded).toEqual([]);
expect(h.kicks).toEqual([]);
});
it(`does not claim the card moved when finalize reported ${outcome}`, async () => {
// Truthfulness, not cosmetics: an operator reading "Specified → todo" for a
// card still sitting in the planner column is being told something false
// about their own board, and that log was the only trace of this path.
const h = harness();
await h.run(outcome);
expect(h.logs).toHaveLength(1);
expect(h.logs[0]).not.toContain("→ todo");
expect(h.logs[0]).toContain(outcome);
});
}
it("still respects an operator pause that lands after the release", async () => {
// Pre-existing guard, kept: the release happened, but the operator parked the
// card before the reaction ran. Nothing is armed.
const h = harness({ id: "FN-1", column: "todo", paused: true } as Task);
await h.run("released");
expect(h.seeded).toEqual([]);
expect(h.kicks).toEqual([]);
});
it("survives a task that vanished between the release and the reaction", async () => {
const h = harness(null);
await expect(h.run("released")).resolves.toBeUndefined();
expect(h.seeded).toEqual([]);
});
it("never resolves the workflow or reads the task for a non-release", async () => {
// The cheap assertion that catches a future "log it but still do the work"
// regression: a non-release must not even reach the store.
const getTask = vi.fn(async () => ({ id: "FN-1", column: "todo" } as Task));
const resolveIr = vi.fn(async () => IR);
await reactToSpecificationComplete({
taskId: "FN-1",
outcome: "parked",
getTask,
resolveIr,
seed: async () => ({ seeded: true }),
kick: () => {},
log: () => {},
});
expect(getTask).not.toHaveBeenCalled();
expect(resolveIr).not.toHaveBeenCalled();
});
});

79
packages/engine/src/runtimes/in-process-runtime.ts
View File

@@ -17,6 +17,7 @@ import type {
NotificationPayload,
WorkflowWorkItem,
WorkflowWorkItemState,
WorkflowIr,
} from "@fusion/core";
import {
AsyncCentralClaimStore,
@@ -29,6 +30,7 @@ import { Scheduler } from "../scheduler.js";
import type { PrMonitor, PrComment } from "../pr-monitor.js";
import type { PrInfo } from "@fusion/core";
import { TaskExecutor, type TaskExecutorOptions } from "../executor.js";
import type { PlanningHandoffOutcome } from "../triage.js";
import { buildPrNodeDeps } from "../pr-nodes.js";
import { isExperimentalFeatureEnabled } from "@fusion/core";
import { createCliAgentRuntime, type BootstrappedCliAgentRuntime } from "../cli-agent/runtime.js";
@@ -243,6 +245,62 @@ export function resolveParkedContinuationDeferral(
* prevents is a property of this bound, so the two belong in one place. */
export const DUE_PLANNING_CONTINUATION_BATCH_LIMIT = 20;
/** Everything the specification-complete reaction touches, injected so the
* reaction is exercisable without constructing a runtime. */
export interface SpecificationCompleteReactionDeps {
taskId: string;
outcome: PlanningHandoffOutcome;
getTask: (taskId: string) => Promise<Task | undefined>;
resolveIr: (taskId: string) => Promise<WorkflowIr>;
seed: (task: Task, ir: WorkflowIr) => Promise<{ seeded: boolean; reason?: string }>;
kick: () => void;
log: (message: string) => void;
}
/**
* FNXC:PlanningHandoffOutcome 2026-07-28-10:05 (U7 / R4, R5 — workflow-owned lifecycle):
* The engine's reaction to a finished specification: arm the graph's pre-release
* Plan Review run for a card that was actually handed off.
*
* WHAT WAS WRONG: this fired on every finished specification, because the seam that
* announces it fired unconditionally. So a card parked at the manual plan-approval
* gate — finalize writes `status: "awaiting-approval"` and RETURNS EARLY, before the
* release move — was logged as "Specified X → todo" and had a Plan Review run armed
* for a plan the operator had not approved. PR #2491 stopped the seeder from acting
* on that, defensively, at the seeder. This removes the reason it was ever asked.
*
* `released` is the ONLY outcome that licenses arming a run: it is the only one that
* means the card crossed into the hold column (or was already resting there) and is
* the graph's now. `parked` belongs to a human, `withheld` belongs to the caller's
* retry budget — arming a run for either is doing work nobody asked for.
*
* The log line reports the real outcome rather than asserting a move that may not
* have happened; an operator reading "Specified → todo" for a card sitting in the
* planner column is being told something false about their own board.
*
* EXTRACTED from the inline `onSpecifyComplete` callback for the same reason the
* continuation drain was in PR #2491: the callback is constructed inside
* `InProcessRuntime`, whose construction attaches to the real project registry, so
* no test could tell "the reaction respects the outcome" from "the reaction ignores
* it". A guard that cannot be shown to fail is not a guard.
*/
export async function reactToSpecificationComplete(
deps: SpecificationCompleteReactionDeps,
): Promise<void> {
if (deps.outcome !== "released") {
deps.log(
`Specification finished for ${deps.taskId} without a handoff (${deps.outcome}) — no plan review armed`,
);
return;
}
deps.log(`Specified ${deps.taskId} → todo`);
const live = await deps.getTask(deps.taskId);
if (!live || live.paused || live.userPaused) return;
const ir = await deps.resolveIr(live.id);
await deps.seed(live, ir);
deps.kick();
}
/** Everything the drain pass touches, injected so the pass is exercisable without
* constructing a runtime (which would attach to the real project registry). */
export interface DuePlanningContinuationDrainDeps {
@@ -1306,16 +1364,19 @@ export class InProcessRuntime
this.recordActivity();
runtimeLog.log(`Specifying ${t.id}...`);
},
onSpecifyComplete: (t) => {
onSpecifyComplete: (t, report) => {
// Activity is recorded for EVERY outcome: a planning session ran either
// way, and idle detection must not depend on whether it released.
this.recordActivity();
runtimeLog.log(`Specified ${t.id} → todo`);
void (async () => {
const live = await this.taskStore.getTask(t.id);
if (!live || live.paused || live.userPaused) return;
const ir = await resolveWorkflowIrForTask(this.taskStore, live.id);
await seedPreReleasePlanReviewContinuation(this.taskStore, live, ir);
this.kickWorkflowContinuationProcessor();
})().catch((error) => {
void reactToSpecificationComplete({
taskId: t.id,
outcome: report.outcome,
getTask: (id) => Promise.resolve(this.taskStore.getTask(id)),
resolveIr: (id) => resolveWorkflowIrForTask(this.taskStore, id),
seed: (task, ir) => seedPreReleasePlanReviewContinuation(this.taskStore, task, ir),
kick: () => this.kickWorkflowContinuationProcessor(),
log: (message) => runtimeLog.log(message),
}).catch((error) => {
runtimeLog.error(`Failed to start Todo plan review for ${t.id}:`, error);
});
},

38
packages/engine/src/triage.ts
View File

@@ -216,7 +216,21 @@ export interface TriageProcessorOptions {
/** Stuck task detector — monitors triage sessions for stagnation and triggers recovery. */
stuckTaskDetector?: StuckTaskDetector;
onSpecifyStart?: (task: Task) => void;
onSpecifyComplete?: (task: Task) => void;
/*
FNXC:PlanningHandoffOutcome 2026-07-28-10:05 (U7 / R4, R5 — workflow-owned lifecycle):
The reaction is told WHAT FINALIZE DID, not merely that specification stopped.
It fired unconditionally before, so a card parked at the manual plan-approval
gate — or one whose release move was refused — was announced as specified, and
the subscriber logged "Specified X -> todo" and armed a Plan Review run for a
card that had not moved.
The event still fires on every outcome. Dropping it for a non-release would also
drop the subscriber's activity/idle signal, and a reaction that silently does not
happen is harder to reason about than one that happens with an accurate payload.
R5's division of labour: the seam announces, the SUBSCRIBER decides what a given
outcome licenses.
*/
onSpecifyComplete?: (task: Task, report: PlanningHandoffReport) => void;
onSpecifyError?: (task: Task, error: Error) => void;
onAgentText?: (taskId: string, delta: string) => void;
/** AgentStore for resolving per-agent custom instructions. */
@@ -2482,11 +2496,12 @@ export class TriageProcessor {
// FN-5220: planning agents that emit a `DUPLICATE: FN-NNNN` redirect
// short-circuit normal spec finalization.
const duplicateReport: PlanningHandoffReport = { outcome: "parked" };
if (await this.tryFinalizeExplicitDuplicateMarker(task, written, settings, {
isReplan,
feedback,
})) {
this.options.onSpecifyComplete?.(task);
}, duplicateReport)) {
this.options.onSpecifyComplete?.(task, duplicateReport);
return;
}
@@ -2535,11 +2550,11 @@ export class TriageProcessor {
return;
}
await this.finalizeApprovedTask(task, written, settings, {
const finalizeReport = await this.finalizeApprovedTask(task, written, settings, {
isReplan,
feedback,
});
this.options.onSpecifyComplete?.(task);
this.options.onSpecifyComplete?.(task, finalizeReport);
} finally {
this.activeSessions.delete(task.id);
stuckDetector?.untrackTask(task.id);
@@ -3200,6 +3215,15 @@ export class TriageProcessor {
isReplan?: boolean;
feedback?: string;
} = {},
/*
FNXC:PlanningHandoffOutcome 2026-07-28-10:20 (U7):
Finalize's outcome is reported through this ref rather than by widening the
return type. The boolean return answers a DIFFERENT question — "was this a
duplicate marker at all?" — and 16 existing tests assert it directly. Folding
two questions into one return would have made every one of those an expectation
edit, which is how a behavior change gets to travel disguised as churn.
*/
report: PlanningHandoffReport = { outcome: "parked" },
): Promise<boolean> {
try {
const explicitDuplicateMarker = parseExplicitDuplicateMarker(written);
@@ -3225,7 +3249,9 @@ export class TriageProcessor {
} else {
planLog.log(`${task.id} explicit duplicate marker detected — redirecting to ${canonicalId}`);
}
await this.finalizeApprovedTask(task, written, settings, options);
// FNXC:PlanningHandoffOutcome 2026-07-28-10:20: surface what finalize did to
// the caller's reaction without changing what this method's boolean means.
report.outcome = (await this.finalizeApprovedTask(task, written, settings, options)).outcome;
return true;
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);