fix(engine): un-deadcode bootstrap-misbinding auto-recovery fallback
The auto-recovery handler in branch-worktree.ts passed foreignCommits: [] to classifyBootstrapMisbinding, and the classifier gated isBootstrapMisbinding on foreignCommits.length > 0. The entire reanchor block was dead code on this path — the FN-5475 cascade hit "human adjudication" instead of recovering. The classifier now derives the foreign-commit count from its own git log walk; the input field is advisory/optional. Result type gains foreignCommitCount. The fallback handler also stops using ctx.task.baseCommitSha (deliberately stale per FN-4417) and computes a fresh merge-base against local main / origin/main, matching the executor's primary contamination path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
28
.changeset/fix-engine-bootstrap-misbinding-fallback.md
Normal file
28
.changeset/fix-engine-bootstrap-misbinding-fallback.md
Normal file
@@ -0,0 +1,28 @@
|
||||
---
|
||||
"@fusion/engine": patch
|
||||
---
|
||||
|
||||
fix(engine): un-deadcode the bootstrap-misbinding auto-recovery fallback
|
||||
|
||||
The auto-recovery handler in `auto-recovery-handlers/branch-worktree.ts`
|
||||
called `classifyBootstrapMisbinding` with `foreignCommits: []` because it
|
||||
had no `BranchCrossContaminationError` in hand (it discovers the conflict
|
||||
via `inspectBranchConflict`). The classifier's predicate gated on
|
||||
`foreignCommits.length > 0`, so the input always resolved to
|
||||
`isBootstrapMisbinding: false` and the re-anchor block was effectively
|
||||
dead code.
|
||||
|
||||
The handler also used `ctx.task.baseCommitSha` as the contamination base,
|
||||
which is deliberately preserved across sessions for diff math (FN-4417)
|
||||
and can lag local `main` by many commits — causing legitimately-merged
|
||||
landings to be classified as foreign at this layer.
|
||||
|
||||
Changes:
|
||||
- `classifyBootstrapMisbinding` now derives the foreign-commit count from
|
||||
its own `git log baseSha..branchName` walk; `input.foreignCommits` is
|
||||
optional and advisory only. The result type gains `foreignCommitCount`.
|
||||
- The `branch-worktree` recovery handler stops passing an empty array and
|
||||
computes a fresh merge-base against local `main` (falling back to
|
||||
`origin/main`), mirroring the executor's primary contamination path.
|
||||
- Regression tests cover both the no-`foreignCommits` call shape and the
|
||||
`foreignCommitCount` field.
|
||||
@@ -60,12 +60,19 @@ describe("BranchWorktreeAutoRecoveryHandler", () => {
|
||||
it("reanchors bootstrap misbinding then requeues", async () => {
|
||||
const f = createFixtures();
|
||||
branchConflictMocks.inspectBranchConflict.mockResolvedValue({ kind: "reclaimable", livePath: "/tmp/wt", tipSha: "abc", taskAttributedCommitCount: 0, strandedCommits: [] });
|
||||
branchConflictMocks.classifyBootstrapMisbinding.mockResolvedValue({ isBootstrapMisbinding: true, ownCommitCount: 0, nonAttributedCount: 0 });
|
||||
branchConflictMocks.classifyBootstrapMisbinding.mockResolvedValue({ isBootstrapMisbinding: true, ownCommitCount: 0, foreignCommitCount: 2, nonAttributedCount: 0 });
|
||||
branchConflictMocks.reanchorBranchToBase.mockResolvedValue({});
|
||||
await f.handler.issueRetry(f.failure, f.decision, f.ctx);
|
||||
expect(branchConflictMocks.reanchorBranchToBase).toHaveBeenCalledTimes(1);
|
||||
expect(f.taskStore.moveTask).toHaveBeenCalledWith("FN-4536", "todo", expect.objectContaining({ moveSource: "engine" }));
|
||||
expect(f.runAudit.database).toHaveBeenCalledWith(expect.objectContaining({ type: "branch-worktree:auto-requeue", metadata: expect.objectContaining({ rationale: "bootstrap-misbinding-reanchor" }) }));
|
||||
|
||||
// Regression: prior to the fix, the handler passed `foreignCommits: []`
|
||||
// to classifyBootstrapMisbinding, which silently disabled the predicate
|
||||
// (foreignCommits.length > 0 was always false) and turned this entire
|
||||
// branch into dead code for the FN-5475-class misbinding.
|
||||
const classifyCall = branchConflictMocks.classifyBootstrapMisbinding.mock.calls[0][0];
|
||||
expect(classifyCall.foreignCommits).toBeUndefined();
|
||||
});
|
||||
|
||||
it("unparks stale paused conflict", async () => {
|
||||
|
||||
@@ -125,10 +125,32 @@ describe("branch contamination recovery classification", () => {
|
||||
expect(result).toEqual({
|
||||
isBootstrapMisbinding: true,
|
||||
ownCommitCount: 0,
|
||||
foreignCommitCount: 1,
|
||||
nonAttributedCount: 0,
|
||||
});
|
||||
}, 20_000);
|
||||
|
||||
// Regression: the auto-recovery fallback in branch-worktree.ts has no
|
||||
// BranchCrossContaminationError in hand (it walks via inspectBranchConflict),
|
||||
// so it cannot supply a foreignCommits array. Before the fix that path
|
||||
// passed `[]` and the predicate became dead code.
|
||||
it("classifies bootstrap misbinding when foreignCommits is omitted by the caller", async () => {
|
||||
const { repoDir, baseSha } = await setupRepo();
|
||||
await makeCommit(repoDir, "foreign-bootstrap-no-list", "feat(FN-4367): dependency change", "FN-4367");
|
||||
|
||||
const result = await classifyBootstrapMisbinding({
|
||||
repoDir,
|
||||
branchName: "feature",
|
||||
baseSha,
|
||||
taskId: "FN-4488",
|
||||
});
|
||||
|
||||
expect(result.isBootstrapMisbinding).toBe(true);
|
||||
expect(result.foreignCommitCount).toBe(1);
|
||||
expect(result.ownCommitCount).toBe(0);
|
||||
expect(result.nonAttributedCount).toBe(0);
|
||||
}, 20_000);
|
||||
|
||||
it("does not classify bootstrap misbinding when an own-task commit exists", async () => {
|
||||
const { repoDir, baseSha } = await setupRepo();
|
||||
const foreign = await makeCommit(repoDir, "foreign-mixed", "feat(FN-4367): dependency change", "FN-4367");
|
||||
@@ -182,6 +204,7 @@ describe("branch contamination recovery classification", () => {
|
||||
expect(result).toEqual({
|
||||
isBootstrapMisbinding: false,
|
||||
ownCommitCount: 0,
|
||||
foreignCommitCount: 0,
|
||||
nonAttributedCount: 0,
|
||||
});
|
||||
});
|
||||
|
||||
@@ -112,6 +112,7 @@ describe("branch cross-contamination recovery (FN-4428/FN-4499)", () => {
|
||||
vi.spyOn(branchConflicts, "classifyBootstrapMisbinding").mockResolvedValueOnce({
|
||||
isBootstrapMisbinding: true,
|
||||
ownCommitCount: 0,
|
||||
foreignCommitCount: 1,
|
||||
nonAttributedCount: 0,
|
||||
});
|
||||
vi.spyOn(branchConflicts, "reanchorBranchToBase").mockResolvedValueOnce({
|
||||
@@ -139,7 +140,7 @@ describe("branch cross-contamination recovery (FN-4428/FN-4499)", () => {
|
||||
});
|
||||
|
||||
mockedCreateFnAgent.mockRejectedValueOnce(contamination);
|
||||
vi.spyOn(branchConflicts, "classifyBootstrapMisbinding").mockResolvedValueOnce({ isBootstrapMisbinding: false, ownCommitCount: 1, nonAttributedCount: 0 });
|
||||
vi.spyOn(branchConflicts, "classifyBootstrapMisbinding").mockResolvedValueOnce({ isBootstrapMisbinding: false, ownCommitCount: 1, foreignCommitCount: 0, nonAttributedCount: 0 });
|
||||
vi.spyOn(branchConflicts, "classifyForeignCommits").mockResolvedValueOnce({ alreadyUpstream: contamination.foreignCommits, unique: [] });
|
||||
const recoverySpy = vi.spyOn(branchConflicts, "autoRecoverCrossContamination").mockResolvedValueOnce({
|
||||
newTipSha: "2222222222222222222222222222222222222222",
|
||||
@@ -173,7 +174,7 @@ describe("branch cross-contamination recovery (FN-4428/FN-4499)", () => {
|
||||
});
|
||||
|
||||
mockedCreateFnAgent.mockRejectedValueOnce(contamination);
|
||||
vi.spyOn(branchConflicts, "classifyBootstrapMisbinding").mockResolvedValueOnce({ isBootstrapMisbinding: false, ownCommitCount: 1, nonAttributedCount: 0 });
|
||||
vi.spyOn(branchConflicts, "classifyBootstrapMisbinding").mockResolvedValueOnce({ isBootstrapMisbinding: false, ownCommitCount: 1, foreignCommitCount: 0, nonAttributedCount: 0 });
|
||||
vi.spyOn(branchConflicts, "classifyForeignCommits").mockResolvedValueOnce({ alreadyUpstream: contamination.foreignCommits, unique: [] });
|
||||
const recoverySpy = vi.spyOn(branchConflicts, "autoRecoverCrossContamination").mockResolvedValueOnce({
|
||||
newTipSha: "2222222222222222222222222222222222222222",
|
||||
@@ -202,7 +203,7 @@ describe("branch cross-contamination recovery (FN-4428/FN-4499)", () => {
|
||||
});
|
||||
|
||||
mockedCreateFnAgent.mockRejectedValueOnce(contamination);
|
||||
vi.spyOn(branchConflicts, "classifyBootstrapMisbinding").mockResolvedValueOnce({ isBootstrapMisbinding: false, ownCommitCount: 1, nonAttributedCount: 0 });
|
||||
vi.spyOn(branchConflicts, "classifyBootstrapMisbinding").mockResolvedValueOnce({ isBootstrapMisbinding: false, ownCommitCount: 1, foreignCommitCount: 0, nonAttributedCount: 0 });
|
||||
vi.spyOn(branchConflicts, "classifyForeignCommits").mockResolvedValueOnce({ alreadyUpstream: [], unique: [misroutedCommit] });
|
||||
vi.spyOn(branchConflicts, "classifyMisroutedForeignCommit").mockResolvedValueOnce({
|
||||
misrouted: true,
|
||||
@@ -237,7 +238,7 @@ describe("branch cross-contamination recovery (FN-4428/FN-4499)", () => {
|
||||
});
|
||||
|
||||
mockedCreateFnAgent.mockRejectedValueOnce(contamination);
|
||||
vi.spyOn(branchConflicts, "classifyBootstrapMisbinding").mockResolvedValueOnce({ isBootstrapMisbinding: false, ownCommitCount: 1, nonAttributedCount: 0 });
|
||||
vi.spyOn(branchConflicts, "classifyBootstrapMisbinding").mockResolvedValueOnce({ isBootstrapMisbinding: false, ownCommitCount: 1, foreignCommitCount: 0, nonAttributedCount: 0 });
|
||||
vi.spyOn(branchConflicts, "classifyForeignCommits").mockResolvedValueOnce({ alreadyUpstream: [], unique: [foreignCommit] });
|
||||
vi.spyOn(branchConflicts, "classifyMisroutedForeignCommit").mockResolvedValueOnce({
|
||||
misrouted: false,
|
||||
@@ -276,7 +277,7 @@ describe("branch cross-contamination recovery (FN-4428/FN-4499)", () => {
|
||||
|
||||
const firstStore = createMockStore();
|
||||
mockedCreateFnAgent.mockRejectedValueOnce(contamination);
|
||||
vi.spyOn(branchConflicts, "classifyBootstrapMisbinding").mockResolvedValueOnce({ isBootstrapMisbinding: false, ownCommitCount: 1, nonAttributedCount: 0 });
|
||||
vi.spyOn(branchConflicts, "classifyBootstrapMisbinding").mockResolvedValueOnce({ isBootstrapMisbinding: false, ownCommitCount: 1, foreignCommitCount: 0, nonAttributedCount: 0 });
|
||||
vi.spyOn(branchConflicts, "classifyForeignCommits").mockResolvedValueOnce({ alreadyUpstream: [upstreamCommit], unique: [misroutedCommit] });
|
||||
vi.spyOn(branchConflicts, "classifyMisroutedForeignCommit").mockResolvedValueOnce({ misrouted: true, foreignTaskId: "FN-5003", paths: [".changeset/fn-5003-fix.md"] });
|
||||
const recoverySpy = vi.spyOn(branchConflicts, "autoRecoverCrossContamination").mockResolvedValueOnce({
|
||||
@@ -290,7 +291,7 @@ describe("branch cross-contamination recovery (FN-4428/FN-4499)", () => {
|
||||
|
||||
const secondStore = createMockStore();
|
||||
mockedCreateFnAgent.mockRejectedValueOnce(contamination);
|
||||
vi.spyOn(branchConflicts, "classifyBootstrapMisbinding").mockResolvedValueOnce({ isBootstrapMisbinding: false, ownCommitCount: 1, nonAttributedCount: 0 });
|
||||
vi.spyOn(branchConflicts, "classifyBootstrapMisbinding").mockResolvedValueOnce({ isBootstrapMisbinding: false, ownCommitCount: 1, foreignCommitCount: 0, nonAttributedCount: 0 });
|
||||
vi.spyOn(branchConflicts, "classifyForeignCommits").mockResolvedValueOnce({ alreadyUpstream: [upstreamCommit], unique: [misroutedCommit] });
|
||||
vi.spyOn(branchConflicts, "classifyMisroutedForeignCommit").mockResolvedValueOnce({ misrouted: true, foreignTaskId: "FN-5003", paths: [".changeset/fn-5003-fix.md"] });
|
||||
|
||||
@@ -309,7 +310,7 @@ describe("branch cross-contamination recovery (FN-4428/FN-4499)", () => {
|
||||
});
|
||||
|
||||
mockedCreateFnAgent.mockRejectedValueOnce(contamination);
|
||||
vi.spyOn(branchConflicts, "classifyBootstrapMisbinding").mockResolvedValueOnce({ isBootstrapMisbinding: true, ownCommitCount: 0, nonAttributedCount: 0 });
|
||||
vi.spyOn(branchConflicts, "classifyBootstrapMisbinding").mockResolvedValueOnce({ isBootstrapMisbinding: true, ownCommitCount: 0, foreignCommitCount: 1, nonAttributedCount: 0 });
|
||||
vi.spyOn(branchConflicts, "reanchorBranchToBase").mockRejectedValueOnce(new Error("reanchor failed"));
|
||||
vi.spyOn(branchConflicts, "classifyForeignCommits").mockResolvedValueOnce({ alreadyUpstream: [], unique: contamination.foreignCommits });
|
||||
|
||||
|
||||
@@ -89,6 +89,27 @@ export class BranchWorktreeAutoRecoveryHandler {
|
||||
return map;
|
||||
}
|
||||
|
||||
/**
|
||||
* Compute a fresh merge-base for contamination checks. Mirrors the
|
||||
* executor's `resolveContaminationBaseRef`: prefer local `main` (the
|
||||
* canonical integration target for Fusion), fall back to `origin/main`
|
||||
* if the local ref isn't resolvable, and finally fall back to the
|
||||
* caller-supplied integration ref if both lookups fail (e.g. in repos
|
||||
* with a non-standard layout).
|
||||
*/
|
||||
private async resolveContaminationBase(worktreePath: string, fallback: string): Promise<string> {
|
||||
try {
|
||||
const out = await this.runGit(
|
||||
worktreePath,
|
||||
"git merge-base HEAD main 2>/dev/null || git merge-base HEAD origin/main",
|
||||
);
|
||||
if (out) return out;
|
||||
} catch {
|
||||
// fall through to fallback
|
||||
}
|
||||
return fallback;
|
||||
}
|
||||
|
||||
private async resolveRepoDir(ctx: AutoRecoveryContext, failure: AutoRecoveryFailure): Promise<string> {
|
||||
const repoFromFailure = typeof failure.evidence?.repoDir === "string" ? failure.evidence.repoDir : undefined;
|
||||
if (repoFromFailure) return repoFromFailure;
|
||||
@@ -199,20 +220,33 @@ export class BranchWorktreeAutoRecoveryHandler {
|
||||
}
|
||||
|
||||
if (inspection.kind === "reclaimable" && inspection.taskAttributedCommitCount === 0) {
|
||||
// Resolve a fresh merge-base against local main (falling back to
|
||||
// origin/main) for the contamination base — `ctx.task.baseCommitSha`
|
||||
// is deliberately preserved across sessions for diff math (FN-4417)
|
||||
// and can lag main by hundreds of commits, which would flag every
|
||||
// legitimate landing as foreign.
|
||||
const misbindingBaseSha = await this.resolveContaminationBase(
|
||||
inspection.livePath,
|
||||
ctx.task.baseCommitSha ?? integrationBranch,
|
||||
);
|
||||
const bootstrap = await classifyBootstrapMisbinding({
|
||||
repoDir,
|
||||
branchName,
|
||||
baseSha: ctx.task.baseCommitSha ?? integrationBranch,
|
||||
baseSha: misbindingBaseSha,
|
||||
taskId: ctx.task.id,
|
||||
foreignCommits: [],
|
||||
}).catch(() => ({ isBootstrapMisbinding: false, ownCommitCount: 0, nonAttributedCount: 0 }));
|
||||
}).catch(() => ({
|
||||
isBootstrapMisbinding: false,
|
||||
ownCommitCount: 0,
|
||||
foreignCommitCount: 0,
|
||||
nonAttributedCount: 0,
|
||||
}));
|
||||
|
||||
if (bootstrap.isBootstrapMisbinding) {
|
||||
const reanchor = await reanchorBranchToBase({
|
||||
repoDir,
|
||||
worktreePath: inspection.livePath,
|
||||
branchName,
|
||||
baseSha: ctx.task.baseCommitSha ?? integrationBranch,
|
||||
baseSha: misbindingBaseSha,
|
||||
taskId: ctx.task.id,
|
||||
}).catch(() => null);
|
||||
|
||||
|
||||
@@ -433,25 +433,34 @@ export interface ClassifyBootstrapMisbindingInput {
|
||||
branchName: string;
|
||||
baseSha: string;
|
||||
taskId: string;
|
||||
foreignCommits: BranchCrossContaminationCommit[];
|
||||
/**
|
||||
* Optional and advisory only. The classifier derives the foreign-commit
|
||||
* count from its own `git log baseSha..branchName` walk because callers
|
||||
* such as the auto-recovery fallback in `branch-worktree.ts` only have a
|
||||
* `BranchConflictInspectionResult` (no foreign-commit list) and used to
|
||||
* pass `[]`, which silently disabled the predicate.
|
||||
*/
|
||||
foreignCommits?: BranchCrossContaminationCommit[];
|
||||
}
|
||||
|
||||
export interface ClassifyBootstrapMisbindingResult {
|
||||
isBootstrapMisbinding: boolean;
|
||||
ownCommitCount: number;
|
||||
foreignCommitCount: number;
|
||||
nonAttributedCount: number;
|
||||
}
|
||||
|
||||
export async function classifyBootstrapMisbinding(
|
||||
input: ClassifyBootstrapMisbindingInput,
|
||||
): Promise<ClassifyBootstrapMisbindingResult> {
|
||||
const { repoDir, branchName, baseSha, taskId, foreignCommits } = input;
|
||||
const { repoDir, branchName, baseSha, taskId } = input;
|
||||
const output = await runGit(repoDir, `git log --format=%H%x1f%s%x1f%b ${quoteShellArg(`${baseSha}..${branchName}`)}`)
|
||||
.catch(() => "");
|
||||
if (!output) {
|
||||
return {
|
||||
isBootstrapMisbinding: false,
|
||||
ownCommitCount: 0,
|
||||
foreignCommitCount: 0,
|
||||
nonAttributedCount: 0,
|
||||
};
|
||||
}
|
||||
@@ -464,6 +473,7 @@ export async function classifyBootstrapMisbinding(
|
||||
|
||||
let ownCommitCount = 0;
|
||||
let nonAttributedCount = 0;
|
||||
let foreignCommitCount = 0;
|
||||
for (const line of output.split("\n").map((entry) => entry.trim()).filter(Boolean)) {
|
||||
const [, subject = "", body = ""] = line.split("\u001f");
|
||||
if (ownSubjectPattern.test(subject) || ownTrailerPattern.test(body)) {
|
||||
@@ -476,12 +486,15 @@ export async function classifyBootstrapMisbinding(
|
||||
const attributedTaskId = (trailerMatch?.[1] ?? subjectMatch?.[2] ?? "").toUpperCase();
|
||||
if (!attributedTaskId) {
|
||||
nonAttributedCount += 1;
|
||||
} else {
|
||||
foreignCommitCount += 1;
|
||||
}
|
||||
}
|
||||
|
||||
return {
|
||||
isBootstrapMisbinding: foreignCommits.length > 0 && ownCommitCount === 0 && nonAttributedCount === 0,
|
||||
isBootstrapMisbinding: foreignCommitCount > 0 && ownCommitCount === 0 && nonAttributedCount === 0,
|
||||
ownCommitCount,
|
||||
foreignCommitCount,
|
||||
nonAttributedCount,
|
||||
};
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user