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
2026-06-03 13:08:06 -07:00
parent ff1bb20b8f
commit e00bc0235b
4 changed files with 237 additions and 4 deletions
--- 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
--- 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> = {}): 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");
+  });
+});
--- 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<typeof vi.fn>).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();
--- 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) &&