fix(engine): quiet graph review-entry audits and label engine aborts truthfully
Recognise workflow-graph moves into in-review so gate entry no longer emits handoff-invariant violations, and split pause-abort provenance so engine teardowns are engine-abort instead of hard-cancel.
This commit is contained in:
7
.changeset/quiet-graph-review-entry-audit.md
Normal file
7
.changeset/quiet-graph-review-entry-audit.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
summary: Stop logging a false handoff-invariant violation every time a task enters a review gate.
|
||||
category: fix
|
||||
dev: `moves.ts` now recognises `workflowMoveSource: "workflow-graph"` (set only by the executor's column boundary) as a legitimate entry into `in-review` via the shared `isRecognizedInReviewEntry` predicate, used by both the backend and SQLite `task:handoff-invariant-violation` emit sites. Non-graph movers (operator drags, engine/self-healing moves, foreign provenance values) still emit the audit unchanged.
|
||||
7
.changeset/truthful-abort-provenance.md
Normal file
7
.changeset/truthful-abort-provenance.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
summary: Task logs no longer report engine-initiated aborts as operator "hard-cancel" pauses.
|
||||
category: fix
|
||||
dev: `awaitAbortInFlightTaskWork` derives pause-abort provenance from `options.userCanceled` — operator withdrawals keep `hard-cancel`, engine/lifecycle teardowns get the new `engine-abort` member of `PausedAbortProvenance`. Benign-abort classifiers in `handleGraphFailure` accept both via `isGenericAbortProvenance()`, so recovery behaviour is unchanged.
|
||||
159
packages/core/src/__tests__/in-review-entry-audit.test.ts
Normal file
159
packages/core/src/__tests__/in-review-entry-audit.test.ts
Normal file
@@ -0,0 +1,159 @@
|
||||
import { beforeAll, beforeEach, afterEach, afterAll, it, expect } from "vitest";
|
||||
import {
|
||||
pgDescribe,
|
||||
createSharedPgTaskStoreTestHarness,
|
||||
type SharedPgTaskStoreHarness,
|
||||
} from "../__test-utils__/pg-test-harness.js";
|
||||
|
||||
/*
|
||||
FNXC:WorkflowReviewGates 2026-07-26-16:40:
|
||||
`task:handoff-invariant-violation` guards the requirement "a card only arrives in `in-review`
|
||||
through a recognised authority". When it was written, `TaskStore.handoffToReview(...)` was the ONLY
|
||||
such authority. After the U1 IR-driven lifecycle cutover the workflow GRAPH also owns column
|
||||
transitions, and moving the pre-merge review gates (`code-review`, `browser-verification`) into the
|
||||
`in-review` column made the graph cross that boundary on EVERY gate entry — so a fully-provenanced,
|
||||
legitimate transition emitted a violation audit each time (observed on FN-8596 at 15:19:22).
|
||||
|
||||
Invariant asserted here (not just the FN-8596 repro): entry into `in-review` audits a violation for
|
||||
exactly the movers that lack a recognised authority. Surface enumeration —
|
||||
- graph-owned crossing (`workflowMoveSource: "workflow-graph"`, the executor column-boundary shape):
|
||||
NO violation, from both `in-progress` (the gate entry) and `todo`-side WIP;
|
||||
- graph-owned crossing that re-enters `in-review` after a remediation bounce: still NO violation
|
||||
(the FN-8596 report was a repeat crossing, not a first one);
|
||||
- operator drag (`moveSource: "user"`, no provenance): violation STILL emitted;
|
||||
- engine/self-healing style move (`moveSource: "engine"`, no graph provenance): violation STILL
|
||||
emitted — this is the class the invariant was written for and must not be silenced;
|
||||
- a foreign `workflowMoveSource` value: violation STILL emitted (only the graph's own literal is
|
||||
recognised);
|
||||
- explicit `allowDirectInReviewMove: true` opt-out: unchanged, no violation;
|
||||
- `handoffToReview(...)`: unchanged — no violation, and it still records `task:handoff`.
|
||||
*/
|
||||
|
||||
const VIOLATION = "task:handoff-invariant-violation";
|
||||
|
||||
pgDescribe("in-review entry audit (handoff invariant)", () => {
|
||||
const h: SharedPgTaskStoreHarness = createSharedPgTaskStoreTestHarness({
|
||||
prefix: "fusion_ir_entry_audit",
|
||||
});
|
||||
|
||||
beforeAll(h.beforeAll);
|
||||
beforeEach(h.beforeEach);
|
||||
afterEach(h.afterEach);
|
||||
afterAll(h.afterAll);
|
||||
|
||||
const violationsFor = async (taskId: string): Promise<unknown[]> => {
|
||||
const store = h.store();
|
||||
return store.getRunAuditEventsAsync({ taskId, mutationType: VIOLATION });
|
||||
};
|
||||
|
||||
/** Create a task and park it in `in-progress`, the column the graph's review gates enter from. */
|
||||
const seedInProgress = async (description: string): Promise<string> => {
|
||||
const store = h.store();
|
||||
const task = await store.createTask({ description });
|
||||
await store.moveTask(task.id, "todo", { moveSource: "user" });
|
||||
await store.moveTask(task.id, "in-progress", { moveSource: "user" });
|
||||
return task.id;
|
||||
};
|
||||
|
||||
it("does not audit a violation for a graph-owned crossing into in-review", async () => {
|
||||
const store = h.store();
|
||||
const id = await seedInProgress("graph-owned review-gate entry");
|
||||
|
||||
// Exact option shape emitted by executor.buildColumnBoundaryHooks / onNodeEntry.
|
||||
const moved = await store.moveTask(id, "in-review", {
|
||||
moveSource: "engine",
|
||||
workflowMoveSource: "workflow-graph",
|
||||
bypassGuards: true,
|
||||
preserveProgress: true,
|
||||
workflowMoveMetadata: { fromColumn: "in-progress", nodeId: "browser-verification" },
|
||||
});
|
||||
|
||||
expect(moved.column).toBe("in-review");
|
||||
expect(await violationsFor(id)).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("does not audit a violation when the graph re-enters in-review after a remediation bounce", async () => {
|
||||
const store = h.store();
|
||||
const id = await seedInProgress("graph re-entry after remediation");
|
||||
|
||||
await store.moveTask(id, "in-review", {
|
||||
moveSource: "engine",
|
||||
workflowMoveSource: "workflow-graph",
|
||||
bypassGuards: true,
|
||||
preserveProgress: true,
|
||||
});
|
||||
// Remediation node re-enters in-progress, then the gate is crossed a second time.
|
||||
await store.moveTask(id, "in-progress", {
|
||||
moveSource: "engine",
|
||||
workflowMoveSource: "workflow-graph",
|
||||
bypassGuards: true,
|
||||
preserveProgress: true,
|
||||
});
|
||||
const reentered = await store.moveTask(id, "in-review", {
|
||||
moveSource: "engine",
|
||||
workflowMoveSource: "workflow-graph",
|
||||
bypassGuards: true,
|
||||
preserveProgress: true,
|
||||
});
|
||||
|
||||
expect(reentered.column).toBe("in-review");
|
||||
expect(await violationsFor(id)).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("still audits a violation for an operator drag straight into in-review", async () => {
|
||||
const store = h.store();
|
||||
const id = await seedInProgress("operator drag into review");
|
||||
|
||||
const moved = await store.moveTask(id, "in-review", { moveSource: "user" });
|
||||
|
||||
expect(moved.column).toBe("in-review");
|
||||
const violations = await violationsFor(id);
|
||||
expect(violations).toHaveLength(1);
|
||||
expect((violations[0] as { metadata?: { fromColumn?: string } }).metadata?.fromColumn).toBe("in-progress");
|
||||
});
|
||||
|
||||
it("still audits a violation for an engine move with no graph provenance", async () => {
|
||||
const store = h.store();
|
||||
const id = await seedInProgress("engine move with no provenance");
|
||||
|
||||
await store.moveTask(id, "in-review", { moveSource: "engine", bypassGuards: true });
|
||||
|
||||
expect(await violationsFor(id)).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("still audits a violation for a foreign workflowMoveSource", async () => {
|
||||
const store = h.store();
|
||||
const id = await seedInProgress("foreign workflow move source");
|
||||
|
||||
await store.moveTask(id, "in-review", {
|
||||
moveSource: "engine",
|
||||
workflowMoveSource: "self-healing-advanced-triage",
|
||||
bypassGuards: true,
|
||||
});
|
||||
|
||||
expect(await violationsFor(id)).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("keeps the explicit allowDirectInReviewMove opt-out silent", async () => {
|
||||
const store = h.store();
|
||||
const id = await seedInProgress("explicit direct-move opt-out");
|
||||
|
||||
await store.moveTask(id, "in-review", { moveSource: "user", allowDirectInReviewMove: true });
|
||||
|
||||
expect(await violationsFor(id)).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("leaves handoffToReview unaffected: no violation, and task:handoff is still recorded", async () => {
|
||||
const store = h.store();
|
||||
const id = await seedInProgress("handoff to review");
|
||||
|
||||
const handed = await store.handoffToReview(id, {
|
||||
ownerAgentId: null,
|
||||
evidence: { reason: "fn_task_done", runId: "run-1", agentId: "agent-1" },
|
||||
});
|
||||
|
||||
expect(handed.column).toBe("in-review");
|
||||
expect(await violationsFor(id)).toHaveLength(0);
|
||||
expect(await store.getRunAuditEventsAsync({ taskId: id, mutationType: "task:handoff" })).toHaveLength(1);
|
||||
});
|
||||
});
|
||||
@@ -92,6 +92,39 @@ function resolveTransitionColumnFacts(ir: WorkflowIr, columnId: string): Transit
|
||||
};
|
||||
}
|
||||
|
||||
/*
|
||||
FNXC:WorkflowReviewGates 2026-07-26-16:40:
|
||||
Single authority for "is this move a RECOGNISED entry into `in-review`?", consumed by both the
|
||||
backend-transaction and SQLite-transaction emit sites of `task:handoff-invariant-violation`.
|
||||
|
||||
Requirement history: the invariant dates from when `TaskStore.handoffToReview(...)` was the ONLY
|
||||
legal way into `in-review`, so any other arrival was genuinely suspicious and worth auditing. After
|
||||
the U1 IR-driven lifecycle cutover the workflow GRAPH owns column transitions — node column
|
||||
assignment is the authority and the graph column boundary (`workflow-column-boundary.ts`
|
||||
`onNodeEntry`) performs the move via `store.moveTask`. Moving the pre-merge review gates
|
||||
(`code-review`, `browser-verification`) into the `in-review` column then made the graph cross that
|
||||
boundary on EVERY gate entry, so a legitimate, fully-provenanced transition emitted a violation
|
||||
audit on each crossing (observed on FN-8596 15:19:22: a violation immediately followed by the
|
||||
`browser-verification` `task:column-transition`).
|
||||
|
||||
The fix is narrow ON PURPOSE: the invariant is NOT retired, because it still catches the movers it
|
||||
was written for — operator drags, merge bounces, self-healing rehomes, and any future call site
|
||||
that lands a card in review without going through handoff. Only a move carrying the graph's own
|
||||
provenance is recognised. `workflowMoveSource: "workflow-graph"` is produced at four call sites,
|
||||
ALL inside the executor's own graph / column-boundary machinery (the boundary hook plus the
|
||||
graph-owned merge-boundary moves). No non-graph mover writes that literal — operator drags carry
|
||||
`moveSource:"user"` and recovery sweeps use their own provenance (e.g.
|
||||
`"self-healing-advanced-triage"`) — so neither can spoof it.
|
||||
*/
|
||||
function isRecognizedInReviewEntry(
|
||||
options: MoveTaskOptions | undefined,
|
||||
internal: MoveTaskInternalOptions,
|
||||
): boolean {
|
||||
if (internal.fromHandoff) return true;
|
||||
if (options?.allowDirectInReviewMove === true) return true;
|
||||
return options?.workflowMoveSource === "workflow-graph";
|
||||
}
|
||||
|
||||
/*
|
||||
FNXC:WorkflowCapacity 2026-07-19-10:35:
|
||||
Shared pooled-capacity enforcement (U4/KTD-9/KTD-10), extracted from the async
|
||||
@@ -994,7 +1027,9 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum
|
||||
// Dequeue from merge queue on column exit (if leaving in-review).
|
||||
await dequeueMergeQueueOnColumnExitInTransaction(tx, id, fromColumn, toColumn, movedAt);
|
||||
|
||||
if (toColumn === "in-review" && !internal.fromHandoff && options?.allowDirectInReviewMove !== true) {
|
||||
// FNXC:WorkflowReviewGates 2026-07-26-16:40: see isRecognizedInReviewEntry — a
|
||||
// graph-owned crossing into the review column is a legitimate arrival, not a violation.
|
||||
if (toColumn === "in-review" && !isRecognizedInReviewEntry(options, internal)) {
|
||||
await recordRunAuditEventWithinTransaction(tx, {
|
||||
taskId: id,
|
||||
agentId: internal.runContext?.agentId ?? "system",
|
||||
@@ -1128,7 +1163,9 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum
|
||||
);
|
||||
}
|
||||
|
||||
if (toColumn === "in-review" && !internal.fromHandoff && options?.allowDirectInReviewMove !== true) {
|
||||
// FNXC:WorkflowReviewGates 2026-07-26-16:40: SQLite-path twin of the backend emit site
|
||||
// above; both consult the one isRecognizedInReviewEntry authority so they cannot drift.
|
||||
if (toColumn === "in-review" && !isRecognizedInReviewEntry(options, internal)) {
|
||||
store.insertRunAuditEventRow({
|
||||
taskId: id,
|
||||
agentId: internal.runContext?.agentId,
|
||||
|
||||
265
packages/engine/src/__tests__/executor-abort-provenance.test.ts
Normal file
265
packages/engine/src/__tests__/executor-abort-provenance.test.ts
Normal file
@@ -0,0 +1,265 @@
|
||||
/*
|
||||
FNXC:WorkflowLifecycle 2026-07-26-11:20:
|
||||
KB-PROV regression suite. Original symptom (FN-8596, production): the graph's `code-review-remediation`
|
||||
node moved a card in-review -> in-progress, `performWorkflowRerunBounce` then bounced it
|
||||
in-progress -> todo -> in-progress to re-dispatch, and the operator saw
|
||||
`Pause abort marked: provenance=hard-cancel source=abort-in-flight:parent moved from in-progress to todo`
|
||||
even though the move source was "engine" and `userCanceled` was correctly false. `hard-cancel` is the
|
||||
provenance AGENTS.md reserves for the operator Move-Task hard cancel, so the log (and any future consumer
|
||||
branching on the label) read an engine bounce as an operator withdrawal.
|
||||
|
||||
Surface enumeration — every `awaitAbortInFlightTaskWork` entry point, asserted below, not just the one
|
||||
reported bounce:
|
||||
ENGINE (must be `engine-abort`): engine-sourced `from === "in-progress"` move (the FN-8596 repro),
|
||||
engine-sourced move out of a planning lane, archive disposal, workspace-archive disposal, task pause,
|
||||
approval-gate suspension, `abortAllInFlight` (shutdown/global stop), stuck-kill force-requeue.
|
||||
OPERATOR (must stay `hard-cancel`): the registered move disposer (user in-progress -> todo),
|
||||
user-sourced move out of a planning lane, task soft-delete.
|
||||
And the invariant that motivated the split must not cost behaviour: the benign-abort classifiers in
|
||||
`handleGraphFailure` exist FOR the engine case, so they must accept `engine-abort` exactly as they
|
||||
accepted the old catch-all `hard-cancel`.
|
||||
*/
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import "./executor-test-helpers.js";
|
||||
import { TaskExecutor } from "../executor.js";
|
||||
import { createMockStore, resetExecutorMocks } from "./executor-test-helpers.js";
|
||||
import type { TaskDetail } from "@fusion/core";
|
||||
|
||||
const now = "2026-07-26T00:00:00.000Z";
|
||||
|
||||
function makeTask(overrides: Partial<TaskDetail> = {}): TaskDetail {
|
||||
return {
|
||||
id: "FN-8596",
|
||||
title: "abort provenance repro",
|
||||
description: "KB-PROV: engine bounce must not be labeled hard-cancel",
|
||||
column: "in-progress",
|
||||
dependencies: [],
|
||||
steps: [{ name: "Implement", status: "pending" }],
|
||||
currentStep: 0,
|
||||
log: [],
|
||||
branch: null,
|
||||
baseBranch: "main",
|
||||
worktree: "/tmp/fusion-kb-prov",
|
||||
status: null,
|
||||
error: null,
|
||||
paused: false,
|
||||
userPaused: false,
|
||||
autoMerge: true,
|
||||
mergeRetries: 0,
|
||||
createdAt: now,
|
||||
updatedAt: now,
|
||||
...overrides,
|
||||
} as TaskDetail;
|
||||
}
|
||||
|
||||
function makeExecutor(taskOverrides: Partial<TaskDetail> = {}) {
|
||||
const store = createMockStore();
|
||||
const task = makeTask(taskOverrides);
|
||||
store.getTask.mockResolvedValue(task);
|
||||
store.getSettings.mockResolvedValue({
|
||||
maxConcurrent: 2,
|
||||
maxWorktrees: 4,
|
||||
pollIntervalMs: 15000,
|
||||
autoMerge: true,
|
||||
maxAutoMergeRetries: 3,
|
||||
});
|
||||
store.recordRunAuditEvent = vi.fn();
|
||||
const executor = new TaskExecutor(store, "/tmp/test", {});
|
||||
return { store, task, executor };
|
||||
}
|
||||
|
||||
function provenanceOf(executor: TaskExecutor, taskId: string): string | undefined {
|
||||
return (executor as any).pausedAbortProvenance.get(taskId);
|
||||
}
|
||||
|
||||
function logText(store: ReturnType<typeof createMockStore>): string {
|
||||
return store.logEntry.mock.calls.map((call: unknown[]) => call[1]).join("\n");
|
||||
}
|
||||
|
||||
/** Every write the executor made to the row, so `userPaused` can be asserted negatively. */
|
||||
function updatePatches(store: ReturnType<typeof createMockStore>): Record<string, unknown>[] {
|
||||
return store.updateTask.mock.calls.map((call: unknown[]) => (call[1] ?? {}) as Record<string, unknown>);
|
||||
}
|
||||
|
||||
describe("pause-abort provenance truthfulness (KB-PROV)", () => {
|
||||
beforeEach(() => {
|
||||
resetExecutorMocks();
|
||||
vi.useFakeTimers();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.useRealTimers();
|
||||
});
|
||||
|
||||
describe("awaitAbortInFlightTaskWork derives provenance from userCanceled", () => {
|
||||
it("labels an engine-initiated abort `engine-abort`, never `hard-cancel`", async () => {
|
||||
const { store, task, executor } = makeExecutor();
|
||||
|
||||
await executor.awaitAbortInFlightTaskWork(task.id, "parent moved from in-progress to todo");
|
||||
|
||||
expect(provenanceOf(executor, task.id)).toBe("engine-abort");
|
||||
expect(provenanceOf(executor, task.id)).not.toBe("hard-cancel");
|
||||
expect(logText(store)).toContain("provenance=engine-abort");
|
||||
expect(logText(store)).not.toContain("provenance=hard-cancel");
|
||||
// Engine rebounds must not claim operator withdrawal (AGENTS.md Move-Task contract).
|
||||
expect((executor as any).userCanceledTaskIds.has(task.id)).toBe(false);
|
||||
});
|
||||
|
||||
it("keeps `hard-cancel` for an operator-canceled abort", async () => {
|
||||
const { store, task, executor } = makeExecutor();
|
||||
|
||||
await executor.awaitAbortInFlightTaskWork(task.id, "user moved task from in-progress to todo", {
|
||||
userCanceled: true,
|
||||
});
|
||||
|
||||
expect(provenanceOf(executor, task.id)).toBe("hard-cancel");
|
||||
expect(logText(store)).toContain("provenance=hard-cancel");
|
||||
expect((executor as any).userCanceledTaskIds.has(task.id)).toBe(true);
|
||||
});
|
||||
|
||||
it("treats an explicit `userCanceled: false` as engine provenance", async () => {
|
||||
const { task, executor } = makeExecutor();
|
||||
|
||||
await executor.awaitAbortInFlightTaskWork(task.id, "engine bounce", { userCanceled: false });
|
||||
|
||||
expect(provenanceOf(executor, task.id)).toBe("engine-abort");
|
||||
});
|
||||
|
||||
it("never sets userPaused on either path (engine rebounds and the abort itself are not pauses)", async () => {
|
||||
const engine = makeExecutor();
|
||||
await engine.executor.awaitAbortInFlightTaskWork(engine.task.id, "engine bounce");
|
||||
const user = makeExecutor();
|
||||
await user.executor.awaitAbortInFlightTaskWork(user.task.id, "user cancel", { userCanceled: true });
|
||||
|
||||
for (const store of [engine.store, user.store]) {
|
||||
expect(updatePatches(store).some((patch) => "userPaused" in patch)).toBe(false);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("surface enumeration: each abort caller gets a truthful label", () => {
|
||||
it("FN-8596 repro — an ENGINE-sourced in-progress -> todo move labels the abort `engine-abort`", async () => {
|
||||
const { store, task, executor } = makeExecutor();
|
||||
|
||||
await store._triggerAsync("task:moved", { task, from: "in-progress", to: "todo", source: "engine" });
|
||||
await (executor as any).pendingTaskDisposals.get(task.id);
|
||||
|
||||
expect(provenanceOf(executor, task.id)).toBe("engine-abort");
|
||||
expect(logText(store)).toContain(
|
||||
"provenance=engine-abort source=abort-in-flight:parent moved from in-progress to todo",
|
||||
);
|
||||
expect(logText(store)).not.toContain("provenance=hard-cancel");
|
||||
expect((executor as any).userCanceledTaskIds.has(task.id)).toBe(false);
|
||||
});
|
||||
|
||||
it("a USER-sourced in-progress -> todo move still labels the abort `hard-cancel`", async () => {
|
||||
const { store, task, executor } = makeExecutor();
|
||||
|
||||
await store._triggerAsync("task:moved", { task, from: "in-progress", to: "todo", source: "user" });
|
||||
await (executor as any).pendingTaskDisposals.get(task.id);
|
||||
|
||||
expect(provenanceOf(executor, task.id)).toBe("hard-cancel");
|
||||
expect((executor as any).userCanceledTaskIds.has(task.id)).toBe(true);
|
||||
});
|
||||
|
||||
it("an ENGINE-sourced move out of a planning lane labels the abort `engine-abort`", async () => {
|
||||
const { store, task, executor } = makeExecutor({ column: "todo" });
|
||||
|
||||
await store._triggerAsync("task:moved", { task, from: "todo", to: "ideas", source: "engine" });
|
||||
await (executor as any).pendingTaskDisposals.get(task.id);
|
||||
|
||||
expect(provenanceOf(executor, task.id)).toBe("engine-abort");
|
||||
});
|
||||
|
||||
it("a USER-sourced move out of a planning lane stays `hard-cancel`", async () => {
|
||||
const { store, task, executor } = makeExecutor({ column: "todo" });
|
||||
|
||||
await store._triggerAsync("task:moved", { task, from: "todo", to: "ideas", source: "user" });
|
||||
await (executor as any).pendingTaskDisposals.get(task.id);
|
||||
|
||||
expect(provenanceOf(executor, task.id)).toBe("hard-cancel");
|
||||
});
|
||||
|
||||
it("soft-delete is an operator withdrawal and stays `hard-cancel`", async () => {
|
||||
const { store, task, executor } = makeExecutor();
|
||||
|
||||
await store._triggerAsync("task:deleted", task);
|
||||
await (executor as any).pendingTaskDisposals.get(task.id);
|
||||
|
||||
expect(provenanceOf(executor, task.id)).toBe("hard-cancel");
|
||||
expect((executor as any).userCanceledTaskIds.has(task.id)).toBe(true);
|
||||
});
|
||||
|
||||
it("archive disposal is engine lifecycle, not an operator cancel", async () => {
|
||||
const { store, task, executor } = makeExecutor();
|
||||
|
||||
await store._triggerAsync("task:moved", { task, from: "in-progress", to: "archived", source: "engine" });
|
||||
await (executor as any).pendingTaskDisposals.get(task.id);
|
||||
|
||||
expect(provenanceOf(executor, task.id)).toBe("engine-abort");
|
||||
expect((executor as any).userCanceledTaskIds.has(task.id)).toBe(false);
|
||||
});
|
||||
|
||||
it("abortAllInFlight (shutdown / global stop) labels every task `engine-abort`", async () => {
|
||||
const { task, executor } = makeExecutor();
|
||||
(executor as any).activeSessions.set(task.id, { dispose: vi.fn() });
|
||||
|
||||
await executor.abortAllInFlight("engine shutdown");
|
||||
|
||||
expect(provenanceOf(executor, task.id)).toBe("engine-abort");
|
||||
});
|
||||
});
|
||||
|
||||
describe("behaviour is unchanged: benign classifiers still accept the engine label", () => {
|
||||
/*
|
||||
The classifiers this split touches (isBenignInReviewPauseAbort, isBenignManualMergeHoldPauseAbort,
|
||||
isReentrantPausedAbortedInFlightNode, the stale plan/parse replays) were written against the old
|
||||
catch-all and exist FOR engine aborts. If the split had narrowed them to `hard-cancel`, every benign
|
||||
engine abort would have been re-parked as an operator-action failure — so assert the recovery path
|
||||
fires under both labels.
|
||||
*/
|
||||
async function invokeGraphFailure(executor: TaskExecutor, task: TaskDetail) {
|
||||
await (executor as any).handleGraphFailure(task, {
|
||||
disposition: "failed",
|
||||
outcome: "failure",
|
||||
visitedNodeIds: ["plan", "execute"],
|
||||
context: {},
|
||||
});
|
||||
}
|
||||
|
||||
it.each(["engine-abort", "hard-cancel"] as const)(
|
||||
"auto-continues a benign todo pause-abort under provenance '%s' (no failed park)",
|
||||
async (provenance) => {
|
||||
const { store, task, executor } = makeExecutor({ column: "todo", worktree: "/tmp/fusion-kb-prov" });
|
||||
(executor as any).markPausedAborted(task.id, provenance);
|
||||
(executor as any).addActiveWorktree(task.id, task.worktree);
|
||||
vi.spyOn(executor as any, "execute").mockResolvedValue(undefined);
|
||||
|
||||
await invokeGraphFailure(executor, task);
|
||||
|
||||
expect(updatePatches(store).some((patch) => patch.status === "failed")).toBe(false);
|
||||
expect(logText(store)).toContain("auto-continuing the agent session");
|
||||
},
|
||||
);
|
||||
|
||||
it.each(["engine-abort", "hard-cancel"] as const)(
|
||||
"classifies a clean completed in-review row as a benign pause-abort under provenance '%s'",
|
||||
async (provenance) => {
|
||||
const { executor } = makeExecutor();
|
||||
const live = makeTask({
|
||||
column: "in-review",
|
||||
steps: [{ name: "Implement", status: "done" }],
|
||||
});
|
||||
const benign = (executor as any).isBenignInReviewPauseAbort(
|
||||
live,
|
||||
{ disposition: "failed", outcome: "failure", visitedNodeIds: ["plan", "execute"], context: {} },
|
||||
provenance,
|
||||
true,
|
||||
false,
|
||||
);
|
||||
|
||||
expect(benign).toBe(true);
|
||||
},
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -289,6 +289,26 @@ import { createFallbackModelObserver } from "./fallback-model-observer.js";
|
||||
import { recordRetry } from "./retry-burned-logger.js";
|
||||
import type { AgentActionGateContext } from "./agent-action-gate.js";
|
||||
|
||||
/*
|
||||
FNXC:WorkflowLifecycle 2026-07-26-11:20:
|
||||
KB-PROV: Provenance of a pause/abort marker, in one named union so the ~10 signatures that pass it around cannot drift apart.
|
||||
|
||||
- `hard-cancel` — OPERATOR withdrawal only. AGENTS.md "Move-Task contract": user `moveTask(in-progress -> todo)`, task soft-delete, and a user-sourced move out of a planning lane. These carry `userCanceled: true` into `awaitAbortInFlightTaskWork`.
|
||||
- `engine-abort` — ENGINE/lifecycle teardown with no operator intent: workflow rerun bounces, archive disposal, approval-gate suspension, engine-sourced moves, `abortAllInFlight` (shutdown/global stop), stuck-kill force-requeue. Before KB-PROV these were mislabeled `hard-cancel`.
|
||||
- `global-pause` / `merge-seam` / `completion-finalize` — unchanged FN-6568/FN-6625 seams.
|
||||
|
||||
`hard-cancel` and `engine-abort` are the two "generic" aborts; test them together with `isGenericAbortProvenance()`.
|
||||
*/
|
||||
export type PausedAbortProvenance = "global-pause" | "merge-seam" | "hard-cancel" | "engine-abort" | "completion-finalize";
|
||||
|
||||
/*
|
||||
FNXC:WorkflowLifecycle 2026-07-26-11:20:
|
||||
KB-PROV: The benign-abort classifiers in handleGraphFailure were written against the pre-split `hard-cancel` catch-all and exist PRECISELY to recover engine-initiated aborts (FN-6796, FN-6735, FN-7143, FN-7214, FN-7749). Splitting the label must not narrow them, so every former `=== "hard-cancel"` test routes through this predicate. Operator intent is still discriminated where it matters by `userCanceledTaskIds` / `live.userPaused`, never by the label alone.
|
||||
*/
|
||||
function isGenericAbortProvenance(provenance: PausedAbortProvenance | undefined): boolean {
|
||||
return provenance === "hard-cancel" || provenance === "engine-abort";
|
||||
}
|
||||
|
||||
// Re-export for backward compatibility (tests import from executor.ts)
|
||||
export { summarizeToolArgs } from "./agent-logger.js";
|
||||
export {
|
||||
@@ -1878,8 +1898,13 @@ export class TaskExecutor {
|
||||
*
|
||||
* FNXC:WorkflowLifecycle 2026-06-17-23:31:
|
||||
* FN-6625 adds completion-finalize provenance for the FN-6614 symptom where a completed/no-commit execution already handed off to in-review, then a trailing graph abort looked like a pause/resume engine abort and re-parked the task failed. Completion-finalize is sibling provenance to FN-6568 merge-seam, not operator pause intent.
|
||||
*
|
||||
* FNXC:WorkflowLifecycle 2026-07-26-11:20:
|
||||
* KB-PROV: `hard-cancel` had become a catch-all bucket: `awaitAbortInFlightTaskWork` stamped it unconditionally, so an ENGINE-initiated teardown was labeled with the provenance AGENTS.md reserves for the operator Move-Task hard cancel ("User moveTask(in-progress -> todo) is a hard cancel ... Engine rebounds must not set userPaused"). Observed on FN-8596: the graph's own `performWorkflowRerunBounce` (in-progress -> todo -> in-progress re-dispatch, moveSource "engine") logged `provenance=hard-cancel source=abort-in-flight:parent moved from in-progress to todo` even though `userCanceled` was correctly false and `userPaused` was never set. Behaviour was right, the LABEL lied.
|
||||
*
|
||||
* `engine-abort` splits that bucket: `hard-cancel` now means ONLY an operator withdrawal (`options.userCanceled === true`), `engine-abort` means an engine/lifecycle teardown. Both are "generic" (non-global-pause, non-merge-seam, non-completion-finalize) aborts, so every downstream classifier that used to accept `hard-cancel` must accept BOTH via `isGenericAbortProvenance()` — those classifiers exist FOR the engine case (see FN-6796's note that "an engine restart/pause-resume abort reaches graph-failure handling as `hard-cancel` provenance even when no user canceled the task") and discriminate real user intent through `userCanceledTaskIds`, not through the provenance label. Narrowing them to `hard-cancel` alone would strand benign engine aborts as operator-action failures.
|
||||
*/
|
||||
private pausedAbortProvenance = new Map<string, "global-pause" | "merge-seam" | "hard-cancel" | "completion-finalize">();
|
||||
private pausedAbortProvenance = new Map<string, PausedAbortProvenance>();
|
||||
/**
|
||||
* FNXC:WorkflowLifecycle 2026-06-18-10:56:
|
||||
* FN-6644 makes completed/no-commit finalize-to-review state durable beyond volatile pause provenance. FN-6641 showed FN-6625 was incomplete because teardown can re-mark `completion-finalize` as `hard-cancel`; this marker keeps the already-finalized handoff from being re-parked as an operator-action pause abort while preserving genuine live pauses and active hard-cancels.
|
||||
@@ -1931,7 +1956,7 @@ export class TaskExecutor {
|
||||
|
||||
private markPausedAborted(
|
||||
taskId: string,
|
||||
provenance: "global-pause" | "merge-seam" | "hard-cancel" | "completion-finalize" = "hard-cancel",
|
||||
provenance: PausedAbortProvenance = "hard-cancel",
|
||||
source = "unspecified",
|
||||
): void {
|
||||
const previousProvenance = this.pausedAbortProvenance.get(taskId);
|
||||
@@ -2810,7 +2835,11 @@ export class TaskExecutor {
|
||||
if (options.userCanceled) {
|
||||
this.userCanceledTaskIds.add(taskId);
|
||||
}
|
||||
this.markPausedAborted(taskId, "hard-cancel", `abort-in-flight:${reason}`);
|
||||
/*
|
||||
FNXC:WorkflowLifecycle 2026-07-26-11:20:
|
||||
KB-PROV: Stamp the provenance the caller actually reported instead of a blanket `hard-cancel`. `options.userCanceled` is already the truthful operator-intent signal every caller computes (`source === "user"`, soft-delete, the registered move disposer), so derive the label from it: operator withdrawal keeps `hard-cancel`, everything else is an `engine-abort`. Without this, the FN-8596 engine rerun bounce told the operator `provenance=hard-cancel` for work the engine itself re-dispatched, and any future consumer branching on `hard-cancel` would read an engine bounce as an operator withdrawal. Behaviour is unchanged: `userPaused` is still never set by engine rebounds, and the downstream classifiers accept both labels via `isGenericAbortProvenance()`.
|
||||
*/
|
||||
this.markPausedAborted(taskId, options.userCanceled ? "hard-cancel" : "engine-abort", `abort-in-flight:${reason}`);
|
||||
this.options.stuckTaskDetector?.untrackTask(taskId);
|
||||
this.clearWorkflowRerunWatchdog(taskId);
|
||||
this.clearCompletedTaskWatchdog(taskId);
|
||||
@@ -9712,7 +9741,7 @@ export class TaskExecutor {
|
||||
private async isRetryableBenignMergePauseAbort(
|
||||
live: TaskDetail,
|
||||
result: WorkflowGraphTaskRunResult,
|
||||
abortProvenance: "global-pause" | "merge-seam" | "hard-cancel" | "completion-finalize" | undefined,
|
||||
abortProvenance: PausedAbortProvenance | undefined,
|
||||
pausedAborted: boolean,
|
||||
): Promise<boolean> {
|
||||
/*
|
||||
@@ -9746,16 +9775,19 @@ export class TaskExecutor {
|
||||
private isBenignInReviewPauseAbort(
|
||||
live: TaskDetail,
|
||||
result: WorkflowGraphTaskRunResult,
|
||||
abortProvenance: "global-pause" | "merge-seam" | "hard-cancel" | "completion-finalize" | undefined,
|
||||
abortProvenance: PausedAbortProvenance | undefined,
|
||||
pausedAborted: boolean,
|
||||
userCanceled: boolean,
|
||||
): boolean {
|
||||
/*
|
||||
FNXC:WorkflowLifecycle 2026-06-20-00:00:
|
||||
FN-6796: an engine restart/pause-resume abort reaches graph-failure handling as `hard-cancel` provenance even when no user canceled the task. A clean completed `in-review` row in that shape is already handed off for review and must not be stranded with the operator-action pause-abort marker; the discriminator is the in-memory `userCanceledTaskIds` set plus the resting column and clean row state, while global/user pause, merge-seam, terminal merge values, merge-confirmed partial landings, and pre-existing status/error still park exactly as before.
|
||||
FNXC:WorkflowLifecycle 2026-07-26-11:20:
|
||||
KB-PROV: post-split the engine case arrives as `engine-abort` and an operator withdrawal as `hard-cancel`; this classifier still accepts BOTH (`isGenericAbortProvenance`) because the `userCanceled` guard below — not the label — is the load-bearing operator-intent discriminator FN-6796 designed. Narrowing to `engine-abort` would change behaviour for the operator path.
|
||||
|
||||
FN-6796: an engine restart/pause-resume abort reaches graph-failure handling as `hard-cancel`/`engine-abort` provenance even when no user canceled the task. A clean completed `in-review` row in that shape is already handed off for review and must not be stranded with the operator-action pause-abort marker; the discriminator is the in-memory `userCanceledTaskIds` set plus the resting column and clean row state, while global/user pause, merge-seam, terminal merge values, merge-confirmed partial landings, and pre-existing status/error still park exactly as before.
|
||||
*/
|
||||
if (!pausedAborted) return false;
|
||||
if (abortProvenance !== "hard-cancel") return false;
|
||||
if (!isGenericAbortProvenance(abortProvenance)) return false;
|
||||
if (userCanceled) return false;
|
||||
if (live.column !== "in-review") return false;
|
||||
if (live.userPaused === true) return false;
|
||||
@@ -9780,15 +9812,15 @@ export class TaskExecutor {
|
||||
private async isBenignManualMergeHoldPauseAbort(
|
||||
live: TaskDetail,
|
||||
result: WorkflowGraphTaskRunResult,
|
||||
abortProvenance: "global-pause" | "merge-seam" | "hard-cancel" | "completion-finalize" | undefined,
|
||||
abortProvenance: PausedAbortProvenance | undefined,
|
||||
pausedAborted: boolean,
|
||||
): Promise<boolean> {
|
||||
/*
|
||||
FNXC:WorkflowLifecycle 2026-07-09-14:54:
|
||||
FN-7749 / Runfusion#1979: with auto-merge off, a manual merge hold is the healthy `in-review` resting state for Merge & Close. A benign hard-cancel pause/resume abort at any merge-region node must not park the task failed; FN-5147 forbids moving, failing, or re-enqueueing the row, so this classifier only permits preserving `in-review` and clearing a stale pause-abort status/error.
|
||||
FN-7749 / Runfusion#1979: with auto-merge off, a manual merge hold is the healthy `in-review` resting state for Merge & Close. A benign generic (`hard-cancel`/`engine-abort`, KB-PROV 2026-07-26) pause/resume abort at any merge-region node must not park the task failed; FN-5147 forbids moving, failing, or re-enqueueing the row, so this classifier only permits preserving `in-review` and clearing a stale pause-abort status/error.
|
||||
*/
|
||||
if (!pausedAborted) return false;
|
||||
if (abortProvenance !== "hard-cancel") return false;
|
||||
if (!isGenericAbortProvenance(abortProvenance)) return false;
|
||||
if (live.paused || live.userPaused === true) return false;
|
||||
if (live.column !== "in-review") return false;
|
||||
if (live.mergeDetails?.mergeConfirmed === true) return false;
|
||||
@@ -9812,7 +9844,7 @@ export class TaskExecutor {
|
||||
private async handleStaleInReviewPlanPauseAbortReplay(
|
||||
live: TaskDetail,
|
||||
result: WorkflowGraphTaskRunResult,
|
||||
abortProvenance: "global-pause" | "merge-seam" | "hard-cancel" | "completion-finalize" | undefined,
|
||||
abortProvenance: PausedAbortProvenance | undefined,
|
||||
pausedAborted: boolean,
|
||||
userCanceled: boolean,
|
||||
): Promise<boolean> {
|
||||
@@ -9821,7 +9853,7 @@ export class TaskExecutor {
|
||||
FN-7143 showed that a stale graph lifecycle replay can surface at `plan` after an in-review pause/resume even though planning is not actually running anymore. Plan is not a safe re-entry point for review rows, typed or generic, so this classifier is clear/log-only: preserve in-review, never route to triage/todo, and keep genuine user/global pauses plus real plan failures on the operator-action path.
|
||||
*/
|
||||
if (!pausedAborted) return false;
|
||||
if (abortProvenance !== "hard-cancel" && abortProvenance !== "global-pause") return false;
|
||||
if (!isGenericAbortProvenance(abortProvenance) && abortProvenance !== "global-pause") return false;
|
||||
if (userCanceled) return false;
|
||||
if (live.column !== "in-review") return false;
|
||||
if (live.paused || live.userPaused === true) return false;
|
||||
@@ -9884,7 +9916,7 @@ export class TaskExecutor {
|
||||
private async handleStaleInReviewParsePauseAbortReplay(
|
||||
live: TaskDetail,
|
||||
result: WorkflowGraphTaskRunResult,
|
||||
abortProvenance: "global-pause" | "merge-seam" | "hard-cancel" | "completion-finalize" | undefined,
|
||||
abortProvenance: PausedAbortProvenance | undefined,
|
||||
pausedAborted: boolean,
|
||||
userCanceled: boolean,
|
||||
): Promise<boolean> {
|
||||
@@ -9893,7 +9925,7 @@ export class TaskExecutor {
|
||||
A stale in-review pause/resume replay at `parse` is not an operator action. Unlike `plan`, parse is a safe workflow re-entry point for review rows, so auto-retry the graph with the shared transient resume budget and suppress the parked failure notification.
|
||||
*/
|
||||
if (!pausedAborted) return false;
|
||||
if (abortProvenance !== "hard-cancel" && abortProvenance !== "global-pause") return false;
|
||||
if (!isGenericAbortProvenance(abortProvenance) && abortProvenance !== "global-pause") return false;
|
||||
if (userCanceled) return false;
|
||||
if (live.column !== "in-review") return false;
|
||||
if (live.paused || live.userPaused === true) return false;
|
||||
@@ -9990,7 +10022,7 @@ export class TaskExecutor {
|
||||
private async isReentrantPausedAbortedInFlightNode(
|
||||
live: TaskDetail,
|
||||
result: WorkflowGraphTaskRunResult,
|
||||
abortProvenance: "global-pause" | "merge-seam" | "hard-cancel" | "completion-finalize" | undefined,
|
||||
abortProvenance: PausedAbortProvenance | undefined,
|
||||
pausedAborted: boolean,
|
||||
userCanceled: boolean,
|
||||
): Promise<boolean> {
|
||||
@@ -10002,7 +10034,7 @@ export class TaskExecutor {
|
||||
A global engine pause aborts active workflow graph controllers with `global-pause` provenance; after the global pause is lifted, the typed interrupted-node marker is sufficient to re-enter that node. Only active global-pause settings and explicit task/user pauses remain terminal so resume never runs behind an operator-controlled pause.
|
||||
*/
|
||||
if (!pausedAborted) return false;
|
||||
if (abortProvenance !== "hard-cancel" && abortProvenance !== "global-pause") return false;
|
||||
if (!isGenericAbortProvenance(abortProvenance) && abortProvenance !== "global-pause") return false;
|
||||
if (userCanceled) return false;
|
||||
if (live.paused || live.userPaused === true) return false;
|
||||
if (live.status != null || live.error != null) return false;
|
||||
@@ -10035,7 +10067,7 @@ export class TaskExecutor {
|
||||
private async reenterPausedAbortedWorkflowNode(
|
||||
live: TaskDetail,
|
||||
result: WorkflowGraphTaskRunResult,
|
||||
abortProvenance: "global-pause" | "merge-seam" | "hard-cancel" | "completion-finalize" | undefined,
|
||||
abortProvenance: PausedAbortProvenance | undefined,
|
||||
): Promise<boolean> {
|
||||
const nodeId = result.interruptedNodeId ?? result.visitedNodeIds[result.visitedNodeIds.length - 1] ?? "unknown";
|
||||
const priorRetries = live.graphResumeRetryCount ?? 0;
|
||||
@@ -10116,13 +10148,13 @@ export class TaskExecutor {
|
||||
private async routeGraphMergeFailureToRetry(
|
||||
live: TaskDetail,
|
||||
result: WorkflowGraphTaskRunResult,
|
||||
abortProvenance: "global-pause" | "merge-seam" | "hard-cancel" | "completion-finalize" | undefined,
|
||||
abortProvenance: PausedAbortProvenance | undefined,
|
||||
): Promise<boolean> {
|
||||
if (!this.mergeRequester) return false;
|
||||
/* FNXC:WorkflowMerge 2026-07-12-17:38: FN-1165 defense in depth — implementation-incomplete merge graph failures must never reach the merge requester, because a no-branch task can otherwise be finalized as an intentional no-op. */
|
||||
if (this.graphFailureValue(result) === "implementation-incomplete") return false;
|
||||
const failedNode = result.visitedNodeIds[result.visitedNodeIds.length - 1] ?? "unknown";
|
||||
const message = `Workflow graph merge failure at node '${failedNode}' routed to bounded auto-merge retry${abortProvenance === "merge-seam" ? " after merge-seam abort" : abortProvenance === "hard-cancel" || abortProvenance === undefined ? " after benign pause/resume abort" : ""}`;
|
||||
const message = `Workflow graph merge failure at node '${failedNode}' routed to bounded auto-merge retry${abortProvenance === "merge-seam" ? " after merge-seam abort" : isGenericAbortProvenance(abortProvenance) || abortProvenance === undefined ? " after benign pause/resume abort" : ""}`;
|
||||
executorLog.warn(`${live.id}: ${message}`);
|
||||
await this.store.logEntry(live.id, message, undefined, this.getRunContextFor(live.id));
|
||||
try {
|
||||
@@ -10524,7 +10556,8 @@ export class TaskExecutor {
|
||||
: "task pause";
|
||||
// Typed discriminant for the engine-internal abort case (mirrors the
|
||||
// `pauseProvenance === "engine abort during pause/resume"` arm above):
|
||||
// a hard-cancel teardown that is NOT a user pause or global pause. Used
|
||||
// a generic (`hard-cancel`/`engine-abort`, KB-PROV 2026-07-26) teardown that is
|
||||
// NOT a user pause or global pause. Used
|
||||
// to gate the auto-continue branch so the gate cannot silently drift if
|
||||
// the human-readable provenance label is ever revised.
|
||||
const isEngineInternalAbort =
|
||||
@@ -10681,7 +10714,7 @@ export class TaskExecutor {
|
||||
benign teardown, not an operator problem. The live-acceptance repro:
|
||||
the workflow merge boundary hard-cancels the in-flight executor
|
||||
session when it moves the task in-progress → in-review
|
||||
(abort-in-flight provenance=hard-cancel), the AI merge then lands and
|
||||
(abort-in-flight provenance=engine-abort, formerly hard-cancel — KB-PROV 2026-07-26), the AI merge then lands and
|
||||
the task advances to done — and only afterwards does the aborted
|
||||
graph run reach this sink, where it logged "Workflow graph failure
|
||||
surfaced ... operator action required; retry or explicitly
|
||||
@@ -20143,8 +20176,10 @@ You have access to the file system to review changes.${inlineFixBlock}${verdictB
|
||||
executorLog.warn(`${taskId}: spawned child cleanup failed during force-requeue: ${err instanceof Error ? err.message : String(err)}`);
|
||||
});
|
||||
await this.awaitAbortInFlightTaskWork(taskId, "force-requeue after stuck-kill unwind timeout");
|
||||
// awaitAbortInFlightTaskWork marks pausedAborted as a generic hard-cancel
|
||||
// signal. The force-requeue path has already handled the task move, so
|
||||
// awaitAbortInFlightTaskWork marks pausedAborted as a generic abort
|
||||
// signal (KB-PROV 2026-07-26: `engine-abort`, since the force-requeue is
|
||||
// engine-initiated and passes no `userCanceled`).
|
||||
// The force-requeue path has already handled the task move, so
|
||||
// clear it to prevent a later subprocess unwind from logging/moving as a pause.
|
||||
this.clearPausedAborted(taskId);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user