diff --git a/.changeset/review-gate-lifecycle-followups.md b/.changeset/review-gate-lifecycle-followups.md new file mode 100644 index 0000000000..aabe91fb12 --- /dev/null +++ b/.changeset/review-gate-lifecycle-followups.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Harden review-gate handling: reclaim symbol locks, stall-detect hung gates, and stop premature merges. +category: fix +dev: Follow-ups to running the pre-merge review gates in `in-review`. (1) `moveTaskInternal` now RE-ACQUIRES declared symbol locks on a `!wip -> wip` crossing, mirroring the FN-8306 release branch — the gate crossing released them and nothing reclaimed them for the remediation pass (best-effort; a contended symbol logs and proceeds, matching the prior posture, rather than parking the remediation). (2) `recoverMergeableReviewTasks` now filters `executingIds`, matching its `recoverGhostReviewTasks` sibling: the graph commits the column crossing at node entry and writes the gate's pending lease two round trips later, and `getTaskMergeBlocker` has no notion of "enabled but resultless", so that window could enqueue a merge with Code Review never run. (3) `reconcileOrphanedPendingStepResults` honors a live review-gate lease (`classifyReviewLease` within `PLAN_REVIEW_LEASE_STALENESS_MS`), so a periodic sweep tick can no longer fail a gate that just started; cleanup of genuinely dead leases is delayed by the floor, not defeated. Its audit event gains `needsOperatorBypass` for `autoMerge:false` rows, which self-healing deliberately skips and only `fn_task_bypass_review` can clear. (4) The planner overseer's `reviewer` and `merger` stages gain gate-anchored stall detection keyed on the pending lease's `startedAt` (not `columnMovedAt`, which would fire during a legitimate human merge-wait); both previously returned `progressing` unconditionally, so a hung gate produced no signal. `cumulativeActiveMs` scope is documented rather than changed — adding the `timing` trait to `in-review` would count human merge-wait as active work. diff --git a/packages/core/src/default-workflow-hooks.ts b/packages/core/src/default-workflow-hooks.ts index b0524c9818..d61bf6da7d 100644 --- a/packages/core/src/default-workflow-hooks.ts +++ b/packages/core/src/default-workflow-hooks.ts @@ -108,7 +108,23 @@ export interface DefaultWorkflowMoveContext { // the resolved onEnter/onExit hook bodies for the default workflow's traits. /** `timing` trait (in-progress): accumulate active ms on exit, stamp timing on - * entry. */ + * entry. + * + * FNXC:WorkflowReviewGates 2026-07-26-16:20: + * SCOPE: `cumulativeActiveMs` measures time in WIP columns only — it is a sum of `in-progress` + * segments, closed on each exit. Since the pre-merge review gates moved into `in-review`, gate + * runtime is NOT included: the segment closes when the card crosses into review, and no new + * segment opens until remediation re-enters `in-progress`. Read it as "implementation time", + * not "wall clock from start to merge" — consumers that want the latter must use + * `executionStartedAt`/`executionCompletedAt`, which still span the whole run and therefore + * legitimately diverge from this sum. + * Deliberately NOT fixed by adding the `timing` trait to `in-review`: that column also holds the + * arbitrary human merge-wait, so counting it would overstate active time by hours of idle + * latency — a worse distortion than omitting the gate's own minutes. Attributing gate runtime + * properly needs node-scoped timing (a separate field), not a column trait. + * Consumers of this scope: `packages/core/src/productivity-analytics.ts`, + * `packages/core/src/task-timing.ts`, and the dashboard duration displays. + */ export function applyTimingEffects(ctx: DefaultWorkflowMoveContext): void { const { task, fromColumn, toColumn } = ctx; if (fromColumn === "in-progress" && toColumn !== "in-progress") { diff --git a/packages/core/src/task-store/moves.ts b/packages/core/src/task-store/moves.ts index 4a4c4b99ee..e707ed4279 100644 --- a/packages/core/src/task-store/moves.ts +++ b/packages/core/src/task-store/moves.ts @@ -68,6 +68,15 @@ async function resolveTaskWorkflowIrForMove(store: TaskStore, id: string): Promi } import {enqueueMergeQueueInTransaction, dequeueMergeQueueOnColumnExitInTransaction} from "../task-store/async-merge-coordination.js"; +/* +FNXC:WorkflowReviewGates 2026-07-26-15:05: +Lease length for the symmetric symbol-lock re-acquire on a !wip -> wip crossing. Matches the +engine scheduler's SYMBOL_LOCK_LEASE_MS (10 min) so a lock reclaimed by a lifecycle transition and +one taken at dispatch expire on the same clock; the engine renews both through renewSymbolLocks. +Duplicated rather than imported because @fusion/core must not depend on @fusion/engine. +*/ +const SYMBOL_LOCK_REACQUIRE_LEASE_MS = 10 * 60_000; + /* FNXC:WorkflowTransitionPolicy 2026-07-18-19:52: Resolve a column's trait-derived facts (id + OR-merged flags) for the shared @@ -1201,6 +1210,55 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum await store.releaseSymbolLocks(symbols.symbols, id); } } + /* + FNXC:WorkflowReviewGates 2026-07-26-15:05: + Symmetric RE-ACQUIRE on the reverse crossing (!wip -> wip). FN-8306 made the lifecycle + transition the release authority but wrote no counterpart, which was harmless while a task only + ever left WIP at handoff/terminal. It stopped being harmless when the pre-merge review gates + moved into `in-review`: the gate crossing releases the task's declared symbols, then the paired + remediation node re-enters `in-progress` and edits the SAME files in the SAME live worktree with + its fine-grained locks gone. The only two acquire sites (the scheduler's todo->in-progress + dispatch and `claimDueWorkflowWorkItem`) are not on the graph's re-entry path, so nothing + reclaimed them. + + Deliberately BEST-EFFORT: on conflict we log and proceed unlocked rather than rejecting the + move. Rejecting would park the remediation behind a symbol another task now holds — turning a + lock-granularity problem into the stranding failure this whole change set exists to avoid — and + would need a new `TransitionRejectionCode`. Proceeding unlocked is exactly the pre-fix posture, + so the contended case is no worse than today while the common (symbol free) case is repaired. + Coarse protection still applies via `shouldHoldActiveFileScopeLease`, which keeps a live + in-review task's file-scope lease held against scheduler todo-dispatch overlap. + */ + if (store.backendMode && !fromIsImplementation && toIsImplementation) { + const symbols = resolveTaskSymbolsForTask(task); + if (symbols.resolvable && symbols.symbols.length > 0) { + try { + const reacquired = await store.acquireSymbolLocks( + symbols.symbols, + { ownerTaskId: id, missionId: task.missionId, agentId: "lifecycle-transition" }, + SYMBOL_LOCK_REACQUIRE_LEASE_MS, + ); + if (!reacquired.acquired) { + const conflict = reacquired.conflicts[0]; + storeLog.warn("symbol lock re-acquire on WIP re-entry lost to another holder", { + phase: "moveTaskInternal:symbol-reacquire", + taskId: id, + fromColumn, + toColumn, + symbolKey: conflict?.symbolKey, + ownerTaskId: conflict?.ownerTaskId, + }); + } + } catch (error) { + // Never fail a lifecycle transition on lock bookkeeping. + storeLog.warn("symbol lock re-acquire failed", { + phase: "moveTaskInternal:symbol-reacquire", + taskId: id, + error: error instanceof Error ? error.message : String(error), + }); + } + } + } if (fromColumn === "in-review" && toColumn === "todo" && moveSource === "user") { const handoffAccepted = await store.getCompletionHandoffAcceptedMarker(id); diff --git a/packages/engine/src/__tests__/planner-overseer.test.ts b/packages/engine/src/__tests__/planner-overseer.test.ts index be7014dee6..9638ab988f 100644 --- a/packages/engine/src/__tests__/planner-overseer.test.ts +++ b/packages/engine/src/__tests__/planner-overseer.test.ts @@ -534,3 +534,99 @@ describe("PlannerOverseerMonitor.observeTask — FN-7965 executor failure detect expect(a?.reason).not.toMatch(/failed:|error/i); }); }); + +/* +FNXC:WorkflowReviewGates 2026-07-26-16:45: +Before the pre-merge review gates moved into `in-review`, a hung Code Review sat in `in-progress` +and the FN-7743 executor stall check caught it. After the move it maps to the `reviewer` stage, +which returned `progressing` unconditionally with no time-based check — so a gate that never posted +a verdict produced no stall signal at all, however long it hung. + +These cases pin the replacement, and specifically pin the thing that makes it safe: the anchor is +the GATE's own `startedAt` lease, not `columnMovedAt`. Anchoring on the column would fire during a +legitimate human merge-wait, which is exactly the false positive that would make operators distrust +the signal — so "settled gates + old card = progressing" is asserted alongside the positive case. +*/ +describe("PlannerOverseerMonitor.observeTask — review-gate stall detection (in-review gates)", () => { + const THRESHOLD_MS = 2 * 60 * 60 * 1000; + const NOW = Date.UTC(2026, 6, 26, 12, 0, 0); + const isoMsAgo = (ms: number) => new Date(NOW - ms).toISOString(); + + const gate = (overrides: Record) => ({ + workflowStepId: "code-review", + workflowStepName: "Code Review", + phase: "pre-merge", + ...overrides, + }); + + it("reports stuck when a pre-merge gate has been pending past the threshold", async () => { + const monitor = new PlannerOverseerMonitor(); + const task = taskFixture({ + column: "in-review", + columnMovedAt: isoMsAgo(THRESHOLD_MS + 60 * 60 * 1000), + workflowStepResults: [gate({ status: "pending", startedAt: isoMsAgo(THRESHOLD_MS + 60 * 60 * 1000) })], + } as never); + + const observation = await monitor.observeTask(task, "autonomous", { now: () => NOW, executorStuckAfterMs: THRESHOLD_MS }); + + // A plain in-review card with no reviewState resolves to the `merger` stage, + // not `reviewer` — which is exactly why the check lives on both in-review stages. + expect(observation?.signal).toBe("stuck"); + expect(observation?.stage).toBe("merger"); + expect(observation?.reason).toMatch(/Review gate running for over \d+h/); + }); + + it("also reports stuck on the reviewer stage when reviewState is present", async () => { + const monitor = new PlannerOverseerMonitor(); + const task = taskFixture({ + column: "in-review", + reviewState: { items: [], summary: {} }, + workflowStepResults: [gate({ status: "pending", startedAt: isoMsAgo(THRESHOLD_MS + 60 * 60 * 1000) })], + } as never); + + const observation = await monitor.observeTask(task, "autonomous", { now: () => NOW, executorStuckAfterMs: THRESHOLD_MS }); + + expect(observation?.stage).toBe("reviewer"); + expect(observation?.signal).toBe("stuck"); + expect(observation?.reason).toMatch(/Review gate running for over \d+h/); + }); + + it("stays progressing while the gate is pending but still within the threshold", async () => { + const monitor = new PlannerOverseerMonitor(); + const task = taskFixture({ + column: "in-review", + workflowStepResults: [gate({ status: "pending", startedAt: isoMsAgo(60 * 1000) })], + } as never); + + const observation = await monitor.observeTask(task, "autonomous", { now: () => NOW, executorStuckAfterMs: THRESHOLD_MS }); + + expect(observation?.signal).toBe("progressing"); + }); + + it("stays progressing for a long human merge-wait once the gates have settled", async () => { + const monitor = new PlannerOverseerMonitor(); + const task = taskFixture({ + column: "in-review", + // The card has sat in review for days, but no gate is running — a human owes a merge. + columnMovedAt: isoMsAgo(72 * 60 * 60 * 1000), + updatedAt: isoMsAgo(72 * 60 * 60 * 1000), + workflowStepResults: [gate({ status: "passed", completedAt: isoMsAgo(71 * 60 * 60 * 1000) })], + } as never); + + const observation = await monitor.observeTask(task, "autonomous", { now: () => NOW, executorStuckAfterMs: THRESHOLD_MS }); + + expect(observation?.signal).toBe("progressing"); + }); + + it("degrades to progressing when the pending gate has no usable startedAt", async () => { + const monitor = new PlannerOverseerMonitor(); + const task = taskFixture({ + column: "in-review", + workflowStepResults: [gate({ status: "pending", startedAt: "not-a-date" })], + } as never); + + const observation = await monitor.observeTask(task, "autonomous", { now: () => NOW, executorStuckAfterMs: THRESHOLD_MS }); + + expect(observation?.signal).toBe("progressing"); + }); +}); diff --git a/packages/engine/src/__tests__/self-healing-orphaned-pending-step-results.test.ts b/packages/engine/src/__tests__/self-healing-orphaned-pending-step-results.test.ts index cd89b0fff6..9dc140774f 100644 --- a/packages/engine/src/__tests__/self-healing-orphaned-pending-step-results.test.ts +++ b/packages/engine/src/__tests__/self-healing-orphaned-pending-step-results.test.ts @@ -185,3 +185,75 @@ describe("FN-8492: reconcile orphaned pending step results", () => { expect(recordRunAuditEventMock).toHaveBeenCalledWith(expect.objectContaining({ target: "FN-OK" })); }); }); + +/* +FNXC:WorkflowReviewGates 2026-07-26-16:05: +The pre-merge review gates now run with the card in `in-review`, so this sweep judges them where it +previously skipped them (it skips `in-progress` rows outright). Because it also runs from PERIODIC +maintenance — in the same live process as an active graph run — a tick landing between the gate's +`pending` lease write and its session-registry registration could stamp a genuinely running gate as +`failed`, closing the merge gate on a healthy task. A within-floor lease (`leaseOwner` + recent +`startedAt`) therefore counts as live, matching the semantics Plan Review already had via +`classifyReviewLease`. + +The second case is the one that keeps FN-8492 intact: this must DELAY cleanup by the staleness +floor, not defeat it. A lease past the floor is still rewritten to `failed`. +*/ +describe("review-gate lease liveness (in-review gates)", () => { + beforeEach(() => vi.clearAllMocks()); + afterEach(() => executingTaskLock._clearForTest()); + + const leaseResult = (startedAt: string) => stepResult({ + workflowStepId: "code-review", + workflowStepName: "Code Review", + status: "pending", + leaseOwner: "run-abc", + startedAt, + }); + + it("leaves a code-review gate alone while its lease is still within the staleness floor", async () => { + const live = task("FN-LEASE-LIVE", { + workflowStepResults: [leaseResult(new Date(Date.now() - 60_000).toISOString())], + }); + const store = storeFor([live]); + const manager = new SelfHealingManager(store, "/repo", {} as never); + + const recovered = await manager.reconcileOrphanedPendingStepResults(); + + expect(recovered).toBe(0); + expect(store.updateTask).not.toHaveBeenCalled(); + const after = await store.getTask("FN-LEASE-LIVE"); + expect(after?.workflowStepResults?.[0]?.status).toBe("pending"); + }); + + it("still fails a code-review gate whose lease has aged past the floor (FN-8492 preserved)", async () => { + const stale = task("FN-LEASE-STALE", { + workflowStepResults: [leaseResult(new Date(Date.now() - 60 * 60_000).toISOString())], + }); + const store = storeFor([stale]); + const manager = new SelfHealingManager(store, "/repo", {} as never); + + const recovered = await manager.reconcileOrphanedPendingStepResults(); + + expect(recovered).toBe(1); + const after = await store.getTask("FN-LEASE-STALE"); + expect(after?.workflowStepResults?.[0]?.status).toBe("failed"); + }); + + it("still fails an ownerless pending result — no leaseOwner means no lease to honor", async () => { + const ownerless = task("FN-NO-OWNER", { + workflowStepResults: [stepResult({ + workflowStepId: "code-review", + workflowStepName: "Code Review", + status: "pending", + startedAt: new Date().toISOString(), + })], + }); + const store = storeFor([ownerless]); + const manager = new SelfHealingManager(store, "/repo", {} as never); + + expect(await manager.reconcileOrphanedPendingStepResults()).toBe(1); + const after = await store.getTask("FN-NO-OWNER"); + expect(after?.workflowStepResults?.[0]?.status).toBe("failed"); + }); +}); diff --git a/packages/engine/src/planner-overseer.ts b/packages/engine/src/planner-overseer.ts index f98f5d73f8..1712a19e0e 100644 --- a/packages/engine/src/planner-overseer.ts +++ b/packages/engine/src/planner-overseer.ts @@ -75,6 +75,10 @@ export type OverseerTaskRef = Pick< | "workflowTransitionNotification" | "updatedAt" | "columnMovedAt" + // FNXC:WorkflowReviewGates 2026-07-26-16:35: the in-review stages (reviewer AND merger) need the + // pre-merge gate's pending lease to anchor their stall check on when the GATE started, not when + // the card entered the column. See `reviewGateStallReason`. + | "workflowStepResults" >; /** @@ -177,6 +181,50 @@ export function resolveExecutorStuckAfterMs(raw: unknown): number { return DEFAULT_PLANNER_OVERSEER_EXECUTOR_STUCK_AFTER_MS; } +/* +FNXC:WorkflowReviewGates 2026-07-26-16:50: +Gate-anchored stall detection for the in-review stages. Neither the `reviewer` nor the `merger` +branch had ANY time-based check — both fell through to `progressing` unconditionally. That was +tolerable while the pre-merge gates ran in `in-progress` (the FN-7743 executor check covered them), +but Code Review / Browser Verification now run with the card in `in-review`, so a hung gate that +never posts a verdict produced no stall signal at all, however long it hung. + +Applied to BOTH in-review stages deliberately: `resolveWatchedStage` maps a plain in-review card +with no `reviewState` to `merger`, not `reviewer`, so a task running its first gate usually lands +in the merger branch. The pending pre-merge lease is the authoritative "a gate is running" fact +regardless of which sub-stage was inferred. + +Anchored on the gate's own `startedAt`, never `columnMovedAt`: the latter conflates "entered +review" with "gate started" and would fire during a legitimate human merge-wait — the false +positive that would make the signal untrustworthy. Only engages while a pre-merge result is +actually `pending`; a card whose gates have settled and is awaiting a human merge stays +`progressing`. + +Reuses the resolved `executorStuckAfterMs` threshold — this is a hung agent session, the same +failure the executor check covers, so it needs no second setting. The reason is bucketed to whole +hours so the FN-7577 `stage|signal|reason` feed dedup stays effective (it must never embed a +changing millisecond value). A missing/malformed timestamp degrades to no signal; never fabricate a +stall. The FN-7514 human-control guard still runs upstream, so `autoMerge:false` rows stay +human-owned regardless of what this returns. +*/ +function reviewGateStallReason( + task: Partial, + stallInput: ExecutorStallSignalInput, +): string | undefined { + if (!(stallInput.executorStuckAfterMs > 0)) return undefined; + const pendingGate = task.workflowStepResults?.find((result) => { + const phase = result.phase || "pre-merge"; + return phase === "pre-merge" && result.status === "pending" && Boolean(result.startedAt); + }); + if (!pendingGate?.startedAt) return undefined; + const gateStartedMs = Date.parse(pendingGate.startedAt); + if (!Number.isFinite(gateStartedMs)) return undefined; + const inactiveMs = stallInput.now() - gateStartedMs; + if (inactiveMs < stallInput.executorStuckAfterMs) return undefined; + const inactiveHours = Math.max(1, Math.floor(inactiveMs / 3_600_000)); + return `Review gate running for over ${inactiveHours}h with no verdict`; +} + function deriveSignalAndSources( taskId: string, stage: OverseerWatchedStage, @@ -250,6 +298,15 @@ function deriveSignalAndSources( sources: [{ kind: "review-comment", ref: reviewState?.items?.[0]?.id ?? taskId }], }; } + const reviewerGateStall = reviewGateStallReason(task, stallInput); + if (reviewerGateStall) { + return { + signal: "stuck", + reason: reviewerGateStall, + sources: [{ kind: "review-comment", ref: reviewState?.items?.[0]?.id ?? taskId }], + }; + } + return { signal: "progressing", reason: "Review in progress", @@ -272,6 +329,16 @@ function deriveSignalAndSources( sources: [{ kind: "merge-error", ref: task.workflowTransitionNotification.transitionId ?? taskId }], }; } + // A plain in-review card with no reviewState resolves HERE, not to `reviewer` + // (see resolveWatchedStage), so this is the branch a running first gate lands in. + const mergerGateStall = reviewGateStallReason(task, stallInput); + if (mergerGateStall) { + return { + signal: "stuck", + reason: mergerGateStall, + sources: [{ kind: "merge-error", ref: taskId }], + }; + } return { signal: "progressing", reason: "Task is in the merge/integration phase", diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index 4791e4d6fa..0ff67a8a52 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -31,7 +31,7 @@ import { existsSync, mkdirSync, readdirSync, readFileSync, realpathSync, rmSync, import { readFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { isAbsolute, join, relative, resolve } from "node:path"; -import { resolveColumnFlags, IN_REVIEW_STALL_DEADLOCK_LOG_PREFIX, IN_REVIEW_STALL_LOG_PREFIX, IN_REVIEW_STALL_TERMINAL_LOG_PREFIX, allowsAutoMergeProcessing, resolveEffectiveAutoMerge, countRecentIdenticalStallEntries, detectDependencyCycle, detectSelfDefeatingDependency, evaluateNoCommitsNoOpFinalize, evaluateCompletedPromotionFailureProvenance, evaluateSkipBypassTaint, getInReviewStalledSignal, getInReviewStallReason, getPrimaryPrInfo, getStalePausedReviewSignal, getStalePausedTodoSignal, getTaskHardMergeBlocker, getTaskMergeBlocker, isEphemeralAgent, isMergeRequestContractShadowEnabled, isWorkflowColumnsEnabled, isWorkspaceTask, isSharedBranchGroupMemberIntegration, isNearDuplicateCanonicalInactive, parseExplicitDuplicateMarker, flagTriageDuplicate, isTriageDuplicateKeepAcknowledged, resolveMaxAutoMergeRetries, resolveOptionalStepRevisionBudget, resolveOptionalReviewRevisionBudget, resolveWorkflowIrForTask, resolveReboundTarget, planLegacyAdoption, resolveOrphanedPendingStepResults, 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 { resolveColumnFlags, IN_REVIEW_STALL_DEADLOCK_LOG_PREFIX, IN_REVIEW_STALL_LOG_PREFIX, IN_REVIEW_STALL_TERMINAL_LOG_PREFIX, allowsAutoMergeProcessing, resolveEffectiveAutoMerge, countRecentIdenticalStallEntries, detectDependencyCycle, detectSelfDefeatingDependency, evaluateNoCommitsNoOpFinalize, evaluateCompletedPromotionFailureProvenance, evaluateSkipBypassTaint, getInReviewStalledSignal, getInReviewStallReason, getPrimaryPrInfo, getStalePausedReviewSignal, getStalePausedTodoSignal, getTaskHardMergeBlocker, getTaskMergeBlocker, isEphemeralAgent, isMergeRequestContractShadowEnabled, isWorkflowColumnsEnabled, isWorkspaceTask, isSharedBranchGroupMemberIntegration, isNearDuplicateCanonicalInactive, parseExplicitDuplicateMarker, flagTriageDuplicate, isTriageDuplicateKeepAcknowledged, resolveMaxAutoMergeRetries, resolveOptionalStepRevisionBudget, resolveOptionalReviewRevisionBudget, resolveWorkflowIrForTask, resolveReboundTarget, planLegacyAdoption, resolveOrphanedPendingStepResults, classifyReviewLease, PLAN_REVIEW_LEASE_STALENESS_MS, 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 { finalizePlanningSegment } from "@fusion/core"; import type { MeshLeaseManager } from "./mesh-lease-manager.js"; import { createLogger, schedulerLog } from "./logger.js"; @@ -6972,6 +6972,9 @@ export class SelfHealingManager { const pageSize = 500; let offset = 0; let recovered = 0; + // FNXC:WorkflowReviewGates 2026-07-26-15:55: read once per sweep, used only to LABEL the + // audit event (needsOperatorBypass). This sweep takes no auto-merge-dependent action. + const settings = await this.store.getSettings().catch(() => undefined); const isSessionLive = (taskId: string): boolean => { const livePaths = activeSessionRegistry.pathsForTask(taskId); @@ -6994,9 +6997,31 @@ export class SelfHealingManager { // a merger/planner that wrote a fresh pending lease after the page was fetched. const fresh = await this.store.getTask(task.id); if (!fresh || fresh.userPaused === true || fresh.column === "in-progress") continue; - const { results, orphanedCount } = resolveOrphanedPendingStepResults( + /* + FNXC:WorkflowReviewGates 2026-07-26-15:50: + Honor a LIVE review-gate lease, not just in-process session liveness. + Plan Review has always had lease semantics here (`classifyReviewLease`: a `leaseOwner` + with a `startedAt` inside the staleness floor is adopted, never re-dispatched), but the + `code-review` / `browser-verification` optional groups write a bare `pending` record with + no such guard. That asymmetry became reachable when those gates moved into `in-review`: + this sweep also runs from PERIODIC MAINTENANCE, in the same live process as an active + graph run, so a tick landing between the lease write and the session-registry + registration could stamp a gate that just started — and is genuinely running — as + `failed`, closing the merge gate against a task nothing is wrong with. + Treating a within-floor lease as live closes that window. It does NOT defeat FN-8492: a + lease past the floor classifies as `reclaim` and is still marked failed, so cleanup of a + genuinely dead lease is delayed by at most the staleness floor — the same floor Plan + Review already relies on. Restart-orphaned gates are unaffected in substance: nothing + re-attaches an in-review graph run after a restart, so those leases simply age out and + are then marked failed as before. + */ + const hasLiveReviewLease = (result: WorkflowStepResult): boolean => { + if (!result.leaseOwner || !result.startedAt) return false; + return classifyReviewLease([result], result.workflowStepId, Date.now(), PLAN_REVIEW_LEASE_STALENESS_MS).kind === "adopt"; + }; + const { results, orphanedCount } = resolveOrphanedPendingStepResults( fresh.workflowStepResults, - () => isSessionLive(task.id), + (result) => isSessionLive(task.id) || hasLiveReviewLease(result), { output: "Step session did not survive an engine restart or crash; marked failed by self-healing (FN-8492).", completedAt: new Date().toISOString(), @@ -7023,12 +7048,26 @@ export class SelfHealingManager { }).database({ type: "task:reconcile-orphaned-pending-step-results", target: task.id, + /* + FNXC:WorkflowReviewGates 2026-07-26-15:55: + `needsOperatorBypass` labels the case that has no automatic way out. When the row is + not auto-merge-eligible (`autoMerge:false` / PR-based human-review contract), + `recoverReviewTasksWithFailedPreMergeSteps` deliberately skips it, so the `failed` + result this sweep just wrote will never be auto-recovered — the only resolution is an + operator running `fn_task_bypass_review` / `POST /tasks/:id/bypass-review` (FN-7720). + Without this flag the event is indistinguishable from an ordinary auto-recoverable + rewrite, and the board shows only a generic merge-blocked badge, so the card sits + silently. Metadata stays ids/counts/outcomes-only — this is a boolean label, and the + event takes no lifecycle action, so the autoMerge:false terminal-until-human contract + is untouched. + */ // ids/counts/outcomes only — never step output or reviewer prose. metadata: { taskId: task.id, column: task.column, orphanedCount, resultCount: results.length, + needsOperatorBypass: settings ? !allowsAutoMergeProcessing(fresh, settings) : undefined, }, }); } catch (error) { @@ -7409,11 +7448,29 @@ export class SelfHealingManager { if (settings.globalPause || settings.enginePaused) return 0; const maxAutoMergeRetries = resolveMaxAutoMergeRetries(settings); const tasks = await this.store.listTasks({ column: "in-review", slim: true }); + /* + FNXC:WorkflowReviewGates 2026-07-26-15:30: + Liveness gate, mirroring `recoverGhostReviewTasks` and + `recoverReviewTasksWithFailedPreMergeSteps` which already filter on `executingIds`. This + sweep was the odd one out, and that became reachable once the pre-merge review gates moved + into `in-review`: the graph commits the column crossing at node ENTRY and only then writes the + gate's `pending` lease (two DB round trips later), so there is a durable window where the card + is in `in-review` with every step done and NO pre-merge result yet. `getTaskMergeBlocker` + blocks on pending/failed RESULTS and has no notion of "enabled but resultless", so in that + window it returns undefined and this sweep would enqueue a merge with Code Review never run. + The graph run holds `executingTaskLock` for its whole duration, so excluding executing tasks + closes the window; it only defers the merge until the run releases the lock, which it must do + before the task is genuinely finished with its pre-merge gates. + Same hazard class as FN-8492 (the merge gate reasons about results, not about enablement) — + but a different manifestation: there the entry was stale, here it does not exist yet. + */ + const executingIds = this.options.getExecutingTaskIds?.() ?? new Set(); const mergeable = tasks.filter((t) => t.column === "in-review" && allowsAutoMergeProcessing(t, settings) && !t.paused && + !executingIds.has(t.id) && t.status !== "failed" && // Exclude transient merge statuses. Active merges should be left alone; // stale ones are handled by recoverStaleMergingStatus().