From 68e52e3c35c7e55d4fa25acc81aa548874ffbc16 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 3 Jun 2026 15:24:03 -0700 Subject: [PATCH 1/2] fix(engine): capture baseCommitSha against local main, not origin/main MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In-review tasks showed other tasks' files in their "files changed" list. Task branches fork from local main, but the base capture measured merge-base(HEAD, origin/main) — when local main carried merged-but-unpushed task commits, the recorded base rewound past them, and after the post-merge rebase-and-push rewrote those SHAs, baseCommitSha..HEAD permanently swept the predecessors' files into the new task's diff. Extract the capture into base-commit-capture.ts, measure local main first (origin/main fallback) to match the contamination-base sites, and add a real-git regression suite covering local-ahead-of-origin. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/fix-base-commit-sha-local-main.md | 5 + .../base-commit-capture.real-git.test.ts | 106 ++++++++++++++++++ packages/engine/src/base-commit-capture.ts | 55 +++++++++ packages/engine/src/executor.ts | 22 +--- 4 files changed, 171 insertions(+), 17 deletions(-) create mode 100644 .changeset/fix-base-commit-sha-local-main.md create mode 100644 packages/engine/src/__tests__/base-commit-capture.real-git.test.ts create mode 100644 packages/engine/src/base-commit-capture.ts diff --git a/.changeset/fix-base-commit-sha-local-main.md b/.changeset/fix-base-commit-sha-local-main.md new file mode 100644 index 0000000000..adc5e98796 --- /dev/null +++ b/.changeset/fix-base-commit-sha-local-main.md @@ -0,0 +1,5 @@ +--- +"@runfusion/fusion": patch +--- + +Fix in-review tasks showing other tasks' files in the "files changed" list. `baseCommitSha` was captured as `merge-base(HEAD, origin/main)` at task start, but task branches fork from local main — when local main was ahead by merged-but-unpushed task commits, the recorded base rewound past them, and after the post-merge rebase-and-push rewrote their SHAs the diff range permanently swept the predecessors' files into the new task's diff. The capture now measures against local main first (origin/main as fallback), matching the contamination-base sites. diff --git a/packages/engine/src/__tests__/base-commit-capture.real-git.test.ts b/packages/engine/src/__tests__/base-commit-capture.real-git.test.ts new file mode 100644 index 0000000000..45ca1641f6 --- /dev/null +++ b/packages/engine/src/__tests__/base-commit-capture.real-git.test.ts @@ -0,0 +1,106 @@ +import { execSync, spawnSync } from "node:child_process"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { resolveCapturedBaseCommitSha } from "../base-commit-capture.js"; + +const hasGit = spawnSync("git", ["--version"], { stdio: "pipe" }).status === 0; +const describeIfGit = hasGit ? describe : describe.skip; + +function git(repo: string, command: string): string { + return execSync(command, { cwd: repo, encoding: "utf-8", stdio: ["pipe", "pipe", "pipe"] }).trim(); +} + +describeIfGit("resolveCapturedBaseCommitSha real-git scenarios", { timeout: 30_000 }, () => { + const dirs: string[] = []; + afterEach(() => { + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); + }); + + function tmp(prefix: string): string { + const dir = mkdtempSync(join(tmpdir(), prefix)); + dirs.push(dir); + return dir; + } + + function originFixture(): string { + const origin = tmp("fusion-base-capture-origin-"); + git(origin, "git init -b main"); + git(origin, 'git config user.email "test@example.com"'); + git(origin, 'git config user.name "Test User"'); + writeFileSync(join(origin, "README.md"), "init\n"); + git(origin, "git add README.md && git commit -m 'init'"); + return origin; + } + + function cloneFixture(origin: string): string { + const clone = tmp("fusion-base-capture-clone-"); + git(clone, `git clone ${JSON.stringify(origin)} .`); + git(clone, 'git config user.email "test@example.com"'); + git(clone, 'git config user.name "Test User"'); + return clone; + } + + it("captures the local-main fork point when local main is ahead of origin/main (unpushed merges)", async () => { + // Models the FN-5937 regression: the merger lands other tasks' commits on + // LOCAL main first; new task branches fork from that tip before the + // rebase-and-push rewrites those SHAs. Capturing merge-base against + // origin/main rewinds past the unpushed merges, so the dashboard diff + // (baseCommitSha..HEAD) later surfaces the predecessors' files as this + // task's "files changed". + const origin = originFixture(); + const clone = cloneFixture(origin); + + // Local main advances by a merged-but-unpushed predecessor task commit. + writeFileSync(join(clone, "predecessor.txt"), "FN-5936 work\n"); + git(clone, "git add predecessor.txt && git commit -m 'FN-5936: predecessor task'"); + const localMainTip = git(clone, "git rev-parse HEAD"); + + // New task branch forks from local main (prepareForTask behavior). + git(clone, "git checkout -B fusion/fn-5937-test main"); + + const captured = await resolveCapturedBaseCommitSha(clone); + expect(captured).toBe(localMainTip); + }); + + it("captures the merge-base with main for a branch with its own commits", async () => { + const origin = originFixture(); + const clone = cloneFixture(origin); + const forkPoint = git(clone, "git rev-parse HEAD"); + + git(clone, "git checkout -B fusion/fn-100-test main"); + writeFileSync(join(clone, "feature.txt"), "feature\n"); + git(clone, "git add feature.txt && git commit -m 'FN-100: feature'"); + + const captured = await resolveCapturedBaseCommitSha(clone); + expect(captured).toBe(forkPoint); + }); + + it("falls back to origin/main when no local main branch exists", async () => { + const origin = originFixture(); + const clone = cloneFixture(origin); + const originMainSha = git(clone, "git rev-parse origin/main"); + + // Detach and delete local main so only origin/main can resolve. + git(clone, "git checkout --detach origin/main"); + git(clone, "git branch -D main"); + git(clone, "git checkout -B fusion/fn-200-test"); + + const captured = await resolveCapturedBaseCommitSha(clone); + expect(captured).toBe(originMainSha); + }); + + it("falls back to HEAD when neither main nor origin/main resolves", async () => { + const repo = tmp("fusion-base-capture-nomain-"); + git(repo, "git init -b trunk"); + git(repo, 'git config user.email "test@example.com"'); + git(repo, 'git config user.name "Test User"'); + writeFileSync(join(repo, "README.md"), "init\n"); + git(repo, "git add README.md && git commit -m 'init'"); + const head = git(repo, "git rev-parse HEAD"); + + const captured = await resolveCapturedBaseCommitSha(repo); + expect(captured).toBe(head); + }); +}); diff --git a/packages/engine/src/base-commit-capture.ts b/packages/engine/src/base-commit-capture.ts new file mode 100644 index 0000000000..c449a97558 --- /dev/null +++ b/packages/engine/src/base-commit-capture.ts @@ -0,0 +1,55 @@ +import { exec } from "node:child_process"; +import { promisify } from "node:util"; + +const execAsync = promisify(exec); + +/** + * Resolve the fork-point base SHA for a freshly acquired task worktree. + * + * Called immediately after worktree acquisition, when the task branch was + * just created/force-reset from the local integration branch + * (`prepareForTask` forks from local `main` via `resolveIntegrationBranch`). + * + * The merge-base MUST be measured against LOCAL main first (origin/main only + * as a fallback), matching the contamination-base sites in + * `worktree-acquisition.ts` and `auto-recovery-handlers/branch-worktree.ts`. + * The merger lands tasks on local main before pushing, so at fork time local + * main can be ahead of origin/main by merged-but-unpushed commits. Measuring + * against origin/main rewinds the base past those commits; once the + * post-merge rebase-and-push rewrites their SHAs, `baseCommitSha..HEAD` + * permanently sweeps the predecessors' files into this task's diff (FN-5937: + * in-review tasks showing 31 "files changed" instead of 12). + * + * Returns `undefined` only when every git invocation fails (caller treats a + * missing base as non-fatal). + */ +export async function resolveCapturedBaseCommitSha( + worktreePath: string, + logger?: { warn: (msg: string) => void }, +): Promise { + let baseCommitSha: string | undefined; + try { + const { stdout } = await execAsync( + "git merge-base HEAD main 2>/dev/null || git merge-base HEAD origin/main", + { cwd: worktreePath, encoding: "utf-8" }, + ); + baseCommitSha = stdout.trim() || undefined; + } catch (err: unknown) { + const errorMessage = err instanceof Error ? err.message : String(err); + logger?.warn(`merge-base failed, falling back to HEAD: ${errorMessage}`); + } + + if (!baseCommitSha) { + try { + const { stdout } = await execAsync("git rev-parse HEAD", { + cwd: worktreePath, + encoding: "utf-8", + }); + baseCommitSha = stdout.trim() || undefined; + } catch { + return undefined; + } + } + + return baseCommitSha; +} diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index a46088fe13..e5d4ab4579 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -93,6 +93,7 @@ import type { PluginRunner } from "./plugin-runner.js"; import { isContextLimitError } from "./context-limit-detector.js"; import { StepSessionExecutor } from "./step-session-executor.js"; import { acquireTaskWorktree } from "./worktree-acquisition.js"; +import { resolveCapturedBaseCommitSha } from "./base-commit-capture.js"; import { installTaskWorktreeIdentityGuard } from "./worktree-hooks.js"; import { resolveAgentInstructions, @@ -7559,24 +7560,11 @@ ${failureFeedback} } } - let baseCommitSha: string | undefined; - try { - const { stdout } = await execAsync( - "git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main", - { cwd: worktreePath, encoding: "utf-8" }, - ); - baseCommitSha = stdout.trim() || undefined; - } catch (err: unknown) { - const errorMessage = err instanceof Error ? err.message : String(err); - executorLog.warn(`${task.id}: merge-base failed, falling back to HEAD: ${errorMessage}`); - } - + const baseCommitSha = await resolveCapturedBaseCommitSha(worktreePath, { + warn: (msg) => executorLog.warn(`${task.id}: ${msg}`), + }); if (!baseCommitSha) { - const { stdout } = await execAsync("git rev-parse HEAD", { - cwd: worktreePath, - encoding: "utf-8", - }); - baseCommitSha = stdout.trim(); + throw new Error("could not resolve base commit SHA"); } await this.store.updateTask(task.id, { baseCommitSha }); From 73841cd796f9ae1207231b8aaf6539da0e23e51b Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 3 Jun 2026 15:32:54 -0700 Subject: [PATCH 2/2] docs: capture origin-first base-capture learning and branching vocabulary Document the FN-5937 files-changed inflation root cause in docs/solutions/ and add the Branching & diff attribution cluster (Integration branch, Fork point, Rebase-and-push, Contamination) to CONCEPTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) --- CONCEPTS.md | 14 +++ ...ed-inflated-by-origin-first-base-commit.md | 101 ++++++++++++++++++ 2 files changed, 115 insertions(+) create mode 100644 docs/solutions/logic-errors/files-changed-inflated-by-origin-first-base-commit.md diff --git a/CONCEPTS.md b/CONCEPTS.md index 9f015d0e39..50573e7eb3 100644 --- a/CONCEPTS.md +++ b/CONCEPTS.md @@ -27,6 +27,20 @@ A recurring background scan that detects and repairs stuck Task states — stall ### Shared branch group A set of Tasks integrating into a common shared branch instead of each merging straight to the project's default branch. Member integration (task branch → shared branch) is a soft pre-integration step exempt from the global auto-merge gate; promotion (shared branch → default branch) is gated separately. +## Branching & diff attribution + +### Integration branch +The local branch (by default the project's default branch) where the merger lands Task branches and from whose tip new Task worktrees fork. Because the merger lands commits locally before pushing, the Integration branch can be ahead of its origin counterpart by merged-but-unpushed commits — any fork-point or merge-base computation must measure against the local branch first, with the origin ref only as a fallback. + +### Fork point +The commit on the Integration branch from which a Task's branch was created — the exclusive lower bound of the Task's owned changes. Every "files changed" computation diffs fork point to branch tip, so a recorded base older than the true Fork point permanently attributes predecessors' files to the Task. + +### Rebase-and-push +The post-merge step that rebases locally-landed merge commits onto the upstream branch before pushing, rewriting their SHAs. The original commits become orphaned — no longer reachable from the Integration branch — while still present in the history of any Task branch forked before the push, which is why a too-old recorded Fork point cannot be recovered after this step. + +### Contamination +Foreign commits — work attributed to other Tasks — appearing on a Task's branch beyond its recorded Fork point. Contamination checks must compute their reference base fresh from the Integration branch rather than reuse the Task's stored base, since a stale stored base makes every legitimately merged commit look foreign. + ## Flagged ambiguities - "Merging" a shared-branch-group Task had been used for both member integration and group promotion — these are distinct steps with independent gating and must not be conflated. diff --git a/docs/solutions/logic-errors/files-changed-inflated-by-origin-first-base-commit.md b/docs/solutions/logic-errors/files-changed-inflated-by-origin-first-base-commit.md new file mode 100644 index 0000000000..9fadf0110b --- /dev/null +++ b/docs/solutions/logic-errors/files-changed-inflated-by-origin-first-base-commit.md @@ -0,0 +1,101 @@ +--- +title: In-review files-changed inflated by origin-first baseCommitSha capture +date: 2026-06-03 +category: logic-errors +module: engine +problem_type: logic_error +component: development_workflow +symptoms: + - "In-review tasks showed 20-31 files changed when only 2-12 were actually touched" + - "Extra files in a task's diff belonged to other, already-merged tasks" + - "All in-review tasks in a cohort shared a suspiciously old baseCommitSha" + - "Inflation was permanent — display-time merge-base recovery could not tighten it" +root_cause: logic_error +resolution_type: code_fix +severity: medium +related_components: + - testing_framework +tags: + - git-merge-base + - basecommitsha + - fork-point + - origin-vs-local-main + - files-changed + - worktree-pool + - rebase-and-push + - diff-base +--- + +# In-review files-changed inflated by origin-first baseCommitSha capture + +## Problem + +New task branches recorded a `baseCommitSha` that was too old, so the dashboard's `baseCommitSha..HEAD` diff swept in files belonging to other, already-merged tasks — showing 20–31 "files changed" when the task actually touched 2–12 (FN-5937: 31 shown vs 12 real). `captureBaseCommitSha` computed `git merge-base HEAD origin/main` while the merger lands commits on **local** main before pushing. + +## Symptoms + +- In-review tasks on the dashboard displayed inflated "files changed" counts (20–31) versus their true touched-file count (2–12). +- The extra files all belonged to other tasks that had already merged (FN-5937's inflated diff contained files from FN-5936/FN-5907/FN-5939/FN-5940). +- The inflation was **permanent** — display-time recovery could not tighten it because the orphaned predecessor SHAs were no longer reachable from `main`. +- All in-review tasks in a dispatch cohort shared a suspiciously too-old `baseCommitSha`; the pattern recurred on the next cohort (FN-5953) after the next rebase-push cycle. + +## What Didn't Work + +- **Worktree-pool reassignment** — a recycled worktree hosting a foreign branch could surface another task's commits. Ruled out: the diff routes guard this via `worktreeStillBelongsToTask`, and each worktree's HEAD matched its task's recorded branch. +- **Stale-base display recovery** — the dashboard already re-tightens stale bases at display time via `merge-base(HEAD, main)` (FN-2957/FN-2840). Ruled out: recovery is structurally unable to help here because the orphaned predecessor SHAs no longer exist in `main`, so the merge-base lands on the same too-old commit as the stored base. +- **Branch-group sharing** — tasks sharing a branch group legitimately share commits. Ruled out: the contaminating commits came from unrelated, independently-merged tasks, and the captured base predated the true fork point regardless of grouping. + +## Solution + +Extract the capture into `packages/engine/src/base-commit-capture.ts` (`resolveCapturedBaseCommitSha`) and swap the merge-base command to **local-first**: + +```sh +# before (executor.ts captureBaseCommitSha) — origin-first +git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main + +# after (base-commit-capture.ts) — local-first +git merge-base HEAD main 2>/dev/null || git merge-base HEAD origin/main +``` + +This matches the two sibling contamination-base sites that were **already** local-first (`worktree-acquisition.ts`, `auto-recovery-handlers/branch-worktree.ts`); the capture site was the only origin-first outlier. A real-git regression suite (`packages/engine/src/__tests__/base-commit-capture.real-git.test.ts`) locks the behavior in, including the local-ahead-of-origin scenario. The 5 already-corrupted live in-review tasks were repaired in place via `TaskStore.updateTask`, recomputing each base as the parent of the branch's first own-attributed commit, with ancestry safety checks. Shipped in PR Runfusion/Fusion#1376. + +## Why This Works + +The merger integrates tasks by landing their commits on **local** `main` first, then later rebase-and-pushes. Two consequences flow from this: + +1. At the moment a new task's base is captured (right after worktree acquisition — the worktree forks from the **local** main tip via `prepareForTask` → `resolveIntegrationBranch`), local `main` can be **ahead of `origin/main`** by merged-but-unpushed commits. Measuring `merge-base HEAD origin/main` rewinds the base past those commits. +2. The post-merge rebase-and-push rewrites those commits' SHAs in `main`, **orphaning** the originals that the task branch still descends from. Even display-time `merge-base(HEAD, main)` recovery can't find a tightening point afterward — the orphaned SHAs aren't in `main` anymore. + +``` +fork time: ...59cd9ea ── 839d191 (merged, unpushed) ── 8db04c4 ← local main tip + │ + └─ taskBranch: a1 a2 ... +captured base = merge-base(HEAD, origin/main) = 59cd9ea ← too old: includes 839d191's files + +after rebase-push: main = ...59cd9ea ── ── 419f688 (was 839d191) ── ... + taskBranch still descends from the now-orphaned 839d191/8db04c4 + merge-base(HEAD, main) = 59cd9ea → no recovery possible +diff 59cd9ea..HEAD permanently shows predecessors' files as this task's changes +``` + +**The invariant:** any base / fork-point computation in this codebase must measure against **local `main` first**, with `origin/main` only as a fallback. Because the merger lands commits locally before pushing — and the push rewrites their SHAs — `origin/main` is systematically behind, and an origin-first merge-base will rewind the base into a predecessor's history and then strand it. + +## Prevention + +- **Follow the invariant**: every base/fork-point computation uses `git merge-base HEAD main 2>/dev/null || git merge-base HEAD origin/main` (local-first). Never lead with `origin/main`. +- **Grep check for regressions** — any origin-first site is suspect: + + ```sh + grep -rn "merge-base HEAD origin/" packages/engine/src + ``` + + `origin/main` should only ever appear as the fallback tail after a `||`. +- **Real-git test pattern for local-ahead-of-origin**: build a real repo where local `main` is advanced past `origin/main` (commit locally without pushing), fork a task branch from the local tip, and assert the captured base equals the **local fork point**, not the origin merge-base. String-matched command mocks cannot distinguish ordering inside a shell `||` — this scenario must run against actual git (see `base-commit-capture.real-git.test.ts`). + +## Related Issues + +- PR Runfusion/Fusion#1376 — the fix this doc documents +- Runfusion/Fusion#256 (FN-4425) — introduced the files-changed surface for in-review tasks; lineage of the capture path +- Runfusion/Fusion#424 (FN-4741) — rebase-merge diff truncation for done tasks; same diff-range-after-rebase failure family +- Runfusion/Fusion#304 / Runfusion/Fusion#349 (FN-4576/FN-4647) — earlier done-task diff-mismatch fixes in the same symptom family +- [per-task-auto-merge-override-ignored-by-trigger-gates](./per-task-auto-merge-override-ignored-by-trigger-gates.md) — adjacent in-review lifecycle bug (task silently presents wrong state in review)