From fb7ab6df26ddb8f2f342d1fabdacd068d5fca620 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 29 Jul 2026 10:43:14 -0700 Subject: [PATCH] test: re-green self-healing, worktree-pool and DB-corruption assertions (#2592) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Test-only.** Three files, two commits. No production changes. | File | Before | After | |---|---|---| | `self-healing-db-corruption` | 5 failed / 1 passed | **6 passed** | | `self-healing` | 1 failed / 411 passed | **412 passed** | | `worktree-pool` | 2 failed / 57 passed | **59 passed** | All three are the same underlying story in different costumes: **the assertion is watching a channel production stopped using**, or a step that aborts before it can log at all. ## Commit 1 — the fake store was missing the health refreshers `surfaceDbCorruption` *refreshes* health before reading the snapshot (`FNXC:IncompletePgPorts 2026-07-26-20:45`, so PG connectivity is re-checked instead of trusting an always-healthy sentinel). The fake carried **neither** refresher, so the async branch fell through to `this.store.refreshDatabaseHealth()` — undefined — and the step threw before reaching dispatch. **Every assertion in the file was measuring zero calls against a step that had already aborted.** Both stubs are **no-ops on purpose.** Production ignores the refresh return and reads `getDatabaseHealth()` immediately after, so the snapshot mock stays the single source of truth. My first attempt delegated them to `getDatabaseHealth`, which consumed a *second* value per pass from the test that queues three `mockReturnValueOnce` snapshots (one per `runMaintenance`) and broke its corruption → clear → corruption ordering. Faithful beats convenient. ## Commit 2 — two more debug-level assertions - **`self-healing`**: `"auto-archive: archived …"` is emitted at DEBUG (`self-healing.ts:2747`); the test asserted `.log`. The mock already had `debug` (from #2573), so only the target was stale. - **`worktree-pool`**: both checkout-failure cases assert on `console.error`, which is *correct* — `createLogger`'s `debug` writes there. But debug is **gated on `FUSION_DEBUG`** (`logger.ts:43`), unset under vitest, so the line was never emitted. One test is literally named *"logs checkout -- failure at debug level"* while asserting a channel debug could not reach. Fixed by enabling `FUSION_DEBUG="worktree-pool"` for the suite and deleting it in `afterEach` so the flag can't leak into sibling files. **Deliberately not** fixed by re-pointing the assertions at another channel — that describes whatever the code happens to do rather than the behavior the test names. ## Verified each actually guards A test that merely stops failing can still assert nothing, so every fix was mutation-checked: | Mutation | NEW failures | |---|---| | `surfaceDbCorruption` returns early | **5** | | remove the auto-archive debug line | **1** — that test, only it | | remove the checkout-failure debug line | **2** — both cases, only them | ## Known residual, stated rather than hidden `self-healing-db-corruption` **still exits non-zero** with 9 unhandled `this.store.listTasks is not a function` rejections from `openSurfacingCycle` (`self-healing.ts:7737`). These **predate this change** — identical count before and after. The maintenance pass opens one shared surfacing cycle up front, independent of which steps `stubMaintenance` stubs. I tried to clear them and backed it out, twice: - adding `listTasks: async () => []` lets the cycle open, but then *other* unstubbed sweeps run for real — an orphaned-planning-segment audit fires and breaks 3 assertions expecting `recordRunAuditEvent` never to be called; - stubbing the four `surface-*` siblings didn't help either, because the cycle is opened by the **pass**, not by the steps. Making that file honestly green needs a fake complete enough for the whole maintenance registry — a bigger change than the bug in front of me, and one that would bury the fix above. Flagging it rather than shipping a half-sweep. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../self-healing-db-corruption.test.ts | 19 +++++++++++++++++++ .../engine/src/__tests__/self-healing.test.ts | 3 ++- .../src/__tests__/worktree-pool.test.ts | 13 +++++++++++++ 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/packages/engine/src/__tests__/self-healing-db-corruption.test.ts b/packages/engine/src/__tests__/self-healing-db-corruption.test.ts index bee1be4ac5..8d42d6d549 100644 --- a/packages/engine/src/__tests__/self-healing-db-corruption.test.ts +++ b/packages/engine/src/__tests__/self-healing-db-corruption.test.ts @@ -36,6 +36,25 @@ function createMockStore(overrides: Record = {}): TaskStore & E isRunning: false, }), recordRunAuditEvent: vi.fn().mockResolvedValue(undefined), + /* + FNXC:TestInfrastructure 2026-07-29-16:45: + surfaceDbCorruption REFRESHES health before reading the snapshot + (FNXC:IncompletePgPorts 2026-07-26-20:45 — so PG connectivity is re-checked + rather than trusting an always-healthy sentinel). This fake carried neither + refresher, so the async branch fell through to `this.store.refreshDatabaseHealth()` + — undefined — and the step threw before reaching dispatch. Every corruption + assertion in this file was then measuring zero calls against a step that had + already aborted. + + Both are NO-OPS on purpose: production ignores the refresh return value and + reads `this.store.getDatabaseHealth()` immediately after, so the snapshot mock + stays the single source of truth. Delegating them to getDatabaseHealth instead + would consume a SECOND value per pass from the tests that queue + mockReturnValueOnce sequences (one per runMaintenance), silently shifting the + corruption -> clear -> corruption ordering they assert. + */ + refreshDatabaseHealth: vi.fn(), + refreshDatabaseHealthAsync: vi.fn(async () => undefined), ...overrides, }) as unknown as TaskStore & EventEmitter; } diff --git a/packages/engine/src/__tests__/self-healing.test.ts b/packages/engine/src/__tests__/self-healing.test.ts index 1b3c336679..ffc508d1c8 100644 --- a/packages/engine/src/__tests__/self-healing.test.ts +++ b/packages/engine/src/__tests__/self-healing.test.ts @@ -2328,7 +2328,8 @@ describe("SelfHealingManager", () => { expect(result).toBe(1); expect(store.archiveTaskAndCleanup).toHaveBeenCalledWith("FN-030"); expect(store.archiveTaskAndCleanup).not.toHaveBeenCalledWith("FN-031"); - expect(getSelfHealingLogger().log).toHaveBeenCalledWith( + // self-healing.ts:2747 emits this at DEBUG level, not log. + expect(getSelfHealingLogger().debug).toHaveBeenCalledWith( "auto-archive: archived FN-030 (age 31d, threshold 30d)", ); }); diff --git a/packages/engine/src/__tests__/worktree-pool.test.ts b/packages/engine/src/__tests__/worktree-pool.test.ts index 968fdfe3d2..8c79be90e5 100644 --- a/packages/engine/src/__tests__/worktree-pool.test.ts +++ b/packages/engine/src/__tests__/worktree-pool.test.ts @@ -93,9 +93,22 @@ let warnSpy: ReturnType; beforeEach(() => { errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + /* + FNXC:TestInfrastructure 2026-07-29-17:05: + worktree-pool logs its checkout-failure at DEBUG level, and createLogger's debug + writes to console.error like the rest — but debug is GATED on FUSION_DEBUG + (logger.ts:43), which is unset under vitest. So the line was never emitted and + the two checkout-failure cases below measured zero calls. One of them is even + named "logs checkout -- failure at debug level" while asserting a channel debug + could not reach without this flag. Enabling it is what makes those assertions + real; re-pointing them at another channel would only describe whatever the code + happened to do. Deleted in afterEach so the flag cannot leak into sibling files. + */ + process.env.FUSION_DEBUG = "worktree-pool"; }); afterEach(() => { + delete process.env.FUSION_DEBUG; errorSpy.mockRestore(); warnSpy.mockRestore(); });