diff --git a/.changeset/fn-8004-merge-review-reasons-and-landing-retry.md b/.changeset/fn-8004-merge-review-reasons-and-landing-retry.md new file mode 100644 index 0000000000..247067f88a --- /dev/null +++ b/.changeset/fn-8004-merge-review-reasons-and-landing-retry.md @@ -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. diff --git a/packages/dashboard/src/__tests__/routes-task-retry-stale-merge-status.test.ts b/packages/dashboard/src/__tests__/routes-task-retry-stale-merge-status.test.ts new file mode 100644 index 0000000000..6bdb76f123 --- /dev/null +++ b/packages/dashboard/src/__tests__/routes-task-retry-stale-merge-status.test.ts @@ -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 { + 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) }); + 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(); + }); +}); diff --git a/packages/dashboard/src/routes.ts b/packages/dashboard/src/routes.ts index 6017605b63..00073a6854 100644 --- a/packages/dashboard/src/routes.ts +++ b/packages/dashboard/src/routes.ts @@ -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), }; } } diff --git a/packages/dashboard/src/routes/register-task-workflow-routes.ts b/packages/dashboard/src/routes/register-task-workflow-routes.ts index a58e705307..e5ece81cff 100644 --- a/packages/dashboard/src/routes/register-task-workflow-routes.ts +++ b/packages/dashboard/src/routes/register-task-workflow-routes.ts @@ -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 }) => Promise; 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. diff --git a/packages/dashboard/src/server.ts b/packages/dashboard/src/server.ts index 51f47e850b..d1aea39b6d 100644 --- a/packages/dashboard/src/server.ts +++ b/packages/dashboard/src/server.ts @@ -354,6 +354,7 @@ export interface ServerOptions { rootDir: string; reconcileInReviewBranchRebind: (opts?: { includeTaskIds?: Set }) => Promise; 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), }, }; } diff --git a/packages/engine/src/__tests__/merge-active-status.test.ts b/packages/engine/src/__tests__/merge-active-status.test.ts new file mode 100644 index 0000000000..bd894b9686 --- /dev/null +++ b/packages/engine/src/__tests__/merge-active-status.test.ts @@ -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); + } + }); +}); diff --git a/packages/engine/src/__tests__/merger-ai-prompts.test.ts b/packages/engine/src/__tests__/merger-ai-prompts.test.ts index d2eb0cf312..55c4180c1c 100644 --- a/packages/engine/src/__tests__/merger-ai-prompts.test.ts +++ b/packages/engine/src/__tests__/merger-ai-prompts.test.ts @@ -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/); + }); +}); diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index ce4cecfdc6..51b37e61f8 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -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, diff --git a/packages/engine/src/merge-active-status.ts b/packages/engine/src/merge-active-status.ts new file mode 100644 index 0000000000..fbc2afac14 --- /dev/null +++ b/packages/engine/src/merge-active-status.ts @@ -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 = 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, + 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; +} diff --git a/packages/engine/src/merger-ai-prompts.ts b/packages/engine/src/merger-ai-prompts.ts index 9e755d80ee..727b418a80 100644 --- a/packages/engine/src/merger-ai-prompts.ts +++ b/packages/engine/src/merger-ai-prompts.ts @@ -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"); } diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index 4e64da9148..952e27ec42 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -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 { 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;