root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Unowned: the R7 sweep guessed with another workflow's columns — its "do not guess" guard was unreachable dead code (#2600)

Browse Source 
## Unowned: the R7 sweep's "do not guess a column" guard could not fire

Picked up from my own #2543 finding. Independent of my other PRs.

### The guard existed in comment form only

`reconcileUndeclaredTaskColumns` wraps IR resolution in a try/catch
whose comment reads:

> An unresolvable workflow is its own fault path; do not guess a column.

But `resolveWorkflowIrById` catches **every** failure and returns
`defaultCodingWorkflowIr()`, and `resolveWorkflowIrForTask` does the
same for a failed selection read. The resolver never rejects, so that
catch is **dead code**.

What actually happened to a card whose workflow could not be loaded: it
was judged against the **default** workflow, and if its column was not
one the default declares, the sweep re-homed it to the **default's**
rebound target. It guessed, using a workflow that is not the card's own
— the precise outcome the guard was written to prevent, in a **startup
recovery path that runs against every task**.

### How it was found, which is the part worth keeping

By being **unable to make a test of the guard fail**. Three separate
mutations all passed — deleting the `continue`, deleting the try/catch,
and simulating a whole-sweep abort at that very catch. I had written
that off once as "this case pins the outcome, not the mechanism". The
inability was the signal, not a limitation of the assertion: the branch
is unreachable.

This is the seventh instance of the program's core shape, and the first
I found in a guard I had just finished writing coverage for.

### The fix

The sweep now **proves the resolved IR belongs to the task** before
moving its card: it reads the task's workflow selection and confirms
that id resolves to a real definition (built-in or stored).

- A task with **no** selection legitimately resolves to the default
workflow — not treated as unresolvable.
- An unreadable selection **read** is itself grounds not to guess.

Placed at the **move site**, not at resolution, deliberately: it costs
one definition read only for a card already about to be moved — a
healthy board reaches that line for nobody — and it keeps the fix inside
the sweep instead of changing a resolver whose soft-failure many other
callers depend on. Changing `resolveWorkflowIrById` to reject would have
been the tidier-looking fix and a much wider blast radius.

### Revert-proof, both directions

- Remove the proof → the case fails `expected 2 to be 1`: the unloadable
card is re-homed on a guess.
- The same case asserts the neighbour **is** still repaired, so the fix
cannot be mistaken for letting one bad card disable the sweep for
everyone else. That is the per-task isolation property, and a
single-task fixture cannot distinguish it from a whole-sweep abort —
verified by injecting a throw at the loop head (`expected 0 to be 2`).

### Verification

`pnpm test:gate` (482 + 10 + 71), `pnpm lint`, engine typecheck green.
Sweep suite + `legacy-tombstones`: 13 passed.


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* Prevented startup recovery from moving cards into incorrect columns
when their workflow cannot be loaded or resolved.
* Cards with unreadable workflow information now remain in place, while
other recoverable cards continue to be repaired correctly.
* Added safeguards to avoid guessing a fallback workflow during column
reconciliation.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-07-29 21:21:04 -07:00
committed by GitHub
parent 534798dea0
commit 89d6d76d60
3 changed files with 188 additions and 39 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
.changeset/u12-sweep-unresolvable-guard.md Normal file
View File

@@ -0,0 +1,7 @@
---
"@runfusion/fusion": patch
---
summary: Startup recovery no longer moves a card using another workflow's columns when its own cannot be loaded.
category: fix
dev: `reconcileUndeclaredTaskColumns`'s unresolvable-workflow guard was unreachable — `resolveWorkflowIrById` swallows every failure and returns the default IR, so a card with an unloadable workflow was judged against `builtin:coding` and re-homed to ITS rebound target. The sweep now proves the task's selection resolves to a real definition before moving, checked only for cards already about to move.

149
packages/engine/src/__tests__/self-healing-undeclared-column.test.ts
View File

