fix(FN-8004): make AI merge rejections actionable and stranded merges retryable (#2160)
Two follow-ups to FN-8004. Both were found by watching FN-8004's *own* merge livelock for 40 minutes — it turned out to be blocked by the very class of bug it was filed to fix. ## 1. AI merge rejections lost their reasons The reviewer prompt said **both** of these: > "**End with a single decision line**: `REVIEW_VERDICT: approve|reject`" > "**Then list each concrete reason as a bullet.**" Those are impossible to satisfy at once. Reviewers obeyed "End with" and wrote their reasoning *above* the verdict — but `extractRejectReasons` only scanned lines *after* it. So every such rejection collapsed to the placeholder `reviewer rejected the merge without a stated reason`, and that placeholder was then handed to the corrective re-merge pass **as its instruction**. The pass got no actionable feedback and just re-rolled the merge. The evidence, from FN-8004's own merge — the pattern repeated across *both* attempts: | | Attempt A | Attempt B | |---|---|---| | review pass 1 | rejected, no reason (03:46) | rejected, no reason (03:57) | | corrective pass | 1/3 | 1/3 | | review pass 2 | **approved** `a3a3cc6a8` (03:49) | approved | A reviewer that rejects and then approves identical content isn't objecting — the reason was being thrown away. Each wasted cycle cost ~7 minutes, stretching the merge past main's ~8-minute churn window so every attempt lost to a concurrent advance and rebuilt. **The livelock was caused by the lost-reason bug.** Fix: the parser recovers reasons from either side of the verdict (inline → after → before, nearest-first so the closing argument leads, capped at 8 so a long transcript can't flood the corrective prompt), skipping severity/verdict/markdown scaffolding. The prompt ordering is now unambiguous — reasons first, verdict last, nothing after it. ## 2. An orphaned merge-active stamp was un-retryable by hand The Retry gate refused **every** merge-active status (`Task is not in a retryable state (current status: landing)`), while self-healing cleared stale stamps automatically minutes later. So a merger killed mid-flight — crash, engine restart, operator SIGTERM — blocked the operator's own escape hatch at exactly the moment they'd reach for it. FN-8004 hit this: a killed merge left `landing` stamped and Retry 400'd for the full sweep delay. `isStaleMergeActiveStatus` now lives in the leaf `merge-active-status.ts`, shared by `recoverStaleMergingStatus` and the Retry gate — so **the manual path can never be stricter than the automatic one**. This is the same one-concept-two-definitions bug as FN-8004's transient classifier, which is why it's worth fixing structurally rather than adding another special case. A live merge stays protected by two independent signals: it holds the in-process lease **and** refreshes `updatedAt` each phase. Staleness fails closed on an unparseable timestamp. One subtlety worth reviewing: the bypass feeds `isInReviewRetry` rather than only the gate. A bare gate bypass would fall through to the generic branch and move fully-executed work to `todo`, **re-running finished work** — a bug this fix could easily have introduced. ## Verification - Gate green (294 + 122 + 63) · lint clean · engine + dashboard typecheck clean · `verify:fast` PASS - 70 merger-suite tests green; all 7 pre-existing verdict-parser tests still pass (backward compatible — none of them covered the verdict-last layout, which is exactly why this shipped) - **The route regression test was confirmed non-vacuous**: neutralizing the fix fails the two "now retryable" cases while the three live-merge-protection cases still pass, proving they guard real behavior rather than the new code. - Regression tests assert the invariant across every surface per *Fix the Invariant, Not the Repro*: all five `ACTIVE_MERGE_STATUSES` (a merger can die in any phase, not just the reported `landing`), both live-merge signals, boundary conditions, fail-closed paths, and that pre-existing retry paths are unchanged. Test files carry the required `## Symptom Verification` and `## Surface Enumeration` sections. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * AI merge rejections now reliably include concrete, correctly ordered reasons, even when provided before the verdict line. * Manual retry can recover tasks stuck in stale merge-processing states. * Retry is still blocked for tasks tied to active merge activity or recently updated/advancing merges. * Existing failed-merge retry behavior remains unchanged. * **Reliability** * Improved shared handling of “orphaned” merge-active detection across the engine and dashboard. * **Tests** * Added/expanded coverage for merge-active staleness, retry eligibility, and verdict/reason parsing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
summary: AI merge rejections now say why, and a stranded merge can be retried without waiting.
|
||||
category: fix
|
||||
dev: Two FN-8004 follow-ups. (1) The review prompt said both "End with a single decision line" and "Then list each concrete reason as a bullet"; reviewers obeyed the former, so reasons landed above the verdict where `extractRejectReasons` never looked, degrading every rejection to "rejected the merge without a stated reason" — which was then fed to the corrective re-merge as its instruction. The parser now recovers reasons from either side of the verdict (inline → after → before, capped at 8) and the prompt ordering is unambiguous. (2) `isStaleMergeActiveStatus` moves to the leaf `merge-active-status.ts`, shared by `SelfHealingManager.recoverStaleMergingStatus` and the dashboard Retry gate, which previously refused every merge-active status; an orphaned `landing` stamp is now retryable by hand while a live merge (holding the lease or refreshing `updatedAt`) is still protected.
|
||||
@@ -0,0 +1,207 @@
|
||||
// @vitest-environment node
|
||||
/*
|
||||
FNXC:MergeReliability 2026-07-15-22:05 (FN-8004 follow-up):
|
||||
|
||||
## Symptom Verification
|
||||
|
||||
Original symptom: FN-8004's AI merge was killed mid-flight, leaving `status: "landing"` stamped on
|
||||
the task. `POST /api/tasks/FN-8004/retry` then answered
|
||||
400 — "Task is not in a retryable state (current status: landing)"
|
||||
for the full self-healing sweep delay. The automatic sweep DID recover it minutes later, so the
|
||||
operator's manual escape hatch was blocked at exactly the moment it was needed.
|
||||
|
||||
Exact reproduction: an in-review task with a merge-active status, no live merge lease, and an
|
||||
`updatedAt` older than the staleness floor.
|
||||
|
||||
Assertion it is gone: that POST now succeeds AND takes the merge-retry branch — status/error
|
||||
cleared, mergeRetries reset, task STAYS in in-review. Staying put is load-bearing: routing a
|
||||
fully-executed task to `todo` would re-run finished work, which is the bug this fix could easily
|
||||
have introduced.
|
||||
|
||||
## Surface Enumeration
|
||||
|
||||
- Every status in ACTIVE_MERGE_STATUSES (merging / merging-pr / merging-fix / reviewing / landing),
|
||||
since a merger can die in any phase — not just the reported `landing`.
|
||||
- Live-merge protection via BOTH independent signals: the in-process lease, and a fresh updatedAt.
|
||||
- The pre-existing retry paths (failed / status-none merge stall) must be unchanged.
|
||||
*/
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import express from "express";
|
||||
import type { Task, TaskStore } from "@fusion/core";
|
||||
import { registerTaskWorkflowRoutes } from "../routes/register-task-workflow-routes.js";
|
||||
import { request as performRequest } from "../test-request.js";
|
||||
import { ApiError, sendErrorResponse } from "../api-error.js";
|
||||
import { ACTIVE_MERGE_STATUSES, DEFAULT_STALE_MERGING_STATUS_MIN_AGE_MS } from "@fusion/engine";
|
||||
|
||||
const NOW = Date.now();
|
||||
const STALE_AT = new Date(NOW - DEFAULT_STALE_MERGING_STATUS_MIN_AGE_MS - 60_000).toISOString();
|
||||
const FRESH_AT = new Date(NOW - 5_000).toISOString();
|
||||
|
||||
/** An in-review task whose implementation is complete — the FN-8004 shape. */
|
||||
function mkMergeTask(overrides: Partial<Task> = {}): Task {
|
||||
return {
|
||||
id: "FN-8004",
|
||||
title: "soft-delete heartbeat race",
|
||||
description: "d",
|
||||
column: "in-review",
|
||||
status: "landing",
|
||||
dependencies: [],
|
||||
createdAt: "2026-07-15T09:00:00.000Z",
|
||||
updatedAt: STALE_AT,
|
||||
size: "M",
|
||||
subtasks: [],
|
||||
log: [],
|
||||
tags: [],
|
||||
blockedBy: [],
|
||||
mergeRetries: 3,
|
||||
// All steps complete: this is a merge failure, not an execution failure.
|
||||
steps: [{ status: "done" }, { status: "done" }],
|
||||
source: { sourceType: "api" },
|
||||
...overrides,
|
||||
} as unknown as Task;
|
||||
}
|
||||
|
||||
function buildApp(input: { task: Task; activeMergeTaskId?: string | null; staleMergingStatusMinAgeMs?: number }) {
|
||||
const updateTask = vi.fn(async () => input.task);
|
||||
const moveTask = vi.fn(async () => input.task);
|
||||
const logEntry = vi.fn(async () => {});
|
||||
const store = {
|
||||
getTask: async () => input.task,
|
||||
getTaskDetail: async () => input.task,
|
||||
updateTask,
|
||||
moveTask,
|
||||
logEntry,
|
||||
getSettings: async () => ({}),
|
||||
getSettingsFast: async () => ({}),
|
||||
getRootDir: () => "/tmp/does-not-exist",
|
||||
listTasks: async () => [input.task],
|
||||
} as unknown as TaskStore;
|
||||
|
||||
const runtimeLogger = { warn: vi.fn(), error: vi.fn(), log: vi.fn() };
|
||||
const router = express.Router();
|
||||
registerTaskWorkflowRoutes({
|
||||
router,
|
||||
store,
|
||||
options: {},
|
||||
runtimeLogger: runtimeLogger as never,
|
||||
planningLogger: runtimeLogger as never,
|
||||
chatLogger: runtimeLogger as never,
|
||||
getProjectIdFromRequest: () => undefined,
|
||||
getScopedStore: async () => store,
|
||||
getProjectContext: async () => ({ store, engine: undefined as never, projectId: "p-1" }),
|
||||
prioritizeProjectsForCurrentDirectory: (projects: unknown) => projects,
|
||||
emitRemoteRouteDiagnostic: () => {},
|
||||
emitAuthSyncAuditLog: () => {},
|
||||
parseScopeParam: () => undefined,
|
||||
resolveAutomationStore: () => ({}) as never,
|
||||
resolveRoutineStore: () => ({}) as never,
|
||||
resolveRoutineRunner: () => ({}) as never,
|
||||
registerDispose: () => {},
|
||||
dispose: () => {},
|
||||
rethrowAsApiError: (error: unknown): never => {
|
||||
if (error instanceof ApiError) throw error;
|
||||
throw new ApiError(500, error instanceof Error ? error.message : "Internal server error");
|
||||
},
|
||||
} as never, {
|
||||
runtimeLogger,
|
||||
upload: { single: () => (_req: unknown, _res: unknown, next: () => void) => next() },
|
||||
taskDetailActivityLogLimit: 100,
|
||||
validateOptionalModelField: (value: unknown) => (typeof value === "string" ? value : undefined),
|
||||
normalizeModelSelectionPair: (provider: string | null, modelId: string | null) => ({ provider: provider ?? null, modelId: modelId ?? null }),
|
||||
runGitCommand: async () => "",
|
||||
isGitRepo: async () => true,
|
||||
resolveIntegrationBranch: async () => "main",
|
||||
trimTaskDetailActivityLog: (task: unknown) => task,
|
||||
triggerCommentWakeForAssignedAgent: async () => {},
|
||||
// The seam the fix reads for live-merge proof.
|
||||
resolveSelfHealingManager: () => ({
|
||||
getActiveMergeTaskId: () => input.activeMergeTaskId ?? null,
|
||||
getStaleMergingStatusMinAgeMs: () => input.staleMergingStatusMinAgeMs ?? DEFAULT_STALE_MERGING_STATUS_MIN_AGE_MS,
|
||||
}),
|
||||
} as never);
|
||||
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
app.use("/api", router);
|
||||
app.use((error: unknown, _req: express.Request, res: express.Response, _next: express.NextFunction) => {
|
||||
if (error instanceof ApiError) {
|
||||
sendErrorResponse(res, error.statusCode, error.message, { details: error.details });
|
||||
return;
|
||||
}
|
||||
sendErrorResponse(res, 500, error instanceof Error ? error.message : "Internal server error");
|
||||
});
|
||||
return { app, updateTask, moveTask, logEntry };
|
||||
}
|
||||
|
||||
describe("POST /api/tasks/:id/retry — orphaned merge-active status (FN-8004)", () => {
|
||||
it("retries a task stranded in 'landing' by a killed merger", async () => {
|
||||
const { app, updateTask, moveTask } = buildApp({ task: mkMergeTask() });
|
||||
|
||||
const res = await performRequest(app, "POST", "/api/tasks/FN-8004/retry", "{}", { "content-type": "application/json" });
|
||||
|
||||
// The regression: this used to be 400 "not in a retryable state (current status: landing)".
|
||||
expect(res.status).toBe(200);
|
||||
// Merge-retry branch: clear the stamp and reset the budget...
|
||||
expect(updateTask).toHaveBeenCalledWith(
|
||||
"FN-8004",
|
||||
expect.objectContaining({ status: null, error: null }),
|
||||
);
|
||||
// ...and STAY in in-review. Moving completed work to todo would re-run it.
|
||||
expect(moveTask).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("retries a task stranded in ANY merge-active phase, not just the reported one", async () => {
|
||||
for (const status of [...ACTIVE_MERGE_STATUSES]) {
|
||||
const { app } = buildApp({ task: mkMergeTask({ status } as Partial<Task>) });
|
||||
const res = await performRequest(app, "POST", "/api/tasks/FN-8004/retry", "{}", { "content-type": "application/json" });
|
||||
expect(res.status, `status=${status} must be retryable when orphaned`).toBe(200);
|
||||
}
|
||||
});
|
||||
|
||||
it("uses the configured staleness floor, matching automatic recovery", async () => {
|
||||
const twoMinutesAgo = new Date(Date.now() - 2 * 60_000).toISOString();
|
||||
const { app } = buildApp({
|
||||
task: mkMergeTask({ updatedAt: twoMinutesAgo }),
|
||||
staleMergingStatusMinAgeMs: 60_000,
|
||||
});
|
||||
|
||||
const res = await performRequest(app, "POST", "/api/tasks/FN-8004/retry", "{}", { "content-type": "application/json" });
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
});
|
||||
|
||||
it("still REFUSES to retry a merge holding the live in-process lease", async () => {
|
||||
const { app } = buildApp({ task: mkMergeTask(), activeMergeTaskId: "FN-8004" });
|
||||
|
||||
const res = await performRequest(app, "POST", "/api/tasks/FN-8004/retry", "{}", { "content-type": "application/json" });
|
||||
|
||||
expect(res.status).toBe(400);
|
||||
expect(JSON.stringify(res.body)).toContain("not in a retryable state");
|
||||
});
|
||||
|
||||
it("still REFUSES to retry a merge that is progressing (fresh updatedAt)", async () => {
|
||||
// Each merge phase writes a log entry, refreshing updatedAt — this is what stops
|
||||
// an operator from yanking a slow-but-live merge.
|
||||
const { app } = buildApp({ task: mkMergeTask({ updatedAt: FRESH_AT }) });
|
||||
|
||||
const res = await performRequest(app, "POST", "/api/tasks/FN-8004/retry", "{}", { "content-type": "application/json" });
|
||||
|
||||
expect(res.status).toBe(400);
|
||||
expect(JSON.stringify(res.body)).toContain("not in a retryable state");
|
||||
});
|
||||
|
||||
it("leaves the pre-existing failed-merge retry path unchanged", async () => {
|
||||
const { app, updateTask, moveTask } = buildApp({
|
||||
task: mkMergeTask({ status: "failed", updatedAt: FRESH_AT }),
|
||||
});
|
||||
|
||||
const res = await performRequest(app, "POST", "/api/tasks/FN-8004/retry", "{}", { "content-type": "application/json" });
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(updateTask).toHaveBeenCalledWith(
|
||||
"FN-8004",
|
||||
expect.objectContaining({ status: null, error: null }),
|
||||
);
|
||||
expect(moveTask).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
@@ -1239,6 +1239,7 @@ export function createApiRoutes(store: TaskStore, options?: ServerOptions): Rout
|
||||
rootDir: engine.getWorkingDirectory(),
|
||||
reconcileInReviewBranchRebind: selfHealing.reconcileInReviewBranchRebind.bind(selfHealing),
|
||||
getActiveMergeTaskId: selfHealing.getActiveMergeTaskId.bind(selfHealing),
|
||||
getStaleMergingStatusMinAgeMs: selfHealing.getStaleMergingStatusMinAgeMs.bind(selfHealing),
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
@@ -65,6 +65,9 @@ import {
|
||||
prepareRevertPrBranch,
|
||||
prepareWorkspaceRevertPrBranches,
|
||||
isInReviewMissingWorktreeSessionStartFailure,
|
||||
// FN-8004 follow-up: shared with SelfHealingManager.recoverStaleMergingStatus so the manual
|
||||
// Retry gate and the automatic sweep agree on when a merge-active stamp is orphaned.
|
||||
isStaleMergeActiveStatus,
|
||||
type AiUndoTaskResult,
|
||||
type PrepareRevertPrBranchResult,
|
||||
type PrepareWorkspaceRevertPrBranchesResult,
|
||||
@@ -638,6 +641,7 @@ interface TaskWorkflowRouteDeps {
|
||||
rootDir: string;
|
||||
reconcileInReviewBranchRebind: (opts?: { includeTaskIds?: Set<string> }) => Promise<import("@fusion/engine").RebindResult>;
|
||||
getActiveMergeTaskId: () => string | null;
|
||||
getStaleMergingStatusMinAgeMs: () => number;
|
||||
} | undefined;
|
||||
}
|
||||
|
||||
@@ -2347,12 +2351,41 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork
|
||||
hasIncompleteSteps || (task.steps.length === 0 && (task.mergeRetries ?? 0) === 0);
|
||||
const isInReviewExecutionStall = isInReviewStatusNone && isExecutionFailureInReview;
|
||||
const isInReviewMergeRetryStall = isInReviewStatusNone && (task.mergeRetries ?? 0) > 0;
|
||||
/*
|
||||
FNXC:MergeReliability 2026-07-15-21:45 (FN-8004 follow-up):
|
||||
An orphaned merge-active stamp used to be un-retryable BY HAND: this gate rejected every
|
||||
merge-active status ("Task is not in a retryable state (current status: landing)"), so when a
|
||||
merger died mid-flight — crash, engine restart, operator SIGTERM — the operator's escape hatch
|
||||
was blocked exactly when it was needed, and the only recourse was waiting out self-healing's
|
||||
recoverStaleMergingStatus sweep. Observed on FN-8004: a killed merge left `landing` stamped and
|
||||
manual Retry 400'd for the full sweep delay.
|
||||
|
||||
`isStaleMergeActiveStatus` and its configured age floor are the SAME inputs that sweep uses, so
|
||||
the manual path can never be looser than the automatic one. A genuinely RUNNING merge stays protected: it holds the
|
||||
in-process merge lease (activeMergeTaskId) and refreshes `updatedAt` each phase, so it fails
|
||||
both staleness checks and Retry still refuses it.
|
||||
|
||||
This feeds `isInReviewRetry` rather than only the gate: a bare gate bypass would fall through
|
||||
to the generic retry branch below and move a fully-executed task to `todo`, re-running finished
|
||||
work. Routing it through isInReviewRetry lands it on the merge-retry branch (clear status/error,
|
||||
reset mergeRetries, STAY in in-review) — identical to what the operator's Retry button does for
|
||||
a failed merge. A stale-stamped task that also has incomplete steps still routes to the
|
||||
execution branch via isExecutionFailureInReview, which is the correct handling for that case.
|
||||
*/
|
||||
const selfHealingManager = _resolveSelfHealingManager(scopedStore);
|
||||
const isStaleMergeActiveRetry =
|
||||
task.column === "in-review" &&
|
||||
isStaleMergeActiveStatus(task, {
|
||||
activeMergeTaskId: selfHealingManager?.getActiveMergeTaskId?.() ?? null,
|
||||
minAgeMs: selfHealingManager?.getStaleMergingStatusMinAgeMs?.(),
|
||||
});
|
||||
const isInReviewRetry =
|
||||
task.column === "in-review" &&
|
||||
(task.status === "failed" ||
|
||||
task.status === "stuck-killed" ||
|
||||
isInReviewExecutionStall ||
|
||||
isInReviewMergeRetryStall);
|
||||
isInReviewMergeRetryStall ||
|
||||
isStaleMergeActiveRetry);
|
||||
/*
|
||||
FNXC:MissingWorktreeRetry 2026-07-10-18:32:
|
||||
Dashboard retry must support the upstream #1992 signature where the task is stranded in a merge-active status but the durable failure is an unusable worktree session-start assertion. Only that classifier bypasses the merge-active status gate.
|
||||
|
||||
@@ -354,6 +354,7 @@ export interface ServerOptions {
|
||||
rootDir: string;
|
||||
reconcileInReviewBranchRebind: (opts?: { includeTaskIds?: Set<string> }) => Promise<import("@fusion/engine").RebindResult>;
|
||||
getActiveMergeTaskId: () => string | null;
|
||||
getStaleMergingStatusMinAgeMs: () => number;
|
||||
};
|
||||
/** Optional PluginStore for plugin management routes */
|
||||
pluginStore?: import("@fusion/core").PluginStore;
|
||||
@@ -907,6 +908,7 @@ export function createServer(store: TaskStore, options?: ServerOptions): ReturnT
|
||||
rootDir: engine.getWorkingDirectory(),
|
||||
reconcileInReviewBranchRebind: selfHealing.reconcileInReviewBranchRebind.bind(selfHealing),
|
||||
getActiveMergeTaskId: selfHealing.getActiveMergeTaskId.bind(selfHealing),
|
||||
getStaleMergingStatusMinAgeMs: selfHealing.getStaleMergingStatusMinAgeMs.bind(selfHealing),
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
118
packages/engine/src/__tests__/merge-active-status.test.ts
Normal file
118
packages/engine/src/__tests__/merge-active-status.test.ts
Normal file
@@ -0,0 +1,118 @@
|
||||
/*
|
||||
FNXC:MergeReliability 2026-07-15-21:55 (FN-8004 follow-up):
|
||||
`isStaleMergeActiveStatus` is the SINGLE definition of "orphaned merge-active stamp", shared by
|
||||
SelfHealingManager.recoverStaleMergingStatus (which clears it automatically) and the dashboard's
|
||||
manual Retry gate (which must not refuse a task no merger owns).
|
||||
|
||||
Before the split those two disagreed: the sweep recovered stale stamps after a bounded delay, but
|
||||
manual Retry rejected EVERY merge-active status outright ("Task is not in a retryable state
|
||||
(current status: landing)"). So a merger killed mid-flight — crash, engine restart, operator
|
||||
SIGTERM — left a stamp only the sweep could clear, blocking the operator's escape hatch exactly
|
||||
when it was needed. Observed on FN-8004.
|
||||
|
||||
The safety-critical invariant, asserted below: a LIVE merge is never classified stale, because a
|
||||
live merger either holds the in-process lease or keeps refreshing `updatedAt`. Both signals must
|
||||
independently block staleness — the manual gate leans on this to avoid yanking a running merge.
|
||||
*/
|
||||
import { describe, expect, it } from "vitest";
|
||||
import {
|
||||
ACTIVE_MERGE_STATUSES,
|
||||
DEFAULT_STALE_MERGING_STATUS_MIN_AGE_MS,
|
||||
isMergeActiveStatus,
|
||||
isStaleMergeActiveStatus,
|
||||
} from "../merge-active-status.js";
|
||||
|
||||
const NOW = Date.parse("2026-07-16T00:00:00.000Z");
|
||||
const ago = (ms: number) => new Date(NOW - ms).toISOString();
|
||||
/** Comfortably past the staleness floor. */
|
||||
const LONG_AGO = ago(DEFAULT_STALE_MERGING_STATUS_MIN_AGE_MS + 60_000);
|
||||
|
||||
const task = (over: Partial<{ id: string; status: string | null; updatedAt: string }> = {}) => ({
|
||||
id: "FN-1",
|
||||
status: "landing",
|
||||
updatedAt: LONG_AGO,
|
||||
...over,
|
||||
});
|
||||
|
||||
describe("isMergeActiveStatus", () => {
|
||||
it("covers every phase a merger can die in", () => {
|
||||
// reviewing/landing are as reclaimable as merging — a process killed in either
|
||||
// leaves the identical orphaned stamp. FN-8004 died in `landing`.
|
||||
for (const s of ["merging", "merging-pr", "merging-fix", "reviewing", "landing"]) {
|
||||
expect(isMergeActiveStatus(s)).toBe(true);
|
||||
expect(ACTIVE_MERGE_STATUSES.has(s)).toBe(true);
|
||||
}
|
||||
});
|
||||
|
||||
it("does not treat terminal or absent statuses as merge-active", () => {
|
||||
for (const s of ["failed", "stuck-killed", "needs-replan", null, undefined, ""]) {
|
||||
expect(isMergeActiveStatus(s as string | null | undefined)).toBe(false);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("isStaleMergeActiveStatus — the FN-8004 wedge", () => {
|
||||
it("classifies an orphaned `landing` stamp as stale so manual Retry is unblocked", () => {
|
||||
expect(isStaleMergeActiveStatus(task(), { nowMs: NOW })).toBe(true);
|
||||
});
|
||||
|
||||
it("classifies every merge-active phase as stale once orphaned", () => {
|
||||
for (const status of [...ACTIVE_MERGE_STATUSES]) {
|
||||
expect(isStaleMergeActiveStatus(task({ status }), { nowMs: NOW })).toBe(true);
|
||||
}
|
||||
});
|
||||
|
||||
it("NEVER classifies a task holding the live in-process merge lease as stale", () => {
|
||||
// Safety invariant: a live owner must be protected no matter how old updatedAt looks.
|
||||
expect(
|
||||
isStaleMergeActiveStatus(task({ id: "FN-1" }), { activeMergeTaskId: "FN-1", nowMs: NOW }),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("still classifies a stale task when a DIFFERENT task holds the lease", () => {
|
||||
// The lease only protects its own owner; an unrelated live merge must not keep
|
||||
// every other orphaned stamp un-retryable.
|
||||
expect(
|
||||
isStaleMergeActiveStatus(task({ id: "FN-1" }), { activeMergeTaskId: "FN-2", nowMs: NOW }),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("NEVER classifies a merge that is slow but progressing as stale", () => {
|
||||
// Each merge phase writes a log entry, refreshing updatedAt. This is what stops the
|
||||
// manual Retry button from yanking a running merge.
|
||||
expect(
|
||||
isStaleMergeActiveStatus(task({ updatedAt: ago(30_000) }), { nowMs: NOW }),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("respects the staleness floor exactly at the boundary", () => {
|
||||
const atFloor = ago(DEFAULT_STALE_MERGING_STATUS_MIN_AGE_MS);
|
||||
const justUnder = ago(DEFAULT_STALE_MERGING_STATUS_MIN_AGE_MS - 1_000);
|
||||
expect(isStaleMergeActiveStatus(task({ updatedAt: atFloor }), { nowMs: NOW })).toBe(true);
|
||||
expect(isStaleMergeActiveStatus(task({ updatedAt: justUnder }), { nowMs: NOW })).toBe(false);
|
||||
});
|
||||
|
||||
it("honors a custom minAgeMs and refuses a non-positive one", () => {
|
||||
expect(isStaleMergeActiveStatus(task({ updatedAt: ago(90_000) }), { nowMs: NOW, minAgeMs: 60_000 })).toBe(true);
|
||||
expect(isStaleMergeActiveStatus(task({ updatedAt: ago(90_000) }), { nowMs: NOW, minAgeMs: 120_000 })).toBe(false);
|
||||
// A disabled/invalid floor must not make everything stale.
|
||||
for (const minAgeMs of [0, -1, Number.NaN, Number.POSITIVE_INFINITY]) {
|
||||
expect(isStaleMergeActiveStatus(task(), { nowMs: NOW, minAgeMs })).toBe(false);
|
||||
}
|
||||
});
|
||||
|
||||
it("fails closed on a missing or unparseable updatedAt", () => {
|
||||
// No staleness evidence must never be read as "stale" — that could yank a live merge.
|
||||
expect(isStaleMergeActiveStatus(task({ updatedAt: "" }), { nowMs: NOW })).toBe(false);
|
||||
expect(isStaleMergeActiveStatus(task({ updatedAt: "not-a-date" }), { nowMs: NOW })).toBe(false);
|
||||
expect(
|
||||
isStaleMergeActiveStatus({ id: "FN-1", status: "landing" } as never, { nowMs: NOW }),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("never classifies a non-merge-active status as stale, however old", () => {
|
||||
for (const status of ["failed", "stuck-killed", null]) {
|
||||
expect(isStaleMergeActiveStatus(task({ status }), { nowMs: NOW })).toBe(false);
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -84,3 +84,123 @@ describe("merger-ai prompt/verdict re-exports", () => {
|
||||
expect(buildReviewSystemPrompt()).toContain("Do NOT edit, stage, commit");
|
||||
});
|
||||
});
|
||||
|
||||
/*
|
||||
FNXC:MergerAiReview 2026-07-15-21:50 (FN-8004 follow-up):
|
||||
The prompt says "End with a single decision line", so a COMPLIANT reviewer puts its reasoning
|
||||
ABOVE `REVIEW_VERDICT: reject` and ends on the verdict. The parser only scanned lines AFTER the
|
||||
verdict, so those reasons were dropped and every such rejection degraded to the placeholder
|
||||
"reviewer rejected the merge without a stated reason" — which was then handed to the corrective
|
||||
re-merge pass AS its instruction, making the pass a blind re-roll.
|
||||
|
||||
Observed on FN-8004's own merge: both attempts ran reject(no reason) → corrective pass → approve,
|
||||
~7 min per wasted cycle, pushing each attempt past main's ~8-min churn window into a livelock.
|
||||
|
||||
Per "Fix the Invariant, Not the Repro": the invariant is that a reviewer's stated reasons reach the
|
||||
corrective pass REGARDLESS of which side of the verdict line they were written on.
|
||||
*/
|
||||
describe("parseReviewVerdict — reason recovery (FN-8004)", () => {
|
||||
it("recovers reasons written ABOVE a trailing verdict line", () => {
|
||||
// The exact layout the prompt's "End with a single decision line" produces.
|
||||
const result = parseReviewVerdict(
|
||||
[
|
||||
"The squash drops the run-audit event added by the task branch.",
|
||||
"SEVERITY: blocking",
|
||||
`${REVIEW_VERDICT_MARKER} reject`,
|
||||
].join("\n")
|
||||
);
|
||||
expect(result.verdict).toBe("reject");
|
||||
expect(result.severity).toBe("blocking");
|
||||
expect(result.reasons).toEqual([
|
||||
"The squash drops the run-audit event added by the task branch.",
|
||||
]);
|
||||
// The regression: this used to be the placeholder.
|
||||
expect(result.reasons).not.toContain(
|
||||
"reviewer rejected the merge without a stated reason"
|
||||
);
|
||||
});
|
||||
|
||||
it("orders recovered reasons nearest-the-verdict first (the closing argument)", () => {
|
||||
const result = parseReviewVerdict(
|
||||
[
|
||||
"- first observation",
|
||||
"- final blocking defect",
|
||||
`${REVIEW_VERDICT_MARKER} reject`,
|
||||
].join("\n")
|
||||
);
|
||||
expect(result.reasons).toEqual(["final blocking defect", "first observation"]);
|
||||
});
|
||||
|
||||
it("prefers reasons AFTER the verdict when both sides have content", () => {
|
||||
// Precedence must not change for reviewers that already follow the old layout.
|
||||
const result = parseReviewVerdict(
|
||||
[
|
||||
"some preamble analysis",
|
||||
`${REVIEW_VERDICT_MARKER} reject`,
|
||||
"- dropped api.ts",
|
||||
].join("\n")
|
||||
);
|
||||
expect(result.reasons).toEqual(["dropped api.ts"]);
|
||||
});
|
||||
|
||||
it("ignores markdown scaffolding, severity, and the verdict line itself", () => {
|
||||
const result = parseReviewVerdict(
|
||||
[
|
||||
"## Review",
|
||||
"---",
|
||||
"```",
|
||||
"SEVERITY: blocking",
|
||||
"genuine defect here",
|
||||
`${REVIEW_VERDICT_MARKER} reject`,
|
||||
].join("\n")
|
||||
);
|
||||
expect(result.reasons).toEqual(["genuine defect here"]);
|
||||
});
|
||||
|
||||
it("ignores fenced scaffolding on both sides before recovering a preceding reason", () => {
|
||||
const result = parseReviewVerdict(
|
||||
[
|
||||
"- genuine defect here",
|
||||
"```diff",
|
||||
"- raw evidence that is not reviewer feedback",
|
||||
"```",
|
||||
`${REVIEW_VERDICT_MARKER} reject`,
|
||||
"```",
|
||||
].join("\n")
|
||||
);
|
||||
expect(result.reasons).toEqual(["genuine defect here"]);
|
||||
});
|
||||
|
||||
it("caps recovered reasons so a long transcript cannot flood the corrective prompt", () => {
|
||||
const body = Array.from({ length: 30 }, (_, i) => `- reason ${i}`);
|
||||
const result = parseReviewVerdict(
|
||||
[...body, `${REVIEW_VERDICT_MARKER} reject`].join("\n")
|
||||
);
|
||||
expect(result.reasons.length).toBeLessThanOrEqual(8);
|
||||
expect(result.reasons[0]).toBe("reason 29");
|
||||
});
|
||||
|
||||
it("still reports the placeholder when the reviewer truly stated nothing", () => {
|
||||
const result = parseReviewVerdict(`${REVIEW_VERDICT_MARKER} reject`);
|
||||
expect(result.reasons).toEqual([
|
||||
"reviewer rejected the merge without a stated reason",
|
||||
]);
|
||||
});
|
||||
|
||||
it("does not attach reasons to an approve verdict", () => {
|
||||
const result = parseReviewVerdict(
|
||||
`Everything checks out.\n${REVIEW_VERDICT_MARKER} approve`
|
||||
);
|
||||
expect(result).toEqual({ verdict: "approve", reasons: [] });
|
||||
});
|
||||
|
||||
it("gives the reviewer an unambiguous, self-consistent ordering instruction", () => {
|
||||
const prompt = buildReviewSystemPrompt();
|
||||
// The old prompt said BOTH "End with a single decision line" AND "Then list each
|
||||
// concrete reason as a bullet" — impossible to satisfy simultaneously, and the
|
||||
// direct cause of reasons landing where the parser could not see them.
|
||||
expect(prompt).toContain("first list each concrete reason");
|
||||
expect(prompt).toContain("nothing after it");
|
||||
expect(prompt).not.toMatch(/End with a single decision line[\s\S]*Then list each concrete reason/);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -852,6 +852,18 @@ export { StuckTaskDetector, type StuckTaskDetectorOptions, type DisposableSessio
|
||||
export { HeartbeatMonitor, HeartbeatTriggerScheduler, type WakeContext } from "./agent-heartbeat.js";
|
||||
export { TokenCapDetector, type TokenCapCheckResult } from "./token-cap-detector.js";
|
||||
export { SelfHealingManager, type SelfHealingOptions, type RebindResult } from "./self-healing.js";
|
||||
/*
|
||||
FNXC:MergeReliability 2026-07-15-21:45 (FN-8004 follow-up):
|
||||
Exported for the dashboard's manual Retry gate, which must share ONE definition of "orphaned
|
||||
merge-active stamp" with SelfHealingManager.recoverStaleMergingStatus. Two copies is how the
|
||||
manual path drifted into refusing every merge-active status while the sweep cleared it.
|
||||
*/
|
||||
export {
|
||||
ACTIVE_MERGE_STATUSES,
|
||||
DEFAULT_STALE_MERGING_STATUS_MIN_AGE_MS,
|
||||
isMergeActiveStatus,
|
||||
isStaleMergeActiveStatus,
|
||||
} from "./merge-active-status.js";
|
||||
export { PluginRunner, type PluginRunnerOptions } from "./plugin-runner.js";
|
||||
export {
|
||||
registerPluginTraits,
|
||||
|
||||
77
packages/engine/src/merge-active-status.ts
Normal file
77
packages/engine/src/merge-active-status.ts
Normal file
@@ -0,0 +1,77 @@
|
||||
/**
|
||||
* Shared definition of "a task is stamped with an active merge status".
|
||||
*
|
||||
* FNXC:MergeReliability 2026-07-15-21:45 (FN-8004 follow-up):
|
||||
* A merge-active status (`merging`/`reviewing`/`landing`/…) means "a merger owns this task right
|
||||
* now". Two consumers must agree on when that stamp is STALE — i.e. no live merger holds it:
|
||||
*
|
||||
* - `SelfHealingManager.recoverStaleMergingStatus()` clears the stamp automatically.
|
||||
* - The dashboard's manual Retry gate refuses to retry a task that a live merger owns.
|
||||
*
|
||||
* Before this module they did NOT agree: self-healing recovered stale stamps after a bounded
|
||||
* delay, but the manual gate rejected EVERY merge-active status outright ("Task is not in a
|
||||
* retryable state (current status: landing)"). So when a merger died mid-flight — a crash, an
|
||||
* engine restart, an operator SIGTERM — the operator's own escape hatch was blocked precisely
|
||||
* when they needed it, and the only recourse was waiting out the sweep.
|
||||
*
|
||||
* This is the same class of bug as the FN-8004 transient-classifier split: one concept, two
|
||||
* definitions, silently diverging. Keeping the predicate here means the manual path can never be
|
||||
* stricter than the automatic one.
|
||||
*
|
||||
* Leaf module by design — types only, no logger/store imports — so both the engine's sweep and
|
||||
* the dashboard route can import it without inheriting a runtime dependency chain.
|
||||
*/
|
||||
import type { Task } from "@fusion/core";
|
||||
|
||||
/**
|
||||
* Statuses meaning "a merger owns this task right now".
|
||||
*
|
||||
* `reviewing` is the AI-merge review pass; `landing` is the ref-advance/finalize phase. Both are
|
||||
* as reclaimable as `merging` when no live owner exists — a process killed during either leaves
|
||||
* the same orphaned stamp.
|
||||
*/
|
||||
export const ACTIVE_MERGE_STATUSES: ReadonlySet<string> = new Set([
|
||||
"merging",
|
||||
"merging-pr",
|
||||
"merging-fix",
|
||||
"reviewing",
|
||||
"landing",
|
||||
]);
|
||||
|
||||
/** How long a merge-active stamp must sit untouched before it counts as orphaned. */
|
||||
export const DEFAULT_STALE_MERGING_STATUS_MIN_AGE_MS = 5 * 60_000;
|
||||
|
||||
export function isMergeActiveStatus(status: string | null | undefined): boolean {
|
||||
return Boolean(status && ACTIVE_MERGE_STATUSES.has(status));
|
||||
}
|
||||
|
||||
/**
|
||||
* True when `task` carries a merge-active stamp that no live merger owns.
|
||||
*
|
||||
* Deliberately conservative — a task is stale only when EVERY check passes:
|
||||
* 1. it carries a merge-active status;
|
||||
* 2. it is not the in-process merge owner (`activeMergeTaskId`), which proves a live merger; and
|
||||
* 3. its `updatedAt` has not moved for `minAgeMs`, so a merger that is slow but progressing
|
||||
* (each phase writes a log entry, refreshing `updatedAt`) is never mistaken for a dead one.
|
||||
*
|
||||
* Rule 3 is what makes this safe to expose to a manual Retry button: an operator cannot yank a
|
||||
* merge that is actually running, because a running merge keeps its own stamp fresh.
|
||||
*/
|
||||
export function isStaleMergeActiveStatus(
|
||||
task: Pick<Task, "id" | "status" | "updatedAt">,
|
||||
opts: { activeMergeTaskId?: string | null; nowMs?: number; minAgeMs?: number } = {},
|
||||
): boolean {
|
||||
if (!isMergeActiveStatus(task.status)) return false;
|
||||
|
||||
// A live in-process owner is authoritative proof the merge is running.
|
||||
if (opts.activeMergeTaskId && opts.activeMergeTaskId === task.id) return false;
|
||||
|
||||
const minAgeMs = opts.minAgeMs ?? DEFAULT_STALE_MERGING_STATUS_MIN_AGE_MS;
|
||||
if (!Number.isFinite(minAgeMs) || minAgeMs <= 0) return false;
|
||||
|
||||
const updatedAtMs = task.updatedAt ? Date.parse(task.updatedAt) : Number.NaN;
|
||||
// Unparseable timestamp = no staleness evidence. Fail closed: never call it stale.
|
||||
if (!Number.isFinite(updatedAtMs)) return false;
|
||||
|
||||
return (opts.nowMs ?? Date.now()) - updatedAtMs >= minAgeMs;
|
||||
}
|
||||
@@ -26,6 +26,14 @@ export const REVIEW_VERDICT_MARKER = "REVIEW_VERDICT:";
|
||||
const VERDICT_LINE_RE = /REVIEW_VERDICT:\s*(approve|reject)\b/i;
|
||||
const SEVERITY_LINE_RE = /SEVERITY:\s*(blocking|advisory)\b/i;
|
||||
|
||||
/*
|
||||
FNXC:MergerAiReview 2026-07-15-21:30:
|
||||
Cap on reasons recovered from lines PRECEDING the verdict (FN-8004 follow-up). The reviewer's
|
||||
free-form analysis can run long; the corrective re-merge prompt only needs the closing argument,
|
||||
and an unbounded splice would paste an entire transcript into it.
|
||||
*/
|
||||
const MAX_RECOVERED_PRECEDING_REASONS = 8;
|
||||
|
||||
/**
|
||||
* Parse the reviewer's free-form output. Fail-safe: no/garbled output, or a
|
||||
* rejection with no explicit severity, is treated as a BLOCKING reject — an
|
||||
@@ -75,6 +83,60 @@ export function parseReviewVerdict(
|
||||
};
|
||||
}
|
||||
|
||||
/** Strip bullet/numeric list markers and surrounding whitespace from one line. */
|
||||
function cleanReasonLine(line: string): string {
|
||||
return line.replace(/^\s*(?:[-*•]|\d+[.)])\s+/, "").trim();
|
||||
}
|
||||
|
||||
/** Lines that carry no reviewer reasoning and must never be reported as a reason. */
|
||||
function isNonReasonLine(line: string): boolean {
|
||||
const t = line.trim();
|
||||
if (!t) return true;
|
||||
if (SEVERITY_LINE_RE.test(t)) return true;
|
||||
if (VERDICT_LINE_RE.test(t)) return true;
|
||||
// Markdown scaffolding the reviewer may emit around its analysis.
|
||||
if (/^#{1,6}\s/.test(t)) return true;
|
||||
if (/^(?:-{3,}|={3,}|`{3,})/.test(t)) return true;
|
||||
return false;
|
||||
}
|
||||
|
||||
/*
|
||||
FNXC:MergerAiReview 2026-07-15-14:45:
|
||||
FN-8004 corrective merges must receive reviewer conclusions, never pasted diff or tool output. Track Markdown fences while recovering reasons on either side of a verdict so nearby evidence cannot displace actionable feedback.
|
||||
*/
|
||||
function collectReasonLines(lines: string[], start: number, end: number, step: 1 | -1): string[] {
|
||||
const reasons: string[] = [];
|
||||
let inFence = false;
|
||||
for (let i = start; step === 1 ? i < end : i >= end; i += step) {
|
||||
if (/^\s*(?:`{3,}|~{3,})/.test(lines[i])) {
|
||||
inFence = !inFence;
|
||||
continue;
|
||||
}
|
||||
if (inFence || isNonReasonLine(lines[i])) continue;
|
||||
reasons.push(cleanReasonLine(lines[i]));
|
||||
if (reasons.length >= MAX_RECOVERED_PRECEDING_REASONS) break;
|
||||
}
|
||||
return reasons;
|
||||
}
|
||||
|
||||
/*
|
||||
FNXC:MergerAiReview 2026-07-15-21:30:
|
||||
FN-8004 follow-up. The prompt tells the reviewer to "End with a single decision line", so a
|
||||
compliant reviewer writes its reasoning ABOVE `REVIEW_VERDICT: reject` and ends on the verdict.
|
||||
This function only scanned lines AFTER the verdict line, so those reasons were discarded and the
|
||||
rejection degraded to the placeholder "rejected the merge without a stated reason" — which was then
|
||||
handed to the corrective re-merge pass as its instruction. The corrective pass thus received no
|
||||
actionable feedback and merely re-rolled the merge, which typically passed on the next review.
|
||||
|
||||
Observed on FN-8004's own merge: BOTH attempts went reject(no reason) → corrective pass → approve,
|
||||
burning ~7 min per cycle and pushing each attempt past main's ~8-min churn window into a livelock.
|
||||
That is a lost-reason bug, not a reviewer that genuinely objects twice and then relents.
|
||||
|
||||
Reason precedence: inline (same line as the verdict) → lines after the verdict → lines before it
|
||||
(nearest-first, the "End with the verdict" layout). Falling back to the preceding lines is what
|
||||
makes a compliant reviewer's rejection actionable. Keep the prompt's ordering guidance and this
|
||||
precedence in sync — see buildMergeReviewSystemPrompt.
|
||||
*/
|
||||
function extractRejectReasons(
|
||||
lines: string[],
|
||||
verdictLineIndex: number
|
||||
@@ -85,10 +147,11 @@ function extractRejectReasons(
|
||||
.replace(/^[\s:–—-]+/, "")
|
||||
.trim();
|
||||
if (inline) reasons.push(inline);
|
||||
for (let i = verdictLineIndex + 1; i < lines.length; i++) {
|
||||
if (SEVERITY_LINE_RE.test(lines[i])) continue;
|
||||
const cleaned = lines[i].replace(/^\s*(?:[-*•]|\d+[.)])\s+/, "").trim();
|
||||
if (cleaned) reasons.push(cleaned);
|
||||
reasons.push(...collectReasonLines(lines, verdictLineIndex + 1, lines.length, 1));
|
||||
if (reasons.length === 0) {
|
||||
// Walk backwards from the verdict so the reviewer's closing argument — the part
|
||||
// most likely to state the blocking defect — is reported first.
|
||||
reasons.push(...collectReasonLines(lines, verdictLineIndex - 1, 0, -1));
|
||||
}
|
||||
if (reasons.length === 0)
|
||||
reasons.push("reviewer rejected the merge without a stated reason");
|
||||
@@ -228,14 +291,29 @@ export function buildReviewSystemPrompt(): string {
|
||||
"",
|
||||
"Bias toward rejection when uncertain.",
|
||||
"",
|
||||
`End with a single decision line: "${REVIEW_VERDICT_MARKER} approve" or`,
|
||||
`"${REVIEW_VERDICT_MARKER} reject". When rejecting, add a "SEVERITY:" line:`,
|
||||
/*
|
||||
FNXC:MergerAiReview 2026-07-15-21:30:
|
||||
FN-8004 follow-up. This block used to say "End with a single decision line ... Then list each
|
||||
concrete reason as a bullet" — self-contradictory: a reviewer cannot both END on the verdict
|
||||
and list reasons after it. Reviewers obeyed "End with", so the reasons landed above the verdict
|
||||
where the parser did not look, and every rejection degraded to "without a stated reason".
|
||||
The ordering below is now unambiguous: reasons FIRST, verdict LAST. The parser additionally
|
||||
recovers reasons from either side (see extractRejectReasons), so both layouts stay actionable.
|
||||
*/
|
||||
"When rejecting, first list each concrete reason as its own bullet — state the",
|
||||
"specific defect (what was dropped, lost, or wrongly resolved), not a generic",
|
||||
"objection. These bullets are fed verbatim to the corrective re-merge pass, so a",
|
||||
"vague reason produces a blind retry rather than a fix.",
|
||||
"",
|
||||
`Then add a "SEVERITY:" line:`,
|
||||
" - SEVERITY: blocking — a correctness problem (dropped/lost task changes,",
|
||||
" incomplete squash, or a conflict resolution that discards intent). The",
|
||||
" merge must NOT land if this is unfixable.",
|
||||
" - SEVERITY: advisory — a quality/style concern that does not risk",
|
||||
" correctness; acceptable to land if unresolved.",
|
||||
"Then list each concrete reason as a bullet.",
|
||||
"",
|
||||
`Finish with a single decision line, and nothing after it:`,
|
||||
`"${REVIEW_VERDICT_MARKER} approve" or "${REVIEW_VERDICT_MARKER} reject".`,
|
||||
].join("\n");
|
||||
}
|
||||
|
||||
|
||||
@@ -458,7 +458,15 @@ AI merge sets status="reviewing" during the clean-room review pass (merger-ai me
|
||||
FNXC:MergeQueue 2026-07-15-10:40:
|
||||
Include landing (post-approve advance/cleanup) with reviewing so a hung worktree remove cannot leave the single-flight pump un-reclaimable while the board shows no merging badge.
|
||||
*/
|
||||
const ACTIVE_MERGE_STATUSES = new Set(["merging", "merging-pr", "merging-fix", "reviewing", "landing"]);
|
||||
/*
|
||||
FNXC:MergeReliability 2026-07-15-21:45 (FN-8004 follow-up):
|
||||
Moved to the leaf `merge-active-status.ts` and re-exported so the dashboard's manual Retry gate
|
||||
shares ONE definition with this sweep. Previously the manual gate hardcoded its own stricter view
|
||||
and refused to retry ANY merge-active status, so an orphaned `landing` stamp was un-retryable by
|
||||
hand while this sweep cleared it automatically minutes later.
|
||||
*/
|
||||
import { ACTIVE_MERGE_STATUSES, DEFAULT_STALE_MERGING_STATUS_MIN_AGE_MS, isStaleMergeActiveStatus } from "./merge-active-status.js";
|
||||
export { ACTIVE_MERGE_STATUSES, DEFAULT_STALE_MERGING_STATUS_MIN_AGE_MS, isMergeActiveStatus, isStaleMergeActiveStatus } from "./merge-active-status.js";
|
||||
const NON_TERMINAL_STEP_STATUSES = new Set(["pending", "in-progress"]);
|
||||
const STRANDED_COMPLETED_TODO_ACTIVE_STATUSES = new Set([
|
||||
"in-progress",
|
||||
@@ -537,7 +545,8 @@ import { classifyTransientMergeError } from "./transient-merge-error-classifier.
|
||||
export { classifyTransientMergeError } from "./transient-merge-error-classifier.js";
|
||||
const MAX_STARVATION_DROPS = 3;
|
||||
const DEADLOCK_RECOVERY_COOLDOWN_MS = 15 * 60_000;
|
||||
const DEFAULT_STALE_MERGING_STATUS_MIN_AGE_MS = 5 * 60_000;
|
||||
// DEFAULT_STALE_MERGING_STATUS_MIN_AGE_MS now lives in ./merge-active-status.js (imported above)
|
||||
// so the manual Retry gate and this sweep cannot drift apart (FN-8004 follow-up).
|
||||
const DEFAULT_STALE_MERGING_FANOUT_MIN_AGE_MS = 15 * 60_000;
|
||||
const DEFAULT_UNBACKED_MERGING_FANOUT_GRACE_MS = 60_000;
|
||||
const DURABLE_ERROR_RECOVERY_BASE_COOLDOWN_MS = 30_000;
|
||||
@@ -951,6 +960,11 @@ export class SelfHealingManager {
|
||||
return this.options.getActiveMergeTaskId?.() ?? null;
|
||||
}
|
||||
|
||||
/** The configured staleness floor shared by automatic recovery and manual Retry. */
|
||||
public getStaleMergingStatusMinAgeMs(): number {
|
||||
return this.options.staleMergingStatusMinAgeMs ?? DEFAULT_STALE_MERGING_STATUS_MIN_AGE_MS;
|
||||
}
|
||||
|
||||
private async isMergeLaneOwned(taskId: string): Promise<boolean> {
|
||||
if (this.options.getActiveMergeTaskId?.() === taskId) return true;
|
||||
|
||||
@@ -2995,7 +3009,7 @@ export class SelfHealingManager {
|
||||
const settings = await this.store.getSettings();
|
||||
if (settings.globalPause || settings.enginePaused) return 0;
|
||||
|
||||
const minAgeMs = this.options.staleMergingStatusMinAgeMs ?? DEFAULT_STALE_MERGING_STATUS_MIN_AGE_MS;
|
||||
const minAgeMs = this.getStaleMergingStatusMinAgeMs();
|
||||
if (!Number.isFinite(minAgeMs) || minAgeMs <= 0) return 0;
|
||||
|
||||
const now = Date.now();
|
||||
@@ -3003,15 +3017,15 @@ export class SelfHealingManager {
|
||||
const tasks = await this.store.listTasks({ column: "in-review", slim: true });
|
||||
const stale = tasks.filter((task) => {
|
||||
if (task.column !== "in-review" || task.paused) return false;
|
||||
// Include AI-merge "reviewing" (same ACTIVE_MERGE_STATUSES as interrupted recovery).
|
||||
if (!task.status || !ACTIVE_MERGE_STATUSES.has(task.status)) return false;
|
||||
// Live owner is reclaimed by recoverWedgedActiveMerge / recoverInterruptedMergingTasks
|
||||
// using merger-silence clocks; this path only clears orphan status without a live owner.
|
||||
if (activeMergeTaskId && activeMergeTaskId === task.id) return false;
|
||||
|
||||
const updatedAtMs = task.updatedAt ? Date.parse(task.updatedAt) : Number.NaN;
|
||||
if (!Number.isFinite(updatedAtMs)) return false;
|
||||
return now - updatedAtMs >= minAgeMs;
|
||||
/*
|
||||
FNXC:MergeReliability 2026-07-15-21:45 (FN-8004 follow-up):
|
||||
Staleness now comes from the shared `isStaleMergeActiveStatus` leaf, which the dashboard's
|
||||
manual Retry gate also calls. Inlining the rule here is what let the manual path drift into
|
||||
refusing every merge-active status. Covers: merge-active stamp, no live in-process owner
|
||||
(reclaimed instead by recoverWedgedActiveMerge / recoverInterruptedMergingTasks via
|
||||
merger-silence clocks), and `updatedAt` untouched for minAgeMs.
|
||||
*/
|
||||
return isStaleMergeActiveStatus(task, { activeMergeTaskId, nowMs: now, minAgeMs });
|
||||
});
|
||||
|
||||
if (stale.length === 0) return 0;
|
||||
|
||||
Reference in New Issue
Block a user