fix(merger): short-circuit out-of-scope fix loop to prevent limbo recovery cycle
When the in-merge fix agent makes no changes AND all failing test files are outside the branch's diff, the merger now throws OutOfScopeVerificationError and marks the task status: "failed" with a clear error message: "Merge verification failed in files outside branch scope — likely pre-existing flake on main. Fix the base-branch test breakage separately and retry." This prevents the task from entering the completion-handoff-limbo recovery cycle (which would retry the merge endlessly) when the verification failure is caused by pre-existing flakiness in an unrelated package (e.g. engine reliability-interaction tests failing while only dashboard was changed). Failing file paths are parsed from vitest/jest output (FAIL lines and ❯ summary lines). If parsing yields no file list, the existing retry behavior is preserved. The OutOfScopeVerificationError propagates through the catch block so it does not count toward completionHandoffLimboRecoveryCount. New exports: OutOfScopeVerificationError, parseFailingFilesFromOutput, getBranchChangedFiles. Tests added: parseFailingFilesFromOutput (4), getBranchChangedFiles (3), OutOfScopeVerificationError constructor (1). All 58 merger-verification tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
9
.changeset/scope-merger-verification.md
Normal file
9
.changeset/scope-merger-verification.md
Normal file
@@ -0,0 +1,9 @@
|
|||||||
|
---
|
||||||
|
"@fusion/engine": minor
|
||||||
|
---
|
||||||
|
|
||||||
|
feat(merger): scope pnpm verification to changed packages and short-circuit out-of-scope fix loop
|
||||||
|
|
||||||
|
In a pnpm workspace, inferDefaultTestCommand now derives the set of packages touched by the branch diff and emits `pnpm --filter "<pkg>...^" test` instead of `pnpm test`. This prevents flakes in unrelated packages from blocking merges. When git context is unavailable or changes are root-only, the command falls back to the unscoped `pnpm test`.
|
||||||
|
|
||||||
|
When the in-merge fix agent makes no changes and all failing test files are outside the branch's diff, the merger now marks the task `status: "failed"` immediately with a clear "out-of-scope flake" message rather than retrying into the limbo-recovery cycle.
|
||||||
@@ -146,10 +146,13 @@ import {
|
|||||||
resolveTaskDiffBaseRef,
|
resolveTaskDiffBaseRef,
|
||||||
commitOrAmendMergeWithFixes,
|
commitOrAmendMergeWithFixes,
|
||||||
MergeAbortedError,
|
MergeAbortedError,
|
||||||
|
OutOfScopeVerificationError,
|
||||||
parsePnpmWorkspaceGlobs,
|
parsePnpmWorkspaceGlobs,
|
||||||
resolveWorkspacePackageRoots,
|
resolveWorkspacePackageRoots,
|
||||||
mapChangedFilesToPackageNames,
|
mapChangedFilesToPackageNames,
|
||||||
deriveScopedPnpmTestCommand,
|
deriveScopedPnpmTestCommand,
|
||||||
|
parseFailingFilesFromOutput,
|
||||||
|
getBranchChangedFiles,
|
||||||
type ConflictCategory,
|
type ConflictCategory,
|
||||||
} from "../merger.js";
|
} from "../merger.js";
|
||||||
import { mergerLog } from "../logger.js";
|
import { mergerLog } from "../logger.js";
|
||||||
@@ -2955,3 +2958,81 @@ describe("inferDefaultTestCommand — pnpm workspace scoping", () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// ── parseFailingFilesFromOutput ──────────────────────────────────────────
|
||||||
|
|
||||||
|
describe("parseFailingFilesFromOutput", () => {
|
||||||
|
it("parses FAIL lines from jest/vitest output", () => {
|
||||||
|
const output = [
|
||||||
|
"FAIL packages/engine/src/__tests__/reliability-interactions/foo.test.ts",
|
||||||
|
"FAIL packages/engine/src/__tests__/bar.test.ts",
|
||||||
|
"● some test name",
|
||||||
|
].join("\n");
|
||||||
|
const files = parseFailingFilesFromOutput(output);
|
||||||
|
expect(files).toContain("packages/engine/src/__tests__/reliability-interactions/foo.test.ts");
|
||||||
|
expect(files).toContain("packages/engine/src/__tests__/bar.test.ts");
|
||||||
|
expect(files.length).toBe(2);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("parses vitest summary ❯ lines", () => {
|
||||||
|
const output = [
|
||||||
|
" ❯ packages/engine/src/__tests__/merger.test.ts (5 tests | 2 failed)",
|
||||||
|
].join("\n");
|
||||||
|
const files = parseFailingFilesFromOutput(output);
|
||||||
|
expect(files).toContain("packages/engine/src/__tests__/merger.test.ts");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns empty array when output has no file paths", () => {
|
||||||
|
const output = "● some test title\n● another test\n";
|
||||||
|
expect(parseFailingFilesFromOutput(output)).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("deduplicates repeated file paths", () => {
|
||||||
|
const output = [
|
||||||
|
"FAIL packages/engine/src/__tests__/foo.test.ts",
|
||||||
|
"FAIL packages/engine/src/__tests__/foo.test.ts",
|
||||||
|
].join("\n");
|
||||||
|
expect(parseFailingFilesFromOutput(output)).toHaveLength(1);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
// ── getBranchChangedFiles ────────────────────────────────────────────────
|
||||||
|
|
||||||
|
describe("getBranchChangedFiles", () => {
|
||||||
|
beforeEach(() => {
|
||||||
|
vi.clearAllMocks();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns changed files from git diff output", () => {
|
||||||
|
mockedExecSync.mockReturnValue("packages/dashboard/src/a.ts\npackages/dashboard/src/b.ts\n" as any);
|
||||||
|
const files = getBranchChangedFiles("/repo", "main", "fusion/fn-123");
|
||||||
|
expect(files).toEqual(["packages/dashboard/src/a.ts", "packages/dashboard/src/b.ts"]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns empty array when git diff fails", () => {
|
||||||
|
mockedExecSync.mockImplementation(() => { throw new Error("not a git repo"); });
|
||||||
|
const files = getBranchChangedFiles("/repo", "main", "fusion/fn-123");
|
||||||
|
expect(files).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("filters out empty lines", () => {
|
||||||
|
mockedExecSync.mockReturnValue("\npackages/engine/src/merger.ts\n\n" as any);
|
||||||
|
const files = getBranchChangedFiles("/repo", "main", "fusion/fn-123");
|
||||||
|
expect(files).toEqual(["packages/engine/src/merger.ts"]);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
// ── OutOfScopeVerificationError ─────────────────────────────────────────
|
||||||
|
|
||||||
|
describe("OutOfScopeVerificationError", () => {
|
||||||
|
it("is constructable with message, failingFiles, and branchFiles", () => {
|
||||||
|
const err = new OutOfScopeVerificationError(
|
||||||
|
"test failure outside branch scope",
|
||||||
|
["packages/engine/src/__tests__/reliability-interactions/foo.test.ts"],
|
||||||
|
["packages/dashboard/src/index.ts"],
|
||||||
|
);
|
||||||
|
expect(err.name).toBe("OutOfScopeVerificationError");
|
||||||
|
expect(err.message).toContain("outside branch scope");
|
||||||
|
expect(err.failingFiles).toHaveLength(1);
|
||||||
|
expect(err.branchFiles).toHaveLength(1);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -1030,6 +1030,25 @@ export class MergeAbortedError extends Error {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Raised when fix agent made no changes and the failing test files are all
|
||||||
|
* outside the branch's diff. This signals that the failure is pre-existing on
|
||||||
|
* the base branch (e.g. a flaky engine test) and retrying cannot help.
|
||||||
|
*
|
||||||
|
* The merger catches this and marks the task `failed` with a clear error
|
||||||
|
* message, bypassing limbo recovery so the user sees an actionable status.
|
||||||
|
*/
|
||||||
|
export class OutOfScopeVerificationError extends Error {
|
||||||
|
constructor(
|
||||||
|
message: string,
|
||||||
|
public readonly failingFiles: string[],
|
||||||
|
public readonly branchFiles: string[],
|
||||||
|
) {
|
||||||
|
super(message);
|
||||||
|
this.name = "OutOfScopeVerificationError";
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
export class SquashAuditError extends Error {
|
export class SquashAuditError extends Error {
|
||||||
constructor(
|
constructor(
|
||||||
taskId: string,
|
taskId: string,
|
||||||
@@ -1046,6 +1065,68 @@ export function throwIfAborted(signal: AbortSignal | undefined, taskId: string):
|
|||||||
throw new MergeAbortedError(`Merge aborted for ${taskId}: engine shutdown requested`);
|
throw new MergeAbortedError(`Merge aborted for ${taskId}: engine shutdown requested`);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Parse failing test file paths from vitest/jest output.
|
||||||
|
*
|
||||||
|
* Looks for lines matching:
|
||||||
|
* - `FAIL <path>` (jest/vitest)
|
||||||
|
* - ` × <path>` / ` ✕ <path>` (vitest unicode markers)
|
||||||
|
* - `● <test suite> › <test name>` is NOT a file path — skip those
|
||||||
|
*
|
||||||
|
* Returns an array of unique relative file paths. Returns an empty array when
|
||||||
|
* no file paths can be parsed (callers treat this as "unknown", not "in-scope").
|
||||||
|
*
|
||||||
|
* @internal Exported for testing only.
|
||||||
|
*/
|
||||||
|
export function parseFailingFilesFromOutput(output: string): string[] {
|
||||||
|
const paths = new Set<string>();
|
||||||
|
for (const line of output.split("\n")) {
|
||||||
|
// jest/vitest: "FAIL packages/engine/src/__tests__/foo.test.ts"
|
||||||
|
const failMatch = line.match(/^FAIL\s+(\S+)/);
|
||||||
|
if (failMatch && failMatch[1]) {
|
||||||
|
paths.add(failMatch[1]);
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
// vitest summary: " ❯ packages/engine/src/__tests__/foo.test.ts (2 tests | 1 failed)"
|
||||||
|
const vitestSummaryMatch = line.match(/^\s*[❯>]\s+(\S+\.(?:test|spec)\.[jt]sx?)\s/);
|
||||||
|
if (vitestSummaryMatch && vitestSummaryMatch[1]) {
|
||||||
|
paths.add(vitestSummaryMatch[1]);
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
// vitest: " × src/__tests__/foo.test.ts > some test name"
|
||||||
|
const crossMatch = line.match(/^\s*[×✕✗]\s+(\S+\.(?:test|spec)\.[jt]sx?)\s/);
|
||||||
|
if (crossMatch && crossMatch[1]) {
|
||||||
|
paths.add(crossMatch[1]);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return Array.from(paths);
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Get the set of files changed in the branch relative to the base branch.
|
||||||
|
* Uses `git diff --name-only <baseBranch>...HEAD` (three-dot range so it
|
||||||
|
* computes the diff from the merge-base, not the current HEAD of baseBranch).
|
||||||
|
*
|
||||||
|
* Returns an empty array on git errors (callers treat this as "unknown").
|
||||||
|
*
|
||||||
|
* @internal Exported for testing only.
|
||||||
|
*/
|
||||||
|
export function getBranchChangedFiles(rootDir: string, baseBranch: string, branch: string): string[] {
|
||||||
|
try {
|
||||||
|
// Use the branch ref directly when it's not HEAD
|
||||||
|
const range = branch === "HEAD"
|
||||||
|
? `${baseBranch}...HEAD`
|
||||||
|
: `${baseBranch}...${branch}`;
|
||||||
|
const output = execSync(
|
||||||
|
`git diff --name-only ${range}`,
|
||||||
|
{ cwd: rootDir, stdio: "pipe", encoding: "utf-8" },
|
||||||
|
).toString();
|
||||||
|
return output.split("\n").map((f) => f.trim()).filter(Boolean);
|
||||||
|
} catch {
|
||||||
|
return [];
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Return the union of all dirty paths in `rootDir`:
|
* Return the union of all dirty paths in `rootDir`:
|
||||||
* - tracked files modified vs the index (`git diff --name-only`)
|
* - tracked files modified vs the index (`git diff --name-only`)
|
||||||
@@ -1444,13 +1525,20 @@ async function runVerificationCommand(
|
|||||||
/**
|
/**
|
||||||
* Attempt an in-merge verification fix by spawning an AI agent on the main branch.
|
* Attempt an in-merge verification fix by spawning an AI agent on the main branch.
|
||||||
* Returns true if verification passes after the fix, false otherwise.
|
* Returns true if verification passes after the fix, false otherwise.
|
||||||
* Never throws — errors are caught and logged, and the function returns false.
|
*
|
||||||
|
* Throws OutOfScopeVerificationError when the fix agent made no changes AND the
|
||||||
|
* failing files are all outside the branch's diff — meaning the failure is
|
||||||
|
* pre-existing on the base branch and cannot be fixed by this task's agent.
|
||||||
*
|
*
|
||||||
* @param fixModifiedFiles - Mutable set that this function populates with every
|
* @param fixModifiedFiles - Mutable set that this function populates with every
|
||||||
* path that changed during the fix agent's run (post-snapshot minus
|
* path that changed during the fix agent's run (post-snapshot minus
|
||||||
* pre-snapshot). The caller passes this set across all fix attempts so that
|
* pre-snapshot). The caller passes this set across all fix attempts so that
|
||||||
* `commitOrAmendMergeWithFixes` can build an allowlist that covers every file
|
* `commitOrAmendMergeWithFixes` can build an allowlist that covers every file
|
||||||
* the fix agent touched, regardless of how many retries were needed.
|
* the fix agent touched, regardless of how many retries were needed.
|
||||||
|
* @param baseBranch - Integration branch name (e.g. "main"). Used for
|
||||||
|
* out-of-scope detection; pass undefined to skip detection.
|
||||||
|
* @param branch - Feature branch name being merged. Used for out-of-scope
|
||||||
|
* detection; pass undefined to skip detection.
|
||||||
*/
|
*/
|
||||||
async function attemptInMergeVerificationFix(
|
async function attemptInMergeVerificationFix(
|
||||||
store: TaskStore,
|
store: TaskStore,
|
||||||
@@ -1471,6 +1559,8 @@ async function attemptInMergeVerificationFix(
|
|||||||
testSource?: "explicit" | "inferred" | "inferred-scoped",
|
testSource?: "explicit" | "inferred" | "inferred-scoped",
|
||||||
buildSource?: "explicit" | "inferred",
|
buildSource?: "explicit" | "inferred",
|
||||||
fixModifiedFiles?: Set<string>,
|
fixModifiedFiles?: Set<string>,
|
||||||
|
baseBranch?: string,
|
||||||
|
branch?: string,
|
||||||
): Promise<boolean> {
|
): Promise<boolean> {
|
||||||
// Snapshot the working tree before doing anything so the diff reflects only
|
// Snapshot the working tree before doing anything so the diff reflects only
|
||||||
// what the fix agent touched, not pre-existing dirty state.
|
// what the fix agent touched, not pre-existing dirty state.
|
||||||
@@ -1649,6 +1739,32 @@ ${failureContext.output.slice(0, VERIFICATION_LOG_MAX_CHARS)}
|
|||||||
undefined,
|
undefined,
|
||||||
"merger",
|
"merger",
|
||||||
);
|
);
|
||||||
|
|
||||||
|
// Out-of-scope detection: if we have git context and can parse failing
|
||||||
|
// file paths, check whether ALL failing files are outside the branch
|
||||||
|
// diff. If so, throw OutOfScopeVerificationError so the caller can mark
|
||||||
|
// the task failed immediately rather than retrying into limbo.
|
||||||
|
if (baseBranch && branch) {
|
||||||
|
const failingFiles = parseFailingFilesFromOutput(failureContext.output);
|
||||||
|
if (failingFiles.length > 0) {
|
||||||
|
const branchFiles = getBranchChangedFiles(rootDir, baseBranch, branch);
|
||||||
|
if (branchFiles.length > 0) {
|
||||||
|
const allOutOfScope = failingFiles.every((ff) =>
|
||||||
|
!branchFiles.some((bf) => bf === ff || ff.startsWith(`${bf}/`) || bf.startsWith(`${ff}/`)),
|
||||||
|
);
|
||||||
|
if (allOutOfScope) {
|
||||||
|
const msg =
|
||||||
|
`Merge verification failed in files outside branch scope — likely pre-existing flake on ${baseBranch}. ` +
|
||||||
|
`Failing files: [${failingFiles.join(", ")}]. Branch diff files: [${branchFiles.slice(0, 10).join(", ")}${branchFiles.length > 10 ? ", ..." : ""}].`;
|
||||||
|
mergerLog.warn(`${taskId}: ${msg}`);
|
||||||
|
await store.logEntry(taskId, msg);
|
||||||
|
await store.appendAgentLog(taskId, "Out-of-scope verification failure detected — not retrying", "text", undefined, "merger");
|
||||||
|
throw new OutOfScopeVerificationError(msg, failingFiles, branchFiles);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1689,6 +1805,11 @@ ${failureContext.output.slice(0, VERIFICATION_LOG_MAX_CHARS)}
|
|||||||
}
|
}
|
||||||
} catch (err: unknown) {
|
} catch (err: unknown) {
|
||||||
rethrowIfMergeAborted(err);
|
rethrowIfMergeAborted(err);
|
||||||
|
// OutOfScopeVerificationError must propagate so the caller can mark the
|
||||||
|
// task failed without entering the limbo-recovery cycle.
|
||||||
|
if (err instanceof OutOfScopeVerificationError) {
|
||||||
|
throw err;
|
||||||
|
}
|
||||||
// Even on failure, try to surface any paths the agent partially touched.
|
// Even on failure, try to surface any paths the agent partially touched.
|
||||||
if (fixModifiedFiles) {
|
if (fixModifiedFiles) {
|
||||||
try {
|
try {
|
||||||
@@ -8597,6 +8718,28 @@ export async function aiMergeTask(
|
|||||||
throw error;
|
throw error;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Out-of-scope verification failure: the failing tests are in files that
|
||||||
|
// this branch never touched. Retrying will not help. Mark the task failed
|
||||||
|
// immediately with a clear message so it does not enter limbo recovery.
|
||||||
|
if (error instanceof OutOfScopeVerificationError || error?.name === "OutOfScopeVerificationError") {
|
||||||
|
try {
|
||||||
|
execSync("git reset --merge", { cwd: rootDir, stdio: "pipe" });
|
||||||
|
} catch {
|
||||||
|
// best-effort cleanup
|
||||||
|
}
|
||||||
|
const outOfScopeMsg =
|
||||||
|
`Merge verification failed in files outside branch scope — likely pre-existing flake on ${mergeTarget.branch}. ` +
|
||||||
|
`Fix the base-branch test breakage separately and retry.`;
|
||||||
|
mergerLog.error(`${taskId}: ${outOfScopeMsg}`);
|
||||||
|
await store.updateTask(taskId, {
|
||||||
|
status: "failed",
|
||||||
|
error: outOfScopeMsg,
|
||||||
|
});
|
||||||
|
await store.logEntry(taskId, outOfScopeMsg, "OutOfScopeVerificationError");
|
||||||
|
// Re-throw so the outer merge runner does not attempt further retries.
|
||||||
|
throw error;
|
||||||
|
}
|
||||||
|
|
||||||
if (
|
if (
|
||||||
error instanceof DiffVolumeRegressionError
|
error instanceof DiffVolumeRegressionError
|
||||||
|| error?.name === "DiffVolumeRegressionError"
|
|| error?.name === "DiffVolumeRegressionError"
|
||||||
@@ -8665,6 +8808,8 @@ export async function aiMergeTask(
|
|||||||
effectiveTestSource,
|
effectiveTestSource,
|
||||||
effectiveBuildSource,
|
effectiveBuildSource,
|
||||||
verificationFixModifiedFiles,
|
verificationFixModifiedFiles,
|
||||||
|
mergeTarget.branch,
|
||||||
|
branch,
|
||||||
);
|
);
|
||||||
|
|
||||||
const fixAttemptDurationMs = Date.now() - fixAttemptStartedAt;
|
const fixAttemptDurationMs = Date.now() - fixAttemptStartedAt;
|
||||||
@@ -8816,6 +8961,8 @@ export async function aiMergeTask(
|
|||||||
effectiveTestSource,
|
effectiveTestSource,
|
||||||
effectiveBuildSource,
|
effectiveBuildSource,
|
||||||
buildFixModifiedFiles,
|
buildFixModifiedFiles,
|
||||||
|
mergeTarget.branch,
|
||||||
|
branch,
|
||||||
);
|
);
|
||||||
|
|
||||||
const fixAttemptDurationMs = Date.now() - fixAttemptStartedAt;
|
const fixAttemptDurationMs = Date.now() - fixAttemptStartedAt;
|
||||||
|
|||||||
Reference in New Issue
Block a user