FN-6695: block unsafe in-review branch rebinds
Protect in-review branch metadata repair from overriding user or checkout ownership. - Replace the stale optional auto-mutate TODO with an explicit rebind safety gate. - Skip and audit rebind attempts for user-paused tasks or live checked-out tasks. - Extend reliability coverage for safe autoMerge=false repairs and existing skip outcomes. - Document the self-healing contract for unsafe metadata repair skips. Files changed: docs/architecture.md | 2 +- .../in-review-branch-rebind.test.ts | 131 ++++++++++++++++++++- packages/engine/src/self-healing.ts | 64 ++++++++-- 3 files changed, 187 insertions(+), 10 deletions(-) Fusion-Task-Id: FN-6695 Fusion-Task-Lineage: dfed3013-3bbf-433d-bb3e-434f6a6fe0e9
This commit is contained in:
gsxdsm
@@ -1800,7 +1800,7 @@ This section preserves the detailed lifecycle/self-healing contracts that were f
|
||||
- **Scheduler overlap priority/age guard (FN-5325)**: with `groupOverlappingFiles=true`, scheduler now defers a lower-priority (or younger same-priority) candidate when an overlapping queued todo task exists, preserving priority→age→task-id order for overlap serialization without preempting in-progress work. If the inversion is against an already-running lower-priority blocker, scheduler still defers the candidate; the per-pairing audit event was removed in FN-6174 due to zero consumers and table bloat.
|
||||
- **Empty-commit refusal + early empty-own-diff finalize (FN-5345/FN-5377)**: Fusion task worktrees install a `prepare-commit-msg` hook that refuses `git commit --allow-empty` and other zero-staged-diff commits, preventing verification-only tasks from manufacturing empty handoff commits that defeat the merger's no-op classifier. The hook allows legitimate empty-tree paths (amend, merge, squash, cherry-pick, revert, rebase). Amend detection tokenizes the parent process command line (`ps -o args=` with `/proc/$PPID/cmdline` fallback for Alpine/busybox) and stops at the first message-supplying flag (`-m`/`-F`/`--message`/`--file`) so a commit message containing the substring `--amend` cannot bypass the guard. In `aiMergeTask`, an early empty-own-diff fast-path runs BEFORE any reuse-handoff acquisition: when integration mode is `reuse-task-worktree`, the branch exists, `git rev-list --count <mergeTarget>..<branch>` is > 0, and `git diff --quiet <mergeBase>..<branch>` exits 0, the task auto-finalizes as no-op with `mergeDetails.noOpMerge: true` and emits `task:auto-recover-finalize-already-on-main` with `reason: "empty-own-diff-early-fast-path"`. The fast-path best-effort removes the stranded worktree (FN-4811 same-task/foreign-owner guard) and deletes the `fusion/<id>` branch so empty-own-diff residuals do not accumulate. This unsticks tasks where a stale empty handoff commit combined with drifted worktree↔branch mapping would otherwise wedge the handoff gate with `registered-branch-mismatch`. The explicit `cwd-integration-branch` mode is unchanged (`cwd-main` remains a deprecated alias normalized to it). `classifyOwnedLandedEvidence` also detects empty-own-diff (aheadCount > 0, zero net diff) and returns `proven-no-op` so downstream self-healing and post-handoff finalize paths benefit too. Additionally, merger's reuse-fallback path now consults `git worktree list --porcelain` before creating a new worktree: extant usable registrations of `fusion/<id>` are reused directly (rather than blindly `git worktree add -f` producing a duplicate registration), and stale registrations are pruned first. The direct-reuse shortcut is guarded by FN-4811 (refuses paths owned by a different task in `activeSessionRegistry`) and FN-4954 (skipped when `recycleWorktrees=true` with a pool attached, so `WorktreePool.acquire` lease bookkeeping stays consistent). Two audit subtypes — `merge:reuse-fallback-pruned-stale-registration` and `merge:reuse-fallback-reused-existing-registration` — replace the prior overloading of `merge:reuse-fallback-new-worktree` for these cases.
|
||||
- **Verified no-op/duplicate executor completion (FN-6275)**: explicit `fn_task_done` may complete with zero branch commits only when the summary starts with a recognized sentinel (`PREMISE STALE:`, `NO-OP:`, `NOOP:`, `DUPLICATE: FN-NNNN ...`, or `REDUNDANT:`) or the task already carries a no-commit contract. The sentinel only relaxes the `no_commits` invariant; `wrong_toplevel`, `wrong_branch`, pending-step/review refusals, and scope-leak guards still run. Accepted sentinel completions persist `noCommitsExpected: true`, write task-log audit details with marker kind/reason/raw summary/run/agent IDs, and add a task timeline activity so the no-code terminal path remains explainable. Ordinary zero-commit implementation completions without a leading sentinel are still refused.
|
||||
- **In-review branch-binding self-heal (FN-5083)**: `reconcile-in-review-branch-rebind` runs after `reconcile-task-worktree-metadata` and before `reclaim-stale-active-branches`. It restores `task.branch` (and clears `task.worktree` for fresh acquisition) for `in-review` tasks when exactly one case-insensitive `fusion/<id>` candidate branch has unique commits versus the integration base. Ambiguous candidates emit `task:auto-rebind-skipped` (`reason: "ambiguous-candidates"`) and are never auto-resolved. Branch construction across executor/worktree-pool/worktree-acquisition/merger/self-healing canonicalizes to lowercase via `canonicalFusionBranchName`; `fn_task_done` wrong-branch checks now auto-canonicalize case-only mismatches and emit `branch:auto-canonicalize-case`.
|
||||
- **In-review branch-binding self-heal (FN-5083/FN-6695)**: `reconcile-in-review-branch-rebind` runs after `reconcile-task-worktree-metadata` and before `reclaim-stale-active-branches`. It restores `task.branch` (and clears `task.worktree` for fresh acquisition) for `in-review` tasks when exactly one case-insensitive `fusion/<id>` candidate branch has unique commits versus the integration base. Ambiguous candidates emit `task:auto-rebind-skipped` (`reason: "ambiguous-candidates"`) and are never auto-resolved. Unsafe metadata repair is also skipped with `task:auto-rebind-skipped`: `userPaused` preserves authoritative user intent, and `checkedOutBy` preserves live agent checkout ownership. Branch construction across executor/worktree-pool/worktree-acquisition/merger/self-healing canonicalizes to lowercase via `canonicalFusionBranchName`; `fn_task_done` wrong-branch checks now auto-canonicalize case-only mismatches and emit `branch:auto-canonicalize-case`.
|
||||
- **In-review is terminal-until-merged under `autoMerge: false` (FN-5147)**: when a project sets `settings.autoMerge: false`, `in-review` is the intended resting state until a human merges the PR. No lifecycle-mutating self-healing sweep (`reclaimSelfOwnedBranchConflicts`, `recoverGhostReviewTasks`, `recoverStaleIncompleteReviewTasks`, `recoverInterruptedMergingTasks`, `recoverStuckMergeDeadlocks`, `recoverMissingWorktreeReviewFailures`, `recoverPartialProgressNoTaskDoneFailures`, `recoverCompletionHandoffLimbo`, `recoverPostDoneNonContinuableWedge`, `recoverMergeableReviewTasks`, `recoverMergedReviewTasks`, `recoverAlreadyMergedReviewTasks`, `recoverOrphanOnlyScopeViolations`, `recoverForeignOnlyContaminatedInReviewTasks`, `recoverReviewTasksWithFailedPreMergeSteps`, `finalizeNoOpReviewTasks`, `surfaceInReviewStalls`, `surfaceInReviewStalled`) may move the task out of `in-review`, mark it `paused`/`failed`, or re-enqueue it for execution. Explicit per-task overrides are distinguished by `task.autoMergeProvenance: "user"`; ambiguous legacy rows stamped `autoMerge: true` by the pre-FN-6245 review-entry path are marked `"legacy-stamp"` once and surfaced in run-audit/logs, but are only cleared by the operator-driven `reconcileLegacyAutoMergeStamps({ apply: true })` action. Scoped FN-5819 exception: shared-group members (`branchContext.assignmentMode === "shared"`) are still allowed through the member→`branch_groups.branchName` integration step while `autoMerge` is off; this is a soft pre-integration only and does not permit shared-branch → default-branch promotion. RECONCILE-ONLY sweeps (branch rebind, blocker fan-out, stale-status clears, contamination metadata cleanup, attribution restore, PR refresh, misclassified-failure error clearing) continue to run.
|
||||
- **Auto-merge integration-root default (FN-5279)**: direct auto-merge now defaults `mergeIntegrationWorktree` to `reuse-task-worktree`; merger must pass the reuse handoff gates or emit `merge:reuse-handoff-refused` and leave the task in `in-review` without silently falling back to `cwd-integration-branch` (`cwd-main` remains a deprecated alias normalized to that mode).
|
||||
- **Orphaned execution sweep is observation-only (FN-5337)**: `recoverOrphanedExecutions` only annotates stale in-progress candidates with `task:orphan-detected-no-action` and `[orphan-detected] ... no action (operator-decides)` logs. It must never move `in-progress`/`in-review` backward to `todo` or mutate lease/worktree metadata. Proof-based backward recovery remains exclusively in `recoverInProgressLimbo` (FN-5219), `RestartRecoveryCoordinator`, `recoverMissingWorktreeReviewFailures`, and explicit executor/merger failure paths. Reintroducing lifecycle mutation here requires hard git/session proof gating plus CEO+CTO+PM sign-off.
|
||||
|
||||
@@ -53,17 +53,106 @@ describe("FN-5083 reliability interactions: in-review branch rebind", () => {
|
||||
return branch;
|
||||
}
|
||||
|
||||
function spyOnRebindAudit(manager: SelfHealingManager) {
|
||||
return vi.spyOn(manager as any, "emitBranchRebindAuditEvent");
|
||||
}
|
||||
|
||||
it("rebinds and remains stable on repeated sweeps", async () => {
|
||||
const id = await createTaskInReview("stable rebind");
|
||||
const branch = await createUniqueFusionBranch(id, "stable");
|
||||
await store.updateTask(id, { branch: null, worktree: null });
|
||||
|
||||
const manager = new SelfHealingManager(store, { rootDir });
|
||||
const audit = spyOnRebindAudit(manager);
|
||||
const first = await manager.reconcileInReviewBranchRebind({ includeTaskIds: new Set([id]) });
|
||||
const second = await manager.reconcileInReviewBranchRebind({ includeTaskIds: new Set([id]) });
|
||||
const updated = await store.getTask(id);
|
||||
|
||||
expect(first.outcomes).toEqual(expect.arrayContaining([expect.objectContaining({ taskId: id, result: "applied", branch })]));
|
||||
expect(second.outcomes).toEqual(expect.arrayContaining([expect.objectContaining({ taskId: id })]));
|
||||
expect(updated?.branch).toBe(branch);
|
||||
expect(audit).toHaveBeenCalledWith(expect.objectContaining({
|
||||
taskId: id,
|
||||
mutationType: "task:auto-rebind-applied",
|
||||
metadata: expect.objectContaining({ branch, source: "auto-rebind-in-review" }),
|
||||
}));
|
||||
expect(second.outcomes).toEqual(expect.arrayContaining([
|
||||
expect.objectContaining({ taskId: id, result: "skipped", reason: "binding-intact" }),
|
||||
]));
|
||||
});
|
||||
|
||||
it("skips auto-rebind when user-paused task intent blocks engine mutation", async () => {
|
||||
const id = await createTaskInReview("user paused safety gate");
|
||||
const branch = await createUniqueFusionBranch(id, "paused");
|
||||
const brokenBranch = `fusion/missing-${id.toLowerCase()}`;
|
||||
await store.updateTask(id, { branch: brokenBranch, worktree: null });
|
||||
const originalListTasks = store.listTasks.bind(store);
|
||||
vi.spyOn(store, "listTasks").mockImplementationOnce(async (opts: any) => {
|
||||
const tasks = await originalListTasks(opts);
|
||||
return tasks.map((task: any) => task.id === id ? { ...task, userPaused: true } : task);
|
||||
});
|
||||
|
||||
const manager = new SelfHealingManager(store, { rootDir });
|
||||
const audit = spyOnRebindAudit(manager);
|
||||
const result = await manager.reconcileInReviewBranchRebind({ includeTaskIds: new Set([id]) });
|
||||
const updated = await store.getTask(id);
|
||||
|
||||
expect(result.outcomes).toEqual(expect.arrayContaining([
|
||||
expect.objectContaining({ taskId: id, result: "skipped", reason: "unsafe-to-auto-mutate:user-paused" }),
|
||||
]));
|
||||
expect(updated?.branch).toBe(brokenBranch);
|
||||
expect(updated?.branch).not.toBe(branch);
|
||||
expect(audit).toHaveBeenCalledWith(expect.objectContaining({
|
||||
taskId: id,
|
||||
mutationType: "task:auto-rebind-skipped",
|
||||
metadata: expect.objectContaining({
|
||||
reason: "unsafe-to-auto-mutate:user-paused",
|
||||
branch,
|
||||
}),
|
||||
}));
|
||||
});
|
||||
|
||||
it("skips auto-rebind when a checked-out task has a live metadata lease", async () => {
|
||||
const id = await createTaskInReview("checked out safety gate");
|
||||
const branch = await createUniqueFusionBranch(id, "checked-out");
|
||||
const brokenBranch = `fusion/missing-${id.toLowerCase()}`;
|
||||
await store.updateTask(id, { branch: brokenBranch, worktree: null, checkedOutBy: "agent-123" });
|
||||
|
||||
const manager = new SelfHealingManager(store, { rootDir });
|
||||
const audit = spyOnRebindAudit(manager);
|
||||
const result = await manager.reconcileInReviewBranchRebind({ includeTaskIds: new Set([id]) });
|
||||
const updated = await store.getTask(id);
|
||||
|
||||
expect(result.outcomes).toEqual(expect.arrayContaining([
|
||||
expect.objectContaining({ taskId: id, result: "skipped", reason: "unsafe-to-auto-mutate:checked-out" }),
|
||||
]));
|
||||
expect(updated?.branch).toBe(brokenBranch);
|
||||
expect(updated?.branch).not.toBe(branch);
|
||||
expect(audit).toHaveBeenCalledWith(expect.objectContaining({
|
||||
taskId: id,
|
||||
mutationType: "task:auto-rebind-skipped",
|
||||
metadata: expect.objectContaining({
|
||||
reason: "unsafe-to-auto-mutate:checked-out",
|
||||
branch,
|
||||
}),
|
||||
}));
|
||||
});
|
||||
|
||||
it("metadata-repairs safe autoMerge false in-review tasks without lifecycle mutation", async () => {
|
||||
const id = await createTaskInReview("manual merge metadata repair");
|
||||
const branch = await createUniqueFusionBranch(id, "manual-merge");
|
||||
const mainSha = git(rootDir, "rev-parse main");
|
||||
await store.updateTask(id, { branch: null, worktree: null, baseCommitSha: mainSha, autoMerge: false });
|
||||
|
||||
const manager = new SelfHealingManager(store, { rootDir });
|
||||
const result = await manager.reconcileInReviewBranchRebind({ includeTaskIds: new Set([id]) });
|
||||
const updated = await store.getTask(id);
|
||||
|
||||
expect(result.outcomes).toEqual(expect.arrayContaining([expect.objectContaining({ taskId: id, result: "applied", branch })]));
|
||||
expect(updated?.branch).toBe(branch);
|
||||
expect(updated?.column).toBe("in-review");
|
||||
expect(updated?.status).not.toBe("failed");
|
||||
expect(updated?.paused).not.toBe(true);
|
||||
expect(updated?.baseCommitSha).toBe(mainSha);
|
||||
});
|
||||
|
||||
it("preserves FN-4962 ordering with metadata reconcile before rebind", async () => {
|
||||
@@ -88,8 +177,48 @@ describe("FN-5083 reliability interactions: in-review branch rebind", () => {
|
||||
|
||||
const manager = new SelfHealingManager(store, { rootDir });
|
||||
const result = await manager.reconcileInReviewBranchRebind({ includeTaskIds: new Set([id]) });
|
||||
const updated = await store.getTask(id);
|
||||
|
||||
expect(result.outcomes).toEqual(expect.arrayContaining([expect.objectContaining({ taskId: id, result: "applied", branch })]));
|
||||
expect(updated?.baseCommitSha).toMatch(/^[0-9a-f]{40}$/);
|
||||
});
|
||||
|
||||
it("preserves no-live-branch skip outcome", async () => {
|
||||
const id = await createTaskInReview("no live branch");
|
||||
await store.updateTask(id, { branch: null, worktree: null });
|
||||
|
||||
const manager = new SelfHealingManager(store, { rootDir });
|
||||
const audit = spyOnRebindAudit(manager);
|
||||
const result = await manager.reconcileInReviewBranchRebind({ includeTaskIds: new Set([id]) });
|
||||
|
||||
expect(result.outcomes).toEqual(expect.arrayContaining([
|
||||
expect.objectContaining({ taskId: id, result: "skipped", reason: "no-live-branch" }),
|
||||
]));
|
||||
expect(audit).toHaveBeenCalledWith(expect.objectContaining({
|
||||
taskId: id,
|
||||
mutationType: "task:auto-rebind-skipped",
|
||||
metadata: expect.objectContaining({ reason: "no-live-branch" }),
|
||||
}));
|
||||
});
|
||||
|
||||
it("preserves no-unique-work skip outcome", async () => {
|
||||
const id = await createTaskInReview("no unique work");
|
||||
const branch = `fusion/${id.toLowerCase()}`;
|
||||
git(rootDir, `branch ${branch} main`);
|
||||
await store.updateTask(id, { branch: null, worktree: null });
|
||||
|
||||
const manager = new SelfHealingManager(store, { rootDir });
|
||||
const audit = spyOnRebindAudit(manager);
|
||||
const result = await manager.reconcileInReviewBranchRebind({ includeTaskIds: new Set([id]) });
|
||||
|
||||
expect(result.outcomes).toEqual(expect.arrayContaining([
|
||||
expect.objectContaining({ taskId: id, result: "skipped", reason: "no-unique-work" }),
|
||||
]));
|
||||
expect(audit).toHaveBeenCalledWith(expect.objectContaining({
|
||||
taskId: id,
|
||||
mutationType: "task:auto-rebind-skipped",
|
||||
metadata: expect.objectContaining({ reason: "no-unique-work" }),
|
||||
}));
|
||||
});
|
||||
|
||||
it("skips ambiguous case-variant candidates when filesystem permits both refs", async () => {
|
||||
|
||||
@@ -517,10 +517,24 @@ type RebindOutcome =
|
||||
| {
|
||||
taskId: string;
|
||||
result: "skipped";
|
||||
reason: "binding-intact" | "no-live-branch" | "ambiguous-candidates" | "no-unique-work";
|
||||
reason:
|
||||
| "binding-intact"
|
||||
| "no-live-branch"
|
||||
| "ambiguous-candidates"
|
||||
| "no-unique-work"
|
||||
| "unsafe-to-auto-mutate:user-paused"
|
||||
| "unsafe-to-auto-mutate:checked-out";
|
||||
candidates?: Array<{ branch: string; aheadCount: number }>;
|
||||
};
|
||||
|
||||
type AutoRebindSafetyResult =
|
||||
| { safe: true }
|
||||
| {
|
||||
safe: false;
|
||||
reason: "unsafe-to-auto-mutate:user-paused" | "unsafe-to-auto-mutate:checked-out";
|
||||
detail: string;
|
||||
};
|
||||
|
||||
export type RebindResult = { repaired: number; outcomes: RebindOutcome[] };
|
||||
|
||||
interface LandedTaskCommit {
|
||||
@@ -3561,6 +3575,29 @@ export class SelfHealingManager {
|
||||
}
|
||||
}
|
||||
|
||||
private assertSafeToAutoRebind(task: Task): AutoRebindSafetyResult {
|
||||
/*
|
||||
FNXC:SelfHealingRebind 2026-06-19-12:00:
|
||||
In-review branch rebind is metadata repair only, but it is still an engine-owned mutation.
|
||||
Block instead of warn when authoritative user intent or a live checkout is present so recovery never overrides a user pause or rewrites task metadata underneath an active agent lease.
|
||||
*/
|
||||
if (task.userPaused === true) {
|
||||
return {
|
||||
safe: false,
|
||||
reason: "unsafe-to-auto-mutate:user-paused",
|
||||
detail: "task is user-paused; authoritative user intent blocks automatic branch rebind",
|
||||
};
|
||||
}
|
||||
if (task.checkedOutBy) {
|
||||
return {
|
||||
safe: false,
|
||||
reason: "unsafe-to-auto-mutate:checked-out",
|
||||
detail: `task is checked out by ${task.checkedOutBy}; automatic branch rebind would mutate metadata under a live lease`,
|
||||
};
|
||||
}
|
||||
return { safe: true };
|
||||
}
|
||||
|
||||
private async emitBranchRebindAuditEvent(input: {
|
||||
taskId: string;
|
||||
mutationType: "task:auto-rebind-applied" | "task:auto-rebind-skipped";
|
||||
@@ -3703,13 +3740,24 @@ export class SelfHealingManager {
|
||||
patch.baseCommitSha = derivedBaseCommit;
|
||||
}
|
||||
}
|
||||
// TODO(FN-5066): tighten composition once helper API is final.
|
||||
try {
|
||||
const maybeAsserting = this as unknown as { assertSafeToAutoMutate?: (opts: unknown) => Promise<void> };
|
||||
await maybeAsserting.assertSafeToAutoMutate?.({ taskId: task.id, reason: "in-review-branch-rebind" });
|
||||
} catch (assertErr: unknown) {
|
||||
const message = assertErr instanceof Error ? assertErr.message : String(assertErr);
|
||||
log.warn(`[self-healing] assertSafeToAutoMutate warning for ${task.id}: ${message}; continuing rebind`);
|
||||
const safety = this.assertSafeToAutoRebind(task);
|
||||
if (!safety.safe) {
|
||||
await this.emitBranchRebindAuditEvent({
|
||||
taskId: task.id,
|
||||
mutationType: "task:auto-rebind-skipped",
|
||||
metadata: {
|
||||
taskId: task.id,
|
||||
reason: safety.reason,
|
||||
detail: safety.detail,
|
||||
branch: selected.branch,
|
||||
aheadCount: selected.aheadCount,
|
||||
integrationBase,
|
||||
source: "auto-rebind-in-review",
|
||||
previousBranch: task.branch ?? null,
|
||||
},
|
||||
});
|
||||
result.outcomes.push({ taskId: task.id, result: "skipped", reason: safety.reason });
|
||||
continue;
|
||||
}
|
||||
await this.store.updateTask(task.id, patch);
|
||||
await this.emitBranchRebindAuditEvent({
|
||||
|
||||
Reference in New Issue
Block a user