fix(merger): prevent tasks landing in Done with no commit on main
Two root-cause fixes for the "fake done" patterns surfaced while debugging FN-5475's stuck preflight (it depended on FN-5233, which the board reported as Done but whose squash had stranded on a sibling fusion/fn-* branch). 1. resolveTaskMergeTarget rejects fusion/fn-* sibling branches as a merge destination — when a task's baseBranch was inherited from a sibling/dependent dispatch, the merger detached onto and squashed against that branch instead of advancing main. New audit event surfaces the steering miss so the underlying baseBranch-propagation bug stays observable. 2. self-healing findLandedTaskCommit verifies ownership against each grep candidate's body before attribution. The previous code blindly accepted the first hit of `git log --grep=FN-XXXX` (which matches the entire commit message); FN-5441 and FN-5446 were both marked done against an unrelated FN-5483 commit whose body merely mentioned them in prose. commitOwnedByTask is also tightened: trailers must be line-anchored and the subject fallback must match conventional-commit form, not a bare substring. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
35
.changeset/fix-merger-fake-done-sibling-branch.md
Normal file
35
.changeset/fix-merger-fake-done-sibling-branch.md
Normal file
@@ -0,0 +1,35 @@
|
||||
---
|
||||
"@fusion/core": patch
|
||||
"@fusion/engine": patch
|
||||
---
|
||||
|
||||
fix(merger): two root-cause fixes for tasks landing in Done with no commit on main
|
||||
|
||||
**Bug 1: sibling fusion/fn-\* branch as merge target** — `resolveTaskMergeTarget`
|
||||
previously returned `task.baseBranch` unconditionally before falling back to the
|
||||
project default. When a task was dispatched as a sibling/dependent off another
|
||||
in-flight task's worktree, `baseBranch` ended up as the upstream's
|
||||
`fusion/fn-<id>` branch. The merger then detached onto that sibling, squashed
|
||||
on top of it, and advanced `refs/heads/fusion/fn-<id>` — never main. FN-5233's
|
||||
squash (`84563e549`) stranded on `fusion/fn-5339`; FN-5530's
|
||||
(`4140a3e0a`) stranded on `fusion/fn-5543`. The resolver now refuses any
|
||||
`fusion/fn-\*` candidate as a merge destination and falls through to the
|
||||
project default. The merger emits a new `merge:merge-target-rejected-fusion-sibling`
|
||||
audit event so the upstream `baseBranch`-propagation bug stays observable.
|
||||
|
||||
**Bug 2: deadlock-recovery mis-attributed tasks to unrelated commits** —
|
||||
`findLandedTaskCommit` step (4) used `git log --grep=FN-XXXX` which matches the
|
||||
entire commit message (not just the subject) and blindly accepted the first
|
||||
hit. FN-5441 and FN-5446 were both marked done against `e3dbfaae` — an
|
||||
FN-5483 commit whose body merely *mentioned* them by name in a paragraph about
|
||||
a refusal. The grep fallback now fetches each candidate's body and re-verifies
|
||||
ownership via a tightened `commitOwnedByTask`: trailers must be line-anchored
|
||||
(`(?:^|\n)Fusion-Task-Id: <id>(?:\n|$)`), and the subject fallback must match
|
||||
a conventional-commit form (`<type>(<id>):` or `<id>:`), not a substring.
|
||||
Prose mentions can no longer claim a task.
|
||||
|
||||
The historical recovery for FN-5233 has been cherry-picked to main as
|
||||
`2d2e5b809`. The other 11 affected tasks (FN-5441, FN-5446, FN-5472, FN-5484,
|
||||
FN-5487, FN-5490, FN-5515, FN-5517, FN-5526, FN-5539, FN-5540, FN-5542)
|
||||
remain in Done but need separate triage — 3 look like legitimate
|
||||
verification-only no-ops, the remaining 9 likely lost real work.
|
||||
@@ -79,6 +79,55 @@ describe("resolveTaskMergeTarget", () => {
|
||||
source: "legacy-main",
|
||||
});
|
||||
});
|
||||
|
||||
// Regression for FN-5233/FN-5530: when a sibling-dispatched task inherits
|
||||
// `baseBranch = fusion/fn-<id>`, the merger must NOT use that as the squash
|
||||
// destination — otherwise the commit lands on the sibling branch and is
|
||||
// lost from main. Falls through to projectDefault, and reports the rejection.
|
||||
it("rejects task baseBranch when it points at a sibling fusion/fn-* branch", () => {
|
||||
const result = resolveTaskMergeTarget(
|
||||
{ baseBranch: "fusion/fn-5339", branchContext: undefined },
|
||||
{ projectDefaultBranch: "main" },
|
||||
);
|
||||
expect(result.branch).toBe("main");
|
||||
expect(result.source).toBe("project-default");
|
||||
expect(result.rejected).toEqual({
|
||||
branch: "fusion/fn-5339",
|
||||
source: "task-base-branch",
|
||||
reason: "fusion-sibling-branch",
|
||||
});
|
||||
});
|
||||
|
||||
it("rejects inherited branch context that points at a sibling fusion/fn-* branch", () => {
|
||||
const result = resolveTaskMergeTarget(
|
||||
{
|
||||
baseBranch: undefined,
|
||||
branchContext: {
|
||||
groupId: "G-1",
|
||||
source: "planning",
|
||||
assignmentMode: "shared",
|
||||
inheritedBaseBranch: "FUSION/FN-1234",
|
||||
},
|
||||
},
|
||||
{ projectDefaultBranch: "main" },
|
||||
);
|
||||
expect(result.branch).toBe("main");
|
||||
expect(result.source).toBe("project-default");
|
||||
expect(result.rejected).toEqual({
|
||||
branch: "FUSION/FN-1234",
|
||||
source: "task-branch-context",
|
||||
reason: "fusion-sibling-branch",
|
||||
});
|
||||
});
|
||||
|
||||
it("does not reject non-fusion branches that happen to share a prefix", () => {
|
||||
// `fusion/release-1.0` is a legitimate human-chosen base; only the
|
||||
// canonical `fusion/fn-<id>` pattern is a sibling-task marker.
|
||||
expect(resolveTaskMergeTarget({ baseBranch: "fusion/release-1.0", branchContext: undefined })).toEqual({
|
||||
branch: "fusion/release-1.0",
|
||||
source: "task-base-branch",
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("getTaskMergeBlocker", () => {
|
||||
|
||||
@@ -3,6 +3,13 @@ import type { Task, WorkflowStepResult } from "./types.js";
|
||||
export interface MergeTargetResolution {
|
||||
branch: string;
|
||||
source: "task-base-branch" | "task-branch-context" | "project-default" | "legacy-main";
|
||||
/**
|
||||
* When the resolver rejects a candidate (e.g. baseBranch points at a sibling
|
||||
* `fusion/fn-*` branch), this records the rejected value and the reason. The
|
||||
* merger uses this to emit an audit event so the steering bug is observable
|
||||
* in the run-audit timeline rather than failing silently.
|
||||
*/
|
||||
rejected?: { branch: string; source: "task-base-branch" | "task-branch-context"; reason: "fusion-sibling-branch" };
|
||||
}
|
||||
|
||||
export interface MergeTargetResolverOptions {
|
||||
@@ -10,27 +17,49 @@ export interface MergeTargetResolverOptions {
|
||||
legacyFallbackBranch?: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* Sibling task branches (`fusion/fn-<id>`) MUST NOT be used as merge targets.
|
||||
* They are start-point/rebase anchors, not destinations: landing a squash onto
|
||||
* a sibling branch strands the commit on a feature ref instead of advancing
|
||||
* the project integration branch (root cause of FN-5233/FN-5530 lost-on-main).
|
||||
*/
|
||||
const FUSION_SIBLING_BRANCH_RE = /^fusion\/fn-/i;
|
||||
|
||||
function isFusionSiblingBranch(branch: string): boolean {
|
||||
return FUSION_SIBLING_BRANCH_RE.test(branch);
|
||||
}
|
||||
|
||||
export function resolveTaskMergeTarget(
|
||||
task: Pick<Task, "baseBranch" | "branchContext">,
|
||||
options: MergeTargetResolverOptions = {},
|
||||
): MergeTargetResolution {
|
||||
let rejected: MergeTargetResolution["rejected"];
|
||||
|
||||
const configuredBase = task.baseBranch?.trim();
|
||||
if (configuredBase) {
|
||||
return { branch: configuredBase, source: "task-base-branch" };
|
||||
if (isFusionSiblingBranch(configuredBase)) {
|
||||
rejected = { branch: configuredBase, source: "task-base-branch", reason: "fusion-sibling-branch" };
|
||||
} else {
|
||||
return { branch: configuredBase, source: "task-base-branch" };
|
||||
}
|
||||
}
|
||||
|
||||
const inheritedBase = task.branchContext?.inheritedBaseBranch?.trim();
|
||||
if (inheritedBase) {
|
||||
return { branch: inheritedBase, source: "task-branch-context" };
|
||||
if (isFusionSiblingBranch(inheritedBase)) {
|
||||
rejected = rejected ?? { branch: inheritedBase, source: "task-branch-context", reason: "fusion-sibling-branch" };
|
||||
} else {
|
||||
return { branch: inheritedBase, source: "task-branch-context", rejected };
|
||||
}
|
||||
}
|
||||
|
||||
const projectDefault = options.projectDefaultBranch?.trim();
|
||||
if (projectDefault) {
|
||||
return { branch: projectDefault, source: "project-default" };
|
||||
return { branch: projectDefault, source: "project-default", rejected };
|
||||
}
|
||||
|
||||
const legacyFallback = options.legacyFallbackBranch?.trim() || "main";
|
||||
return { branch: legacyFallback, source: "legacy-main" };
|
||||
return { branch: legacyFallback, source: "legacy-main", rejected };
|
||||
}
|
||||
|
||||
export const HARD_BLOCKING_TASK_STATUSES = new Set([
|
||||
|
||||
@@ -3820,6 +3820,9 @@ describe("SelfHealingManager", () => {
|
||||
mockedExecSync.mockImplementation((command: string | Buffer) => {
|
||||
const cmd = String(command);
|
||||
if (cmd.includes("Fusion-Task-Id: FN-stuck")) return "abc12345\x1fRecovered subject\n" as any;
|
||||
// FN-5441 ownership verification: post-grep body fetch must contain
|
||||
// the anchored trailer so commitOwnedByTask accepts the candidate.
|
||||
if (cmd.includes("--format=%b") && cmd.includes("abc12345")) return "Fusion-Task-Id: FN-stuck\n" as any;
|
||||
if (cmd.includes("--shortstat")) return " 2 files changed, 3 insertions(+), 1 deletions(-)\n" as any;
|
||||
return "" as any;
|
||||
});
|
||||
@@ -3958,6 +3961,50 @@ describe("SelfHealingManager", () => {
|
||||
managerWithRecovery.stop();
|
||||
});
|
||||
|
||||
// FN-5441/FN-5446 regression: a deadlock-recovery sweep mis-attributed
|
||||
// both to e3dbfaae, an FN-5483 commit whose body merely *mentioned* them
|
||||
// by name. findLandedTaskCommit step (4) used `git log --grep=FN-XXXX`
|
||||
// which matches the entire commit message (not just subject) and the
|
||||
// previous code blindly accepted the first hit. The fix anchors ownership
|
||||
// on trailer/subject so prose mentions can never claim a task.
|
||||
it("FN-5441/FN-5446: does not attribute to a commit that only mentions the task ID in prose", async () => {
|
||||
const managerWithRecovery = new SelfHealingManager(store, { rootDir: "/tmp/test-project" });
|
||||
(store.getSettings as ReturnType<typeof vi.fn>).mockResolvedValue(baseSettings);
|
||||
(store.listTasks as ReturnType<typeof vi.fn>)
|
||||
.mockResolvedValueOnce([
|
||||
{ id: "FN-5441", column: "in-review", paused: false, status: "failed", mergeRetries: 3, mergeDetails: undefined, worktree: "/tmp/wt-a", log: [] },
|
||||
])
|
||||
.mockResolvedValueOnce([])
|
||||
.mockResolvedValueOnce([])
|
||||
.mockResolvedValueOnce([]);
|
||||
mockedExecSync.mockImplementation((command: string | Buffer) => {
|
||||
const cmd = String(command);
|
||||
// grep step finds the unrelated FN-5483 commit whose body mentions FN-5441 in prose
|
||||
if (cmd.includes("FN-5441") && cmd.includes("--grep")) return "e3dbfaae\x1ffix(FN-5483): allow merger commits past identity-guard\n" as any;
|
||||
// ownership-verification body fetch returns prose-mention body, no anchored trailer
|
||||
if (cmd.includes("--format=%b") && cmd.includes("e3dbfaae")) {
|
||||
return "The refusal surfaced as merge-deadlock-detected on FN-5441 and FN-5446. ...\n" as any;
|
||||
}
|
||||
return "" as any;
|
||||
});
|
||||
|
||||
const result = await managerWithRecovery.recoverStuckMergeDeadlocks();
|
||||
|
||||
// No attribution → no recovery → no move to done.
|
||||
expect(store.moveTask).not.toHaveBeenCalledWith("FN-5441", "done");
|
||||
// result of 0 OR a "paused-for-manual" path (proof gate) is acceptable;
|
||||
// the load-bearing assertion is that we did NOT advance the task to done
|
||||
// against the wrong commit.
|
||||
expect(result).toBeLessThanOrEqual(1);
|
||||
const updateCalls = (store.updateTask as ReturnType<typeof vi.fn>).mock.calls;
|
||||
const movedToDone = updateCalls.some(([id, patch]) =>
|
||||
id === "FN-5441" && (patch as any)?.mergeDetails?.commitSha === "e3dbfaae",
|
||||
);
|
||||
expect(movedToDone).toBe(false);
|
||||
|
||||
managerWithRecovery.stop();
|
||||
});
|
||||
|
||||
it("recovers worktree-only orphans and reproduces three-task incident", async () => {
|
||||
const managerWithRecovery = new SelfHealingManager(store, { rootDir: "/tmp/test-project" });
|
||||
(store.getSettings as ReturnType<typeof vi.fn>).mockResolvedValue(baseSettings);
|
||||
@@ -3975,6 +4022,11 @@ describe("SelfHealingManager", () => {
|
||||
if (cmd.includes("Fusion-Task-Id: FN-3794")) return "278a2825\x1fone\n" as any;
|
||||
if (cmd.includes("Fusion-Task-Id: FN-3814")) return "69c25e2b\x1ftwo\n" as any;
|
||||
if (cmd.includes("Fusion-Task-Id: FN-3829")) return "0d3f51b6\x1fthree\n" as any;
|
||||
// FN-5441 ownership verification: post-grep body fetch must contain
|
||||
// the anchored trailer so commitOwnedByTask accepts each candidate.
|
||||
if (cmd.includes("--format=%b") && cmd.includes("278a2825")) return "Fusion-Task-Id: FN-3794\n" as any;
|
||||
if (cmd.includes("--format=%b") && cmd.includes("69c25e2b")) return "Fusion-Task-Id: FN-3814\n" as any;
|
||||
if (cmd.includes("--format=%b") && cmd.includes("0d3f51b6")) return "Fusion-Task-Id: FN-3829\n" as any;
|
||||
return "" as any;
|
||||
});
|
||||
|
||||
|
||||
@@ -7297,6 +7297,31 @@ export async function aiMergeTask(
|
||||
const mergeTarget = resolveTaskMergeTarget(task, {
|
||||
projectDefaultBranch: resolvedIntegrationBranch,
|
||||
});
|
||||
if (mergeTarget.rejected) {
|
||||
// FN-5233/FN-5530 regression: the task's baseBranch/inheritedBaseBranch
|
||||
// pointed at a sibling fusion/fn-* branch. The resolver fell through to
|
||||
// projectDefault, but we surface the steering miss in the audit timeline
|
||||
// so the underlying baseBranch-propagation bug stays observable.
|
||||
mergerLog.warn(
|
||||
`${taskId}: merge target rejected (${mergeTarget.rejected.reason}): ${mergeTarget.rejected.source}=${mergeTarget.rejected.branch} → using ${mergeTarget.branch}`,
|
||||
);
|
||||
try {
|
||||
await (store as any).recordRunAuditEvent?.({
|
||||
domain: "git",
|
||||
mutationType: "merge:merge-target-rejected-fusion-sibling",
|
||||
target: taskId,
|
||||
metadata: {
|
||||
rejectedBranch: mergeTarget.rejected.branch,
|
||||
rejectedSource: mergeTarget.rejected.source,
|
||||
reason: mergeTarget.rejected.reason,
|
||||
fallbackBranch: mergeTarget.branch,
|
||||
fallbackSource: mergeTarget.source,
|
||||
},
|
||||
});
|
||||
} catch {
|
||||
// best-effort audit; never block the merge on telemetry
|
||||
}
|
||||
}
|
||||
const integrationBranch = resolvedIntegrationBranch;
|
||||
let branch = task.branch || canonicalFusionBranchName(taskId);
|
||||
|
||||
|
||||
@@ -427,11 +427,37 @@ interface LandedTaskCommit {
|
||||
rebaseBaseSha?: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* Decide whether a git commit belongs to a given task.
|
||||
*
|
||||
* Ownership is line-anchored and subject-anchored: it is NOT sufficient for the
|
||||
* task ID to appear in prose. FN-5441/FN-5446 regression — both were
|
||||
* mis-attributed to e3dbfaae (an FN-5483 commit whose body merely *mentioned*
|
||||
* them by name) because the previous `subject.includes(taskId)` check matched
|
||||
* any substring anywhere in the subject.
|
||||
*
|
||||
* Accept (any of):
|
||||
* - `Fusion-Task-Lineage: <lineageId>` as a complete trailer line in the body
|
||||
* - `Fusion-Task-Id: <taskId>` as a complete trailer line in the body
|
||||
* - Subject anchored on the task ID in conventional-commit form:
|
||||
* `<type>(<taskId>): …` or `<taskId>: …` or `<type>(<taskId>/...): …`
|
||||
*/
|
||||
function commitOwnedByTask(taskId: string, lineageId: string | undefined, subject: string, body: string): boolean {
|
||||
if (lineageId && body.includes(`Fusion-Task-Lineage: ${lineageId}`)) {
|
||||
if (lineageId && new RegExp(`(?:^|\\n)Fusion-Task-Lineage: ${escapeRegex(lineageId)}\\s*(?:\\n|$)`).test(body)) {
|
||||
return true;
|
||||
}
|
||||
return body.includes(`Fusion-Task-Id: ${taskId}`) || subject.includes(taskId);
|
||||
if (new RegExp(`(?:^|\\n)Fusion-Task-Id: ${escapeRegex(taskId)}\\s*(?:\\n|$)`).test(body)) {
|
||||
return true;
|
||||
}
|
||||
// Subject anchor: `<scope>(<taskId>...): …` or `<taskId>: …` at start.
|
||||
const subjectAnchor = new RegExp(
|
||||
`^(?:[A-Za-z]+(?:\\([^)]*\\b${escapeRegex(taskId)}\\b[^)]*\\))?:|${escapeRegex(taskId)}:)`,
|
||||
);
|
||||
return subjectAnchor.test(subject);
|
||||
}
|
||||
|
||||
function escapeRegex(value: string): string {
|
||||
return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
|
||||
}
|
||||
|
||||
function shellQuote(value: string): string {
|
||||
@@ -1217,10 +1243,32 @@ export class SelfHealingManager {
|
||||
stdout = await search(shellQuote(task.id), true);
|
||||
}
|
||||
|
||||
const firstLine = stdout.trim().split("\n").find(Boolean);
|
||||
if (!firstLine) return null;
|
||||
|
||||
const [sha, subject] = firstLine.split("\x1f");
|
||||
// FN-5441/FN-5446 regression: `git log --grep=FN-XXXX` matches the entire
|
||||
// commit message, including prose body mentions. The previous code blindly
|
||||
// accepted the first match — which is how FN-5441/5446 got attributed to
|
||||
// an unrelated FN-5483 commit that *mentioned* them by name. Walk the
|
||||
// candidates and accept only the first one that actually owns the task
|
||||
// (anchored lineage/id trailer or subject-anchored conventional commit).
|
||||
const candidateLines = stdout.trim().split("\n").filter(Boolean);
|
||||
let sha = "";
|
||||
let subject = "";
|
||||
for (const line of candidateLines) {
|
||||
const [candidateSha, candidateSubject = ""] = line.split("\x1f");
|
||||
if (!candidateSha) continue;
|
||||
try {
|
||||
const { stdout: bodyOut } = await execAsync(
|
||||
`git log -1 --format=%b ${shellQuote(candidateSha)}`,
|
||||
{ cwd: this.options.rootDir, maxBuffer: 1024 * 1024 },
|
||||
);
|
||||
if (commitOwnedByTask(task.id, task.lineageId, candidateSubject, bodyOut)) {
|
||||
sha = candidateSha;
|
||||
subject = candidateSubject;
|
||||
break;
|
||||
}
|
||||
} catch {
|
||||
// If we can't read the body, conservatively skip this candidate.
|
||||
}
|
||||
}
|
||||
if (!sha) return null;
|
||||
|
||||
const commit: LandedTaskCommit = { sha, subject, rebaseBaseSha };
|
||||
|
||||
Reference in New Issue
Block a user