fix(merger): refuse no-op finalize when modifiedFiles claims work was done
Third root-cause fix in the FN-5475 sweep. When `aiMergeTask` / `recoverNoOpReviewTasks` classified a task as `proven-no-op` or `no-changes-finalized`, both call sites moved the task to Done while clearing `modifiedFiles: []` — silently destroying the audit trail when the work product was uncommitted in the worktree, squashed against the wrong branch, or dropped by reuse-handoff churn. This was the load-bearing site of the FN-5490 / FN-5517 / FN-5526 / FN-5540 lost-work patterns. Both call sites now check `task.modifiedFiles.length` before finalizing as no-op. If the task claims work was done but no commit landed, the task is moved back to `todo` with progress preserved and a new `task:finalize-lost-work-blocked` audit event is emitted. The next executor run re-attempts the work; the operator sees the audit event in the timeline. The post-hoc `reconcileDoneTaskIntegrity` path is intentionally NOT gated — it cleans up already-Done tasks (legacy state) and is out-of-scope for prevention. 9 lost-work tasks already in this state at sweep time are cataloged in docs/incidents/2026-05-23-lost-work-tasks.md for fresh re-spec. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
30
.changeset/fix-merger-lost-work-guard.md
Normal file
30
.changeset/fix-merger-lost-work-guard.md
Normal file
@@ -0,0 +1,30 @@
|
||||
---
|
||||
"@fusion/engine": patch
|
||||
---
|
||||
|
||||
fix(merger): refuse to finalize a task as no-op when modifiedFiles is non-empty
|
||||
|
||||
Third root-cause fix for tasks marked Done with no commit on main (the first
|
||||
two — sibling-branch merge target + grep mis-attribution — landed in the
|
||||
previous commit). When the executor produced edits but the squash didn't
|
||||
land them as a commit (uncommitted in the worktree, squashed against the
|
||||
wrong branch, branch dropped by reuse-handoff churn, etc.), the merger's
|
||||
`classifyOwnedLandedEvidence` would return `proven-no-op` or
|
||||
`no-changes-finalized` and both `aiMergeTask` and `recoverNoOpReviewTasks`
|
||||
would happily move the task to Done while clearing `modifiedFiles` to `[]`
|
||||
— silently destroying the audit trail of what was lost.
|
||||
|
||||
Both call sites now gate the no-op finalize on `task.modifiedFiles.length`:
|
||||
if the task claims work was done but no commit landed, move the task back
|
||||
to `todo` with progress preserved and emit a new
|
||||
`task:finalize-lost-work-blocked` audit event. The next executor run
|
||||
re-attempts the work; the operator sees the audit event in the run-audit
|
||||
timeline.
|
||||
|
||||
The post-hoc `reconcileDoneTaskIntegrity` path is intentionally NOT gated —
|
||||
it cleans up tasks that are already in Done (legacy state), which is
|
||||
out-of-scope for the lost-work prevention. This matters: 9 lost-work tasks
|
||||
were already in this state at sweep time (FN-5441, FN-5446, FN-5487,
|
||||
FN-5490, FN-5517, FN-5526, FN-5539, FN-5540, FN-5542) and need to be
|
||||
re-spec'd as fresh tasks rather than auto-reconciled. See
|
||||
`docs/incidents/2026-05-23-lost-work-tasks.md` for the per-task catalog.
|
||||
69
docs/incidents/2026-05-23-lost-work-tasks.md
Normal file
69
docs/incidents/2026-05-23-lost-work-tasks.md
Normal file
@@ -0,0 +1,69 @@
|
||||
# Lost-work tasks discovered 2026-05-23
|
||||
|
||||
While investigating why FN-5475 was stuck in preflight, a board audit found
|
||||
14 tasks marked Done but missing from `main`:
|
||||
|
||||
- **2 recovered** to main by cherry-pick or supersession
|
||||
- FN-5233 → cherry-picked as `2d2e5b809` (stranded squash on `fusion/fn-5339`)
|
||||
- FN-5530 → already on main via FN-5482's `b22112af8`
|
||||
- **3 legitimate verification-only no-ops** (kept as Done)
|
||||
- FN-5472, FN-5484 — branch existed but had zero commits ahead of base
|
||||
- FN-5515 — auto-finalized as no-op (also see "incomplete-step finalize" below)
|
||||
- **9 lost-work tasks** — need re-execution (catalog below)
|
||||
|
||||
Three engine bugs produced these outcomes; all three are fixed in this
|
||||
sweep so the same patterns can't reproduce going forward:
|
||||
|
||||
1. **`resolveTaskMergeTarget` returned a sibling `fusion/fn-*` branch as the
|
||||
merge target** when `task.baseBranch` was inherited from a
|
||||
sibling-dispatched parent. The squash landed on the sibling branch and
|
||||
`advanceIntegrationBranchRef` advanced that ref instead of `main`. Fixed
|
||||
in `packages/core/src/task-merge.ts` — sibling branches now rejected with
|
||||
a `merge:merge-target-rejected-fusion-sibling` audit event.
|
||||
2. **`findLandedTaskCommit` blindly accepted the first `git log --grep`
|
||||
hit**, mis-attributing FN-5441/5446 to an unrelated FN-5483 commit whose
|
||||
body merely *mentioned* them. Fixed in
|
||||
`packages/engine/src/self-healing.ts` — `commitOwnedByTask` is now
|
||||
line-anchored (trailers or conventional-commit subject), and step (4)
|
||||
re-verifies each candidate's body.
|
||||
3. **No-op finalize cleared `modifiedFiles` when the task claimed real
|
||||
work.** Fixed in `merger.ts:aiMergeTask` and
|
||||
`self-healing.ts:recoverNoOpReviewTasks` — if a task claims
|
||||
`modifiedFiles` but the classifier would finalize as no-op, the task is
|
||||
moved back to `todo` (with progress preserved) and a
|
||||
`task:finalize-lost-work-blocked` audit event is emitted instead of
|
||||
silently destroying the audit trail.
|
||||
|
||||
## Re-spec catalog (the 9 lost tasks)
|
||||
|
||||
The original `PROMPT.md` files are intact under `.fusion/tasks/<FN-ID>/`. The
|
||||
table below condenses each task's file scope and acceptance criteria so a
|
||||
fresh executor run can re-implement them. Re-create each as a new Fusion
|
||||
task referencing the original ID for context.
|
||||
|
||||
| FN-ID | Title | File scope | What needs to land |
|
||||
|---|---|---|---|
|
||||
| **FN-5441** | Document mergeIntegrationWorktree modes | `docs/settings-reference.md` (+ `docs/architecture.md` if it enumerates modes) | Row per value in `MergeIntegrationWorktreeMode`; default from `DEFAULT_PROJECT_SETTINGS`; cover reuse-task-worktree opt-in semantics, deprecation/alias normalization, worktrunk shortcut, `merge:cwd-integration-fallback-removed` audit. Docs-only. |
|
||||
| **FN-5446** | Fix soft-delete deadlock + `blockedBy` residue | `packages/core/src/store.ts`, `packages/engine/src/scheduler.ts`, `packages/engine/src/self-healing.ts`, `packages/engine/src/run-audit.ts` + new tests in `engine/__tests__/reliability-interactions/soft-delete-blocker-residue.test.ts` and `core/__tests__/store-delete-task-blocker-residue.test.ts` | `deleteTask` clears `blockedBy`+`status` on dependents in same transaction; scheduler `task:deleted` handler mirrors `task:moved→done/archived`; new `reconcileSoftDeletedColumnDrift` self-heal w/ `task:soft-delete-column-reconciled` audit; `clearStaleBlockedBy` recognizes soft-deleted blockers; preserve FN-5147/FN-5208 invariants. |
|
||||
| **FN-5487** | Self-edge removal blocked by cycle guard | `packages/core/src/store.ts`, regression tests in `core/__tests__/store-dependency-cycle.test.ts` and `engine/__tests__/reliability-interactions/dependency-cycle-reconcile.test.ts` | Make `updateTask` cycle guard delta-aware — skip `assertNoDependencyCycle` when `updates.dependencies` adds no new edges; `createTask`/`createTaskWithReservedId`/`applyReplicatedTaskCreate` untouched; self-edge guard preserved; depends on FN-5432 already on main. |
|
||||
| **FN-5490** | Tokenize bare hex colors in dashboard CSS | `packages/dashboard/app/components/ScriptsModal.css`, `SettingsSyncLog.css`, `app/__tests__/dashboard-component-color-tokenization.test.ts` | Replace `#58a6ff` + `#0969da` in ScriptsModal diff-hunk with `var(--color-info, …)`; replace `#d29922` in SettingsSyncLog `--conflict` with `var(--color-warning, …)`; keep light-theme override structure; regression test asserts no bare hex outside `var(…)` fallbacks. |
|
||||
| **FN-5517** | Compound @media regex helpers (768px/480px) | ~55 dashboard test files only (no CSS, no prod) | Replace broken `\)\s*\{` after `\(max-width: 768px\)` with `\)[^{]*\{`; same fix to `[^)]*` variant; convert literal `indexOf("@media (max-width: 768px) {")` to regex search; add compound-query synthetic unit test in `board-mobile.test.tsx`. |
|
||||
| **FN-5526** | Reuse-worktree audit emit sites | `packages/engine/src/merger.ts`, `engine/__tests__/reliability-interactions/merge-reuse-task-worktree.test.ts` | Extend `emitReuseHandoffAuditEvent` union with `"merge:reuse-worktree-fresh-acquire"` + `"merge:reuse-worktree-fresh-acquired"`; capture `priorWorktreePath`; emit fresh-acquire/fresh-acquired before existing `merge:reuse-fallback-new-worktree`. Final order: `fresh-acquire → fresh-acquired → fallback-new-worktree`. Step 0 short-circuit if FN-5449 already landed it. |
|
||||
| **FN-5539** | Typecheck fix: undefined `worktreePool` | `packages/cli/src/commands/dashboard.ts` only | Remove trailing `worktreePool` arg from `processPullRequestMergeTask(...)` call inside `onMergeImpl` (~line 1206); leave the non-dev `ProjectEngineManager` callback (~line 1520) untouched. |
|
||||
| **FN-5540** | Re-spec FN-5515 regression matrix | `.fusion/tasks/FN-5515/PROMPT.md` only (gitignored) | Rewrite FN-5515's PROMPT so the matrix is permanent backstop using `it(...)` only; append 4 cases to `executor-abort-all-in-flight.test.ts` and 8 to `engine-stop-aborts-execution.test.ts`; extend existing AGENTS FN-5403 Reliability bullet; explicit denylist of existing case titles; save via `fn_task_document_write(key="docs")`. |
|
||||
| **FN-5542** | Message-delivery test deflake | `packages/engine/src/agent-tools.ts`, `engine/__tests__/reliability-interactions/auto-recovery-message-delivery.test.ts` | Add optional `deliveryHandler?: MessageDeliveryAutoRecoveryHandler` to `createSendMessageTool` and `createPostRoomMessageTool`; default preserved when omitted; un-skip both flaky cases and inject a handler with `sleep: vi.fn(async () => {})` + inert `runAudit`; full reliability pool 3× clean. |
|
||||
|
||||
## Patterns
|
||||
|
||||
- **Mis-attributed-to-unrelated-commit** (FN-5441, FN-5446): both attributed
|
||||
to `e3dbfaae` — an FN-5483 commit whose body mentioned them in prose.
|
||||
- **Tree-verification-only finalize** (FN-5490, FN-5539, FN-5542): logs show
|
||||
"Recorded verification pass for tree `<SHA>`" but the tree was never
|
||||
promoted to a commit; the no-op finalize then cleared `modifiedFiles`.
|
||||
- **Finalized with incomplete steps** (FN-5526 at 1/7, FN-5517 at 8/8 with
|
||||
no commit, FN-5487 at 6/6 with no commit, FN-5540 at 5/5): step-gate
|
||||
passed but the work product never made it to a commit.
|
||||
|
||||
The three engine fixes in this sweep prevent the *recurrence* of these
|
||||
patterns. The lost work itself still needs to be re-implemented as fresh
|
||||
Fusion tasks.
|
||||
@@ -151,7 +151,13 @@ describeIfGit("aiMergeTask finalize no-op unproven reproduction (real git)", ()
|
||||
expect(classification).toEqual({ kind: "proven-no-op", baseRef: "main", ownDiffEmpty: true });
|
||||
});
|
||||
|
||||
it("auto-finalizes proven no-op and clears stale modifiedFiles", async () => {
|
||||
// FN-5490/FN-5517/FN-5526/FN-5540 regression: the previous contract here
|
||||
// was "auto-finalize proven no-op and clear stale modifiedFiles", which
|
||||
// turned out to be the bug — claimed modifiedFiles + no commit = lost work
|
||||
// (uncommitted in the worktree or squashed against the wrong branch), not
|
||||
// a legitimate no-op. The merger now refuses to finalize and moves the
|
||||
// task back to todo with progress preserved instead.
|
||||
it("FN-5490: refuses no-op finalize when modifiedFiles are claimed without a commit", async () => {
|
||||
const repo = mkdtempSync(join(tmpdir(), "fusion-merger-noop-finalize-"));
|
||||
repos.push(repo);
|
||||
git(repo, "git init -b main");
|
||||
@@ -183,10 +189,17 @@ describeIfGit("aiMergeTask finalize no-op unproven reproduction (real git)", ()
|
||||
const store = createStore(task);
|
||||
const result = await aiMergeTask(store, repo, "FN-C");
|
||||
|
||||
expect(result.merged).toBe(true);
|
||||
expect(result.noOpMerge).toBe(true);
|
||||
expect((store.updateTask as ReturnType<typeof vi.fn>).mock.calls.some(([, patch]) => patch?.modifiedFiles?.length === 0)).toBe(true);
|
||||
expect((store.moveTask as ReturnType<typeof vi.fn>).mock.calls.some(([, column]) => column === "done")).toBe(true);
|
||||
// Lost-work guard fires — task does NOT advance to done, does NOT have
|
||||
// modifiedFiles cleared, and gets moved back to todo with progress.
|
||||
expect(result.merged).toBe(false);
|
||||
expect(result.error).toMatch(/lost-work/);
|
||||
expect(
|
||||
(store.updateTask as ReturnType<typeof vi.fn>).mock.calls.some(
|
||||
([, patch]) => Array.isArray(patch?.modifiedFiles) && patch.modifiedFiles.length === 0,
|
||||
),
|
||||
).toBe(false);
|
||||
expect((store.moveTask as ReturnType<typeof vi.fn>).mock.calls.some(([, column]) => column === "done")).toBe(false);
|
||||
expect((store.moveTask as ReturnType<typeof vi.fn>).mock.calls.some(([, column]) => column === "todo")).toBe(true);
|
||||
}, 20_000);
|
||||
|
||||
it("blocks FN-4653 shape: foreign start-point branch with no FN-owned commits", async () => {
|
||||
|
||||
@@ -621,8 +621,23 @@ export type OwnedLandedClassification =
|
||||
details: Record<string, unknown>;
|
||||
};
|
||||
|
||||
function escapeRegexForOwnership(value: string): string {
|
||||
return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
|
||||
}
|
||||
|
||||
/**
|
||||
* Decide whether a git commit belongs to a given task. Line-anchored trailers
|
||||
* and subject-anchored conventional commits only — prose mentions never count.
|
||||
* Mirrors `commitOwnedByTask` in self-healing.ts (FN-5441/FN-5446 regression).
|
||||
*/
|
||||
function commitOwnedByTask(taskId: string, subject: string, body: string): boolean {
|
||||
return body.includes(`${FUSION_TASK_ID_TRAILER_KEY}: ${taskId}`) || subject.includes(taskId);
|
||||
if (new RegExp(`(?:^|\\n)${escapeRegexForOwnership(FUSION_TASK_ID_TRAILER_KEY)}: ${escapeRegexForOwnership(taskId)}\\s*(?:\\n|$)`).test(body)) {
|
||||
return true;
|
||||
}
|
||||
const subjectAnchor = new RegExp(
|
||||
`^(?:[A-Za-z]+(?:\\([^)]*\\b${escapeRegexForOwnership(taskId)}\\b[^)]*\\))?:|${escapeRegexForOwnership(taskId)}:)`,
|
||||
);
|
||||
return subjectAnchor.test(subject);
|
||||
}
|
||||
|
||||
async function findOwnedLandedCommitForTask(rootDir: string, task: Task): Promise<OwnedLandedCommit | null> {
|
||||
@@ -7886,6 +7901,44 @@ export async function aiMergeTask(
|
||||
}
|
||||
|
||||
if (classification.kind === "proven-no-op" || classification.kind === "no-changes-finalized") {
|
||||
// FN-5490/FN-5517/FN-5526/FN-5540 guard: the classifier only sees git
|
||||
// evidence, but the task itself can attest that work happened. When
|
||||
// modifiedFiles is non-empty AND no commit landed, that's lost work
|
||||
// (uncommitted in the worktree, or the squash committed the wrong tree)
|
||||
// — NOT a legitimate no-op. Demote to the unproven-recovery path which
|
||||
// moves the task back to todo with progress preserved instead of
|
||||
// clearing modifiedFiles to [].
|
||||
if (task.modifiedFiles && task.modifiedFiles.length > 0) {
|
||||
const reason = `lost-work-detected: ${task.modifiedFiles.length} modifiedFiles claimed but no commit landed`;
|
||||
await store.updateTask(taskId, { error: reason });
|
||||
await store.logEntry(
|
||||
taskId,
|
||||
`Finalize blocked (lost-work guard): task claims ${task.modifiedFiles.length} modifiedFiles but classification would finalize as no-op — moving back to todo with progress preserved`,
|
||||
JSON.stringify({
|
||||
modifiedFilesSample: task.modifiedFiles.slice(0, 5),
|
||||
classification: classification.kind,
|
||||
}, null, 2),
|
||||
);
|
||||
await (store as any).recordRunAuditEvent?.({
|
||||
domain: "database",
|
||||
mutationType: "task:finalize-lost-work-blocked",
|
||||
target: taskId,
|
||||
metadata: {
|
||||
modifiedFilesCount: task.modifiedFiles.length,
|
||||
classification: classification.kind,
|
||||
},
|
||||
});
|
||||
await store.moveTask(taskId, "todo", { preserveProgress: true, moveSource: "engine" } as any);
|
||||
await releaseReuseHandoffEarly("lost-work-blocked");
|
||||
return {
|
||||
task,
|
||||
branch,
|
||||
merged: false,
|
||||
worktreeRemoved: false,
|
||||
branchDeleted: false,
|
||||
error: reason,
|
||||
};
|
||||
}
|
||||
const noOpReason = classification.kind === "proven-no-op"
|
||||
? `branch has zero commits ahead of ${classification.baseRef}`
|
||||
: "verification-only finalize: no branch and no owned commits";
|
||||
|
||||
@@ -497,6 +497,14 @@ export type DatabaseMutationType =
|
||||
| "session:runtime-resolved"
|
||||
| "task:in-review-stall-deadlock-disposed"
|
||||
| "task:finalize-unproven-blocked"
|
||||
/**
|
||||
* FN-5490/FN-5517/FN-5526/FN-5540 lost-work guard: the merger or self-heal
|
||||
* sweep refused to finalize a task as no-op because its record claimed
|
||||
* `modifiedFiles` while no commit landed. Task is moved back to todo with
|
||||
* progress preserved instead of silently clearing modifiedFiles to [].
|
||||
* Metadata: { modifiedFilesCount, classification, baseRef? }
|
||||
*/
|
||||
| "task:finalize-lost-work-blocked"
|
||||
| "task:integrity-reconcile-modified-files"
|
||||
| "task:integrity-warning"
|
||||
/** FN-5092 watchdog: stale `status: "merging"` / `"merging-pr"` cleared on a done/archived task. Metadata: { previousColumn, previousStatus, ageMs, mergeConfirmed?: boolean } */
|
||||
|
||||
@@ -3989,7 +3989,7 @@ export class SelfHealingManager {
|
||||
return recovered;
|
||||
}
|
||||
|
||||
private async recordIntegrityAudit(taskId: string, mutationType: "task:finalize-unproven-blocked" | "task:integrity-reconcile-modified-files" | "task:integrity-warning" | "task:auto-recover-stale-merger-status", metadata: Record<string, unknown>): Promise<void> {
|
||||
private async recordIntegrityAudit(taskId: string, mutationType: "task:finalize-unproven-blocked" | "task:finalize-lost-work-blocked" | "task:integrity-reconcile-modified-files" | "task:integrity-warning" | "task:auto-recover-stale-merger-status", metadata: Record<string, unknown>): Promise<void> {
|
||||
const auditor = createRunAuditor(this.store, {
|
||||
runId: generateSyntheticRunId("self-healing-integrity", taskId),
|
||||
agentId: "self-healing",
|
||||
@@ -4159,6 +4159,32 @@ export class SelfHealingManager {
|
||||
await this.store.updateTask(task.id, { mergeDetails });
|
||||
await this.store.logEntry(task.id, `Auto-finalized: recovered owned landed commit ${classification.commit.sha.slice(0, 8)}`);
|
||||
} else {
|
||||
// FN-5490/FN-5517/FN-5526/FN-5540 guard: same lost-work check as
|
||||
// merger.ts:aiMergeTask. The self-heal path was the historical
|
||||
// primary site of the bug — it would clear `modifiedFiles: []`
|
||||
// (line below) while moving the task to Done, silently destroying
|
||||
// the audit trail of the lost work. Now we refuse to finalize and
|
||||
// move the task back to todo with progress preserved so the next
|
||||
// executor run can re-attempt.
|
||||
if (task.modifiedFiles && task.modifiedFiles.length > 0) {
|
||||
await this.store.logEntry(
|
||||
task.id,
|
||||
`Finalize blocked (lost-work guard): task claims ${task.modifiedFiles.length} modifiedFiles but classification would finalize as no-op — moving back to todo with progress preserved`,
|
||||
JSON.stringify({
|
||||
modifiedFilesSample: task.modifiedFiles.slice(0, 5),
|
||||
classification: "proven-no-op",
|
||||
baseRef: classification.baseRef,
|
||||
}, null, 2),
|
||||
);
|
||||
await this.recordIntegrityAudit(task.id, "task:finalize-lost-work-blocked", {
|
||||
modifiedFilesCount: task.modifiedFiles.length,
|
||||
classification: "proven-no-op",
|
||||
baseRef: classification.baseRef,
|
||||
});
|
||||
await this.store.moveTask(task.id, "todo", { preserveProgress: true, moveSource: "engine" });
|
||||
recovered++;
|
||||
continue;
|
||||
}
|
||||
const noOpReason = `branch has zero commits ahead of ${classification.baseRef}`;
|
||||
const mergeDetails: MergeDetails = {
|
||||
...(task.mergeDetails || {}),
|
||||
|
||||
Reference in New Issue
Block a user