fix(FN-5345): address code-review findings on engine fixes

Follow-up to 1983dac6e addressing nine findings from a code review of the FN-5345/FN-5377 engine fixes. Includes a real bug fix (commit-message bypass of the amend detection), two reliability invariant restorations (FN-4811 + FN-4954 in the new D3 reuse-fallback path), a resource-leak cleanup, plus test/audit/taxonomy polish. HIGH - D3 reuse-fallback now respects FN-4811 active-session safety: matches whose path is currently owned by a different task in activeSessionRegistry are skipped, never silently rebound. Skipped owners are recorded in audit metadata for forensics. - D3 reuse-fallback now respects FN-4954 pool-lease bookkeeping: when recycleWorktrees=true AND a worktree pool is attached, the direct-reuse shortcut is bypassed and the existing acquireTaskWorktree path is used so WorktreePool.acquire/.release stays consistent. Without this guard the new path could trip PoolDoubleLeaseError. - prepare-commit-msg amend detection tokenizes the parent command line and stops at the first message-supplying flag (-m/-F/--message/--file/=variants) so a commit message containing the substring '--amend' cannot bypass the guard. New regression test in prepare-commit-msg-empty-guard.real-git.test.ts. MEDIUM - Early empty-own-diff fast-path extracted into tryEarlyEmptyOwnDiffFinalize() helper. Removes the exception-as-control-flow sentinel ('skip-early-fast-path: not-reuse-mode') in favor of a plain if (eligible) { try { ... } catch {} } block. - Fast-path best-effort cleans up the stranded fusion/<id> worktree and branch before completeTask(), so empty-own-diff residuals do not accumulate in .worktrees/ or the branch namespace. FN-4811 guard ensures we never remove a foreign-owned worktree. - Two new audit subtypes in run-audit.ts replace the prior overloading of merge:reuse-fallback-new-worktree: - merge:reuse-fallback-pruned-stale-registration - merge:reuse-fallback-reused-existing-registration merge:reuse-fallback-new-worktree is now reserved for actual new-worktree creation. Local emitReuseHandoffAuditEvent type union updated to match. - New direct classifier test in merger-finalize-unproven.real-git.test.ts ('classifies proven-no-op for empty-own-diff branches') covers the new branch in classifyOwnedLandedEvidence that self-healing and post-handoff paths also depend on. LOW - Alpine/busybox ps fallback: prepare-commit-msg hook reads /proc/$PPID/cmdline if 'ps -o args=' returns empty (busybox ps often lacks '-o args=' support). - New backstop test variant 'FN-5345: empty-own-diff fast-path fires even when branch is registered to two worktrees' reproduces the actual FN-5345 production wedge geometry where fusion/<id> was double-registered to two worktrees. Tests - Full @fusion/engine suite: 448 files / 5881 tests / 9 skipped, all green - pnpm lint green, pnpm build green
2026-05-20 17:54:26 -07:00
parent 1983dac6e4
commit 8e67404680
8 changed files with 464 additions and 205 deletions
--- a/packages/engine/src/tests/merger-finalize-unproven.real-git.test.ts
+++ b/packages/engine/src/tests/merger-finalize-unproven.real-git.test.ts
@@ -122,6 +122,35 @@ describeIfGit("aiMergeTask finalize no-op unproven reproduction (real git)", ()
    expect(classification).toEqual({ kind: "proven-no-op", baseRef: "main", ownDiffEmpty: true });
  });

