diff --git a/.changeset/fn-7736-approval-hold.md b/.changeset/fn-7736-approval-hold.md new file mode 100644 index 0000000000..2f4a8352c1 --- /dev/null +++ b/.changeset/fn-7736-approval-hold.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Recovery and oversight now wait for approval-blocked tasks instead of resuming them early. +category: fix +dev: Adds canonical `awaiting-approval` pause reason + `isTaskBlockedOnApproval` predicate; excludes the hold from paused-scope-decay rebound and keeps the planner overseer withholding (FN-7736). diff --git a/docs/architecture.md b/docs/architecture.md index b10c798745..d723e97769 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1540,20 +1540,27 @@ are implemented elsewhere. FN-7514 supplies the comprehensive human-control safeguard the FN-7512/FN-7513 layers deferred: the overseer must be fully inert — no steering, retry, targeted-fix, or FN-7513 confirmation-required action (merge/PR progression, destructive/external-service side effect) may fire, and no pending confirmation -may even be recorded — whenever a task is (a) user-paused, or (b) ineligible for auto-merge processing -per the FN-5147 `autoMerge:false` / PR-based human-review terminal contract. +may even be recorded — whenever a task is (a) user-paused, (b) blocked on a pending human approval +decision (FN-7736), or (c) ineligible for auto-merge processing per the FN-5147 `autoMerge:false` / +PR-based human-review terminal contract. `packages/engine/src/overseer-human-control-policy.ts` exports the pure predicate `evaluateOverseerHumanControl(task, settings)` (no I/O, mirrors the `recovery-policy.ts` style), returning -`{ withhold: boolean; reason?: "user-paused" | "auto-merge-off-human-review" }`. It reuses -`allowsAutoMergeProcessing` from `@fusion/core` VERBATIM for the auto-merge-off half — never re-derives -the predicate inline. For the pause half, it distinguishes: +`{ withhold: boolean; reason?: "user-paused" | "approval-blocked" | "auto-merge-off-human-review" }`. It +reuses `allowsAutoMergeProcessing` from `@fusion/core` VERBATIM for the auto-merge-off half, and +`isTaskBlockedOnApproval` from `@fusion/core` VERBATIM for the approval-hold half (checked FIRST, ahead of +the user-pause/auto-merge-off checks — see the approval-hold contract below) — never re-derives either +predicate inline. For the pause half, it distinguishes: - Explicit user pause: `task.userPaused === true`, OR `task.paused === true` with NO `task.pausedReason` (the `fn_task_pause` tool / `TaskStore.pauseTask` never stamps a `pausedReason`). - Engine/self-healing park (NOT user pause): `task.paused === true` WITH a `pausedReason` (every self-healing park path — branch-conflict-unrecoverable, token_budget_exceeded, in-review-stall-deadlock, worktrunk_operation_failed, etc. — always stamps one). +- Approval hold (also NOT user pause, checked before the user-pause branch above so it is never + shadowed): `task.paused === true` WITH `task.pausedReason === "awaiting-approval"` (the canonical + `AWAITING_APPROVAL_PAUSE_REASON`), OR `task.status === "awaiting-approval"`. See the approval-hold + contract section below for the full rationale. `PlannerRecoveryController.tick()` (`planner-recovery-controller.ts`) consults this guard FIRST — before the snapshot lookup, before `decidePlannerRecovery`, before FN-7513's confirmation classification. When @@ -1574,6 +1581,53 @@ same settings self-healing already gates lifecycle mutation on. **Downstream ownership (not this layer):** the dashboard UI/badges surfacing withheld state (FN-7515+), a persisted intervention timeline (FN-7519), and richer run-audit/activity presentation (FN-7520). +### Approval-hold contract for recovery and oversight (FN-7736) + +A task can be blocked awaiting a human approval decision via two distinct mechanisms, and every +automated recovery (self-healing) and oversight (planner overseer) path must refuse to rebound, requeue, +resume, re-plan, or otherwise advance a task while either hold is active: + +1. **Triage plan-approval gate** — parks the task spec at `status: "awaiting-approval"`. Already a member + of `HARD_BLOCKING_TASK_STATUSES` (`packages/core/src/task-merge.ts`) and preserved by + `GHOST_REVIEW_PRESERVED_STATUSES` in `self-healing.ts`; `POST /api/tasks/:id/approve-plan` is the only + route that clears it. +2. **Runtime tool-approval gate** — a gated tool call parks a RUNNING task via `pauseForApproval` + (`executor.ts` and `agent-heartbeat.ts`), which calls `store.pauseTask(taskId, true, ..., { + pausedByAgentId, pausedReason: AWAITING_APPROVAL_PAUSE_REASON })`. Before FN-7736 this call stamped + only `paused: true` with NO durable `pausedReason`, so recovery/oversight code keying on + `pausedReason` could not recognize the hold, and self-healing's `autoReboundPausedScopeDecay` could + rebound the held task back to `todo` (defeating the approval gate) once a follower task's scope-decay + threshold elapsed. + +`packages/core/src/task-merge.ts` exports the canonical `AWAITING_APPROVAL_PAUSE_REASON = +"awaiting-approval"` constant and the shared pure predicate `isTaskBlockedOnApproval(task)`, which returns +`true` for EITHER hold shape (`task.paused === true && task.pausedReason === +AWAITING_APPROVAL_PAUSE_REASON`, or `task.status === "awaiting-approval"`). Both are re-exported via +`index.ts`/`index.gate.ts` for engine consumption. Consulted at: + +- **`SelfHealingManager.PAUSED_SCOPE_DECAY_EXCLUDED_REASONS`** (`self-healing.ts`) — includes + `AWAITING_APPROVAL_PAUSE_REASON` alongside `branch-conflict-unrecoverable`, `worktrunk_operation_failed`, + and `token_budget_exceeded`, so `autoReboundPausedScopeDecay` skips an approval-held holder task even + when a follower is present and the age threshold has elapsed. +- **`classifyPausedAbortWorkflowRecovery`** (`self-healing.ts`) — already skips any `task.paused === true` + row before this task; the approval-hold shape is covered by that generic guard (regression-tested + explicitly rather than relying on the incidental coverage). +- **`evaluateOverseerHumanControl`** (`overseer-human-control-policy.ts`) — checks `isTaskBlockedOnApproval` + FIRST, ahead of the accidental "paused with no reason" user-pause heuristic, returning + `{ withhold: true, reason: "approval-blocked" }`. This ordering is load-bearing: once FN-7736 started + stamping a durable `pausedReason` on the tool-approval hold, the old no-reason heuristic would stop + matching it — the explicit `approval-blocked` branch prevents that from silently flipping the overseer + from withhold to act. +- **Every other self-healing sweep** that gates on generic `task.paused === true` (or `task.paused || + task.userPaused`) before taking a lifecycle-advancing action already skips an approval-held task + regardless of `pausedReason`, and needed no change. + +`TaskStore.pauseTask(id, paused, runContext?, agentOptions?)` accepts an optional `agentOptions.pausedReason` +seam (widened for FN-7736) so the durable reason lands atomically with the pause write, avoiding a second +racy `updateTask` call; unpausing (`pauseTask(id, false)`, used by the approval-resolution route) clears +the caller-supplied `pausedReason` the same way it already clears `pausedByAgentId`/`userPaused`, so the +hold is not sticky once the operator decides. + ### Planner overseer runtime-state exposure (FN-7531) /* diff --git a/packages/core/src/__tests__/store-persistence.test.ts b/packages/core/src/__tests__/store-persistence.test.ts index 247ab2f398..de28919286 100644 --- a/packages/core/src/__tests__/store-persistence.test.ts +++ b/packages/core/src/__tests__/store-persistence.test.ts @@ -416,6 +416,24 @@ describe("TaskStore", () => { expect(updated.paused).toBe(true); expect(updated.pausedByAgentId).toBe("agent-other"); }); + + // FN-7736: pauseTask's agentOptions.pausedReason seam durably stamps WHY a + // task was paused (e.g. the canonical awaiting-approval reason) and the + // reason is cleared on unpause, matching pausedByAgentId/userPaused. + it("stamps pausedReason via agentOptions and clears it on unpause", async () => { + const task = await harness.store().createTask({ description: "Approval-held task" }); + const paused = await harness.store().pauseTask(task.id, true, undefined, { pausedByAgentId: "agent-1", pausedReason: "awaiting-approval" }); + + expect(paused.paused).toBe(true); + expect(paused.pausedReason).toBe("awaiting-approval"); + + const detail = await harness.store().getTask(task.id); + expect(detail.pausedReason).toBe("awaiting-approval"); + + const unpaused = await harness.store().pauseTask(task.id, false); + expect(unpaused.paused).toBeFalsy(); + expect(unpaused.pausedReason).toBeUndefined(); + }); }); describe("branch field persistence", () => { diff --git a/packages/core/src/__tests__/task-merge.test.ts b/packages/core/src/__tests__/task-merge.test.ts index 7e369d50eb..3cec6b2f68 100644 --- a/packages/core/src/__tests__/task-merge.test.ts +++ b/packages/core/src/__tests__/task-merge.test.ts @@ -5,6 +5,8 @@ import { HARD_BLOCKING_TASK_STATUSES, SCHEDULER_TRANSIENT_STATUSES, TASK_DONE_BYPASS_BLOCKER_MESSAGE, + AWAITING_APPROVAL_PAUSE_REASON, + isTaskBlockedOnApproval, getTaskCompletionBlocker, getTaskDoneBypassBlocker, getTaskHardMergeBlocker, @@ -875,3 +877,35 @@ describe("getTaskCompletionBlocker", () => { .resolves.toBe("task has unresolved dependencies: FN-999"); }); }); + +// FN-7736: isTaskBlockedOnApproval covers both approval-hold shapes (pause-reason +// and awaiting-approval status) and must not false-positive on a bare user pause. +describe("isTaskBlockedOnApproval", () => { + it("is true when paused with the canonical approval pause reason", () => { + expect(isTaskBlockedOnApproval({ paused: true, pausedReason: AWAITING_APPROVAL_PAUSE_REASON, status: undefined })).toBe(true); + }); + + it("is true when status is awaiting-approval regardless of paused", () => { + expect(isTaskBlockedOnApproval({ paused: false, pausedReason: undefined, status: "awaiting-approval" })).toBe(true); + }); + + it("is true when both the pause-reason and status shapes are present", () => { + expect(isTaskBlockedOnApproval({ paused: true, pausedReason: AWAITING_APPROVAL_PAUSE_REASON, status: "awaiting-approval" })).toBe(true); + }); + + it("is false for a task with neither hold shape", () => { + expect(isTaskBlockedOnApproval({ paused: false, pausedReason: undefined, status: undefined })).toBe(false); + }); + + it("is false for a bare user pause (paused true, no reason) — must not conflate with approval hold", () => { + expect(isTaskBlockedOnApproval({ paused: true, pausedReason: undefined, status: undefined })).toBe(false); + }); + + it("is false when paused with a different (non-approval) pause reason", () => { + expect(isTaskBlockedOnApproval({ paused: true, pausedReason: "branch-conflict-unrecoverable", status: undefined })).toBe(false); + }); + + it("is false when pausedReason is the approval reason but paused is not true", () => { + expect(isTaskBlockedOnApproval({ paused: false, pausedReason: AWAITING_APPROVAL_PAUSE_REASON, status: undefined })).toBe(false); + }); +}); diff --git a/packages/core/src/index.gate.ts b/packages/core/src/index.gate.ts index cb24589c42..00459b006f 100644 --- a/packages/core/src/index.gate.ts +++ b/packages/core/src/index.gate.ts @@ -897,6 +897,8 @@ export { resolveEffectiveAutoMerge, resolveEffectiveGroupAutoMerge, resolveTaskMergeTarget, + AWAITING_APPROVAL_PAUSE_REASON, + isTaskBlockedOnApproval, type MergeTargetResolution, type MergeTargetResolverOptions, } from "./task-merge.js"; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 8c2a414cc1..a6e4125c6e 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -906,6 +906,8 @@ export { resolveEffectiveAutoMerge, resolveEffectiveGroupAutoMerge, resolveTaskMergeTarget, + AWAITING_APPROVAL_PAUSE_REASON, + isTaskBlockedOnApproval, type MergeTargetResolution, type MergeTargetResolverOptions, } from "./task-merge.js"; diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index bdf9a1c44b..90c1d8a027 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -9693,11 +9693,23 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS} * Pause or unpause a task. Paused tasks are excluded from all automated * agent and scheduler interaction. Logs the action and emits `task:updated`. */ + /* + * FNXC:ApprovalHold 2026-07-09-00:05: + * FN-7736: `agentOptions.pausedReason` is the minimal seam for durably + * stamping WHY a task was paused (e.g. the canonical + * `AWAITING_APPROVAL_PAUSE_REASON` from a tool-approval gate). Widening + * this existing options bag avoids a second, racy `updateTask` write right + * after `pauseTask` — the reason lands atomically with the pause itself. + * On unpause the caller-supplied reason is cleared here (mirroring how + * `pausedByAgentId`/`userPaused` are already cleared below); sweep-set + * built-in reasons like `branch-conflict-unrecoverable` are cleared by + * their own dedicated resume code paths and are unaffected. + */ async pauseTask( id: string, paused: boolean, runContext?: RunMutationContext, - agentOptions?: { pausedByAgentId?: string }, + agentOptions?: { pausedByAgentId?: string; pausedReason?: string }, ): Promise { return this.withTaskLock(id, async () => { const dir = this.taskDir(id); @@ -9713,9 +9725,13 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS} if (paused && agentOptions?.pausedByAgentId) { task.pausedByAgentId = agentOptions.pausedByAgentId; } + if (paused && agentOptions?.pausedReason) { + task.pausedReason = agentOptions.pausedReason; + } if (!paused) { task.pausedByAgentId = undefined; task.userPaused = undefined; + task.pausedReason = undefined; } // When pausing an in-progress/in-review task, set status so the UI can show the state. // When unpausing, clear the "paused" status. diff --git a/packages/core/src/task-merge.ts b/packages/core/src/task-merge.ts index 4a7fe50abf..23dd93b914 100644 --- a/packages/core/src/task-merge.ts +++ b/packages/core/src/task-merge.ts @@ -141,6 +141,40 @@ export function resolveTaskMergeTarget( return { branch: legacyFallback, source: "legacy-main", rejected }; } +/* + * FNXC:ApprovalHold 2026-07-09-00:00: + * FN-7736: two distinct mechanisms park a task on a pending human approval — + * (1) the triage plan-approval gate sets `task.status === "awaiting-approval"` + * (already a HARD_BLOCKING_TASK_STATUSES member below), and (2) a gated tool + * call parks a RUNNING task via `pauseForApproval` -> `store.pauseTask(id, + * true, ...)`, which historically only set `paused:true` with no durable + * `pausedReason`, so recovery/oversight code keying on `pausedReason` could + * not recognize it and at least one sweep (self-healing's + * `autoReboundPausedScopeDecay`) could rebound the held task back to `todo` + * before the operator ever decided. `AWAITING_APPROVAL_PAUSE_REASON` is the + * canonical, durable marker both `executor.ts` and `agent-heartbeat.ts` + * `pauseForApproval` now stamp via `TaskStore.pauseTask`'s `pausedReason` + * option, and `isTaskBlockedOnApproval` is the single shared predicate core + * and engine code must consult before rebounding, requeuing, resuming, + * re-planning, or otherwise advancing a task — it must return `true` for + * EITHER hold shape so callers never have to special-case which mechanism + * parked the task. + */ +export const AWAITING_APPROVAL_PAUSE_REASON = "awaiting-approval"; + +/** + * Returns true when `task` is blocked on a pending human approval decision, + * via either hold mechanism (see FNXC:ApprovalHold above). Every automated + * recovery (self-healing) and oversight (planner overseer) path must treat + * `true` as "take no lifecycle-advancing action on this task". + */ +export function isTaskBlockedOnApproval( + task: Pick, +): boolean { + if (task.paused === true && task.pausedReason === AWAITING_APPROVAL_PAUSE_REASON) return true; + return task.status === "awaiting-approval"; +} + export const HARD_BLOCKING_TASK_STATUSES = new Set([ "failed", // ── User-attention / awaiting-handoff states ───────────────────────── diff --git a/packages/engine/src/__tests__/executor-approval-gate-suspend.test.ts b/packages/engine/src/__tests__/executor-approval-gate-suspend.test.ts index 3efc51ad3d..d76a123b9f 100644 --- a/packages/engine/src/__tests__/executor-approval-gate-suspend.test.ts +++ b/packages/engine/src/__tests__/executor-approval-gate-suspend.test.ts @@ -61,7 +61,10 @@ describe("TaskExecutor.buildActionGateContext pauseForApproval", () => { await gateContext.pauseForApproval({ approvalRequestId: "apr-1", decision }); - expect(store.pauseTask).toHaveBeenCalledWith("FN-1", true, undefined, expect.objectContaining({ pausedByAgentId: expect.any(String) })); + // FN-7736: pauseForApproval must durably stamp the canonical + // AWAITING_APPROVAL_PAUSE_REASON so recovery/oversight code can recognize + // this hold via isTaskBlockedOnApproval (not just paused:true). + expect(store.pauseTask).toHaveBeenCalledWith("FN-1", true, undefined, expect.objectContaining({ pausedByAgentId: expect.any(String), pausedReason: "awaiting-approval" })); expect(store.logEntry).toHaveBeenCalled(); // Session suspension must be triggered synchronously (called, not merely // scheduled for some later tick) as part of this same pauseForApproval diff --git a/packages/engine/src/__tests__/heartbeat-executor.test.ts b/packages/engine/src/__tests__/heartbeat-executor.test.ts index b5653a1b9a..fefdff86d3 100644 --- a/packages/engine/src/__tests__/heartbeat-executor.test.ts +++ b/packages/engine/src/__tests__/heartbeat-executor.test.ts @@ -758,7 +758,10 @@ describe("executeHeartbeat", () => { }, }); - expect(pauseTask).toHaveBeenCalledWith("FN-001", true, undefined, { pausedByAgentId: "agent-001" }); + // FN-7736: heartbeat pauseForApproval must mirror executor.ts and stamp + // the canonical AWAITING_APPROVAL_PAUSE_REASON on the task, not just the + // agent's pauseReason. + expect(pauseTask).toHaveBeenCalledWith("FN-001", true, undefined, { pausedByAgentId: "agent-001", pausedReason: "awaiting-approval" }); expect((store.updateAgentState as any)).toHaveBeenCalledWith("agent-001", "paused"); expect((store.updateAgent as any)).toHaveBeenCalledWith("agent-001", { pauseReason: "awaiting-approval" }); }); diff --git a/packages/engine/src/__tests__/overseer-human-control-policy.test.ts b/packages/engine/src/__tests__/overseer-human-control-policy.test.ts index 9171e6e1b6..4769bbbf21 100644 --- a/packages/engine/src/__tests__/overseer-human-control-policy.test.ts +++ b/packages/engine/src/__tests__/overseer-human-control-policy.test.ts @@ -10,6 +10,7 @@ function task(overrides: Partial = {}): OverseerHumanC userPaused: undefined, paused: undefined, pausedReason: undefined, + status: undefined, autoMerge: undefined, prInfo: undefined, prInfos: undefined, @@ -83,4 +84,47 @@ describe("evaluateOverseerHumanControl", () => { const decision = evaluateOverseerHumanControl(task({ userPaused: true }), settings({ autoMerge: false })); expect(decision).toEqual({ withhold: true, reason: "user-paused" }); }); + + // FN-7736: the planner overseer must keep withholding for a task blocked on + // a pending human approval decision, via either hold mechanism, and must + // NOT regress FN-7514's accidental "paused with no reason" hold once the + // canonical durable reason is introduced. + describe("approval hold (FN-7736)", () => { + it("withholds with reason approval-blocked for the canonical pause-reason hold", () => { + const decision = evaluateOverseerHumanControl( + task({ paused: true, pausedReason: "awaiting-approval" }), + settings(), + ); + expect(decision).toEqual({ withhold: true, reason: "approval-blocked" }); + }); + + it("withholds with reason approval-blocked for the status-based hold (triage plan-approval gate)", () => { + const decision = evaluateOverseerHumanControl( + task({ paused: false, status: "awaiting-approval" }), + settings(), + ); + expect(decision).toEqual({ withhold: true, reason: "approval-blocked" }); + }); + + it("withholds approval-blocked (not auto-merge-off-human-review) even when settings.autoMerge is false", () => { + const decision = evaluateOverseerHumanControl( + task({ paused: true, pausedReason: "awaiting-approval" }), + settings({ autoMerge: false }), + ); + expect(decision).toEqual({ withhold: true, reason: "approval-blocked" }); + }); + + it("does not classify a bare user pause (no reason) as approval-blocked -- still user-paused", () => { + const decision = evaluateOverseerHumanControl(task({ paused: true, pausedReason: undefined }), settings()); + expect(decision).toEqual({ withhold: true, reason: "user-paused" }); + }); + + it("does not classify an engine park with a different reason as approval-blocked", () => { + const decision = evaluateOverseerHumanControl( + task({ paused: true, pausedReason: "branch-conflict-unrecoverable" }), + settings(), + ); + expect(decision).toEqual({ withhold: false }); + }); + }); }); diff --git a/packages/engine/src/__tests__/reliability-interactions/paused-scope-decay.test.ts b/packages/engine/src/__tests__/reliability-interactions/paused-scope-decay.test.ts index 584ea724ba..6751a400ac 100644 --- a/packages/engine/src/__tests__/reliability-interactions/paused-scope-decay.test.ts +++ b/packages/engine/src/__tests__/reliability-interactions/paused-scope-decay.test.ts @@ -123,6 +123,8 @@ describe("reliability interactions: paused scope decay", () => { { name: "excluded paused reason", holder: { paused: true, pausedReason: "branch-conflict-unrecoverable" as const } }, { name: "not paused", holder: { paused: false } }, { name: "age below threshold", holder: { paused: true }, settings: { pausedScopeDecayMs: 60_000 }, ageMs: 500 }, + // FN-7736: the canonical approval-hold reason must be excluded too. + { name: "approval-held (canonical reason)", holder: { paused: true, pausedReason: "awaiting-approval" as const } }, ])("no-op: $name", async ({ holder, settings, ageMs }) => { const now = Date.now(); const effectiveAgeMs = ageMs ?? 31 * 60_000; @@ -136,4 +138,46 @@ describe("reliability interactions: paused scope decay", () => { const manager = new SelfHealingManager(store, { rootDir: process.cwd(), getExecutingTaskIds: () => new Set() }); expect(await manager.autoReboundPausedScopeDecay()).toBe(0); }); + + /* + * FNXC:ApprovalHold 2026-07-09-00:20: + * FN-7736 symptom-verification regression. Reproduces the exact original + * failure shape (approval-held in-progress task, no pausedReason, follower + * present, decay threshold elapsed) alongside a same-shaped control task + * that IS paused but for an unrelated (non-approval) reason, proving the + * assertion actually exercises the exclusion mechanism rather than a + * vacuously-true "nothing ever reboundeds" check. + */ + it("symptom verification: leaves an approval-held task in place while still rebounding a control paused task", async () => { + const now = Date.now(); + const approvalHeld = makeTask("FN-APPROVAL", { + column: "in-progress", + paused: true, + pausedReason: "awaiting-approval", + executionStartedAt: new Date(now - 31 * 60_000).toISOString(), + columnMovedAt: new Date(now - 31 * 60_000).toISOString(), + }); + const approvalFollower = makeTask("FN-APPROVAL-FOLLOWER", { column: "todo", blockedBy: "FN-APPROVAL" }); + const controlPaused = makeTask("FN-CONTROL", { + column: "in-progress", + paused: true, + pausedReason: "some-other-reason", + executionStartedAt: new Date(now - 31 * 60_000).toISOString(), + columnMovedAt: new Date(now - 31 * 60_000).toISOString(), + }); + const controlFollower = makeTask("FN-CONTROL-FOLLOWER", { column: "todo", blockedBy: "FN-CONTROL" }); + const { store, byId } = makeStore([approvalHeld, approvalFollower, controlPaused, controlFollower]); + const manager = new SelfHealingManager(store, { rootDir: process.cwd(), getExecutingTaskIds: () => new Set() }); + + const count = await manager.autoReboundPausedScopeDecay(); + + // Only the control task is rebounded -- the approval-held task is untouched. + expect(count).toBe(1); + expect(store.moveTask).toHaveBeenCalledWith("FN-CONTROL", "todo", expect.anything()); + expect(store.moveTask).not.toHaveBeenCalledWith("FN-APPROVAL", "todo", expect.anything()); + expect(byId.get("FN-APPROVAL")?.column).toBe("in-progress"); + expect(byId.get("FN-APPROVAL")?.paused).toBe(true); + expect(byId.get("FN-APPROVAL")?.pausedReason).toBe("awaiting-approval"); + expect(byId.get("FN-CONTROL")?.column).toBe("todo"); + }); }); diff --git a/packages/engine/src/__tests__/self-healing-paused-abort-recovery.test.ts b/packages/engine/src/__tests__/self-healing-paused-abort-recovery.test.ts index 68ee2a68f0..70e2851a58 100644 --- a/packages/engine/src/__tests__/self-healing-paused-abort-recovery.test.ts +++ b/packages/engine/src/__tests__/self-healing-paused-abort-recovery.test.ts @@ -211,6 +211,27 @@ describe("recoverPausedAbortFailures", () => { expect(store.updateTask).not.toHaveBeenCalled(); }); + // FN-7736: explicit regression for the approval-hold shape (paused, canonical + // awaiting-approval reason) -- classifyPausedAbortWorkflowRecovery's existing + // generic `task.paused` skip already covers this, but the invariant + // ("approval-blocked tasks are never advanced") must be asserted directly + // rather than relying on incidental generic-pause coverage. + it("skips an approval-held pause-abort park (canonical awaiting-approval reason)", async () => { + const store = createMockStore([ + parkTask({ id: "FN-APPROVAL", paused: true, pausedReason: "awaiting-approval" }), + ]); + const manager = new SelfHealingManager(store, { + rootDir: "/tmp/test-project", + getExecutingTaskIds: () => new Set(), + }); + + const recovered = await manager.recoverPausedAbortFailures(); + + expect(recovered).toBe(0); + expect(store.updateTask).not.toHaveBeenCalled(); + expect(store.moveTask).not.toHaveBeenCalled(); + }); + it("leaves guarded in-review pause-abort parks untouched", async () => { const candidates = [ parkTask({ id: "FN-U", column: "in-review", error: IN_REVIEW_PARK_ERROR, steps: DONE_STEPS, userPaused: true, autoMerge: true }), diff --git a/packages/engine/src/agent-heartbeat.ts b/packages/engine/src/agent-heartbeat.ts index e9f8d65a34..8dda450ef9 100644 --- a/packages/engine/src/agent-heartbeat.ts +++ b/packages/engine/src/agent-heartbeat.ts @@ -19,7 +19,7 @@ import type { AgentStore, AgentHeartbeatRun, HeartbeatInvocationSource, AgentHeartbeatConfig, AgentBudgetStatus, Message, MessageStore, TaskStore, TaskDetail, AgentRole, Agent, InboxTask, RunMutationContext, Settings, AgentConfigRevision, ReflectionStore, ChatStore, ChatRoom, ChatRoomMessage, AgentMemoryInclusionMode } from "@fusion/core"; import { AutoClaimSnapshotManager, resolveFreshAutoClaimCandidates, type AutoClaimCandidate } from "./auto-claim-snapshot.js"; -import { ApprovalRequestStore, buildExecutionMemoryInstructions, isEphemeralAgent, hasAgentIdentity, resolveEffectiveAgentPermissionPolicy, canAgentTakeImplementationTask, canAgentTakeImplementationTaskForExplicitRouting, resolvePersistAgentThinkingLog, resolveAgentMemoryInclusionMode, FUSION_RUNTIME_SELF_AWARENESS } from "@fusion/core"; +import { ApprovalRequestStore, buildExecutionMemoryInstructions, isEphemeralAgent, hasAgentIdentity, resolveEffectiveAgentPermissionPolicy, canAgentTakeImplementationTask, canAgentTakeImplementationTaskForExplicitRouting, resolvePersistAgentThinkingLog, resolveAgentMemoryInclusionMode, FUSION_RUNTIME_SELF_AWARENESS, AWAITING_APPROVAL_PAUSE_REASON } from "@fusion/core"; import type { ToolDefinition } from "@earendil-works/pi-coding-agent"; import { Type, type Static } from "@earendil-works/pi-ai"; import { createHash } from "node:crypto"; @@ -1249,7 +1249,11 @@ export class HeartbeatMonitor { */ pauseForApproval: async ({ approvalRequestId, decision }) => { if (taskId && this.taskStore) { - await this.taskStore.pauseTask(taskId, true, undefined, { pausedByAgentId: agent.id }); + // FNXC:ApprovalHold 2026-07-09-00:10: FN-7736 -- mirror executor.ts's + // stamping of the canonical AWAITING_APPROVAL_PAUSE_REASON so + // recovery/oversight code recognizes this hold on the task, not just + // the agent's `pauseReason`. + await this.taskStore.pauseTask(taskId, true, undefined, { pausedByAgentId: agent.id, pausedReason: AWAITING_APPROVAL_PAUSE_REASON }); await this.taskStore.logEntry( taskId, `Approval required for ${decision.toolName}. Request ${approvalRequestId} created; task and agent paused awaiting decision.`, diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 3362d523d3..859fe89c7b 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -11,7 +11,7 @@ import { existsSync, lstatSync, realpathSync } from "node:fs"; import { readFile, rm, writeFile } from "node:fs/promises"; import type { TaskStore, Task, TaskDetail, TaskTokenUsage, StepStatus, Settings, WorkflowStep, MissionStore, Slice, AgentState, AgentCapability, RunMutationContext, AgentHeartbeatConfig, Agent, AgentMemoryInclusionMode, ProjectSettings, MergeResult, WorkflowIrNode, WorkflowIrNodeKind, WorkflowStepResult as CoreWorkflowStepResult } from "@fusion/core"; import { getUnmetSchedulingDependencies } from "./scheduler.js"; -import { RetryStormError, TaskDeletedError, serializeRetryStormError, isExperimentalFeatureEnabled, resolveWorkflowIrForTask, resolveColumnAgentBinding, resolveEffectiveAgent, instanceNodeId, getWorkflowExtensionRegistry, getBuiltinWorkflow, parseNoOpCompletionMarker, allowsAutoMergeProcessing, isSharedBranchGroupMemberIntegration, resolveMaxAutoMergeRetries, resolveOptionalStepRevisionBudget, resolveOptionalReviewRevisionBudget, COMPLETION_SUMMARY_NODE_ID, upsertWorkflowStepResult } from "@fusion/core"; +import { RetryStormError, TaskDeletedError, serializeRetryStormError, isExperimentalFeatureEnabled, resolveWorkflowIrForTask, resolveColumnAgentBinding, resolveEffectiveAgent, instanceNodeId, getWorkflowExtensionRegistry, getBuiltinWorkflow, parseNoOpCompletionMarker, allowsAutoMergeProcessing, isSharedBranchGroupMemberIntegration, resolveMaxAutoMergeRetries, resolveOptionalStepRevisionBudget, resolveOptionalReviewRevisionBudget, COMPLETION_SUMMARY_NODE_ID, upsertWorkflowStepResult, AWAITING_APPROVAL_PAUSE_REASON } from "@fusion/core"; import { finalizeProvenAutoMergeTask } from "./auto-merge-finalization.js"; import { mergeEffectiveSettings } from "./effective-settings.js"; import type { TaskStep, WorkflowIr, WorkflowFieldDefinition, WorkflowColumnAgent, EffectiveAgentInput, WorkflowWorkEngineDispatchResult } from "@fusion/core"; @@ -2278,7 +2278,16 @@ export class TaskExecutor { }, pauseForApproval: async ({ approvalRequestId, decision }) => { if (taskId) { - await this.store.pauseTask(taskId, true, this.getRunContextFor(taskId), { pausedByAgentId: actorId }); + /* + FNXC:ApprovalHold 2026-07-09-00:10: + FN-7736: stamp the canonical AWAITING_APPROVAL_PAUSE_REASON on the + task (not just the agent) so recovery/oversight code can durably + recognize this hold via isTaskBlockedOnApproval -- previously only + `paused: true` was set with no reason, which self-healing's + autoReboundPausedScopeDecay could rebound before the operator ever + decided. + */ + await this.store.pauseTask(taskId, true, this.getRunContextFor(taskId), { pausedByAgentId: actorId, pausedReason: AWAITING_APPROVAL_PAUSE_REASON }); await this.store.logEntry( taskId, `Approval required for ${decision.toolName}. Request ${approvalRequestId} created; task and agent paused awaiting decision.`, diff --git a/packages/engine/src/overseer-human-control-policy.ts b/packages/engine/src/overseer-human-control-policy.ts index 77a6cc2097..1656630156 100644 --- a/packages/engine/src/overseer-human-control-policy.ts +++ b/packages/engine/src/overseer-human-control-policy.ts @@ -5,11 +5,12 @@ * — no steering, retry, targeted-fix, or FN-7513 confirmation-required * action (merge/PR progression, destructive git, external-service side * effect) may fire, and no pending confirmation may even be recorded — - * whenever a task is (a) user-paused, or (b) not eligible for auto-merge - * processing per the FN-5147 `autoMerge:false` / PR-based human-review - * terminal contract. This module supplies a single PURE predicate - * (`evaluateOverseerHumanControl`, no I/O) that the FN-7512/FN-7513 dispatch - * seam (`PlannerRecoveryController.tick`) must consult BEFORE any action + * whenever a task is (a) user-paused, (b) blocked on a pending human + * approval decision, or (c) not eligible for auto-merge processing per the + * FN-5147 `autoMerge:false` / PR-based human-review terminal contract. This + * module supplies a single PURE predicate (`evaluateOverseerHumanControl`, + * no I/O) that the FN-7512/FN-7513 dispatch seam + * (`PlannerRecoveryController.tick`) must consult BEFORE any action * classification, confirmation gating, steering, retry, or dispatch — * mirroring the pure-decision style of `recovery-policy.ts` / * `overseer-confirmation-policy`-equivalent `planner-confirmation.ts`. @@ -29,7 +30,22 @@ * DOES carry a `pausedReason` is therefore an engine-originated park, not * a user pause, and must NOT withhold oversight on that basis alone (the * separate `allowsAutoMergeProcessing` / autoMerge-off check still - * applies independently). + * applies independently) — UNLESS that `pausedReason` is specifically the + * canonical `AWAITING_APPROVAL_PAUSE_REASON` (see below). + * + * FNXC:ApprovalHold 2026-07-09-00:25: + * FN-7736: a task can also be blocked on a pending human tool-approval + * decision (`pauseForApproval` -> `pauseTask(id, true, { pausedReason: + * AWAITING_APPROVAL_PAUSE_REASON })`, or the triage plan-approval gate's + * `status: "awaiting-approval"`). Before FN-7736 stamped a durable reason, + * this hold shape (`paused:true`, no reason) was indistinguishable from — + * and only accidentally covered by — the user-pause branch above. Once the + * durable reason was introduced, that branch would STOP matching (the task + * now carries a `pausedReason`), which would silently regress FN-7514's + * hold. This module therefore checks `isTaskBlockedOnApproval` as its own + * explicit, non-accidental branch, checked ahead of the user-pause / + * auto-merge-off checks so introducing the durable reason can never flip an + * approval-held task from withhold to act. * * Auto-merge-off / human-review half: delegates verbatim to * `allowsAutoMergeProcessing` from `@fusion/core` (the canonical FN-5147 @@ -38,9 +54,9 @@ */ import type { Settings, Task } from "@fusion/core"; -import { allowsAutoMergeProcessing } from "@fusion/core"; +import { allowsAutoMergeProcessing, isTaskBlockedOnApproval } from "@fusion/core"; -export type OverseerHumanControlWithholdReason = "user-paused" | "auto-merge-off-human-review"; +export type OverseerHumanControlWithholdReason = "user-paused" | "approval-blocked" | "auto-merge-off-human-review"; export interface OverseerHumanControlDecision { /** `true` when the overseer must take NO action of any kind for this task. */ @@ -50,7 +66,7 @@ export interface OverseerHumanControlDecision { } /** The minimal task shape the predicate needs — narrowed for testability and to keep the module engine-local/pure. */ -export type OverseerHumanControlTask = Pick; +export type OverseerHumanControlTask = Pick; /** The minimal settings shape the predicate needs (forwarded to `allowsAutoMergeProcessing`). */ export type OverseerHumanControlSettings = Pick; @@ -58,9 +74,10 @@ export type OverseerHumanControlSettings = Pick; /** * Pure predicate — no I/O, no throws on well-formed input. Returns whether * the planner overseer must withhold ALL oversight action for `task`, and - * why. Precedence: user-pause is checked first (it is the stronger signal — - * a user explicitly stopped the world for this task), then the FN-5147 - * auto-merge-off / human-review terminal contract. + * why. Precedence: the approval hold is checked first (FN-7736 — a durable, + * unambiguous marker that must never be shadowed by the weaker + * accidental-no-reason user-pause heuristic below it), then user-pause, then + * the FN-5147 auto-merge-off / human-review terminal contract. */ export function evaluateOverseerHumanControl( task: OverseerHumanControlTask | null | undefined, @@ -73,6 +90,10 @@ export function evaluateOverseerHumanControl( return { withhold: true }; } + if (isTaskBlockedOnApproval(task)) { + return { withhold: true, reason: "approval-blocked" }; + } + const isUserPaused = task.userPaused === true || (task.paused === true && !task.pausedReason); if (isUserPaused) { diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index d7eac6023f..9ba5df7522 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -30,7 +30,7 @@ import { setImmediate as setImmediateCb } from "node:timers"; import { existsSync, mkdirSync, readdirSync, readFileSync, realpathSync, rmSync, statSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { isAbsolute, join, relative, resolve } from "node:path"; -import { IN_REVIEW_STALL_DEADLOCK_LOG_PREFIX, IN_REVIEW_STALL_LOG_PREFIX, IN_REVIEW_STALL_TERMINAL_LOG_PREFIX, allowsAutoMergeProcessing, countRecentIdenticalStallEntries, detectDependencyCycle, detectSelfDefeatingDependency, evaluateNoCommitsNoOpFinalize, getInReviewStalledSignal, getInReviewStallReason, getPrimaryPrInfo, getStalePausedReviewSignal, getStalePausedTodoSignal, getTaskHardMergeBlocker, getTaskMergeBlocker, isEphemeralAgent, isMergeRequestContractShadowEnabled, isWorkflowColumnsEnabled, isWorkspaceTask, isSharedBranchGroupMemberIntegration, parseExplicitDuplicateMarker, resolveMaxAutoMergeRetries, resolveOptionalStepRevisionBudget, resolveOptionalReviewRevisionBudget, resolveWorkflowIrForTask, type Agent, type AgentStore, type ChatStore, type MessageStore, type TaskStore, type Settings, type Task, type MergeDetails, type TaskPriority, type MergeResult, type WorkflowStepResult } from "@fusion/core"; +import { IN_REVIEW_STALL_DEADLOCK_LOG_PREFIX, IN_REVIEW_STALL_LOG_PREFIX, IN_REVIEW_STALL_TERMINAL_LOG_PREFIX, allowsAutoMergeProcessing, countRecentIdenticalStallEntries, detectDependencyCycle, detectSelfDefeatingDependency, evaluateNoCommitsNoOpFinalize, getInReviewStalledSignal, getInReviewStallReason, getPrimaryPrInfo, getStalePausedReviewSignal, getStalePausedTodoSignal, getTaskHardMergeBlocker, getTaskMergeBlocker, isEphemeralAgent, isMergeRequestContractShadowEnabled, isWorkflowColumnsEnabled, isWorkspaceTask, isSharedBranchGroupMemberIntegration, parseExplicitDuplicateMarker, resolveMaxAutoMergeRetries, resolveOptionalStepRevisionBudget, resolveOptionalReviewRevisionBudget, resolveWorkflowIrForTask, AWAITING_APPROVAL_PAUSE_REASON, type Agent, type AgentStore, type ChatStore, type MessageStore, type TaskStore, type Settings, type Task, type MergeDetails, type TaskPriority, type MergeResult, type WorkflowStepResult } from "@fusion/core"; import type { MeshLeaseManager } from "./mesh-lease-manager.js"; import { createLogger, schedulerLog } from "./logger.js"; import { mergeEffectiveSettings } from "./effective-settings.js"; @@ -795,10 +795,20 @@ export class SelfHealingManager { lastNtfyAt: number | null; } | null = null; + /* + * FNXC:ApprovalHold 2026-07-09-00:15: + * FN-7736: AWAITING_APPROVAL_PAUSE_REASON must be excluded here so a task + * parked mid-execution on a pending tool-approval decision (`pauseForApproval` + * -> `pauseTask(id, true, { pausedReason: AWAITING_APPROVAL_PAUSE_REASON })`) + * is never rebounded to `todo` by this scope-decay sweep before the operator + * approves or denies -- this was the reported symptom (a follower task's + * scope-decay threshold elapsing could silently defeat the approval gate). + */ private static readonly PAUSED_SCOPE_DECAY_EXCLUDED_REASONS = new Set([ "branch-conflict-unrecoverable", "worktrunk_operation_failed", "token_budget_exceeded", + AWAITING_APPROVAL_PAUSE_REASON, ]); constructor(