From 3a71237624899aa25c8c7e01c0f2cfcd3b8c4784 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 22 Jun 2026 03:04:21 -0700 Subject: [PATCH] fix(review): address PR #1717 Phase C merge-loop review feedback - merger-ai: resolve+persist concrete landedSha when a sub-repo is recognized already-landed via the Fusion-Task-Id trailer fallback, so finalize no longer drops it and mis-finalizes a fully-landed workspace task as a no-op - project-engine: manual-merge land-lease busy errors reject the resolver without burning mergeRetries; clear stale busy-reenqueue counter on real partial land; persist retry count before arming the backoff timer (fail closed on write error) - cli/dashboard + task: use shared isWorkspaceTask predicate instead of inlining - base-commit-capture: POSIX single-quote shell escaping for integration ref - git-repository: validate workspace.json repos elements are strings - merger-ai: drop dead store param from landOneRepo - tests: assert the 60s backoff cap across cycles; exercise the real runAiMerge merge door; fix non-git-root assertion; re-export real workspace error classes in the merger-ai mock (fixes 24 pre-existing instanceof-undefined failures); remove generic fake-timer smoke test now covered by the live engine assertion Co-Authored-By: Claude Opus 4.8 (1M context) --- .../fix-workspace-phase-c-review-round-2.md | 5 ++ packages/cli/src/commands/dashboard.ts | 6 +- packages/cli/src/commands/task.ts | 7 +- packages/core/src/git-repository.ts | 6 +- .../src/__tests__/executor-workspace.test.ts | 6 +- .../__tests__/merge-error-recovery.test.ts | 15 +++- .../src/__tests__/project-engine.test.ts | 39 +++++++++- .../workspace-merger-idempotency.test.ts | 21 ++---- .../src/__tests__/workspace-merger.test.ts | 22 +++++- packages/engine/src/base-commit-capture.ts | 12 ++-- packages/engine/src/merger-ai.ts | 71 ++++++++++++++++--- packages/engine/src/project-engine.ts | 48 ++++++++++++- 12 files changed, 212 insertions(+), 46 deletions(-) create mode 100644 .changeset/fix-workspace-phase-c-review-round-2.md diff --git a/.changeset/fix-workspace-phase-c-review-round-2.md b/.changeset/fix-workspace-phase-c-review-round-2.md new file mode 100644 index 0000000000..7eba89c430 --- /dev/null +++ b/.changeset/fix-workspace-phase-c-review-round-2.md @@ -0,0 +1,5 @@ +--- +"@runfusion/fusion": patch +--- + +Address Phase C workspace merge-loop review feedback. A sub-repo recognized as already-landed via the `Fusion-Task-Id` trailer fallback (when its `landedSha` persist was lost) now resolves and re-records a concrete `landedSha`, so finalize no longer drops it and mis-reports a fully-landed workspace task as a no-op (`mergeConfirmed:false`). A manual merge that hits sub-repo land-lease contention now surfaces the busy error to the user without consuming the persisted `mergeRetries` quota (matching the auto path's separate busy counter). The partial-land retry persists the incremented retry count before arming the backoff timer — a failed write now fails closed instead of looping without consuming budget — and clears the stale busy-contention counter when a real partial land supersedes transient busy failures. The CLI and dashboard merge doors use the shared `isWorkspaceTask` predicate instead of re-inlining the workspace check, and integration-branch shell interpolation in base-commit capture uses POSIX single-quote escaping. diff --git a/packages/cli/src/commands/dashboard.ts b/packages/cli/src/commands/dashboard.ts index 7986396445..cce2877d43 100644 --- a/packages/cli/src/commands/dashboard.ts +++ b/packages/cli/src/commands/dashboard.ts @@ -17,6 +17,7 @@ import { resolveGlobalDir, DEFAULT_AGENT_HEARTBEAT_INTERVAL_MS, isWorkflowColumnsEnabled, + isWorkspaceTask, resolveColumnFlags, BUILTIN_CODING_WORKFLOW_IR, mergeBuiltInZaiProviderModels, @@ -1312,8 +1313,9 @@ export async function runDashboard(port: number, opts: { paused?: boolean; dev?: // Phase C (user decision). U0's R7 throw is replaced here by routing; the engine // chokepoint + store.mergeTask/aiMergeTask keep throwing as defense-in-depth. const mergeTask = await store.getTask(taskId).catch(() => null); - const isWorkspaceMerge = - !!mergeTask?.workspaceWorktrees && Object.keys(mergeTask.workspaceWorktrees).length > 0; + // FNXC:Workspace 2026-06-22-09:30 (Phase C review B10): use the exported `isWorkspaceTask` + // (the engine/CLI canonical predicate) instead of re-inlining the workspaceWorktrees check. + const isWorkspaceMerge = !!mergeTask && isWorkspaceTask(mergeTask); if (isWorkspaceMerge) { const workspaceResult = await landWorkspaceTask(store, mergeTask!, cwd, { agentStore, diff --git a/packages/cli/src/commands/task.ts b/packages/cli/src/commands/task.ts index b763676d38..3fca040855 100644 --- a/packages/cli/src/commands/task.ts +++ b/packages/cli/src/commands/task.ts @@ -1,4 +1,4 @@ -import { TaskStore, COLUMNS, COLUMN_LABELS, CentralCore, buildAutoPauseClearPatch, buildManualRetryResetPatch, extractIntentSignature, findNearDuplicates, getTaskDuplicateLineage, reconcileDeterministicDuplicate, runDeterministicDuplicateGuard, type Settings, type Column, type ColumnId, type StepStatus, type AgentLogType, type AgentLogEntry, type IntentSignature, type NearDuplicateCandidate, type NearDuplicateMatch, type TaskDependencyMutation } from "@fusion/core"; +import { TaskStore, COLUMNS, COLUMN_LABELS, CentralCore, buildAutoPauseClearPatch, buildManualRetryResetPatch, extractIntentSignature, findNearDuplicates, getTaskDuplicateLineage, isWorkspaceTask, reconcileDeterministicDuplicate, runDeterministicDuplicateGuard, type Settings, type Column, type ColumnId, type StepStatus, type AgentLogType, type AgentLogEntry, type IntentSignature, type NearDuplicateCandidate, type NearDuplicateMatch, type TaskDependencyMutation } from "@fusion/core"; import { runAiMerge, landWorkspaceTask } from "@fusion/engine"; import { createInterface } from "node:readline/promises"; import type { PlanningQuestion, PlanningSummary } from "@fusion/core"; @@ -858,8 +858,9 @@ export async function runTaskMerge(id: string, projectName?: string) { // Phase C (user decision). U0's R7 throw is replaced here by routing; the // engine chokepoint + store.mergeTask/aiMergeTask keep throwing. const mergeTaskRecord = await store.getTask(id).catch(() => null); - const isWorkspaceMerge = - !!mergeTaskRecord?.workspaceWorktrees && Object.keys(mergeTaskRecord.workspaceWorktrees).length > 0; + // FNXC:Workspace 2026-06-22-09:30 (Phase C review B10): use the exported `isWorkspaceTask` + // (the engine/CLI canonical predicate) instead of re-inlining the workspaceWorktrees check. + const isWorkspaceMerge = !!mergeTaskRecord && isWorkspaceTask(mergeTaskRecord); if (isWorkspaceMerge) { const workspaceResult = await landWorkspaceTask(store, mergeTaskRecord!, projectPath, { onAgentText: (delta) => process.stdout.write(delta), diff --git a/packages/core/src/git-repository.ts b/packages/core/src/git-repository.ts index 974c5d12a6..148179a163 100644 --- a/packages/core/src/git-repository.ts +++ b/packages/core/src/git-repository.ts @@ -139,11 +139,15 @@ export async function loadWorkspaceConfig(rootDir: string): Promise typeof r === "string") ) { return parsed as WorkspaceConfig; } diff --git a/packages/engine/src/__tests__/executor-workspace.test.ts b/packages/engine/src/__tests__/executor-workspace.test.ts index 330915e966..685fd85984 100644 --- a/packages/engine/src/__tests__/executor-workspace.test.ts +++ b/packages/engine/src/__tests__/executor-workspace.test.ts @@ -48,8 +48,10 @@ describeIfGit("workspace fixture", () => { it("builds a non-git root with two real git sub-repos and a resolvable workspace config", async () => { fx = await createWorkspaceFixture(); - // Root is NOT a git repo. - expect(() => fx.git("..", "git rev-parse --git-dir")).toThrow(); + // Root itself is NOT a git repo (`.` resolves to rootDir, not its parent — `..` would + // test tmpdir, which proves nothing about the invariant). git rev-parse --git-dir throws + // (exits non-zero) only when run outside any git repo. + expect(() => fx.git(".", "git rev-parse --git-dir")).toThrow(); // Each sub-repo is a real git repo with a commit on main. expect(fx.git("repo-a", "git rev-parse --abbrev-ref HEAD")).toBe("main"); expect(fx.git("repo-b", "git rev-list --count HEAD")).toBe("1"); diff --git a/packages/engine/src/__tests__/merge-error-recovery.test.ts b/packages/engine/src/__tests__/merge-error-recovery.test.ts index 57465cf121..32e8fae4ac 100644 --- a/packages/engine/src/__tests__/merge-error-recovery.test.ts +++ b/packages/engine/src/__tests__/merge-error-recovery.test.ts @@ -28,9 +28,18 @@ vi.mock("../merger.js", () => ({ VerificationError: testState.VerificationError, })); -vi.mock("../merger-ai.js", () => ({ - runAiMerge: testState.runAiMerge, -})); +// FNXC:Workspace 2026-06-22-09:30 (Phase C review fix): the dispatch's error handler does +// `err instanceof WorkspaceRepoLandBusyError` / `WorkspacePartialLandError` on EVERY merge error +// (these classes are imported from ./merger-ai.js). A bare replacement mock left them undefined, +// so `instanceof undefined` threw on every recovery path (24 pre-existing red tests). Re-export the +// REAL error classes via importOriginal so the instanceof guards evaluate; only runAiMerge is faked. +vi.mock("../merger-ai.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + runAiMerge: testState.runAiMerge, + }; +}); vi.mock("../runtimes/in-process-runtime.js", () => ({ InProcessRuntime: vi.fn().mockImplementation(function () { diff --git a/packages/engine/src/__tests__/project-engine.test.ts b/packages/engine/src/__tests__/project-engine.test.ts index d88bdd9483..e43fea6ee3 100644 --- a/packages/engine/src/__tests__/project-engine.test.ts +++ b/packages/engine/src/__tests__/project-engine.test.ts @@ -1546,9 +1546,42 @@ describe("ProjectEngine workspace merge dispatch hardening (Phase C review)", () .some((c) => c[0] === "FN-WSH" && typeof c[1]?.mergeRetries === "number"); expect(burnedRetries).toBe(false); - // Drive several busy re-enqueues; the backoff must stay capped at 60s. - enqueueSpy.mockClear(); - await vi.advanceTimersByTimeAsync(60_000); // first backoff (5s) fires → re-enqueue + /* + FNXC:Workspace 2026-06-22-09:30 (Phase C review B5b — assert the 60s CAP, not just the first retry): + Advancing 60s once only proves the first 5s timer fired; an UNcapped exponential + (5s,10s,20s,40s,80s,160s,…) would still pass that. Capture EVERY scheduled busy backoff delay + across enough cycles to pass the cap point (busyCount=4 → 5000*2^4 = 80_000ms, clamped to 60_000) + and assert no delay exceeds 60_000 AND the cap is actually reached. Each advance fires the pending + timer → re-enqueue → landWorkspaceTask rejects busy again → next backoff is scheduled. + */ + const scheduledBusyDelays: number[] = []; + // `globalThis.setTimeout` is already the fake-timer impl here (vi.useFakeTimers above). + // Wrap it to record the requested delay, then delegate to the SAME fake timer so the + // fake clock still drives the callback — no real-timer leakage. + const fakeSetTimeout = globalThis.setTimeout; + const setTimeoutSpy = vi + .spyOn(globalThis, "setTimeout") + .mockImplementation(((cb: (...a: unknown[]) => void, ms?: number, ...rest: unknown[]) => { + if (typeof ms === "number") scheduledBusyDelays.push(ms); + return (fakeSetTimeout as (...a: unknown[]) => unknown)(cb, ms, ...rest); + }) as typeof setTimeout); + + try { + // Drive enough busy cycles to climb past the cap point (busyCount 0..5 = 6 cycles). + for (let i = 0; i < 6; i++) { + await vi.advanceTimersByTimeAsync(60_000); + } + } finally { + setTimeoutSpy.mockRestore(); + } + + // The exponential climbed (more than one distinct delay) AND every delay is capped at 60s. + expect(scheduledBusyDelays.length).toBeGreaterThanOrEqual(5); + expect(Math.max(...scheduledBusyDelays)).toBe(60_000); + expect(scheduledBusyDelays.every((d) => d <= 60_000)).toBe(true); + // The cap was actually exercised: at least one delay sits at the 60s ceiling. + expect(scheduledBusyDelays).toContain(60_000); + // Each fired backoff re-enqueued the merge (the contention retry loop is live). expect(enqueueSpy).toHaveBeenCalledWith("FN-WSH"); await engine.stop(); diff --git a/packages/engine/src/__tests__/workspace-merger-idempotency.test.ts b/packages/engine/src/__tests__/workspace-merger-idempotency.test.ts index fce5724b44..c53e10ffb3 100644 --- a/packages/engine/src/__tests__/workspace-merger-idempotency.test.ts +++ b/packages/engine/src/__tests__/workspace-merger-idempotency.test.ts @@ -20,7 +20,7 @@ Coverage (FN-5893 surfaces): - retry/park: a partial-land failure consumes one mergeRetry; after MAX it parks (shouldRetryWorkspacePartialLand boundary, fake timers). */ -import { afterEach, beforeEach, afterAll, describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { EventEmitter } from "node:events"; import { execSync } from "node:child_process"; import { writeFileSync } from "node:fs"; @@ -426,10 +426,11 @@ describeIfGit("landWorkspaceTask — DB-failure resilience (Phase C review A1/A4 }); }); -describe("workspace partial-land retry/park decision (engine seam, fake timers)", () => { - beforeEach(() => vi.useFakeTimers()); - afterAll(() => vi.useRealTimers()); - +// FNXC:Workspace 2026-06-22-09:30 (Phase C review nit): the former generic "fake-timer backoff +// schedule does not spin real retries" smoke test only proved Vitest's fake timers work — it never +// drove the production retry seam. The real backoff-cap invariant is now asserted against the live +// ProjectEngine in project-engine.test.ts ("B4/B5: busy contention re-enqueues with capped backoff"). +describe("workspace partial-land retry/park decision (engine seam)", () => { it("consumes a mergeRetry up to MAX, then parks (shouldRetryWorkspacePartialLand)", () => { // Default MAX = 3. currentRetries + 1 < MAX gates retry. expect(shouldRetryWorkspacePartialLand(0, {})).toMatchObject({ @@ -452,14 +453,4 @@ describe("workspace partial-land retry/park decision (engine seam, fake timers)" expect(shouldRetryWorkspacePartialLand(3, { maxAutoMergeRetries: 5 }).shouldRetry).toBe(true); expect(shouldRetryWorkspacePartialLand(4, { maxAutoMergeRetries: 5 }).shouldRetry).toBe(false); }); - - it("fake-timer backoff schedule does not spin real retries", () => { - // The dispatch schedules internalEnqueueMerge via setTimeout(5000 * 2^retries). - // Assert a scheduled callback exists and only fires when advanced — no real wait. - const fired: number[] = []; - setTimeout(() => fired.push(1), 5000); - expect(fired).toHaveLength(0); - vi.advanceTimersByTime(5000); - expect(fired).toHaveLength(1); - }); }); diff --git a/packages/engine/src/__tests__/workspace-merger.test.ts b/packages/engine/src/__tests__/workspace-merger.test.ts index fe15703435..8973c66c70 100644 --- a/packages/engine/src/__tests__/workspace-merger.test.ts +++ b/packages/engine/src/__tests__/workspace-merger.test.ts @@ -30,7 +30,7 @@ import { writeFileSync } from "node:fs"; import path from "node:path"; import type { Task, TaskStore } from "@fusion/core"; import { assertNotWorkspaceTaskMerge } from "@fusion/core"; -import { landWorkspaceTask } from "../merger-ai.js"; +import { landWorkspaceTask, runAiMerge } from "../merger-ai.js"; import { createWorkspaceFixture, hasGit, type WorkspaceFixture } from "./_workspace-fixture.js"; const describeIfGit = hasGit ? describe : describe.skip; @@ -292,4 +292,24 @@ describe("workspace merge defense-in-depth (non-routed doors keep throwing)", () const task = { id: TASK_ID } as unknown as Task; expect(() => assertNotWorkspaceTaskMerge(task)).not.toThrow(); }); + + /* + FNXC:Workspace 2026-06-22-09:30 (Phase C review B11 — exercise the REAL merge door, not only the helper): + Calling `assertNotWorkspaceTaskMerge` directly proves the helper, but a regression where `runAiMerge` + (the sole engine merge door, R7 chokepoint) stopped invoking it would slip through. Drive the actual + door with a minimal store whose `getTask` returns the workspace task: `runAiMerge` reads the task and + calls the guard BEFORE any git work, so it rejects with WorkspaceTaskMergeError without a real repo. + */ + it("runAiMerge (engine merge door) rejects a workspace task with WorkspaceTaskMergeError", async () => { + const workspaceTask = { + id: TASK_ID, + workspaceWorktrees: { "repo-a": { worktreePath: "/x/repo-a", branch: BRANCH } }, + } as unknown as Task; + const store = { + getTask: vi.fn(async () => workspaceTask), + } as unknown as TaskStore; + await expect(runAiMerge(store, "/x", TASK_ID)).rejects.toMatchObject({ + name: "WorkspaceTaskMergeError", + }); + }); }); diff --git a/packages/engine/src/base-commit-capture.ts b/packages/engine/src/base-commit-capture.ts index 4d9e778774..aea3bfbedc 100644 --- a/packages/engine/src/base-commit-capture.ts +++ b/packages/engine/src/base-commit-capture.ts @@ -39,10 +39,14 @@ export async function resolveCapturedBaseCommitSha( integrationBranch: string = "main", ): Promise { const branch = integrationBranch.trim() || "main"; - // Shell-quote defensively; integration branch names are normalized upstream - // but may carry slashes (e.g. "release/2026-06") that are valid in refs. - const localRef = JSON.stringify(branch); - const originRef = JSON.stringify(`origin/${branch}`); + // FNXC:Workspace 2026-06-22-09:30 (Phase C review nit — proper POSIX single-quote shell escaping): + // Integration branch names are normalized upstream but may carry slashes (e.g. "release/2026-06") + // and, in principle, other ref-legal chars. JSON.stringify uses DOUBLE quotes, under which `$`, + // backticks, and `!` still undergo shell expansion. Single-quote and escape embedded single quotes + // ('\'') so the value is passed verbatim to git with no shell interpretation. + const shellQuote = (s: string): string => `'${s.replace(/'/g, "'\\''")}'`; + const localRef = shellQuote(branch); + const originRef = shellQuote(`origin/${branch}`); let baseCommitSha: string | undefined; try { const { stdout } = await execAsync( diff --git a/packages/engine/src/merger-ai.ts b/packages/engine/src/merger-ai.ts index a9f66a45c8..a9a407e447 100644 --- a/packages/engine/src/merger-ai.ts +++ b/packages/engine/src/merger-ai.ts @@ -1023,8 +1023,11 @@ export type LandOneRepoResult = * repo-scoped clean room, retrying on concurrent advance. No remote push. See * the FNXC note above for the extraction contract. */ +// FNXC:Workspace 2026-06-22-09:30 (Phase C review B12): `landOneRepo` takes its store access +// exclusively through the `ctx` callbacks (log/setStatus/audit) and pre-built agents — it never +// touches a TaskStore directly. The former leading `store` param was dead and misleading at the +// call sites (they looked like they forwarded a store the function ignored), so it was dropped. export async function landOneRepo( - store: TaskStore, repoRootDir: string, branch: string, integrationBranch: string, @@ -1273,7 +1276,7 @@ export async function runAiMerge( // once; the task-global finalization below (empty no-op / no-commits demote / // finalizeMerged) is unchanged byte-for-byte — only the inline clean-room land // loop moved into `landOneRepo` so `landWorkspaceTask` can reuse it per sub-repo. - const landResult = await landOneRepo(store, projectRootDir, branch, integrationBranch, { + const landResult = await landOneRepo(projectRootDir, branch, integrationBranch, { taskId, settings, audit, log, setStatus, maxPasses, mergeAgent, reviewAgent, stashResolveAgent, includeTaskId, trailers, taskTitle, signal: options.signal, @@ -1561,11 +1564,28 @@ export async function landWorkspaceTask( // ancestor of (or equals) its CURRENT integration tip is already landed — SKIP // it so a retry never re-advances the ref. This makes a re-run after a partial // land idempotent for the already-landed repos. - if (await isRepoLanded(repoRootDir, integrationBranch, entry.landedSha, taskId, entry.branch)) { - await log(`AI merge (workspace): sub-repo ${repoRel} already landed (${short(entry.landedSha!)} ⊑ ${integrationBranch}) — skipping`); + /* + FNXC:Workspace 2026-06-22-09:30 (Phase C review A1 — concrete landedSha on the skip path): + Resolve a CONCRETE landed sha (recorded landedSha OR the trailer-fallback squash sha) rather + than trusting `entry.landedSha`, which is `undefined` when the land's persist was lost and only + the A1 trailer fallback recognises the repo. If we recovered the sha via the fallback, REPAIR + the persisted entry so a later run (and `finalizeWorkspaceTask`) sees a present landedSha. A + repair-persist failure is non-fatal: we still carry the concrete sha in-memory for this run's + finalize, and the trailer fallback will re-recover it next time. + */ + const recoveredLandedSha = await resolveLandedShaIfLanded( + repoRootDir, integrationBranch, entry.landedSha, taskId, entry.branch, + ); + if (recoveredLandedSha) { + if (!entry.landedSha) { + await persistRepoLandedSha(store, taskId, repoRel, recoveredLandedSha).catch(async (persistErr: unknown) => { + await log(`AI merge (workspace): sub-repo ${repoRel} re-recorded landedSha (${short(recoveredLandedSha)}) persist failed (non-fatal, trailer fallback will re-recover): ${getErrorMessage(persistErr)}`); + }); + } + await log(`AI merge (workspace): sub-repo ${repoRel} already landed (${short(recoveredLandedSha)} ⊑ ${integrationBranch}) — skipping`); repos.push({ repo: repoRel, repoRootDir, integrationBranch, branch: entry.branch, - status: "landed", landedSha: entry.landedSha, alreadyLanded: true, + status: "landed", landedSha: recoveredLandedSha, alreadyLanded: true, }); continue; } @@ -1601,7 +1621,7 @@ export async function landWorkspaceTask( }); try { - const landResult = await landOneRepo(store, repoRootDir, entry.branch, integrationBranch, { + const landResult = await landOneRepo(repoRootDir, entry.branch, integrationBranch, { taskId, settings, audit, log, setStatus, maxPasses, mergeAgent, reviewAgent, stashResolveAgent, includeTaskId, trailers, taskTitle, signal: options.signal, @@ -1723,9 +1743,36 @@ export async function isRepoLanded( taskId?: string, branch?: string, ): Promise { + return ( + (await resolveLandedShaIfLanded(repoRootDir, integrationBranch, landedSha, taskId, branch)) !== + undefined + ); +} + +/** + * FNXC:Workspace 2026-06-22-09:30 (Phase C review A1 — concrete landedSha on trailer fallback): + * The shared core of {@link isRepoLanded}: returns a CONCRETE landed sha when the sub-repo is + * already landed, else `undefined`. When the recorded `landedSha` survives it is returned as-is; + * when the A1 trailer fallback matches (the persist was lost so no `landedSha` is recorded) the + * concrete squash sha is read off the integration ref via the same bounded trailer scan. + * + * Why this matters (review A1 / finalize misfinalise): the `landWorkspaceTask` skip path and + * `finalizeWorkspaceTask` both key off a present `landedSha`. A trailer-fallback match with a + * `undefined` recorded sha would be dropped by the finalize filter, finalizing an already-landed + * task as a no-op (`mergeConfirmed:false`, empty `workspaceLandedShas`) — the exact dashboard + * `merged:false` contradiction Phase C set out to eliminate. Resolving the concrete sha here lets + * the skip path persist+propagate it so the repo is correctly counted as landed. + */ +async function resolveLandedShaIfLanded( + repoRootDir: string, + integrationBranch: string, + landedSha: string | undefined, + taskId?: string, + branch?: string, +): Promise { const intRef = `refs/heads/${integrationBranch}`; if (!(await gitOk(["rev-parse", "--verify", intRef], repoRootDir))) { - return false; + return undefined; } // Primary: recorded landedSha is an ancestor of (or equals) the integration tip. // `merge-base --is-ancestor X Y` exits 0 iff X is an ancestor of (or equal to) Y. @@ -1733,12 +1780,13 @@ export async function isRepoLanded( landedSha && (await gitOk(["merge-base", "--is-ancestor", landedSha, intRef], repoRootDir)) ) { - return true; + return landedSha; } // A1 fallback: even without a recorded landedSha, the repo is already landed if the // integration ref carries a commit with this task's Fusion-Task-Id trailer (the squash // we lost the persist for). Bound the scan to commits gained since the branch's land base - // so a stale historical trailer of the same id cannot false-positive. + // so a stale historical trailer of the same id cannot false-positive. Return the MOST RECENT + // matching commit sha (the squash) so callers can persist a concrete landedSha. if (taskId) { const branchRef = branch ? `refs/heads/${branch}` : undefined; let range = intRef; @@ -1751,9 +1799,10 @@ export async function isRepoLanded( ["log", "--format=%H", `--grep=${trailer}`, "--fixed-strings", range], repoRootDir, ); - if (found && found.trim().length > 0) return true; + const firstSha = found?.split(/\r?\n/).map((s) => s.trim()).find((s) => s.length > 0); + if (firstSha) return firstSha; } - return false; + return undefined; } /** diff --git a/packages/engine/src/project-engine.ts b/packages/engine/src/project-engine.ts index 5152a36fc1..2a5c072950 100644 --- a/packages/engine/src/project-engine.ts +++ b/packages/engine/src/project-engine.ts @@ -2495,6 +2495,23 @@ export class ProjectEngine { retries on busy-errors before either makes a real land attempt, then parking a never-failed task. Detect via `instanceof` now that both are exported classes (B7). */ + /* + FNXC:Workspace 2026-06-22-09:30 (Phase C review B7b — manual-merge busy must NOT burn mergeRetries): + A manual merge (hasManualResolver) that hits sub-repo land contention is the SAME transient + lease contention as the auto path, NOT a real land failure. Without this branch it falls + through to the generic handler below, which increments the persisted `mergeRetries` quota — + so a user mashing the merge button during contention could exhaust retries before any real + land attempt. Reject the resolver so the busy error surfaces to the user (they can retry), + WITHOUT consuming a mergeRetry. No re-enqueue: manual merges are user-driven, not engine-timed. + */ + if (err instanceof WorkspaceRepoLandBusyError && hasManualResolver) { + await store + .logEntry(taskId, `Workspace sub-repo land busy (contention): ${errorMsg}`, "WorkspaceRepoLandBusy") + .catch(() => undefined); + this.rejectMergeResolvers(taskId, err instanceof Error ? err : new Error(errorMsg)); + continue; + } + if (err instanceof WorkspaceRepoLandBusyError && !hasManualResolver) { const busyCount = this.workspaceBusyReenqueues.get(taskId) ?? 0; await store @@ -2537,6 +2554,15 @@ export class ProjectEngine { // (B6). Detect via `instanceof` (B7). Manual merges fall through to // rejectMergeResolvers at the hasManualResolver early-return below. if (err instanceof WorkspacePartialLandError && !hasManualResolver) { + /* + FNXC:Workspace 2026-06-22-09:30 (Phase C review B8 — clear stale busy quota on real outcome): + Reaching a REAL partial land means the prior transient busy contention is over. The + `workspaceBusyReenqueues` counter is otherwise only cleared on success or busy-cap + exhaustion, so a few transient busy failures followed by a real partial land would leave + a stale count — later UNRELATED contention would then resume from it and park the task + early. Clear it here so each fresh contention episode gets the full busy budget. + */ + this.workspaceBusyReenqueues.delete(taskId); const wsSettings = await store.getSettings().catch(() => null); const wsTask = await store.getTask(taskId).catch(() => null); /* @@ -2574,7 +2600,27 @@ export class ProjectEngine { .logEntry(taskId, `Workspace partial land: ${errorMsg}`, "WorkspacePartialLand") .catch(() => undefined); if (decision.shouldRetry) { - await store.updateTask(taskId, { mergeRetries: decision.nextRetryCount, status: null }).catch(() => undefined); + /* + FNXC:Workspace 2026-06-22-09:30 (Phase C review B9 — persist retry count BEFORE arming the timer): + The retry-count write must succeed before we schedule the retry. A swallowed + `.catch(() => undefined)` here armed the timer even when the `mergeRetries` increment + never landed — so the next attempt re-read the OLD `mergeRetries` and could loop without + consuming budget, defeating the fail-closed DB-outage guard above. FAIL CLOSED: if the + write throws, park as failed (best-effort) and do NOT schedule a retry storm against a + non-responsive DB; the cooldown sweep re-evaluates once the DB recovers. + */ + try { + await store.updateTask(taskId, { mergeRetries: decision.nextRetryCount, status: null }); + } catch (persistErr: unknown) { + const pmsg = persistErr instanceof Error ? persistErr.message : String(persistErr); + runtimeLog.error( + `Auto-merge: ${taskId} workspace partial land retry NOT scheduled — mergeRetries could not be persisted (DB outage?), failing closed instead of a retry storm: ${pmsg}`, + ); + await store + .updateTask(taskId, { status: "failed", error: errorMsg }) + .catch(() => undefined); + continue; + } // Capped exponential backoff (B5): cap at 60s so a tuned maxAutoMergeRetries doesn't // push the delay toward ~85 minutes at the ceiling. const delayMs = Math.min(5000 * Math.pow(2, wsRetries), 60_000);