+  // FN-5345/FN-5377 direct classifier coverage: empty-own-diff (aheadCount > 0
+  // but zero net diff vs merge-base) is logically equivalent to proven-no-op.
+  // This pairs with the merger's early fast-path and exercises the new branch
+  // in classifyOwnedLandedEvidence that self-healing and post-handoff finalize
+  // paths also rely on.
+  it("classifies proven-no-op for empty-own-diff branches (FN-5345/FN-5377)", async () => {
+    const repo = mkdtempSync(join(tmpdir(), "fusion-merger-empty-own-diff-"));
+    repos.push(repo);
+    git(repo, "git init -b main");
+    git(repo, 'git config user.email "test@example.com"');
+    git(repo, 'git config user.name "Test User"');
+    git(repo, "git commit --allow-empty -m 'init'");
+    const baseSha = git(repo, "git rev-parse HEAD");
+
+    git(repo, "git checkout -b fusion/fn-empty-own-diff");
+    // 1 own commit with zero net tree change vs merge-base.
+    git(repo, "git commit --allow-empty -m 'test(FN-EMPTY-OWN-DIFF): handoff'");
+    const branchTipSha = git(repo, "git rev-parse HEAD");
+    expect(branchTipSha).not.toBe(baseSha); // aheadCount >= 1
+    git(repo, "git checkout main");
+
+    const classification = await classifyOwnedLandedEvidence(
+      repo,
+      { id: "FN-EMPTY-OWN-DIFF", branch: "fusion/fn-empty-own-diff", baseCommitSha: baseSha } as Task,
+      { mergeTargetBranch: "main" },
+    );
+    expect(classification).toEqual({ kind: "proven-no-op", baseRef: "main", ownDiffEmpty: true });
+  });
+
  it("auto-finalizes proven no-op and clears stale modifiedFiles", async () => {
    const repo = mkdtempSync(join(tmpdir(), "fusion-merger-noop-finalize-"));
    repos.push(repo);
--- a/packages/engine/src/tests/real-git/prepare-commit-msg-empty-guard.real-git.test.ts
+++ b/packages/engine/src/tests/real-git/prepare-commit-msg-empty-guard.real-git.test.ts
@@ -45,6 +45,17 @@ describe("prepare-commit-msg empty-commit guard (real git, FN-5345/FN-5377)", ()
      expect(empty.stderr).toContain("refusing empty commit");
      expect(empty.stderr).toContain("FN-5345/FN-5377");

+      // Review-finding regression: a commit message containing the substring
+      // '--amend' must NOT trick the parent-cmd tokenized check into allowing
+      // the empty commit. The original glob pattern (*' --amend'*) would have
+      // matched this; the tokenized check rejects it.
+      const sneaky = git(
+        worktreeDir,
+        "git commit --allow-empty -m 'feat(FN-5345): fix --amend handling'",
+      );
+      expect(sneaky.status).not.toBe(0);
+      expect(sneaky.stderr).toContain("refusing empty commit");
+
      // --amend --no-edit (no staged changes, amend HEAD) is ALLOWED.
      const amendNoEdit = git(worktreeDir, "git commit --amend --no-edit");
      expect(amendNoEdit.status).toBe(0);
--- a/packages/engine/src/tests/reliability-interactions/merge-reuse-task-worktree.test.ts
+++ b/packages/engine/src/tests/reliability-interactions/merge-reuse-task-worktree.test.ts
@@ -586,4 +586,71 @@ describe("FN-5279 reliability interactions: merge reuse task worktree", () => {
    },
    30_000,
  );
+
+  // FN-5345/FN-5377 backstop variant: reproduce the actual production wedge
+  // geometry where `fusion/<id>` is registered to TWO worktrees simultaneously
+  // (e.g. faint-creek + hazy-quail in the FN-5345 incident). The early
+  // fast-path runs against projectRootDir and is immune to the worktree drift,
+  // so it must still finalize without acquiring any reuse handoff.
+  it.skipIf(!hasGit)(
+    "FN-5345: empty-own-diff fast-path fires even when branch is registered to two worktrees",
+    async () => {
+      const fixture = await makeReliabilityFixture({
+        taskId: "FN-5279-RI-DOUBLE-REG",
+        settings: {
+          baseBranch: "master",
+          mergeIntegrationWorktree: "reuse-task-worktree",
+        } as any,
+      });
+
+      try {
+        const { rootDir, store, task } = fixture;
+        const actualTask = await store.getTask(task.id);
+        const branch = `fusion/${actualTask!.id.toLowerCase()}`;
+        const worktreeRoot = `${rootDir}-worktrees`;
+        const pathA = join(worktreeRoot, `${actualTask!.id.toLowerCase()}-a`);
+        const pathB = join(worktreeRoot, `${actualTask!.id.toLowerCase()}-b`);
+
+        git(rootDir, "git branch -m main master");
+        const completedSteps = (actualTask?.steps ?? []).map((step) => ({ ...step, status: "done" as const }));
+        await store.updateTask(task.id, {
+          baseBranch: "master",
+          branch,
+          steps: completedSteps,
+          currentStep: completedSteps.length,
+        } as any);
+        await fixture.createBranch(branch);
+        git(rootDir, `git commit --allow-empty -m 'test(${actualTask!.id}): verification-only handoff'`);
+        await fixture.checkout("master");
+
+        // Register branch at pathA, then force-register at pathB — reproduces
+        // FN-5345's two-worktree-one-branch state.
+        await mkdir(worktreeRoot, { recursive: true });
+        git(rootDir, `git worktree add ${JSON.stringify(pathA)} ${JSON.stringify(branch)}`);
+        git(rootDir, `git worktree add -f ${JSON.stringify(pathB)} ${JSON.stringify(branch)}`);
+
+        // task.worktree points at one of them — doesn't matter which; the
+        // fast-path operates against projectRootDir.
+        await store.updateTask(task.id, { worktree: pathA, branch } as any);
+        store.enqueueMergeQueue(task.id);
+
+        const result = await aiMergeTask(store, rootDir, task.id);
+
+        expect(result.merged).toBe(true);
+        expect(result.noOp).toBe(true);
+        expect(result.mergeConfirmed).toBe(true);
+        expect((await store.getTask(task.id))?.column).toBe("done");
+
+        const auditTypes = store.getRunAuditEvents({ taskId: task.id }).map((event) => event.mutationType);
+        // No reuse-handoff lifecycle event should have fired — the fast-path
+        // ran first against projectRootDir.
+        expect(auditTypes).not.toContain("merge:reuse-handoff-acquired");
+        expect(auditTypes).not.toContain("merge:reuse-handoff-refused");
+        expect(auditTypes).toContain("task:auto-recover-finalize-already-on-main");
+      } finally {
+        await fixture.cleanup();
+      }
+    },
+    30_000,
+  );
 });