fix(FN-5304): guard against silent no-op landed-file attribution mismatches
- Add a merger guard that verifies source fusion/FN branch attribution when rebase capture reports zero own commits - Fail finalize with explicit no-op attribution mismatch handling instead of marking mergeConfirmed on ambiguous no-op ranges - Type and emit dedicated audit events for mismatch and source-ref-unavailable skip diagnostics - Expand branch attribution, merger, and reliability interaction tests to cover the full no-op guard matrix Fusion-Task-Id: FN-5304
This commit is contained in:
committed by
gsxdsm
parent
9011c2107e
commit
c5e9dc43f1
@@ -1,5 +1,10 @@
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { BranchAttributionError, filterFilesToOwnTaskCommits } from "../branch-attribution.js";
|
||||
import {
|
||||
BranchAttributionError,
|
||||
SilentNoOpAttributionMismatchError,
|
||||
collectOwnTaskCommitsForRange,
|
||||
filterFilesToOwnTaskCommits,
|
||||
} from "../branch-attribution.js";
|
||||
|
||||
describe("FN-5039 branch-attribution", () => {
|
||||
it("returns empty attribution for empty range", async () => {
|
||||
@@ -153,6 +158,44 @@ describe("FN-5039 branch-attribution", () => {
|
||||
expect(result.foreignCommits).toEqual([]);
|
||||
});
|
||||
|
||||
it("FN-5304: collectOwnTaskCommitsForRange counts only attributable commits", async () => {
|
||||
const log = [
|
||||
"sha-a\x00fix(FN-5304): one\x00body\x1e",
|
||||
"sha-b\x00fix(FN-9999): foreign\x00body\x1e",
|
||||
"sha-c\x00chore: none\x00body\x1e",
|
||||
].join("");
|
||||
const execMock = vi.fn().mockResolvedValueOnce({ stdout: log });
|
||||
|
||||
const result = await collectOwnTaskCommitsForRange({
|
||||
worktreePath: "/tmp/wt",
|
||||
rangeRef: "base..fusion/fn-5304",
|
||||
taskId: "FN-5304",
|
||||
execAsyncImpl: execMock as never,
|
||||
});
|
||||
|
||||
expect(result).toEqual({ ownCommitCount: 1, ownCommitShas: ["sha-a"] });
|
||||
});
|
||||
|
||||
it("FN-5304: SilentNoOpAttributionMismatchError carries expected metadata", () => {
|
||||
const err = new SilentNoOpAttributionMismatchError({
|
||||
taskId: "FN-5304",
|
||||
recordedSha: "recorded123",
|
||||
rebaseMergeBaseSha: "base123",
|
||||
sourceBranchRef: "fusion/fn-5304",
|
||||
sourceBranchOwnCommitCount: 2,
|
||||
sourceBranchOwnCommitShas: ["own1", "own2"],
|
||||
});
|
||||
|
||||
expect(err.name).toBe("SilentNoOpAttributionMismatchError");
|
||||
expect(err.taskId).toBe("FN-5304");
|
||||
expect(err.recordedSha).toBe("recorded123");
|
||||
expect(err.rebaseMergeBaseSha).toBe("base123");
|
||||
expect(err.sourceBranchRef).toBe("fusion/fn-5304");
|
||||
expect(err.sourceBranchOwnCommitCount).toBe(2);
|
||||
expect(err.sourceBranchOwnCommitShas).toEqual(["own1", "own2"]);
|
||||
expect(err.message).toContain("silent no-op attribution mismatch");
|
||||
});
|
||||
|
||||
it("throws BranchAttributionError when git command fails", async () => {
|
||||
const execMock = vi.fn().mockRejectedValueOnce(new Error("fatal: bad revision"));
|
||||
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { BranchAttributionError } from "../branch-attribution.js";
|
||||
import { BranchAttributionError, SilentNoOpAttributionMismatchError } from "../branch-attribution.js";
|
||||
import * as attributionModule from "../branch-attribution.js";
|
||||
import { createMockStore, mockedCreateFnAgent, mockedExecSync, mockedExistsSync, type Task } from "./merger-test-helpers.js";
|
||||
import * as mergerModule from "../merger.js";
|
||||
@@ -109,6 +109,107 @@ describe("FN-4646 aiMergeTask landedFiles capture", () => {
|
||||
expect(detailsUpdate?.[1].modifiedFiles).toBeUndefined();
|
||||
});
|
||||
|
||||
it.each([
|
||||
{ sourceOwnCommitCount: 0, expectedNoOp: true },
|
||||
{ sourceOwnCommitCount: 2, expectedNoOp: false },
|
||||
])("FN-5304: source tip attribution guard (sourceOwnCommitCount=$sourceOwnCommitCount)", async ({ sourceOwnCommitCount, expectedNoOp }) => {
|
||||
vi.spyOn(attributionModule, "filterFilesToOwnTaskCommits").mockResolvedValue({
|
||||
files: [],
|
||||
foreignCommits: [],
|
||||
ownCommitCount: 0,
|
||||
ownCommitShas: [],
|
||||
rawDiffFileCount: 0,
|
||||
commitAttributions: [],
|
||||
});
|
||||
vi.spyOn(attributionModule, "collectOwnTaskCommitsForRange").mockResolvedValue({
|
||||
ownCommitCount: sourceOwnCommitCount,
|
||||
ownCommitShas: sourceOwnCommitCount > 0 ? ["own1", "own2"] : [],
|
||||
});
|
||||
|
||||
const capturePromise = mergerModule.captureRebaseLandedFilesForTask({
|
||||
rootDir: "/tmp/root",
|
||||
rebaseMergeBaseSha: "base123",
|
||||
recordedSha: "recorded123",
|
||||
taskId: "FN-5304",
|
||||
sourceBranchRef: "fusion/fn-5304",
|
||||
});
|
||||
|
||||
if (expectedNoOp) {
|
||||
const capture = await capturePromise;
|
||||
expect(capture.noOpVerifiedShortCircuit).toBe(true);
|
||||
expect(capture.landedFilesAttributionRestricted).toBe(true);
|
||||
return;
|
||||
}
|
||||
|
||||
await expect(capturePromise).rejects.toMatchObject({
|
||||
name: "SilentNoOpAttributionMismatchError",
|
||||
taskId: "FN-5304",
|
||||
recordedSha: "recorded123",
|
||||
rebaseMergeBaseSha: "base123",
|
||||
sourceBranchRef: "fusion/fn-5304",
|
||||
sourceBranchOwnCommitCount: 2,
|
||||
sourceBranchOwnCommitShas: ["own1", "own2"],
|
||||
});
|
||||
});
|
||||
|
||||
it("FN-5304: source ref unavailable keeps no-op short-circuit and emits skip callback", async () => {
|
||||
vi.spyOn(attributionModule, "filterFilesToOwnTaskCommits").mockResolvedValue({
|
||||
files: [],
|
||||
foreignCommits: [],
|
||||
ownCommitCount: 0,
|
||||
ownCommitShas: [],
|
||||
rawDiffFileCount: 0,
|
||||
commitAttributions: [],
|
||||
});
|
||||
vi.spyOn(attributionModule, "collectOwnTaskCommitsForRange").mockRejectedValue(new Error("missing ref"));
|
||||
const onNoOpGuardSkipped = vi.fn();
|
||||
|
||||
const capture = await mergerModule.captureRebaseLandedFilesForTask({
|
||||
rootDir: "/tmp/root",
|
||||
rebaseMergeBaseSha: "base123",
|
||||
recordedSha: "recorded123",
|
||||
taskId: "FN-5304",
|
||||
sourceBranchRef: "fusion/fn-5304",
|
||||
onNoOpGuardSkipped,
|
||||
});
|
||||
|
||||
expect(capture.noOpVerifiedShortCircuit).toBe(true);
|
||||
expect(onNoOpGuardSkipped).toHaveBeenCalledWith("source-ref-unavailable");
|
||||
});
|
||||
|
||||
it("FN-5304: aiMergeTask refuses no-op mismatch and parks task in failed in-review", async () => {
|
||||
const store = makeStore({ directMergeCommitStrategy: "always-rebase" });
|
||||
vi.spyOn(attributionModule, "filterFilesToOwnTaskCommits").mockResolvedValue({
|
||||
files: [],
|
||||
foreignCommits: [],
|
||||
ownCommitCount: 0,
|
||||
ownCommitShas: [],
|
||||
rawDiffFileCount: 0,
|
||||
commitAttributions: [],
|
||||
});
|
||||
vi.spyOn(attributionModule, "collectOwnTaskCommitsForRange").mockResolvedValue({ ownCommitCount: 1, ownCommitShas: ["own1"] });
|
||||
mockedExecSync.mockImplementation((cmd: any) => {
|
||||
const s = String(cmd);
|
||||
if (s.includes("rev-parse --verify")) return Buffer.from("abc123");
|
||||
if (s === "git rev-parse HEAD" || s.startsWith("git rev-parse HEAD ")) return "rebasesha123";
|
||||
if (s.includes("git log")) return "- feat: summary";
|
||||
if (s.includes("merge-base")) return Buffer.from("abc123");
|
||||
if (s.includes("rev-parse \"abc123\"")) return "rebasebase123";
|
||||
if (s.includes("rev-list --reverse \"rebasebase123..fusion/FN-4646\"")) return "";
|
||||
if (s.includes("status --porcelain")) return "";
|
||||
if (s.includes("rev-parse --git-path CHERRY_PICK_HEAD")) return ".git/CHERRY_PICK_HEAD";
|
||||
if (s.includes("rev-parse --git-path sequencer")) return ".git/sequencer";
|
||||
if (s.includes("branch -d") || s.includes("branch -D") || s.includes("worktree remove")) return Buffer.from("");
|
||||
return Buffer.from("");
|
||||
});
|
||||
|
||||
await expect(mergerModule.aiMergeTask(store, "/tmp/root", "FN-4646")).rejects.toBeInstanceOf(SilentNoOpAttributionMismatchError);
|
||||
expect(store.moveTask).toHaveBeenCalledWith("FN-4646", "in-review", expect.any(Object));
|
||||
expect(store.updateTask).toHaveBeenCalledWith("FN-4646", expect.objectContaining({ status: "failed" }));
|
||||
const detailsUpdate = (store.updateTask as any).mock.calls.find((call: any[]) => call[1]?.mergeDetails?.noOpVerifiedShortCircuit);
|
||||
expect(detailsUpdate).toBeUndefined();
|
||||
});
|
||||
|
||||
it("sums shortstat across multiple own commits on rebase", async () => {
|
||||
const store = makeStore({ directMergeCommitStrategy: "always-rebase" });
|
||||
vi.spyOn(attributionModule, "filterFilesToOwnTaskCommits").mockResolvedValue({
|
||||
|
||||
@@ -4,7 +4,7 @@ import { join } from "node:path";
|
||||
import { tmpdir } from "node:os";
|
||||
import { execSync, spawnSync } from "node:child_process";
|
||||
import { captureRebaseLandedFilesForTask, sumShortstatsForCommits } from "../../merger.js";
|
||||
import { filterFilesToOwnTaskCommits } from "../../branch-attribution.js";
|
||||
import { filterFilesToOwnTaskCommits, SilentNoOpAttributionMismatchError } from "../../branch-attribution.js";
|
||||
|
||||
const hasGit = spawnSync("git", ["--version"], { stdio: "pipe" }).status === 0;
|
||||
const describeIfGit = hasGit ? describe : describe.skip;
|
||||
@@ -91,6 +91,32 @@ describeIfGit("FN-5103 reliability interaction: landed-files attribution", () =>
|
||||
expect(capture.deletions).toBe(0);
|
||||
});
|
||||
|
||||
it("FN-5304: refuses no-op fast-path when source branch tip still carries own commits", async () => {
|
||||
const { repoDir, baseSha } = await initRepo("fn-5304-ri-");
|
||||
dirs.push(repoDir);
|
||||
const taskId = "FN-5304";
|
||||
|
||||
git(repoDir, `git checkout -b fusion/${taskId.toLowerCase()}`);
|
||||
await commitFile(repoDir, "task-owned-a.ts", "a\n", "fix(FN-5304): owned A", taskId);
|
||||
await commitFile(repoDir, "task-owned-b.ts", "b\n", "fix(FN-5304): owned B", taskId);
|
||||
|
||||
git(repoDir, "git checkout main");
|
||||
await commitFile(repoDir, "upstream-a.ts", "u\n", "fix(FN-9999): upstream A", "FN-9999");
|
||||
await commitFile(repoDir, "upstream-b.ts", "v\n", "fix(FN-9999): upstream B", "FN-9999");
|
||||
|
||||
const recordedSha = git(repoDir, "git rev-parse HEAD");
|
||||
|
||||
await expect(
|
||||
captureRebaseLandedFilesForTask({
|
||||
rootDir: repoDir,
|
||||
rebaseMergeBaseSha: baseSha,
|
||||
recordedSha,
|
||||
taskId,
|
||||
sourceBranchRef: `fusion/${taskId.toLowerCase()}`,
|
||||
}),
|
||||
).rejects.toBeInstanceOf(SilentNoOpAttributionMismatchError);
|
||||
});
|
||||
|
||||
it("surfaces attribution failure when git reads fail", async () => {
|
||||
const { repoDir, baseSha } = await initRepo("fn-5103-ri-");
|
||||
dirs.push(repoDir);
|
||||
|
||||
Reference in New Issue
Block a user