From cad44b1f56a1b4557405f688191f9777311dccc0 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 3 Jun 2026 09:40:30 -0700 Subject: [PATCH] fix(FN-5846): commit-ownership-anchor already-merged attribution (U3) Audit of all shared-member merge + self-healing finalize paths: routing, merger finalize-success, and the 6 self-healing recovery paths were already group-branch-safe (FN-5846). Found a residual of the 2026-05-23 lost-work incident bug #2: already-merged-detector's ancestry strategy used bare git log --grep first-hit, and the ownership regex made the conventional scope optional (bare 'feat:' matched). Anchor attribution on trailers or task-scoped subject; scan candidates instead of accepting the first grep hit. Adds real-git characterization tests. --- .../fn-5846-shared-group-merge-routing.md | 2 +- .../already-merged-detector.real-git.test.ts | 143 ++++++++++++++++++ .../branch-group-merge-routing.test.ts | 55 +++++++ .../engine/src/__tests__/self-healing.test.ts | 11 ++ .../engine/src/already-merged-detector.ts | 66 +++++++- packages/engine/src/self-healing.ts | 8 +- 6 files changed, 276 insertions(+), 9 deletions(-) create mode 100644 packages/engine/src/__tests__/already-merged-detector.real-git.test.ts diff --git a/.changeset/fn-5846-shared-group-merge-routing.md b/.changeset/fn-5846-shared-group-merge-routing.md index 4efc18ddc0..65aa0efd9a 100644 --- a/.changeset/fn-5846-shared-group-merge-routing.md +++ b/.changeset/fn-5846-shared-group-merge-routing.md @@ -2,4 +2,4 @@ "@runfusion/fusion": patch --- -Fix shared-branch-group member finalization so routed members land on the group's shared branch instead of being auto-finalized against the project default branch. +Fix shared-branch-group member finalization so routed members land on the group's shared branch instead of being auto-finalized against the project default branch. Also harden already-landed commit attribution so the recovery detector never claims a commit that merely mentions a task ID in prose (2026-05-23 lost-work regression): the `git log --grep` ancestry fallback is now ownership-anchored on a Fusion trailer or a task-scoped conventional-commit subject. diff --git a/packages/engine/src/__tests__/already-merged-detector.real-git.test.ts b/packages/engine/src/__tests__/already-merged-detector.real-git.test.ts new file mode 100644 index 0000000000..fa8289aca0 --- /dev/null +++ b/packages/engine/src/__tests__/already-merged-detector.real-git.test.ts @@ -0,0 +1,143 @@ +// Real-git characterization of findAlreadyMergedTaskCommit's ownership +// anchoring. These tests pin the 2026-05-23 lost-work incident's bug #2: +// the detector must NOT attribute a task to a commit that merely *mentions* +// the task ID in prose (the historical `git log --grep` first-hit bug). +import { afterEach, describe, expect, it } from "vitest"; +import { execSync, spawnSync } from "node:child_process"; +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +import { findAlreadyMergedTaskCommit } from "../already-merged-detector.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("findAlreadyMergedTaskCommit ownership anchoring (real git)", () => { + const repos: string[] = []; + + afterEach(() => { + for (const repo of repos.splice(0)) { + rmSync(repo, { recursive: true, force: true }); + } + }); + + function setupRepo(): string { + const repo = mkdtempSync(path.join(os.tmpdir(), "fn-amd-")); + 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"'); + git(repo, "git commit --allow-empty -m 'init'"); + return repo; + } + + it("attributes via trailer when the owned commit carries Fusion-Task-Id", async () => { + const repo = setupRepo(); + mkdirSync(path.join(repo, "src"), { recursive: true }); + writeFileSync(path.join(repo, "src", "owned.txt"), "owned\n", "utf-8"); + git(repo, "git add src/owned.txt && git commit -m 'feat: landed work' -m 'Fusion-Task-Id: FN-AMD-1'"); + const landedSha = git(repo, "git rev-parse HEAD"); + + const result = await findAlreadyMergedTaskCommit({ + taskId: "FN-AMD-1", + repoDir: repo, + baseBranch: "main", + }); + + expect(result).not.toBeNull(); + expect(result!.sha).toBe(landedSha); + expect(result!.strategy).toBe("trailer"); + }); + + it("attributes via lineage trailer when present", async () => { + const repo = setupRepo(); + mkdirSync(path.join(repo, "src"), { recursive: true }); + writeFileSync(path.join(repo, "src", "lineage.txt"), "lineage\n", "utf-8"); + git(repo, "git add src/lineage.txt && git commit -m 'feat: lineage work' -m 'Fusion-Task-Lineage: LINEAGE-XYZ'"); + const landedSha = git(repo, "git rev-parse HEAD"); + + const result = await findAlreadyMergedTaskCommit({ + taskId: "FN-AMD-LIN", + lineageId: "LINEAGE-XYZ", + repoDir: repo, + baseBranch: "main", + }); + + expect(result).not.toBeNull(); + expect(result!.sha).toBe(landedSha); + expect(result!.strategy).toBe("trailer"); + }); + + // Incident bug #2 regression: a commit that merely *mentions* the task ID in + // its prose body (no anchored trailer) must NOT be attributed to the task, + // even when the task's own branch tip is already an ancestor of base. The + // ancestry `git log --grep=` strategy historically accepted the first + // such prose-mention hit and stranded/mis-attributed work. + it("does NOT attribute to a commit that only mentions the task ID in prose (ancestry path)", async () => { + const repo = setupRepo(); + + // An unrelated commit whose BODY mentions FN-AMD-2 in prose only. + mkdirSync(path.join(repo, "src"), { recursive: true }); + writeFileSync(path.join(repo, "src", "unrelated.txt"), "unrelated\n", "utf-8"); + git( + repo, + "git add src/unrelated.txt && git commit -m 'feat: unrelated change' -m 'This also touches things related to FN-AMD-2 in passing.'", + ); + + // The task's own branch landed by being merged into main, but its commits + // carry NO trailer and NO conventional-subject anchor — only a generic + // message — so the only `--grep=FN-AMD-2` hit is the prose-mention above. + git(repo, "git checkout -b fusion/fn-amd-2"); + writeFileSync(path.join(repo, "src", "task.txt"), "task work\n", "utf-8"); + git(repo, "git add src/task.txt && git commit -m 'wip: generic message with no anchor'"); + git(repo, "git checkout main"); + git(repo, "git merge --no-ff --no-edit fusion/fn-amd-2 -m 'merge generic branch'"); + + const result = await findAlreadyMergedTaskCommit({ + taskId: "FN-AMD-2", + repoDir: repo, + baseBranch: "main", + taskBranch: "fusion/fn-amd-2", + }); + + // It may legitimately attribute via patch-id/tree-equal to the REAL owned + // content, but it must NEVER return the unrelated prose-mention commit. + if (result && result.strategy === "ancestry") { + const subject = git(repo, `git show -s --format=%s ${result.sha}`); + const body = git(repo, `git show -s --format=%b ${result.sha}`); + const ownedBySubject = /^(?:[A-Za-z]+\([^)]*FN-AMD-2[^)]*\):|FN-AMD-2:)/.test(subject); + const ownedByTrailer = /(?:^|\n)Fusion-Task-Id: FN-AMD-2\s*(?:\n|$)/.test(body); + expect(ownedBySubject || ownedByTrailer).toBe(true); + } + }); + + it("attributes via ancestry when the landed commit carries a conventional-subject anchor", async () => { + const repo = setupRepo(); + + // The merge into main carries a conventional subject anchored on the task + // ID; ancestry attribution should accept it (it is genuinely owned). + git(repo, "git checkout -b fusion/fn-amd-3"); + mkdirSync(path.join(repo, "src"), { recursive: true }); + writeFileSync(path.join(repo, "src", "anchored.txt"), "anchored\n", "utf-8"); + git(repo, "git add src/anchored.txt && git commit -m 'feat(FN-AMD-3): real anchored work'"); + git(repo, "git checkout main"); + git(repo, "git merge --ff-only fusion/fn-amd-3"); + + const result = await findAlreadyMergedTaskCommit({ + taskId: "FN-AMD-3", + repoDir: repo, + baseBranch: "main", + taskBranch: "fusion/fn-amd-3", + }); + + expect(result).not.toBeNull(); + // Trailer path won't match (no trailer); ownership-anchored ancestry should. + const subject = git(repo, `git show -s --format=%s ${result!.sha}`); + expect(subject).toContain("FN-AMD-3"); + }); +}); diff --git a/packages/engine/src/__tests__/reliability-interactions/branch-group-merge-routing.test.ts b/packages/engine/src/__tests__/reliability-interactions/branch-group-merge-routing.test.ts index cc11e8ab7d..fe13539c95 100644 --- a/packages/engine/src/__tests__/reliability-interactions/branch-group-merge-routing.test.ts +++ b/packages/engine/src/__tests__/reliability-interactions/branch-group-merge-routing.test.ts @@ -74,6 +74,61 @@ describe("FN-5782 reliability interactions: branch group merge routing", () => { } }, 30_000); + it.skipIf(!hasGit)("routes a shared member to the group branch even when it inherited a sibling fusion/fn-* baseBranch (lost-work regression)", async () => { + const fixture = await makeReliabilityFixture({ taskId: "FN-5782-RI-SIBLING", settings: { testMode: true } as any }); + + try { + const { rootDir, store, task } = fixture; + await stageMergeBranch(store, rootDir, task.id, "fn5782SiblingInherit"); + + const group = store.createBranchGroup({ + sourceType: "planning", + sourceId: "PS-FN5782-SIBLING", + branchName: "fusion/groups/fn-5782-sibling", + }); + await store.setTaskBranchGroup(task.id, group.id); + + // 2026-05-23 lost-work shape: a shared member inherited a sibling + // `fusion/fn-*` branch as its base/inherited base (propagated from a + // sibling-dispatched parent). The resolver MUST still land it on the + // group branch, never on the sibling, and never on main. + await store.updateTask(task.id, { + baseBranch: "fusion/fn-9999-sibling-parent", + branchContext: { + groupId: group.id, + source: "planning", + assignmentMode: "shared", + inheritedBaseBranch: "fusion/fn-9999-sibling-parent", + }, + } as any); + + const auditSpy = vi.spyOn(store as any, "recordRunAuditEvent"); + const result = await aiMergeTask(store, rootDir, task.id); + expect(result.merged).toBe(true); + + // Landed on the group branch; NOT on the sibling, NOT on main. + expect(git(rootDir, `git show ${group.branchName}:packages/engine/src/fn5782SiblingInherit.ts`)).toContain("fn5782SiblingInherit"); + expect(() => git(rootDir, "git show main:packages/engine/src/fn5782SiblingInherit.ts")).toThrow(); + expect(() => git(rootDir, "git show fusion/fn-9999-sibling-parent:packages/engine/src/fn5782SiblingInherit.ts")).toThrow(); + + const recovered = await store.getTask(task.id); + expect(recovered?.mergeDetails?.mergeTargetSource).toBe("branch-group-integration"); + expect(recovered?.mergeDetails?.mergeTargetBranch).toBe(group.branchName); + + expect(auditSpy).toHaveBeenCalledWith(expect.objectContaining({ + domain: "git", + mutationType: "merge:branch-group-routed", + target: task.id, + metadata: expect.objectContaining({ + mergeTargetBranch: group.branchName, + mergeTargetSource: "branch-group-integration", + }), + })); + } finally { + await fixture.cleanup(); + } + }, 45_000); + it.skipIf(!hasGit)("records shared-member landing even when autoMerge is false", async () => { const fixture = await makeReliabilityFixture({ taskId: "FN-5819-RI-AUTO-OFF", diff --git a/packages/engine/src/__tests__/self-healing.test.ts b/packages/engine/src/__tests__/self-healing.test.ts index 18e547aa53..9e8f86b31c 100644 --- a/packages/engine/src/__tests__/self-healing.test.ts +++ b/packages/engine/src/__tests__/self-healing.test.ts @@ -2772,6 +2772,12 @@ describe("SelfHealingManager", () => { if (cmd.includes("Fusion-Task-Id: FN-2900")) { return "trailerSha123feat: ship something opaque\n" as any; } + // Ownership-verification body fetch (FN-5441/5446): the real commit + // located via trailer grep carries the anchored trailer in its body, + // so commitOwnedByTask accepts it though the subject lacks the task ID. + if (cmd.includes("--format=%b") && cmd.includes("trailerSha123")) { + return "Fusion-Task-Id: FN-2900\n" as any; + } if (cmd.includes("--fixed-strings")) return "" as any; } if (cmd.includes("git show --shortstat")) { @@ -2829,6 +2835,11 @@ describe("SelfHealingManager", () => { if (cmd.includes("git log") && cmd.includes("Fusion-Task-Id: FN-2901")) { return "rangeSha901\u001ffeat: ship something opaque\n" as any; } + // Ownership-verification body fetch (FN-5441/5446): real trailer-grep + // hit carries the anchored trailer in its body. + if (cmd.includes("git log") && cmd.includes("--format=%b") && cmd.includes("rangeSha901")) { + return "Fusion-Task-Id: FN-2901\n" as any; + } if (cmd.includes("git diff --shortstat") && cmd.includes("rebasebase901..rangeSha901")) { return " 4 files changed, 104 insertions(+), 1 deletion(-)\n" as any; } diff --git a/packages/engine/src/already-merged-detector.ts b/packages/engine/src/already-merged-detector.ts index e47127f815..b222688715 100644 --- a/packages/engine/src/already-merged-detector.ts +++ b/packages/engine/src/already-merged-detector.ts @@ -34,6 +34,49 @@ function shellQuote(value: string): string { return `'${value.replace(/'/g, "'\\''")}'`; } +function escapeRegex(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +/** + * Ownership anchor shared with self-healing's `commitOwnedByTask`. + * + * The 2026-05-23 lost-work incident (bug #2) was a `git log --grep=` + * first-hit attribution: a commit whose body merely *mentioned* a task ID in + * prose was accepted as that task's landed commit, stranding/mis-attributing + * the real work. The trailer strategies above are already anchored; the + * ancestry strategy below uses a loose `--grep=`, so its candidate must + * be ownership-verified here before it is accepted. + * + * Accept when ANY of: + * - `Fusion-Task-Lineage: ` is a complete trailer line in the body + * - `Fusion-Task-Id: ` is a complete trailer line in the body + * - the subject is anchored on the task ID in conventional-commit form: + * `(...): …` or `: …` + */ +function commitOwnedByTask( + taskId: string, + lineageId: string | undefined, + subject: string, + body: string, +): boolean { + if (lineageId && new RegExp(`(?:^|\\n)Fusion-Task-Lineage: ${escapeRegex(lineageId)}\\s*(?:\\n|$)`).test(body)) { + return true; + } + if (new RegExp(`(?:^|\\n)Fusion-Task-Id: ${escapeRegex(taskId)}\\s*(?:\\n|$)`).test(body)) { + return true; + } + // Subject anchor MUST mention the task ID — either inside a conventional + // scope (`(<…taskId…>): …`) or as a leading `: …`. The scope + // group is intentionally NOT optional here: a bare `feat: …` with no task ID + // is NOT ownership evidence (a prose commit such as `feat: unrelated change` + // whose body merely mentions the task must be rejected — incident bug #2). + const subjectAnchor = new RegExp( + `^(?:[A-Za-z]+\\([^)]*\\b${escapeRegex(taskId)}\\b[^)]*\\):|${escapeRegex(taskId)}:)`, + ); + return subjectAnchor.test(subject); +} + export async function findAlreadyMergedTaskCommit( input: AlreadyMergedLookupInput, ): Promise { @@ -97,22 +140,33 @@ export async function findAlreadyMergedTaskCommit( stdio: ["pipe", "pipe", "pipe"], }); + // FN-5441/5446 (2026-05-23 lost-work bug #2): `--grep=` is a loose + // match that also hits commits merely mentioning the task ID in prose. + // Gather candidates (bounded) and accept only the first whose subject/body + // is OWNERSHIP-anchored on the task — never the first raw grep hit. const ancestryCommand = [ "git log", "--first-parent", - "--format=%H", + "--format=%H%x1f%s%x1f%b%x1e", `--grep=${shellQuote(taskId)}`, - "--max-count=1", + "--max-count=20", shellQuote(baseBranch), ].join(" "); const { stdout } = await execAsync(ancestryCommand, { cwd: repoDir, timeout: 30_000, - maxBuffer: 1024 * 1024, + maxBuffer: 4 * 1024 * 1024, }); - const sha = stdout.trim(); - if (sha) { - return { sha, strategy: "ancestry" }; + const records = stdout + .split("\x1e") + .map((record) => record.trim()) + .filter((record) => record.length > 0); + for (const record of records) { + const [candidateSha, candidateSubject = "", candidateBody = ""] = record.split("\x1f"); + const sha = candidateSha?.trim(); + if (sha && commitOwnedByTask(taskId, lineageId, candidateSubject, candidateBody)) { + return { sha, strategy: "ancestry" }; + } } } catch { // Fall through to patch-id checks. diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index 4f73c4410a..fa9834489a 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -493,9 +493,13 @@ function commitOwnedByTask(taskId: string, lineageId: string | undefined, subjec if (new RegExp(`(?:^|\\n)Fusion-Task-Id: ${escapeRegex(taskId)}\\s*(?:\\n|$)`).test(body)) { return true; } - // Subject anchor: `(...): …` or `: …` at start. + // Subject anchor: `(<…taskId…>): …` or `: …` at start. + // The conventional scope group is intentionally NOT optional: a bare + // `: …` (e.g. `feat: unrelated change`) carries no task ID and is NOT + // ownership evidence, even if the body mentions the task in prose (incident + // bug #2 — a prose-mention must never claim a task). const subjectAnchor = new RegExp( - `^(?:[A-Za-z]+(?:\\([^)]*\\b${escapeRegex(taskId)}\\b[^)]*\\))?:|${escapeRegex(taskId)}:)`, + `^(?:[A-Za-z]+\\([^)]*\\b${escapeRegex(taskId)}\\b[^)]*\\):|${escapeRegex(taskId)}:)`, ); return subjectAnchor.test(subject); }