From e00bc0235b6c07b4b23fa2bf57e392d126c3fe22 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 3 Jun 2026 13:08:06 -0700 Subject: [PATCH] Address PR review feedback (#1356) - Add behavior-level tests for the shared merge-enqueue funnel (enqueueEligibleInReviewTasks) with a Surface Enumeration of all in-review entry surfaces, per review - Seed real stale in-review fixtures in the FN-5147 no-mutation regression block so sweeps enumerate candidates and the assertions are non-vacuous - Keep per-task auto-merge gating uniform across reclaim/contamination candidate columns: the suggested in-review-only scoping broke the FN-5704 regression contract (reclaim short-circuits when autoMerge is off); documented the tension in code comments and the learning doc - Drop hardcoded commit hash from the learning doc --- ...merge-override-ignored-by-trigger-gates.md | 3 +- .../src/__tests__/project-engine.test.ts | 68 ++++++++ .../engine/src/__tests__/self-healing.test.ts | 160 +++++++++++++++++- packages/engine/src/self-healing.ts | 10 ++ 4 files changed, 237 insertions(+), 4 deletions(-) diff --git a/docs/solutions/logic-errors/per-task-auto-merge-override-ignored-by-trigger-gates.md b/docs/solutions/logic-errors/per-task-auto-merge-override-ignored-by-trigger-gates.md index 926d0f86ba..4aeb77af47 100644 --- a/docs/solutions/logic-errors/per-task-auto-merge-override-ignored-by-trigger-gates.md +++ b/docs/solutions/logic-errors/per-task-auto-merge-override-ignored-by-trigger-gates.md @@ -101,12 +101,13 @@ When adding a per-entity override to a behavior that's gated on a global setting - **Grep every gate on the global setting** before declaring the override wired: here `settings.autoMerge` appeared at 1 enqueue gate, 19 sweep guards, and 6 hydration sites — all needed updating. A search for the global key, not just the new override field, surfaces the dead-flag sites. - **Prefer additive gating over effective-value resolution for *processing* gates.** Resolution collapses three states (global-on/off × per-task true/false/unset) into one boolean and can starve a needed downstream branch (the manual-required parking path). Gate on "should this be processed at all," resolve the actual behavior later. +- **Check existing regression contracts before re-scoping a gate.** Review of the fix PR suggested exempting `todo`/`in-progress` candidates (execution-stage repair) from the auto-merge gate — but the repo's FN-5704 regression test ("short-circuits reclaim when autoMerge is false") deliberately keeps execution-stage reclaim inert in manual-review projects. Per-task gating applied uniformly preserves that contract while enabling overrides; exempting execution-stage recovery would be a separate, deliberate behavior change. - **Watch slim projections:** per-row predicates require the override column in the SELECT clause, or they silently read `undefined`. - **Test matrix must cross global × per-task.** The fix shipped red-first unit tests for the predicate (`packages/core/src/__tests__/task-merge.test.ts`), the gate including the shared-group exemption (`packages/engine/src/__tests__/project-engine.test.ts`), and a self-healing test proving an **override task is processed while an override-less sibling stays skipped** (`packages/engine/src/__tests__/self-healing.test.ts`) — the latter is the canonical shape: two tasks differing only in `autoMerge` under global-OFF, asserting divergent outcomes. ## Related Issues -- Runfusion/Fusion#1356 — the fix PR (commit `ad468813d`) +- Runfusion/Fusion#1356 — the fix PR - Runfusion/Fusion#1150, Runfusion/Fusion#1152, Runfusion/Fusion#1153 — the per-task auto-merge feature trio (data model + resolver, engine gating, dashboard control); #1152's gating claim is the gap this bug exposed - Runfusion/Fusion#753 (FN-5147), Runfusion/Fusion#690 (FN-5052) — prior global `autoMerge:false` stall/lifecycle handling that the sweeps' guards came from - AGENTS.md → "`autoMerge: false` callout (FN-5147)" — standing lifecycle rule this fix extends to per-task granularity diff --git a/packages/engine/src/__tests__/project-engine.test.ts b/packages/engine/src/__tests__/project-engine.test.ts index 4c46d088ac..849875483a 100644 --- a/packages/engine/src/__tests__/project-engine.test.ts +++ b/packages/engine/src/__tests__/project-engine.test.ts @@ -2696,3 +2696,71 @@ describe("allowInReviewMergeProcessing per-task autoMerge override", () => { )).toBe(true); }); }); + +// ## Surface Enumeration +// +// Known in-review merge entry surfaces in ProjectEngine, and how each enforces +// the per-task `autoMerge` override invariant (a task with `autoMerge:true` must +// still be enqueued for merge even when the global `autoMerge` setting is off): +// +// 1. Startup merge sweep (project-engine.ts ~:2857) ─┐ +// 2. Periodic merge retry sweep (project-engine.ts ~:2916) ─┼─ all call +// 3. Resume-after-unpause sweep (project-engine.ts ~:2977) ─┘ enqueueEligibleInReviewTasks(...) +// 4. task:moved fast path (project-engine.ts ~:1506) ─── inline allowInReviewMergeProcessing(...) +// +// Surfaces 1–3 funnel through `enqueueEligibleInReviewTasks`, whose filter is +// `!t.paused && canMergeTask(t) && allowInReviewMergeProcessing(t, settings)`. +// The behavior tests below exercise that shared funnel directly on a real engine +// instance (with `internalEnqueueMerge` stubbed), so a regression in any of the +// three sweep wrappers (wireAutoMerge / startupMergeSweep / scheduleMergeRetry / +// resumeAfterUnpauseAndSweepInReview) that still routes through the funnel is +// caught. Surface 4 (the task:moved fast path) shares the same +// `allowInReviewMergeProcessing` gate, which is covered by the direct helper +// tests above. + +describe("enqueueEligibleInReviewTasks honors per-task autoMerge override (shared sweep funnel)", () => { + const inReview = (id: string, overrides: Partial = {}): Task => + ({ + id, + column: "in-review", + paused: false, + mergeRetries: 0, + status: null, + ...overrides, + }) as unknown as Task; + + const setup = () => { + const engine = createEngine() as any; + const enqueueSpy = vi + .spyOn(engine, "internalEnqueueMerge") + .mockImplementation(() => true); + const run = (tasks: Task[], settings: { autoMerge: boolean }): number => + engine.enqueueEligibleInReviewTasks(tasks, settings) as number; + return { engine, enqueueSpy, run }; + }; + + it("enqueues an in-review task with autoMerge:true even when the global setting is off", () => { + const { enqueueSpy, run } = setup(); + const count = run([inReview("FN-override", { autoMerge: true })], { autoMerge: false }); + expect(count).toBe(1); + expect(enqueueSpy).toHaveBeenCalledWith("FN-override"); + }); + + it("does not enqueue a sibling task without an override in the same sweep when the global setting is off", () => { + const { enqueueSpy, run } = setup(); + const count = run( + [inReview("FN-override", { autoMerge: true }), inReview("FN-plain")], + { autoMerge: false }, + ); + expect(count).toBe(1); + expect(enqueueSpy).toHaveBeenCalledWith("FN-override"); + expect(enqueueSpy).not.toHaveBeenCalledWith("FN-plain"); + }); + + it("still enqueues a task with autoMerge:false when the global setting is on (parked manual-required downstream)", () => { + const { enqueueSpy, run } = setup(); + const count = run([inReview("FN-explicit-false", { autoMerge: false })], { autoMerge: true }); + expect(count).toBe(1); + expect(enqueueSpy).toHaveBeenCalledWith("FN-explicit-false"); + }); +}); diff --git a/packages/engine/src/__tests__/self-healing.test.ts b/packages/engine/src/__tests__/self-healing.test.ts index 87db2fbb2f..1b37da510f 100644 --- a/packages/engine/src/__tests__/self-healing.test.ts +++ b/packages/engine/src/__tests__/self-healing.test.ts @@ -97,7 +97,7 @@ vi.mock("../merger.js", () => ({ classifyOwnedLandedEvidence: vi.fn(), })); -import { SelfHealingManager, isBranchAheadOfBase } from "../self-healing.js"; +import { SelfHealingManager, isBranchAheadOfBase, MAX_AUTO_MERGE_RETRIES } from "../self-healing.js"; import type { TaskStore, Settings, Task, AgentStore, Agent, NotificationProvider } from "@fusion/core"; import { EventEmitter } from "node:events"; import { execSync } from "node:child_process"; @@ -8212,6 +8212,153 @@ describe("autoMerge gating for mutating in-review sweeps (FN-5147)", () => { taskStuckTimeoutMs: 1_000, maxPostReviewFixes: 1, }); + + // Seed real, stale in-review sweep candidates with NO per-task autoMerge + // override. Each fixture matches a distinct covered sweep's candidate shape + // and would be mutated if the per-task gate (allowsAutoMergeProcessing) were + // ignored. Because the global setting is autoMerge:false and none of these + // carry autoMerge:true, every sweep must enumerate them and skip them solely + // due to the gate — which is the regression under test. The gate is the + // first/early filter in each sweep, so candidates are dropped before any + // store.getTask / git helper is reached. + const stale = new Date(Date.now() - 600_000).toISOString(); + const seededInReviewCandidates = [ + // recoverStaleIncompleteReviewTasks + recoverGhostReviewTasks: + // idle in-review with incomplete steps, stale. + { + id: "FN-GATE-INCOMPLETE", + column: "in-review", + paused: false, + steps: [{ status: "pending" }], + log: [], + updatedAt: stale, + columnMovedAt: stale, + }, + // recoverInterruptedMergingTasks: stale `merging` status. + { + id: "FN-GATE-MERGING", + column: "in-review", + paused: false, + status: "merging", + steps: [], + log: [], + updatedAt: stale, + columnMovedAt: stale, + }, + // recoverMergedReviewTasks + recoverGhostReviewTasks(skip merge-confirmed): + // mergeConfirmed:true stuck in in-review. + { + id: "FN-GATE-MERGED", + column: "in-review", + paused: false, + steps: [], + log: [], + mergeDetails: { mergeConfirmed: true }, + updatedAt: stale, + columnMovedAt: stale, + }, + // recoverStuckMergeDeadlocks + recoverAlreadyMergedReviewTasks + + // recoverOrphanOnlyScopeViolations: failed in-review, retries exhausted, + // worktree present. + { + id: "FN-GATE-FAILED", + column: "in-review", + paused: false, + status: "failed", + steps: [], + log: [], + mergeRetries: MAX_AUTO_MERGE_RETRIES, + worktree: "/tmp/test-project/.worktrees/FN-GATE-FAILED", + branch: "fn/FN-GATE-FAILED", + updatedAt: stale, + columnMovedAt: stale, + }, + // recoverReviewTasksWithFailedPreMergeSteps: idle in-review whose merge is + // blocked specifically by a failed pre-merge workflow step, worktree set. + { + id: "FN-GATE-PREMERGE", + column: "in-review", + paused: false, + steps: [], + log: [], + worktree: "/tmp/test-project/.worktrees/FN-GATE-PREMERGE", + workflowStepResults: [{ phase: "pre-merge", status: "failed" }], + updatedAt: stale, + columnMovedAt: stale, + }, + // recoverMissingWorktreeReviewFailures: failed by missing-worktree session + // start, with step progress. + { + id: "FN-GATE-MISSINGWT", + column: "in-review", + paused: false, + status: "failed", + error: "Refusing to start coding agent in missing worktree: /tmp/gone", + steps: [{ status: "done" }], + log: [], + updatedAt: stale, + columnMovedAt: stale, + }, + // recoverPartialProgressNoTaskDoneFailures: failed without fn_task_done, + // partial step progress, not work-complete, retries available. + { + id: "FN-GATE-NOTASKDONE", + column: "in-review", + paused: false, + status: "failed", + error: "Agent finished without calling fn_task_done", + steps: [{ status: "done" }, { status: "pending" }], + log: [], + updatedAt: stale, + columnMovedAt: stale, + }, + // recoverForeignOnlyContaminatedInReviewTasks: in-review with branch + + // worktree, not merge-confirmed. + { + id: "FN-GATE-FOREIGN", + column: "in-review", + paused: false, + branch: "fn/FN-GATE-FOREIGN", + worktree: "/tmp/test-project/.worktrees/FN-GATE-FOREIGN", + steps: [], + log: [], + updatedAt: stale, + columnMovedAt: stale, + }, + // recoverCompletionHandoffLimbo: idle in-review with no status/mergeDetails/ + // review, an aged "Task marked done by agent" log marker, no merge blocker. + { + id: "FN-GATE-HANDOFF", + column: "in-review", + paused: false, + steps: [], + log: [{ action: "Task marked done by agent", timestamp: stale }], + updatedAt: stale, + columnMovedAt: stale, + }, + // reclaimSelfOwnedBranchConflicts: in-review branch-conflict-unrecoverable. + // (No worktree, so even absent the gate it is skipped before any git call; + // the gate is what the assertions verify.) + { + id: "FN-GATE-RECLAIM", + column: "in-review", + paused: true, + pausedReason: "branch-conflict-unrecoverable", + branch: "fn/FN-GATE-RECLAIM", + steps: [], + log: [], + updatedAt: stale, + columnMovedAt: stale, + }, + ] as unknown as Task[]; + + // Resolve fixtures only for the in-review column the sweeps enumerate; other + // columns (todo / in-progress / triage) stay empty so the non-auto-merge- + // gated branches of reclaim/foreign-only sweeps don't reach git helpers. + (store.listTasks as ReturnType).mockImplementation( + async (opts?: { column?: string }) => + opts?.column === "in-review" ? seededInReviewCandidates : [], + ); }); afterEach(() => { @@ -8237,8 +8384,11 @@ describe("autoMerge gating for mutating in-review sweeps (FN-5147)", () => { } const result = await (manager as any)[methodName](); expect(result).toBe(0); - // The sweep may list tasks to discover per-task autoMerge overrides, - // but must not mutate anything without one (respects PR-based review flow). + // Enumeration must have happened: the sweep listed real, stale in-review + // candidates seeded above. Mutations are skipped solely because of the + // per-task auto-merge gate (respects PR-based review flow) — so these + // assertions are non-vacuous. + expect(store.listTasks).toHaveBeenCalled(); expect(store.moveTask).not.toHaveBeenCalled(); expect(store.updateTask).not.toHaveBeenCalled(); expect(store.logEntry).not.toHaveBeenCalled(); @@ -8247,6 +8397,10 @@ describe("autoMerge gating for mutating in-review sweeps (FN-5147)", () => { it("performs no mutations when autoMerge is disabled and no per-task override exists: recoverCompletionHandoffLimbo", async () => { const result = await manager.recoverCompletionHandoffLimbo(); expect(result).toBeUndefined(); + // The seeded FN-GATE-HANDOFF candidate carries an aged "Task marked done by + // agent" marker and no merge blocker, so the sweep enumerates it and would + // requeue/fail it absent the per-task gate. + expect(store.listTasks).toHaveBeenCalled(); expect(store.moveTask).not.toHaveBeenCalled(); expect(store.updateTask).not.toHaveBeenCalled(); expect(store.logEntry).not.toHaveBeenCalled(); diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index 929dbaea75..2c60cd7c81 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -2320,6 +2320,12 @@ export class SelfHealingManager { } const inReviewPausedCandidates = (await this.store.listTasks({ column: "in-review", slim: true })) .filter((task) => task.paused === true && task.pausedReason === "branch-conflict-unrecoverable"); + // Per-task auto-merge gating applies to ALL candidate columns, not just + // in-review: the FN-5704 regression contract ("short-circuits reclaim + // when autoMerge is false") deliberately keeps execution-stage reclaim + // and resume-limbo escalation inert in manual-review projects. The + // per-task override preserves that for override-less tasks while letting + // explicit autoMerge:true tasks recover. const candidates = [...todoCandidates, ...inProgressCandidates, ...inReviewPausedCandidates] .filter((task) => allowsAutoMergeProcessing(task, settings)); @@ -6929,6 +6935,10 @@ export class SelfHealingManager { !task.userPaused && !executingIds.has(task.id), ), + // The paused in-progress contamination branch is gated per-task too: + // pre-existing behavior kept this sweep fully inert in manual-review + // projects (mirroring the FN-5704 reclaim contract), so override-less + // tasks stay untouched while explicit autoMerge:true tasks recover. ...inProgress.filter((task) => task.column === "in-progress" && allowsAutoMergeProcessing(task, settings) &&