@@ -23,14 +23,14 @@ assertions exact rather than inferred from absence of change.
MECHANISM COVERAGE, measured rather than claimed. Deleting the user-pause guard fails 2
cases; deleting the already-declared short-circuit fails 2 cases. The
unresolvable-workflow case was REWRITTEN after PR #2543 review: chasing per-task isolation
proved the sweep's unresolvable-workflow guard is UNREACHABLE (the IR resolver never
rejects), so the case now asserts what actually happens — resolution falls back to the
default workflow and the card is re-homed on a guess — plus the isolation property the
review asked for. See the note at that case.
unloadable-workflow case asserts the guard that now EXISTS: the sweep proves the resolved
IR belongs to the task before moving its card. Before that fix the resolver never rejected
and the card was re-homed against the DEFAULT workflow — a guess. See the note at that
case.
*/
import { describe, expect, it, vi } from "vitest";
import type { Task, WorkflowIr } from "@fusion/core";
import { getBuiltinWorkflow } from "@fusion/core";
import { SelfHealingManager } from "../self-healing.js";
/** A workflow declaring `intake` + `hold` + `done`, with NO `todo` column. A card stored
@@ -140,47 +140,57 @@ describe("reconcileUndeclaredTaskColumns (U12 — R7)", () => {
});
/*
FNXC:WorkflowColumns 2026-07-29-00:00 (PR #2543 review — CodeRabbit, and a real finding):
THE SWEEP'S "UNRESOLVABLE WORKFLOW" GUARD CANNOT FIRE. Chasing the review's request to
prove per-task isolation, I could not make the case fail under any mutation — including
simulating a whole-sweep abort. The reason is upstream: `resolveWorkflowIrById` catches
EVERY failure and returns `defaultCodingWorkflowIr()`, and `resolveWorkflowIrForTask`
does the same for a failed selection read. So `resolveWorkflowIrForTask` never rejects,
the sweep's `try/catch` around it is unreachable, and its comment — "do not guess a
column" — describes protection that does not exist.
FNXC:WorkflowColumns 2026-07-29-00:00 (U12 — the guard now EXISTS):
A card whose workflow cannot be loaded is SKIPPED, and its neighbour is still repaired.
What ACTUALLY happens to a card whose workflow cannot be read: it is judged against the
DEFAULT workflow. If its column is not one the default declares, the sweep re-homes it
to the default's rebound target — i.e. it guesses, using a workflow that is not the
card's own. That is the exact outcome the guard was written to prevent.
History, because it matters for reading this file: the sweep's `catch` around IR
resolution was dead code. `resolveWorkflowIrById` swallows every failure and returns
`defaultCodingWorkflowIr()`, so the resolver never rejected and a card with an
unloadable workflow was judged against the DEFAULT and re-homed to the DEFAULT's
rebound target — the sweep guessed with someone else's workflow, the exact outcome the
guard's comment claimed to prevent. Discovered by being unable to make a test of that
guard fail (PR #2543); the protection is added in this change.
This test now asserts the real behaviour rather than the intended behaviour, because a
test asserting the intent would pass for the wrong reason and hide the gap. The fix
belongs upstream (the resolver needs to signal failure, or the sweep needs to detect
"I was handed the default IR but this task selects something else") and is a behaviour
change, not test work — flagged in the PR body rather than smuggled in here.
The per-task isolation the review asked for IS pinned: two stranded cards, one with an
unreadable workflow, and the sweep must still process both rather than dying on the
first.
Both halves are asserted deliberately: the unloadable card must NOT move (no guessing),
and the neighbour MUST move (one bad card cannot disable the sweep for everyone else —
the per-task isolation property, which a single-task fixture cannot distinguish from a
whole-sweep abort).
*/
it("resolves an unreadable workflow to the DEFAULT and keeps processing other cards", async () => {
const unreadable = task({ id: "FN-3", column: "custom-lane" as Task["column"] });
it("skips a card whose workflow cannot be loaded, and still repairs its neighbour", async () => {
const unloadable = task({ id: "FN-3", column: "custom-lane" as Task["column"] });
const neighbour = task({ id: "FN-3b", column: "todo" });
const store = makeStore([unreadable, neighbour], { unresolvableWorkflowFor: "FN-3" });
const store = makeStore([unloadable, neighbour], { unresolvableWorkflowFor: "FN-3" });
const rehomed = await manager(store).reconcileUndeclaredTaskColumns();
expect(await manager(store).reconcileUndeclaredTaskColumns()).toBe(1);
expect(store.moveTask).toHaveBeenCalledTimes(1);
expect(store.moveTask.mock.calls[0]![0]).toBe("FN-3b");
/*
Both cards move: the neighbour correctly (its own workflow lacks `todo`), and FN-3 on a
GUESS against the default workflow. A failure here of `2 -> 0` would mean the resolver
exception aborted the whole sweep, which is the isolation property under test.
*/
expect(rehomed).toBe(2);
expect(store.moveTask.mock.calls.map((call) => call[0]).sort()).toEqual(["FN-3", "FN-3b"]);
// No guess: the card stays exactly where it was.
expect(unloadable.column).toBe("custom-lane");
expect(neighbour.column).toBe("triage");
});
/*
FNXC:WorkflowColumns 2026-07-29-00:00 (PR #2600 review — greptile):
A STALE BUILT-IN id must not pass the resolvability proof. `isBuiltinWorkflowId` is a
PREFIX check, so `builtin:removed-workflow` satisfied it — and an unknown built-in id
resolves to the default coding IR, so the card was still re-homed against a workflow that
is not its own. The guard reproduced the hole it was added to close.
REVERT CHECK: restore the prefix check (`isBuiltinWorkflowId(...) => true`) and this
fails — the card is re-homed to the default's target instead of being left alone.
*/
it("skips a card selecting a built-in workflow that no longer exists", async () => {
const stale = task({ id: "FN-9", column: "custom-lane" as Task["column"] });
const store = makeStore([stale]);
// Prefix-valid, existence-invalid.
store.getTaskWorkflowSelectionAsync = vi.fn(async () => ({ workflowId: "builtin:removed-workflow" }));
expect(await manager(store).reconcileUndeclaredTaskColumns()).toBe(0);
expect(store.moveTask).not.toHaveBeenCalled();
expect(stale.column).toBe("custom-lane");
});
it("is idempotent: a second run does not move the card again", async () => {
const stranded = task({ id: "FN-4", column: "todo" });
const store = makeStore([stranded]);
@@ -215,3 +225,68 @@ describe("reconcileUndeclaredTaskColumns (U12 — R7)", () => {
expect(paused.column).toBe("todo");
});
});
/*
FNXC:WorkflowColumns 2026-07-29-00:00 (PR #2600 review — greptile):
"PLUGIN-GATED WORKFLOWS PASS PROOF" — checked, and the guard is CORRECT as written.
The claim was that `getBuiltinWorkflow` finds a plugin-gated built-in's static definition
even when the store would reject it, so the guard returns true while the resolver falls
back to the default IR and the sweep guesses anyway.
The second half does not hold. `resolveWorkflowIrById`'s built-in branch reads
`getBuiltinWorkflow(workflowId)` and RETURNS ITS IR WITHOUT EVER CALLING THE STORE — a
store rejection is unreachable for a built-in id. So a plugin-gated selection resolves to
that workflow's OWN IR whether or not its plugin is installed, and there is no guess for
the guard to have prevented.
That is not a coincidence to leave undefended: the guard is right precisely BECAUSE it
uses the same lookup the resolver uses. If someone later routes built-in resolution
through the store, the guard and the resolver diverge and this coupling is what breaks
first. Hence a test rather than a reply.
The genuinely unresolvable built-in case — `builtin:`-prefixed but NOT in the registry,
where `getBuiltinWorkflow` returns undefined and the resolver DOES fall back to the
default — is the hole the parent commit closed, and is covered below it.
*/
describe("plugin-gated built-in selections (PR #2600 review)", () => {
it("resolves through the static registry, so a store rejection cannot force a guess", () => {
// Precondition: the static lookup still finds the plugin-gated built-in. If this ever
// returns undefined the case below is vacuous, so assert it rather than assume it.
expect(getBuiltinWorkflow("builtin:compound-engineering")).toBeDefined();
});
/*
THE FIXTURE IS THE ARGUMENT. `builtin:compound-engineering` declares `triage`; the
post-#2515 DEFAULT lineage does not (its columns are todo, in-progress, in-review, done,
archived) — both measured, not assumed.
So a card resting in `triage` separates the two worlds cleanly:
- resolver uses CE's own IR -> `triage` is DECLARED -> left alone (asserted here)
- resolver fell back to default -> `triage` is UNDECLARED -> re-homed (greptile's claim)
This case therefore FAILS in exactly the world the review describes, which is the only
way to answer "the guard passes proof it should not" with evidence instead of assertion.
*/
it("leaves a plugin-gated card in a column ITS OWN workflow declares, even when the store rejects it", async () => {
const card = task({ id: "FN-9", column: "triage" });
const store = makeStore([card]);
// The plugin is unavailable: every store-side definition read fails.
store.getTaskWorkflowSelectionAsync = vi.fn(async () => ({ workflowId: "builtin:compound-engineering" }));
store.getWorkflowDefinition = vi.fn(async () => {
throw new Error("plugin fusion-plugin-compound-engineering is not installed");
});
const rehomed = await manager(store).reconcileUndeclaredTaskColumns();
expect(rehomed).toBe(0);
expect(store.moveTask).not.toHaveBeenCalled();
expect(card.column).toBe("triage");
/*
And the mechanism, pinned directly: the IR came from the static registry, so the
store was never asked. This is the coupling the guard depends on — if built-in
resolution is ever routed through the store, this assertion breaks first and names
the reason.
*/
expect(store.getWorkflowDefinition).not.toHaveBeenCalled();
});
});

71
packages/engine/src/self-healing.ts
View File

@@ -30,7 +30,7 @@ import { existsSync, mkdirSync, readdirSync, readFileSync, realpathSync, rmSync,
import { readFile } from "node:fs/promises";
import { tmpdir } from "node:os";
import { isAbsolute, join, relative, resolve } from "node:path";
import { resolveColumnFlags, IN_REVIEW_STALL_DEADLOCK_LOG_PREFIX, IN_REVIEW_STALL_LOG_PREFIX, IN_REVIEW_STALL_TERMINAL_LOG_PREFIX, allowsAutoMergeProcessing, resolveEffectiveAutoMerge, countRecentIdenticalStallEntries, detectDependencyCycle, detectSelfDefeatingDependency, evaluateNoCommitsNoOpFinalize, evaluateCompletedPromotionFailureProvenance, evaluateSkipBypassTaint, getInReviewStalledSignal, getInReviewStallReason, getPrimaryPrInfo, getStalePausedReviewSignal, getStalePausedTodoSignal, getTaskHardMergeBlocker, getTaskMergeBlocker, isEphemeralAgent, isMergeRequestContractShadowEnabled, isWorkspaceTask, isSharedBranchGroupMemberIntegration, isNearDuplicateCanonicalInactive, parseExplicitDuplicateMarker, flagTriageDuplicate, isTriageDuplicateKeepAcknowledged, resolveMaxAutoMergeRetries, resolveOptionalStepRevisionBudget, resolveOptionalReviewRevisionBudget, resolveWorkflowIrForTask, resolveReboundTarget, resolveLifecycleColumns, workflowHasColumn, planLegacyAdoption, resolveOrphanedPendingStepResults, classifyReviewLease, PLAN_REVIEW_LEASE_STALENESS_MS, DEFAULT_MAX_POST_REVIEW_FIXES, ACTIVE_WORKFLOW_WORK_ITEM_STATES, AWAITING_APPROVAL_PAUSE_REASON, type Agent, type AgentStore, type ChatStore, type MessageStore, type TaskStore, type Settings, type Task, type MergeDetails, type TaskPriority, type MergeResult, type WorkflowStepResult } from "@fusion/core";
import { resolveColumnFlags, IN_REVIEW_STALL_DEADLOCK_LOG_PREFIX, IN_REVIEW_STALL_LOG_PREFIX, IN_REVIEW_STALL_TERMINAL_LOG_PREFIX, allowsAutoMergeProcessing, resolveEffectiveAutoMerge, countRecentIdenticalStallEntries, detectDependencyCycle, detectSelfDefeatingDependency, evaluateNoCommitsNoOpFinalize, evaluateCompletedPromotionFailureProvenance, evaluateSkipBypassTaint, getInReviewStalledSignal, getInReviewStallReason, getPrimaryPrInfo, getStalePausedReviewSignal, getStalePausedTodoSignal, getTaskHardMergeBlocker, getTaskMergeBlocker, isEphemeralAgent, isMergeRequestContractShadowEnabled, isWorkspaceTask, isSharedBranchGroupMemberIntegration, isNearDuplicateCanonicalInactive, parseExplicitDuplicateMarker, flagTriageDuplicate, isTriageDuplicateKeepAcknowledged, resolveMaxAutoMergeRetries, resolveOptionalStepRevisionBudget, resolveOptionalReviewRevisionBudget, getBuiltinWorkflow, isBuiltinWorkflowId, resolveWorkflowIrForTask, resolveReboundTarget, resolveLifecycleColumns, workflowHasColumn, planLegacyAdoption, resolveOrphanedPendingStepResults, classifyReviewLease, PLAN_REVIEW_LEASE_STALENESS_MS, DEFAULT_MAX_POST_REVIEW_FIXES, ACTIVE_WORKFLOW_WORK_ITEM_STATES, AWAITING_APPROVAL_PAUSE_REASON, type Agent, type AgentStore, type ChatStore, type MessageStore, type TaskStore, type Settings, type Task, type MergeDetails, type TaskPriority, type MergeResult, type WorkflowStepResult } from "@fusion/core";
import { finalizePlanningSegment } from "@fusion/core";
import type { MeshLeaseManager } from "./mesh-lease-manager.js";
import { createLogger, schedulerLog } from "./logger.js";
@@ -811,6 +811,9 @@ function hasTerminalInvalidDoneTransition(task: Pick<Task, "error">): boolean {
return error.includes("Invalid transition:") && error.includes("→ 'done'");
}
/** Sentinel for a workflow selection whose READ failed, distinct from "no selection". */
const UNREADABLE_WORKFLOW_SELECTION = "\u0000unreadable-workflow-selection";
export class SelfHealingManager extends SelfHealingGitEvidence {
// ── Auto-unpause state ──────────────────────────────────────────────
private unpauseTimer: ReturnType<typeof setTimeout> | null = null;
@@ -6364,6 +6367,41 @@ export class SelfHealingManager extends SelfHealingGitEvidence {
* it only ever touches a row whose column is UNDECLARED, never one the operator merely disagrees
* with, and it leaves operator parks (`userPaused`) alone.
*/
/** The task's persisted workflow selection id, or undefined when it has none (which
* legitimately resolves to the default workflow). A sentinel marks an unreadable
* selection, which is itself grounds not to guess. */
private async readTaskWorkflowSelectionForRebound(taskId: string): Promise<string | undefined> {
try {
const selection = this.store.getTaskWorkflowSelectionAsync
? await this.store.getTaskWorkflowSelectionAsync(taskId)
: this.store.getTaskWorkflowSelection?.(taskId);
return selection?.workflowId ?? undefined;
} catch {
return UNREADABLE_WORKFLOW_SELECTION;
}
}
/** True when the workflow id resolves to a definition (built-in or stored). */
private async canResolveWorkflowDefinition(workflowId: string): Promise<boolean> {
if (workflowId === UNREADABLE_WORKFLOW_SELECTION) return false;
/*
FNXC:WorkflowColumns 2026-07-29-00:00 (PR #2600 review — greptile):
EXISTENCE, not prefix. `isBuiltinWorkflowId` accepts any `builtin:`-prefixed string, so
a stale selection like `builtin:removed-workflow` passed this proof — and an unknown
built-in id resolves to the default coding IR, meaning the card was still re-homed
against a workflow that is not its own. That is the exact hole this guard was added to
close, reproduced by the guard itself. `getBuiltinWorkflow` returns the definition or
undefined, so it answers "does this exist" rather than "is it spelled like a built-in".
*/
if (getBuiltinWorkflow(workflowId)) return true;
if (isBuiltinWorkflowId(workflowId)) return false;
try {
return Boolean(await this.store.getWorkflowDefinition(workflowId));
} catch {
return false;
}
}
async reconcileUndeclaredTaskColumns(): Promise<number> {
try {
const pageSize = 500;
@@ -6378,12 +6416,41 @@ export class SelfHealingManager extends SelfHealingGitEvidence {
try {
ir = await resolveWorkflowIrForTask(this.store, task.id);
} catch {
continue; // An unresolvable workflow is its own fault path; do not guess a column.
continue; // Defensive only — see the note below; the resolver does not reject.
}
if (!ir || workflowHasColumn(ir, task.column)) continue;
const target = resolveReboundTarget(ir);
if (!target || target === task.column) continue;
/*
FNXC:WorkflowColumns 2026-07-29-00:00 (U12 — the guard above could not fire):
PROVE the resolved IR belongs to THIS task before moving its card.
The `catch` above was dead code. `resolveWorkflowIrById` catches every failure and
returns `defaultCodingWorkflowIr()`, and `resolveWorkflowIrForTask` does the same
for a failed selection read — so the resolver never rejects, and its comment ("do
not guess a column") described protection that did not exist. A card whose
workflow could not be loaded was judged against the DEFAULT workflow and re-homed
to the DEFAULT's rebound target: the sweep guessed, using a workflow that is not
the card's own, which is the one outcome the guard was written to prevent. Found
while trying to make a test of that guard fail (PR #2543) and being unable to.
The check runs HERE, not at resolution, for two reasons: it costs one definition
read only for a card otherwise about to be MOVED (rare — a healthy board reaches
this line for nobody), and it keeps the fix inside the sweep rather than changing a
resolver whose soft-failure many other callers depend on.
A task with NO selection legitimately resolves to the default workflow, so only a
selection naming a workflow we cannot load counts as unresolvable.
*/
const selectionForProof = await this.readTaskWorkflowSelectionForRebound(task.id);
if (selectionForProof && !(await this.canResolveWorkflowDefinition(selectionForProof))) {
log.warn(
`reconcileUndeclaredTaskColumns: skipping ${task.id} — its workflow '${selectionForProof}' could not be loaded, so any rebound target would be a guess`,
);
continue;
}
try {
await this.store.moveTask(task.id, target as Task["column"], {
moveSource: "engine",