feat(FN-4944): add post-finalize verification guard to prevent regressions
Added a verification guard for steps 1-4 in project-engine with companion regression tests covering both the merge-error recovery path and a post-finalize noop scenario using real git fixtures. Fusion-Task-Id: FN-4944 Fusion-Task-Lineage: ca8fca22-c732-42e4-aa8d-a82981d2f36e
This commit is contained in:
committed by
gsxdsm
parent
c161e626e2
commit
300bc233cf
@@ -178,6 +178,7 @@ Detailed mechanism logs live in `docs/architecture.md` and `docs/design/`. The c
|
||||
- **Task title/ID drift (FN-4898)**: active and archived title writes normalize foreign embedded `FN-NNN` tokens via `packages/core/src/task-title-id-drift.ts`. Lineage is preserved in `sourceParentTaskId` / description markers, not title embeds.
|
||||
- **PR-conflict reclaim wiring (FN-4763)**: GitHub PR refresh now persists normalized `prInfo.mergeable` conflict state and, when conflicting, funnels tasks into self-healing’s existing reclaim machinery (`reclaimPrConflictForTask` / `reclaim-pr-conflicts` stage) so branch-conflict handling stays centralized with existing `inspectBranchConflict` outcomes and unrecoverable pause semantics.
|
||||
- **Worktrunk-managed lifecycles**: when `worktrunk.enabled`, self-healing defers prune/idle/worktree-cap sweeps to the worktrunk backend; branch-level reclaim and orphan rescue stay native.
|
||||
- **Post-finalize verification no-op (FN-4944)**: when auto-merge receives a delayed `VerificationError` after a task is already `done` with `mergeDetails.mergeConfirmed === true` (already-on-main fast-path), it must log one `[verification] ... no action` diagnostic and must not bounce the task back to `in-progress` / `merging-fix`.
|
||||
|
||||
## Engine Process Rules
|
||||
|
||||
|
||||
@@ -51,7 +51,7 @@ type MockTask = {
|
||||
error: string | null;
|
||||
paused?: boolean;
|
||||
steps?: Array<{ status: string }>;
|
||||
mergeDetails?: { mergeConfirmed?: boolean } | null;
|
||||
mergeDetails?: { mergeConfirmed?: boolean; commitSha?: string; mergedAt?: string } | null;
|
||||
verificationFailureCount?: number;
|
||||
mergeConflictBounceCount?: number;
|
||||
branch?: string;
|
||||
@@ -722,6 +722,51 @@ describe("ProjectEngine merge error recovery", () => {
|
||||
expect(hasErrorLog(errorSpy, "persist failed")).toBe(true);
|
||||
});
|
||||
|
||||
it("treats post-finalize verification failures as a no-op diagnostic", async () => {
|
||||
const verificationError = new Error("Deterministic test verification failed: assertion mismatch in workspace");
|
||||
verificationError.name = "VerificationError";
|
||||
vi.mocked(aiMergeTask).mockRejectedValueOnce(verificationError);
|
||||
|
||||
const store = makeStore({
|
||||
tasks: [
|
||||
makeTask({
|
||||
column: "in-review",
|
||||
status: "merging",
|
||||
verificationFailureCount: 2,
|
||||
}),
|
||||
makeTask({
|
||||
column: "done",
|
||||
status: null,
|
||||
verificationFailureCount: 2,
|
||||
mergeDetails: { mergeConfirmed: true, commitSha: "abcdef1234567890" },
|
||||
}),
|
||||
],
|
||||
});
|
||||
const engine = createEngine(store);
|
||||
|
||||
await runMergeCycle(engine);
|
||||
|
||||
expect(store.moveTask).not.toHaveBeenCalledWith(TASK_ID, "in-progress");
|
||||
expect(store.updateTask).not.toHaveBeenCalledWith(
|
||||
TASK_ID,
|
||||
expect.objectContaining({ status: "merging-fix" }),
|
||||
);
|
||||
expect(store.updateTask).not.toHaveBeenCalledWith(
|
||||
TASK_ID,
|
||||
expect.objectContaining({ verificationFailureCount: 3 }),
|
||||
);
|
||||
expect(store.addTaskComment).not.toHaveBeenCalledWith(
|
||||
TASK_ID,
|
||||
expect.stringContaining("Please fix the failing"),
|
||||
"agent",
|
||||
);
|
||||
expect(store.logEntry).toHaveBeenCalledWith(
|
||||
TASK_ID,
|
||||
expect.stringContaining("[verification] post-finalize verification failed for already-on-main fast-path; no action"),
|
||||
"VerificationError",
|
||||
);
|
||||
});
|
||||
|
||||
it("moves task back to in-progress with merge-remediation status on verification errors", async () => {
|
||||
const verificationError = new Error("Deterministic test verification failed");
|
||||
verificationError.name = "VerificationError";
|
||||
|
||||
@@ -0,0 +1,197 @@
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { mkdtempSync, rmSync, writeFileSync } from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import { execSync } from "node:child_process";
|
||||
import { EventEmitter } from "node:events";
|
||||
import type { Settings, Task, TaskStore } from "@fusion/core";
|
||||
import { commitOrAmendMergeWithFixes } from "../../merger.js";
|
||||
import { SelfHealingManager } from "../../self-healing.js";
|
||||
|
||||
const testState = vi.hoisted(() => ({
|
||||
aiMergeTask: vi.fn(),
|
||||
currentStore: null as (TaskStore & EventEmitter) | null,
|
||||
}));
|
||||
|
||||
vi.mock("../../merger.js", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("../../merger.js")>();
|
||||
return {
|
||||
...actual,
|
||||
aiMergeTask: testState.aiMergeTask,
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock("../../runtimes/in-process-runtime.js", () => ({
|
||||
InProcessRuntime: vi.fn().mockImplementation(() => ({
|
||||
start: vi.fn(async () => undefined),
|
||||
stop: vi.fn(async () => undefined),
|
||||
getTaskStore: () => testState.currentStore,
|
||||
getAgentStore: vi.fn(),
|
||||
getMessageStore: vi.fn(),
|
||||
getRoutineStore: vi.fn(),
|
||||
getRoutineRunner: vi.fn(),
|
||||
getHeartbeatMonitor: vi.fn(),
|
||||
getTriggerScheduler: vi.fn(),
|
||||
})),
|
||||
}));
|
||||
|
||||
import { ProjectEngine } from "../../project-engine.js";
|
||||
|
||||
function git(dir: string, cmd: string): string {
|
||||
return execSync(cmd, { cwd: dir, stdio: "pipe" }).toString().trim();
|
||||
}
|
||||
|
||||
function createStore(task: Task, taskSequence?: Task[]) {
|
||||
const emitter = new EventEmitter();
|
||||
const comments: string[] = [];
|
||||
const logs: string[] = [];
|
||||
let taskIdx = 0;
|
||||
const sequence = taskSequence ?? [task];
|
||||
|
||||
const store = Object.assign(emitter, {
|
||||
getSettings: vi.fn(async () => ({
|
||||
autoMerge: true,
|
||||
autoResolveConflicts: true,
|
||||
globalPause: false,
|
||||
enginePaused: false,
|
||||
pollIntervalMs: 15_000,
|
||||
} as Settings)),
|
||||
listTasks: vi.fn(async () => [task]),
|
||||
getTask: vi.fn(async () => {
|
||||
const current = sequence[Math.min(taskIdx, sequence.length - 1)] ?? task;
|
||||
taskIdx += 1;
|
||||
return current;
|
||||
}),
|
||||
updateTask: vi.fn(async (_id: string, updates: Partial<Task>) => Object.assign(task, updates)),
|
||||
addTaskComment: vi.fn(async (_id: string, comment: string) => {
|
||||
comments.push(comment);
|
||||
}),
|
||||
moveTask: vi.fn(async (_id: string, column: Task["column"]) => {
|
||||
task.column = column;
|
||||
}),
|
||||
logEntry: vi.fn(async (_id: string, message: string) => {
|
||||
logs.push(message);
|
||||
}),
|
||||
getActiveMergingTask: vi.fn(() => null),
|
||||
createTask: vi.fn(),
|
||||
on: emitter.on.bind(emitter),
|
||||
off: emitter.off.bind(emitter),
|
||||
walCheckpoint: () => ({ busy: 0, log: 0, checkpointed: 0 }),
|
||||
archiveTaskAndCleanup: async () => ({}),
|
||||
clearStaleExecutionStartBranchReferences: () => [],
|
||||
updateSettings: async () => ({}),
|
||||
mergeTask: async () => undefined,
|
||||
getRootDir: () => "",
|
||||
recordRunAuditEvent: async () => undefined,
|
||||
}) as unknown as TaskStore & EventEmitter;
|
||||
|
||||
return { store, comments, logs };
|
||||
}
|
||||
|
||||
async function runMergeCycle(engine: ProjectEngine, taskId: string): Promise<void> {
|
||||
const privateEngine = engine as unknown as {
|
||||
mergeQueue: string[];
|
||||
mergeActive: Set<string>;
|
||||
drainMergeQueue: () => Promise<void>;
|
||||
};
|
||||
|
||||
privateEngine.mergeActive.add(taskId);
|
||||
privateEngine.mergeQueue.push(taskId);
|
||||
await privateEngine.drainMergeQueue();
|
||||
}
|
||||
|
||||
describe("post-finalize verification failure reliability interactions (real git)", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
testState.aiMergeTask.mockReset();
|
||||
testState.currentStore = null;
|
||||
});
|
||||
|
||||
it("keeps finalized already-on-main tasks in done when delayed verification fails", async () => {
|
||||
const dir = mkdtempSync(join(tmpdir(), "fn-4944-ri-"));
|
||||
try {
|
||||
git(dir, "git init -b main");
|
||||
git(dir, 'git config user.email "test@example.com"');
|
||||
git(dir, 'git config user.name "Test"');
|
||||
git(dir, "git commit --allow-empty -m init");
|
||||
|
||||
git(dir, "git commit --allow-empty -m 'feat(FN-4944): unrelated'");
|
||||
const branchTip = git(dir, "git rev-parse HEAD");
|
||||
writeFileSync(join(dir, "file.txt"), "task\n");
|
||||
git(dir, "git add file.txt");
|
||||
git(dir, "git commit -m 'feat(FN-4944): landed' -m 'Fusion-Task-Id: FN-4944'");
|
||||
const landedSha = git(dir, "git rev-parse HEAD");
|
||||
git(dir, "git commit --allow-empty -m 'chore: post'");
|
||||
const preAttemptHeadSha = git(dir, "git rev-parse HEAD");
|
||||
git(dir, `git branch fusion/fn-4944 ${branchTip}`);
|
||||
|
||||
const finalized = await commitOrAmendMergeWithFixes(
|
||||
dir,
|
||||
"FN-4944",
|
||||
"fusion/fn-4944",
|
||||
"feat(FN-4944): merge",
|
||||
true,
|
||||
preAttemptHeadSha,
|
||||
"",
|
||||
);
|
||||
expect(finalized.ok && finalized.reason === "branch-already-merged-on-main").toBe(true);
|
||||
|
||||
const task = {
|
||||
id: "FN-4944",
|
||||
title: "t",
|
||||
description: "d",
|
||||
column: "done",
|
||||
dependencies: [],
|
||||
steps: [],
|
||||
currentStep: 0,
|
||||
log: [],
|
||||
createdAt: new Date().toISOString(),
|
||||
updatedAt: new Date().toISOString(),
|
||||
branch: "fusion/fn-4944",
|
||||
baseBranch: "main",
|
||||
status: null,
|
||||
mergeDetails: { mergeConfirmed: true, commitSha: landedSha, mergedAt: new Date().toISOString() },
|
||||
} as unknown as Task;
|
||||
|
||||
const preFinalizeTask = {
|
||||
...task,
|
||||
column: "in-review",
|
||||
status: "merging",
|
||||
mergeDetails: undefined,
|
||||
} as unknown as Task;
|
||||
const { store, comments, logs } = createStore(task, [preFinalizeTask, task]);
|
||||
testState.currentStore = store;
|
||||
|
||||
const verificationError = new Error("Deterministic test verification failed");
|
||||
verificationError.name = "VerificationError";
|
||||
testState.aiMergeTask.mockRejectedValueOnce(verificationError);
|
||||
|
||||
const engine = new ProjectEngine(
|
||||
{
|
||||
projectId: "proj_test",
|
||||
workingDirectory: dir,
|
||||
isolationMode: "in-process",
|
||||
maxConcurrent: 1,
|
||||
maxWorktrees: 1,
|
||||
},
|
||||
{} as never,
|
||||
{ skipNotifier: true },
|
||||
);
|
||||
|
||||
await runMergeCycle(engine, task.id);
|
||||
|
||||
expect(task.column).toBe("done");
|
||||
expect(task.status ?? null).toBeNull();
|
||||
expect(task.mergeDetails?.mergeConfirmed).toBe(true);
|
||||
expect(comments.some((comment) => comment.includes("Please fix the failing"))).toBe(false);
|
||||
expect(logs.some((entry) => entry.includes("[verification] post-finalize verification failed for already-on-main fast-path; no action"))).toBe(true);
|
||||
|
||||
const manager = new SelfHealingManager(store, { rootDir: dir, getExecutingTaskIds: () => new Set() });
|
||||
await expect(manager.recoverStaleMergingStatus()).resolves.toBe(0);
|
||||
await expect(manager.recoverInterruptedMergingTasks()).resolves.toBe(0);
|
||||
expect(task.column).toBe("done");
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
}, 20_000);
|
||||
});
|
||||
@@ -545,6 +545,49 @@ export async function classifyForeignCommits(
|
||||
}
|
||||
}
|
||||
|
||||
export interface ClassifyMisroutedForeignCommitInput {
|
||||
repoDir: string;
|
||||
sha: string;
|
||||
commitSubject: string;
|
||||
commitBody: string;
|
||||
currentTaskId: string;
|
||||
}
|
||||
|
||||
export interface ClassifyMisroutedForeignCommitResult {
|
||||
misrouted: boolean;
|
||||
foreignTaskId?: string;
|
||||
paths: string[];
|
||||
}
|
||||
|
||||
export async function classifyMisroutedForeignCommit(
|
||||
input: ClassifyMisroutedForeignCommitInput,
|
||||
): Promise<ClassifyMisroutedForeignCommitResult> {
|
||||
const { repoDir, sha, commitSubject, commitBody, currentTaskId } = input;
|
||||
const subjectPattern = /^(feat|fix|test|chore|docs|refactor|perf|build)\((FN-\d+)\):/i;
|
||||
const trailerPattern = /(?:^|\n)Fusion-Task-Id:\s*(FN-\d+)\s*(?:\n|$)/i;
|
||||
const subjectMatch = commitSubject.match(subjectPattern);
|
||||
const trailerMatch = commitBody.match(trailerPattern);
|
||||
const foreignTaskId = (trailerMatch?.[1] ?? subjectMatch?.[2] ?? "").toUpperCase();
|
||||
if (!foreignTaskId || foreignTaskId === currentTaskId.toUpperCase()) {
|
||||
return { misrouted: false, paths: [] };
|
||||
}
|
||||
|
||||
const pathsOutput = await runGit(
|
||||
repoDir,
|
||||
`git diff-tree --root --no-commit-id --name-only -r ${quoteShellArg(sha)}`,
|
||||
).catch(() => "");
|
||||
const paths = pathsOutput
|
||||
.split("\n")
|
||||
.map((line) => line.trim())
|
||||
.filter(Boolean);
|
||||
|
||||
return {
|
||||
misrouted: paths.length > 0 && paths.every((path) => path.startsWith(".changeset/")),
|
||||
foreignTaskId,
|
||||
paths,
|
||||
};
|
||||
}
|
||||
|
||||
export async function classifyForeignOnlyContamination(
|
||||
input: ClassifyForeignOnlyContaminationInput,
|
||||
): Promise<ClassifyForeignOnlyContaminationResult> {
|
||||
|
||||
@@ -59,6 +59,7 @@ import {
|
||||
classifyBootstrapMisbinding,
|
||||
classifyForeignCommits,
|
||||
classifyForeignOnlyContamination,
|
||||
classifyMisroutedForeignCommit,
|
||||
isBranchConflictError,
|
||||
reanchorBranchToBase,
|
||||
inspectBranchConflict,
|
||||
|
||||
@@ -1563,6 +1563,22 @@ export class ProjectEngine {
|
||||
errorMsg.includes("Deterministic build verification failed");
|
||||
|
||||
if (taskOnErr && isVerificationError) {
|
||||
const refreshedTaskOnVerificationError = await store.getTask(taskId).catch(() => null);
|
||||
if (
|
||||
refreshedTaskOnVerificationError?.column === "done"
|
||||
&& refreshedTaskOnVerificationError.mergeDetails?.mergeConfirmed === true
|
||||
) {
|
||||
const commitSha = refreshedTaskOnVerificationError.mergeDetails.commitSha;
|
||||
const shortSha = typeof commitSha === "string" && commitSha.length > 0
|
||||
? commitSha.slice(0, 8)
|
||||
: "unknown";
|
||||
const errorTail = errorMsg.length > 200 ? `${errorMsg.slice(0, 200)}…` : errorMsg;
|
||||
const message = `[verification] post-finalize verification failed for already-on-main fast-path; no action (commit=${shortSha}, error=${errorTail})`;
|
||||
await store.logEntry(taskId, message, "VerificationError").catch(() => undefined);
|
||||
runtimeLog.log(`Auto-merge: ${taskId} ${message}`);
|
||||
continue;
|
||||
}
|
||||
|
||||
if (
|
||||
err instanceof VerificationError
|
||||
&& err.verificationResult?.environmentFault?.kind === "missing-workspace-entry"
|
||||
|
||||
@@ -162,6 +162,7 @@ export type DatabaseMutationType =
|
||||
| "task:auto-recover-already-merged"
|
||||
| "task:auto-recover-finalize-already-on-main"
|
||||
| "task:auto-recover-branch-misbound"
|
||||
| "task:auto-recover-misrouted-foreign-commit"
|
||||
| "task:auto-recover-foreign-only-contamination"
|
||||
| "task:auto-recover-foreign-only-contamination-skipped"
|
||||
| "task:auto-recover-node-unreachable"
|
||||
|
||||
Reference in New Issue
Block a user