From 8288e4a8aba9ac1e0ce0266191cfc3ab7c4e8fdf Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Tue, 28 Jul 2026 17:15:29 -0700 Subject: [PATCH] U7 PR3: the specification reaction acts on what finalize DID, not on the fact that planning stopped (#2506) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .changeset/specify-complete-reaction.md | 7 + .../specify-complete-reaction.test.ts | 133 ++++++++++++++++++ .../engine/src/runtimes/in-process-runtime.ts | 79 +++++++++-- packages/engine/src/triage.ts | 38 ++++- 4 files changed, 242 insertions(+), 15 deletions(-) create mode 100644 .changeset/specify-complete-reaction.md create mode 100644 packages/engine/src/__tests__/specify-complete-reaction.test.ts diff --git a/.changeset/specify-complete-reaction.md b/.changeset/specify-complete-reaction.md new file mode 100644 index 0000000000..f943ebd6e6 --- /dev/null +++ b/.changeset/specify-complete-reaction.md @@ -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. diff --git a/packages/engine/src/__tests__/specify-complete-reaction.test.ts b/packages/engine/src/__tests__/specify-complete-reaction.test.ts new file mode 100644 index 0000000000..473c785e0e --- /dev/null +++ b/packages/engine/src/__tests__/specify-complete-reaction.test.ts @@ -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(); + }); +}); diff --git a/packages/engine/src/runtimes/in-process-runtime.ts b/packages/engine/src/runtimes/in-process-runtime.ts index c3caffaf67..84ae3f2069 100644 --- a/packages/engine/src/runtimes/in-process-runtime.ts +++ b/packages/engine/src/runtimes/in-process-runtime.ts @@ -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; + resolveIr: (taskId: string) => Promise; + 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 { + 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); }); }, diff --git a/packages/engine/src/triage.ts b/packages/engine/src/triage.ts index 93c0981830..92229e62ca 100644 --- a/packages/engine/src/triage.ts +++ b/packages/engine/src/triage.ts @@ -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 { 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);