From 218086bea2b7cc2e97cb7153f6207c48f21c5cb1 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 31 Jul 2026 05:15:14 -0700 Subject: [PATCH] =?UTF-8?q?fleet(engine):=20self-healing=206=20=E2=86=92?= =?UTF-8?q?=201=20=E2=80=94=20the=20board-stall=20counter,=20the=20last=20?= =?UTF-8?q?guard=20that=20needed=20a=20sync=20answer=20(#3121)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last fan-out guard, and the one I explicitly said needed a synchronous answer. #3109 made that answer available without an await, so the flag comes off. ## Why this one was last The other two guards in this listener gated work the listener **already `void`s**, so they moved onto the async resolver in #3094. This one increments in-memory state **in the handler's own tick**, so it genuinely needed a synchronous answer. The sync IR path was never that answer: `resolveTaskWorkflowIrSync` cannot resolve a **custom** workflow at all — two independent blockers, #3103 — which is why I wrote that conversion, measured it, and withdrew it. #3109's emitter-carried `lanes` removes the dilemma rather than trading one horn for the other: reading them needs **no await**, so the increment stays in the same tick *and* the guard becomes correct. ## What it fixes On a renamed board this counter read **zero**. The board-stall watchdog was blind to a board whose cards were moving out of implementation the whole time — the signal it exists to raise was never raised. ## Census | | before | after | |---|---|---| | `self-healing.ts` | 6 | **1** | | repo backlog | 29 | **24** | The remaining 1 is the log-dedup closure — a pre-existing flag whose degraded answer costs a duplicate log line, not a lifecycle decision. ## Measured - 3 new cases; `self-healing-completion-fanout.test.ts` **13/13 pass**. - **MUTATION**: restoring the literal pair fails the renamed case. - **The paired negative is the load-bearing one.** The guard means *"left implementation for somewhere that is not implementation"*, so a move **between two non-wip lanes** must not count. Without that case, a conversion that counted every move would pass the positive and inflate the watchdog's denominator — breaking it in the opposite direction, which is harder to notice than a zero. - A **fail-soft** case pins that an emit carrying no `lanes` still counts on the legacy ids. - **Asserted through the counter itself**, not a downstream alert. The increment *is* what this guard decides; routing the assertion through the watchdog would let an unrelated threshold change mask a regression here. - `src/__tests__/self-healing*` + `task-agent*` — **42 files / 848 tests pass**. - `tsc --noEmit -p packages/engine` clean; census `--strict`, `check-lane-wiring`, `check-inert-sync-lane-conversions`, `check-fnxc-future-dates` clean. ## On the withdrawal this reverses #3094 withdrew a sync-IR conversion of this listener and recorded why, precisely. That record is what made this cheap: I could tell in one read that #3109 addressed the *specific* obstacle rather than a general "async is hard". A flag that names its blocker exactly is a flag that can be retired the day the blocker goes. Co-authored-by: Claude Opus 5 (1M context) --- .../self-healing-completion-fanout.test.ts | 73 +++++++++++++++++++ packages/engine/src/self-healing.ts | 65 ++++++++--------- .../lib/lifecycle-column-census-baseline.json | 4 +- 3 files changed, 107 insertions(+), 35 deletions(-) diff --git a/packages/engine/src/__tests__/self-healing-completion-fanout.test.ts b/packages/engine/src/__tests__/self-healing-completion-fanout.test.ts index d6020a41d6..cb6acb06b8 100644 --- a/packages/engine/src/__tests__/self-healing-completion-fanout.test.ts +++ b/packages/engine/src/__tests__/self-healing-completion-fanout.test.ts @@ -292,3 +292,76 @@ describe("the task:moved fan-out resolves the board's own lanes", () => { mgr.stop(); }); }); + +/* +FNXC:WorkflowResolvedColumns 2026-07-31-23:45: +THE BOARD-STALL COUNTER, the last fan-out guard and the only one that needed a SYNCHRONOUS answer. + +It increments in-memory state in the handler's own tick, so it could not follow the other two guards +onto the async resolver, and the sync IR path cannot resolve a custom workflow at all — a conversion +through it would have been inert. #3109's emitter-carried `lanes` removes the dilemma: reading them +needs no await, so the increment stays in the same tick and the guard becomes correct. + +On a renamed board this counter read ZERO, so the board-stall watchdog was blind to a board whose +cards were moving out of implementation the whole time. + +Asserted through the counter itself rather than a downstream alert: the increment IS what the guard +decides, and routing the assertion through the watchdog would let an unrelated threshold change mask +a regression here. +*/ +describe("the board-stall counter follows the board's own lanes", () => { + const RENAMED_LANES = { hold: "drafting", intake: "inbox", wip: "building", review: "checking", complete: "shipped", archived: "filed" }; + + function startedManager(store: TaskStore & EventEmitter) { + const mgr = new SelfHealingManager(store, { rootDir: "/repo" }); + vi.spyOn(mgr as unknown as { startMaintenance: () => void }, "startMaintenance").mockImplementation(() => {}); + vi.spyOn(mgr, "reconcileCompletedTask").mockResolvedValue({ blockedByCleared: 0, worktreeRemoved: false, branchRemoved: false }); + vi.spyOn(mgr, "reconcileInReviewBranchRebind").mockResolvedValue(0 as never); + mgr.start(); + (mgr as unknown as { boardStallWindow: { transitionsOutOfInProgressInWindow: number } }).boardStallWindow = + { transitionsOutOfInProgressInWindow: 0 }; + return mgr; + } + + const counterOf = (mgr: SelfHealingManager) => + (mgr as unknown as { boardStallWindow: { transitionsOutOfInProgressInWindow: number } }).boardStallWindow + .transitionsOutOfInProgressInWindow; + + it("counts a move out of the RENAMED wip lane into the renamed review lane", () => { + const t = makeTask("FN-C1", { column: "checking" }); + const store = createStore([t]); + const mgr = startedManager(store); + + store.emit("task:moved", { task: t, from: "building", to: "checking", source: "engine", lanes: RENAMED_LANES }); + + expect(counterOf(mgr)).toBe(1); + mgr.stop(); + }); + + /* + The paired negative. The guard is "left implementation for somewhere that is NOT implementation", + so a move BETWEEN two non-wip lanes must not count — otherwise the watchdog's denominator inflates + and it stops firing for the opposite reason. + */ + it("does NOT count a move that did not leave the wip lane", () => { + const t = makeTask("FN-C2", { column: "shipped" }); + const store = createStore([t]); + const mgr = startedManager(store); + + store.emit("task:moved", { task: t, from: "checking", to: "shipped", source: "engine", lanes: RENAMED_LANES }); + + expect(counterOf(mgr)).toBe(0); + mgr.stop(); + }); + + it("falls back to the legacy ids when the emitter sent no lanes", () => { + const t = makeTask("FN-C3", { column: "in-review" }); + const store = createStore([t]); + const mgr = startedManager(store); + + store.emit("task:moved", { task: t, from: "in-progress", to: "in-review", source: "engine" }); + + expect(counterOf(mgr)).toBe(1); + mgr.stop(); + }); +}); diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index 957e49073c..3cd3438ecf 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -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, getBuiltinWorkflow, isBuiltinWorkflowId, resolveWorkflowIrForTask, resolveWorkflowIrForTaskWithProvenance, resolveReboundTarget, columnsWithFlag, resolveLifecycleColumns, resolveTaskLifecycleColumns, 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, type WorkflowIr, +import { type TaskMoveLanes, 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, resolveWorkflowIrForTaskWithProvenance, resolveReboundTarget, columnsWithFlag, resolveLifecycleColumns, resolveTaskLifecycleColumns, 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, type WorkflowIr, LEGACY_COLUMN_IDS_BY_ROLE, TERMINAL_ROLES, resolveProjectColumnsForRoles, @@ -877,7 +877,9 @@ export class SelfHealingManager extends SelfHealingGitEvidence { // ── Event listener cleanup ────────────────────────────────────────── private settingsListener: ((data: { settings: Settings; previous: Settings }) => void) | null = null; - private taskMovedFanoutListener: ((data: { task: Task; from: string; to: string; source: string }) => void) | null = null; + /* FNXC:WorkflowResolvedColumns 2026-07-31-23:40: `lanes` is the emitter-resolved payload #3109 + added; optional, because an emit path that cannot resolve sends none. */ + private taskMovedFanoutListener: ((data: { task: Task; from: string; to: string; source: string; lanes?: TaskMoveLanes }) => void) | null = null; // ── Per-task deadlock recovery cooldown ───────────────────────────── private deadlockRecoveryCooldown: Map = new Map(); @@ -1543,43 +1545,40 @@ export class SelfHealingManager extends SelfHealingGitEvidence { this.store.on("settings:updated", this.settingsListener); /* - FNXC:WorkflowResolvedColumns 2026-07-31-23:50 (FLAGGED AND LEFT COUNTED — a conversion I wrote, - measured, and withdrew): + FNXC:WorkflowResolvedColumns 2026-07-31-23:40 (the last fan-out guard, converted — #3109): + THE BOARD-STALL COUNTER WAS THE ONE GUARD HERE THAT GENUINELY NEEDED A SYNCHRONOUS ANSWER, because + it mutates in-memory state in the handler's own tick. The other two gated work the listener already + `void`s and were converted through the async resolver; this one could not follow them. - These four guards are dead on a renamed board: the stall counter reads zero, the review rebind - never runs, and the completion fan-out never reclaims a worktree or clears a dependent's - `blockedBy`. All real. The obvious conversion does NOT fix them. + The sync IR path was never the answer for it either: `resolveTaskWorkflowIrSync` cannot resolve a + CUSTOM workflow at all (two independent blockers, `sync-workflow-ir-second-blocker.test.ts`), so a + conversion routed through it would have been inert — which is why I wrote that conversion, measured + it, and withdrew it. - `task:moved` is emitted synchronously, so an `await` here defers everything after it to a - microtask and reorders this handler against every other subscriber — which points at the store's - sync IR path, the way the scheduler's `resolveTaskParkedColumnsSync` does. I built exactly that - and it is INERT: + #3109 removed the dilemma by having the EMITTER carry resolved lanes on the `task:moved` payload. + Reading them needs NO await, so the counter's increment stays in the same tick and the guard becomes + correct at the same time. - `getTaskWorkflowSelectionImpl` returns `undefined` UNCONDITIONALLY — "Backend mode cannot - synchronously read PostgreSQL" — so `resolveTaskWorkflowIrSync` always takes its `!workflowId` - branch and answers with the DEFAULT builtin IR. `columnsWithFlag` on that IR yields exactly - `todo / in-progress / in-review / done / archived`. Identical to the literals, on every board. + `lanes` is optional and fail-soft to `undefined` — "unknown", never "legacy" — so the literals stay + as the fallback, matching the `mergeParkedColumns` convention in `scheduler.ts`. - Worse than the literal, because the literal is COUNTED: four guards would leave the census, the - file would read as converted, and the next reader would have no reason to look. My own test - passed only because its store mock supplied a renamed IR — it pinned the helper's shape, not - production behaviour. Same defect as #3051's ten scheduler guards; see #3058 and the call-site - allow-list in `sync-workflow-ir-callsite-allowlist.test.ts`. - - SCOPE, after the async conversion below: this note now covers ONLY the board-stall counter, which - mutates in-memory state in the handler's own tick and so genuinely needs a synchronous answer. The - other two guards gate work the listener already `void`s, so they ask the ASYNC resolver instead — - see the note on them. Splitting the four this way is the whole finding: "the listener is sync" was - never the real constraint, "this particular guard's ANSWER is consumed synchronously" is. - - THE REAL BLOCKER for the counter is a sync path that can answer for a CUSTOM workflow, and there - are two things in the way, not one (`sync-workflow-ir-second-blocker.test.ts`). Until then the - literals here stay, counted. + WHAT IT FIXES: on a renamed board this counter read ZERO, so the board-stall watchdog was blind to + a board whose cards were moving out of implementation the whole time — the signal it exists to + raise was never raised. */ - this.taskMovedFanoutListener = ({ task, from, to }) => { + this.taskMovedFanoutListener = ({ task, from, to, lanes }) => { + const wipLane = lanes?.wip ?? "in-progress"; + /* "Left implementation for somewhere that is not implementation" — every lane a card can land + in out of wip, so the counter sees the transition whatever the board calls its columns. */ + const outOfWipLanes = new Set([ + lanes?.hold ?? "todo", + lanes?.review ?? "in-review", + lanes?.complete ?? "done", + lanes?.archived ?? "archived", + ]); if ( - from === "in-progress" - && (to === "todo" || to === "in-review" || to === "done" || to === "archived") + from === wipLane + && outOfWipLanes.has(to) && this.boardStallWindow ) { // In-memory only counter; resets on engine restart. diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index 6cc3573880..7c1c458202 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/scripts/lib/lifecycle-column-census-baseline.json @@ -1,7 +1,6 @@ { "generatedFrom": "node scripts/lifecycle-column-census.mjs --strict --update-baseline", "byFile": { - "packages/engine/src/self-healing.ts": 6, "packages/engine/src/executor.ts": 4, "packages/engine/src/auto-merge-finalization.ts": 3, "packages/dashboard/app/utils/taskRevert.ts": 2, @@ -16,7 +15,8 @@ "packages/core/src/task-store/task-id-integrity.ts": 1, "packages/dashboard/app/components/ResearchTaskActionModal.tsx": 1, "packages/dashboard/app/components/TaskCard.tsx": 1, - "packages/engine/src/notification/notification-service.ts": 1 + "packages/engine/src/notification/notification-service.ts": 1, + "packages/engine/src/self-healing.ts": 1 }, "deliberateByFile": { "packages/core/src/task-store/async-comments-attachments.ts\u0000archived": 5,