fix(dashboard): prevent foreign-branch diffs after worktree pool reuse
When the worktree-recycle pool reassigned a path to a new task, the old task's diff endpoints kept reading the new task's branch state — surfacing unrelated commits as the original task's "files changed" list. - Clear task.worktree/branch in the merger after the worktree is released to the pool or removed, so the path no longer points anywhere. - Validate the worktree's current branch matches task.branch in the three worktree-backed diff endpoints; on mismatch return empty rather than diffing against a foreign branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -8,6 +8,28 @@ export interface SessionDiffRouteDeps {
|
|||||||
getProjectContext: (req: Request) => Promise<ProjectContext>;
|
getProjectContext: (req: Request) => Promise<ProjectContext>;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Confirm the worktree's current branch still matches the task's recorded
|
||||||
|
* branch. Worktrees from the recycle pool can be reassigned to a different
|
||||||
|
* task after a merge; without this check the diff endpoints would happily
|
||||||
|
* read another task's branch state and surface its commits as the original
|
||||||
|
* task's "files changed" list. Returns true when no validation is possible
|
||||||
|
* (e.g. task.branch was never set) so we don't break tests/legacy tasks.
|
||||||
|
*/
|
||||||
|
async function worktreeStillBelongsToTask(
|
||||||
|
worktree: string,
|
||||||
|
expectedBranch: string | undefined | null,
|
||||||
|
): Promise<boolean> {
|
||||||
|
if (!expectedBranch) return true;
|
||||||
|
try {
|
||||||
|
const actual = (await runGitCommand(["rev-parse", "--abbrev-ref", "HEAD"], worktree, 5000)).trim();
|
||||||
|
if (!actual || actual === "HEAD") return true; // detached HEAD — can't validate
|
||||||
|
return actual === expectedBranch;
|
||||||
|
} catch {
|
||||||
|
return true; // best-effort: never block diff just because rev-parse failed
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
const sessionFilesCache = new Map<string, { files: string[]; expiresAt: number }>();
|
const sessionFilesCache = new Map<string, { files: string[]; expiresAt: number }>();
|
||||||
const fileDiffsCache = new Map<
|
const fileDiffsCache = new Map<
|
||||||
string,
|
string,
|
||||||
@@ -56,6 +78,12 @@ export function registerSessionDiffRoutes(router: Router, deps: SessionDiffRoute
|
|||||||
}
|
}
|
||||||
|
|
||||||
const worktree = task.worktree;
|
const worktree = task.worktree;
|
||||||
|
if (!(await worktreeStillBelongsToTask(worktree, task.branch))) {
|
||||||
|
// Pool likely reassigned this path to another task — return empty
|
||||||
|
// rather than diffing against a foreign branch's HEAD.
|
||||||
|
res.json([]);
|
||||||
|
return;
|
||||||
|
}
|
||||||
const cached = sessionFilesCache.get(task.id);
|
const cached = sessionFilesCache.get(task.id);
|
||||||
if (cached && cached.expiresAt > Date.now()) {
|
if (cached && cached.expiresAt > Date.now()) {
|
||||||
res.json(cached.files);
|
res.json(cached.files);
|
||||||
@@ -206,6 +234,10 @@ export function registerSessionDiffRoutes(router: Router, deps: SessionDiffRoute
|
|||||||
res.json({ files: [], stats: { filesChanged: 0, additions: 0, deletions: 0 } });
|
res.json({ files: [], stats: { filesChanged: 0, additions: 0, deletions: 0 } });
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
if (!(await worktreeStillBelongsToTask(resolvedWorktree, task.branch))) {
|
||||||
|
res.json({ files: [], stats: { filesChanged: 0, additions: 0, deletions: 0 } });
|
||||||
|
return;
|
||||||
|
}
|
||||||
const cwd = resolvedWorktree;
|
const cwd = resolvedWorktree;
|
||||||
|
|
||||||
const diffBase = await resolveDiffBase(task, cwd);
|
const diffBase = await resolveDiffBase(task, cwd);
|
||||||
@@ -380,6 +412,10 @@ export function registerSessionDiffRoutes(router: Router, deps: SessionDiffRoute
|
|||||||
}
|
}
|
||||||
|
|
||||||
const worktree = task.worktree;
|
const worktree = task.worktree;
|
||||||
|
if (!(await worktreeStillBelongsToTask(worktree, task.branch))) {
|
||||||
|
res.json([]);
|
||||||
|
return;
|
||||||
|
}
|
||||||
const cached = fileDiffsCache.get(task.id);
|
const cached = fileDiffsCache.get(task.id);
|
||||||
if (cached && cached.expiresAt > Date.now()) {
|
if (cached && cached.expiresAt > Date.now()) {
|
||||||
res.json(cached.files);
|
res.json(cached.files);
|
||||||
|
|||||||
@@ -47,6 +47,12 @@ export interface ResolveDiffBaseTaskInput {
|
|||||||
* 2. **Task-scoped baseCommitSha** — If merge-base is unavailable or equals
|
* 2. **Task-scoped baseCommitSha** — If merge-base is unavailable or equals
|
||||||
* `headRef`, use `baseCommitSha` when still an ancestor of `headRef`.
|
* `headRef`, use `baseCommitSha` when still an ancestor of `headRef`.
|
||||||
* 3. **headRef~1** — Last-resort fallback.
|
* 3. **headRef~1** — Last-resort fallback.
|
||||||
|
*
|
||||||
|
* Note: callers must validate the worktree still belongs to the task (e.g.
|
||||||
|
* compare `git rev-parse --abbrev-ref HEAD` to `task.branch`) before invoking
|
||||||
|
* this. After worktree-pool reassignment the same path may host a foreign
|
||||||
|
* branch, in which case `baseCommitSha..HEAD` would surface other tasks'
|
||||||
|
* commits and this function has no way to detect that.
|
||||||
*/
|
*/
|
||||||
export async function resolveDiffBase(
|
export async function resolveDiffBase(
|
||||||
task: ResolveDiffBaseTaskInput,
|
task: ResolveDiffBaseTaskInput,
|
||||||
|
|||||||
@@ -358,6 +358,29 @@ describe("aiMergeTask — conditional worktree cleanup", () => {
|
|||||||
expect(result.worktreeRemoved).toBe(true);
|
expect(result.worktreeRemoved).toBe(true);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("clears task.worktree/branch after the worktree is removed", async () => {
|
||||||
|
const worktreePath = "/tmp/root/.worktrees/KB-050";
|
||||||
|
const store = createMockStore(
|
||||||
|
{ id: "FN-050", worktree: worktreePath },
|
||||||
|
[
|
||||||
|
{ id: "FN-050", worktree: worktreePath, column: "in-review" } as Task,
|
||||||
|
],
|
||||||
|
);
|
||||||
|
|
||||||
|
await aiMergeTask(store, "/tmp/root", "FN-050");
|
||||||
|
|
||||||
|
// The dashboard's diff endpoint reads task.worktree to decide whether to
|
||||||
|
// run a live git diff; leaving it set after removal would point at a
|
||||||
|
// foreign branch (when the path is recycled) and surface other tasks'
|
||||||
|
// commits as if they belonged to FN-050.
|
||||||
|
const updateCalls = (store.updateTask as any).mock.calls;
|
||||||
|
const cleared = updateCalls.find(
|
||||||
|
([id, patch]: [string, any]) =>
|
||||||
|
id === "FN-050" && patch && patch.worktree === null && patch.branch === null,
|
||||||
|
);
|
||||||
|
expect(cleared).toBeDefined();
|
||||||
|
});
|
||||||
|
|
||||||
it("always deletes the branch regardless of worktree sharing", async () => {
|
it("always deletes the branch regardless of worktree sharing", async () => {
|
||||||
const worktreePath = "/tmp/root/.worktrees/KB-050";
|
const worktreePath = "/tmp/root/.worktrees/KB-050";
|
||||||
const store = createMockStore(
|
const store = createMockStore(
|
||||||
@@ -4049,7 +4072,7 @@ describe("resolveTaskDiffBaseRef", () => {
|
|||||||
expect(diffBase).toBe("parent-123");
|
expect(diffBase).toBe("parent-123");
|
||||||
});
|
});
|
||||||
|
|
||||||
it("returns undefined when no merge base or fallback refs are available", async () => {
|
it("returns undefined when no merge base, no valid baseCommitSha, and no parent commit are available", async () => {
|
||||||
mockedExecSync.mockImplementation((cmd: any) => {
|
mockedExecSync.mockImplementation((cmd: any) => {
|
||||||
const cmdStr = String(cmd);
|
const cmdStr = String(cmd);
|
||||||
if (cmdStr === 'git merge-base "HEAD" "main"') {
|
if (cmdStr === 'git merge-base "HEAD" "main"') {
|
||||||
|
|||||||
@@ -2834,6 +2834,14 @@ export async function aiMergeTask(
|
|||||||
} else if (options.pool && settings.recycleWorktrees) {
|
} else if (options.pool && settings.recycleWorktrees) {
|
||||||
options.pool.release(worktreePath);
|
options.pool.release(worktreePath);
|
||||||
result.worktreeRemoved = false;
|
result.worktreeRemoved = false;
|
||||||
|
// Detach the path from this task so future diff queries don't read
|
||||||
|
// a foreign branch's state once the pool reassigns this worktree.
|
||||||
|
try {
|
||||||
|
await store.updateTask(taskId, { worktree: null, branch: null });
|
||||||
|
} catch (err: unknown) {
|
||||||
|
const msg = err instanceof Error ? err.message : String(err);
|
||||||
|
mergerLog.warn(`${taskId}: failed to clear worktree pointer after pool release: ${msg}`);
|
||||||
|
}
|
||||||
} else {
|
} else {
|
||||||
try {
|
try {
|
||||||
await execAsync(`git worktree remove "${worktreePath}" --force`, {
|
await execAsync(`git worktree remove "${worktreePath}" --force`, {
|
||||||
@@ -2842,6 +2850,12 @@ export async function aiMergeTask(
|
|||||||
// Audit trail: record worktree removal (FN-1404)
|
// Audit trail: record worktree removal (FN-1404)
|
||||||
await audit.git({ type: "worktree:remove", target: worktreePath });
|
await audit.git({ type: "worktree:remove", target: worktreePath });
|
||||||
result.worktreeRemoved = true;
|
result.worktreeRemoved = true;
|
||||||
|
try {
|
||||||
|
await store.updateTask(taskId, { worktree: null, branch: null });
|
||||||
|
} catch (err: unknown) {
|
||||||
|
const msg = err instanceof Error ? err.message : String(err);
|
||||||
|
mergerLog.warn(`${taskId}: failed to clear worktree pointer after removal: ${msg}`);
|
||||||
|
}
|
||||||
} catch { /* non-fatal */ }
|
} catch { /* non-fatal */ }
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user