fix(workflow): harden review-gate lifecycle interactions in In review
Follow-ups to running the pre-merge review gates in `in-review`. Each was verified against the code before being fixed; one reported issue was refuted and is noted below. 1. Symbol locks (packages/core/src/task-store/moves.ts) FN-8306 made the lifecycle transition the symbol-lock RELEASE authority but wrote no counterpart. That was harmless while a task only left WIP at handoff/terminal; the gate crossing now releases the task's declared symbols and the remediation node re-enters `in-progress` to edit the same files in the same live worktree with its locks gone. Neither acquire site (scheduler dispatch, claimDueWorkflowWorkItem) is on the graph re-entry path. Adds a symmetric re-acquire on `!wip -> wip`. Best-effort by design: a contended symbol logs and proceeds, which is exactly the pre-fix posture, rather than parking the remediation behind another holder and re-creating the stranding this change set removed. 2. Premature merge (packages/engine/src/self-healing.ts) `recoverMergeableReviewTasks` was the only in-review sweep with no liveness gate. The graph commits the column crossing at node entry and writes the gate's pending lease two DB round trips later, and `getTaskMergeBlocker` has no notion of "enabled but resultless", so in that window the sweep could enqueue a merge with Code Review never run. Filters `executingIds`, matching recoverGhostReviewTasks. 3. Orphan sweep (packages/engine/src/self-healing.ts) The reported restart hazard is REFUTED: nothing re-attaches an in-review graph run, so those leases are genuinely dead and marking them failed is correct FN-8492 behavior. But the sweep also runs from periodic maintenance in the same live process, where a tick between the lease write and session registration could fail a gate that just started. Honors a within-floor `classifyReviewLease`, matching the semantics Plan Review already had. Cleanup of dead leases is delayed by the staleness floor, not defeated. The audit event gains `needsOperatorBypass` for `autoMerge:false` rows, which self-healing deliberately skips and only fn_task_bypass_review can clear — previously indistinguishable from an auto-recoverable rewrite. 4. Stall detection (packages/engine/src/planner-overseer.ts) The `reviewer` and `merger` stages had no time-based check at all and returned `progressing` unconditionally, so a hung gate produced no signal however long it sat. Adds gate-anchored detection on both (a plain in-review card with no reviewState resolves to `merger`, not `reviewer`), keyed on the pending lease's own `startedAt` rather than `columnMovedAt` so it cannot fire during a legitimate human merge-wait. `cumulativeActiveMs` is documented, not changed: it now excludes gate runtime, but adding the `timing` trait to `in-review` would count arbitrary human merge-wait as active work — a worse distortion than the omission. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
7
.changeset/review-gate-lifecycle-followups.md
Normal file
7
.changeset/review-gate-lifecycle-followups.md
Normal file
@@ -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.
|
||||
@@ -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") {
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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<string, unknown>) => ({
|
||||
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");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<OverseerTaskRef>,
|
||||
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",
|
||||
|
||||
@@ -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<WorkflowStepResult>(
|
||||
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<string>();
|
||||
|
||||
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().
|
||||
|
||||
Reference in New Issue
Block a user