# Tasks 3 & 4: Wire Background Services / CrashCounter & AuthManager -- Audit Report

**Summary:** Task 3 has significant config shape mismatches -- the plan passes simple config objects but the Layer factories require rich callback-based configs with Effect-returning functions. Task 4's CrashCounterLive references a non-existent `CrashCounterImpl` class, and the AuthManager Ref-based approach needs clarification for runtime pin updates.

## Findings

| # | Category | Action | Issue | File:Line | Amendment / Question |
|---|----------|--------|-------|-----------|----------------------|
| 1 | Incorrect Code | Amend Plan | **VersionCheckerLive config shape mismatch.** Plan (Task 3 Step 2) passes `{ enabled: !process.argv.includes("--no-update") }` but `VersionCheckerConfig` requires `{ getCurrentVersion, fetchLatestVersion, broadcast, checkInterval }` -- four callback/duration fields, none of which are `enabled`. | `src/lib/effect/version-checker-layer.ts:13-22` | Amend plan to construct a full `VersionCheckerConfig` object: provide `getCurrentVersion` (from package.json version), `fetchLatestVersion` (Effect wrapping the existing npm check), `broadcast` (Effect publishing to DaemonEventBus or calling `registry.broadcastToAll`), and `checkInterval` (e.g. `Duration.hours(1)`). The `enabled` flag should gate whether the Layer is included at all (conditional composition), not be a config field. |
| 2 | Incorrect Code | Amend Plan | **StorageMonitorLive config shape mismatch.** Plan passes `{ path: registry.allProjects()[0]?.directory ?? process.cwd() }` but `StorageMonitorConfig` requires `{ getStorageUsage, persistence, checkInterval, highWaterMark }` -- four fields, none of which are `path`. | `src/lib/effect/storage-monitor-layer.ts:13-18` | Amend plan to construct a full `StorageMonitorConfig`: `getStorageUsage` (Effect returning disk usage for the path), `persistence` (object with `evictOldEvents` Effect), `checkInterval` (e.g. `Duration.minutes(5)`), `highWaterMark` (threshold in bytes). The `path` is an input to `getStorageUsage`, not a direct config field. |
| 3 | Incorrect Code | Amend Plan | **PortScannerLive config shape mismatch.** Plan passes an empty object `{}` but `PortScannerConfig` requires `{ probeFn, portRange, scanInterval, removalThreshold, onDiscovered, onLost }` plus optional `excludedPorts`. The imperative code (lines 1275-1333) shows these values: portRange `[4096, 4110]`, removalThreshold `3`, intervalMs `30000`, plus `onDiscovered`/`onLost` callbacks that add/remove instances. | `src/lib/effect/port-scanner-layer.ts:20-28` | Amend plan to construct a full `PortScannerConfig` with all required fields. `probeFn` wraps `probeOpenCodePort`, `onDiscovered`/`onLost` must be Effect programs that call InstanceManager. This creates a dependency ordering issue -- PortScannerLive needs InstanceManager access but Task 3 runs before Task 7 (InstanceManager conversion). |
| 4 | Implicit Assumptions | Amend Plan | **Background service Layers handle their own broadcasting internally.** The plan assumes deleting imperative event callbacks (`onUpdateAvailable`, `onLowDiskSpace`, `onScan`) is safe because Layer finalizers handle cleanup. But the broadcasting is in the *config callbacks*, not in Layer finalizers. The `VersionCheckerConfig.broadcast`, `StorageMonitorConfig.persistence.evictOldEvents`, and `PortScannerConfig.onDiscovered/onLost` callbacks must be provided to reproduce the imperative behavior. | `src/lib/effect/version-checker-layer.ts:71`, `storage-monitor-layer.ts:45`, `port-scanner-layer.ts:88-101` | Amend plan to explicitly construct callback Effects that replicate the imperative event handlers (broadcasting to registry, evicting sessions, adding/removing instances). |
| 5 | Implicit Assumptions | Amend Plan | **PortScanner depends on InstanceManager which is not yet converted.** Task 3 runs in parallel with Tasks 2, 4, 5 (per dependency graph). But constructing a proper `PortScannerConfig` requires `instanceManager.addInstance`, `instanceManager.removeInstance`, and `instanceManager.getInstances` -- which are still imperative until Task 7. | Plan dependency graph | Either (a) defer PortScanner wiring to after Task 7, or (b) accept that PortScanner config callbacks will reference the imperative `instanceManager` instance as a transitional bridge, and note this explicitly. |
| 6 | Missing Wiring | Amend Plan | **Task 4: `CrashCounterImpl` does not exist.** Plan Step 3 says `const counter = new CrashCounterImpl()` but the actual class is `CrashCounter` (exported from `src/lib/daemon/crash-counter.ts`). | `src/lib/daemon/crash-counter.ts:13` | Change `new CrashCounterImpl()` to `new CrashCounter()` in the plan. |
| 7 | Incorrect Code | Amend Plan | **Task 4: CrashCounter interface mismatch.** The plan's `CrashCounterLive` returns `{ record: () => Effect.sync(() => { counter.record(); return { count: counter.count, shouldAbort: counter.shouldGiveUp() } }) }`. But `CrashCounter` class has no `.count` property. It has `.getTimestamps()` (returns `number[]`). The `CrashCounter` Effect interface (daemon-startup.ts:28-31) expects `record()` to return `Effect<{ count: number; shouldAbort: boolean }>`. | `src/lib/daemon/crash-counter.ts:13-41`, `src/lib/effect/daemon-startup.ts:28-31` | Amend plan to use `counter.getTimestamps().length` instead of `counter.count`. |
| 8 | Implicit Assumptions | Ask User | **Task 4: AuthManager runtime pin updates via Ref.** The plan creates `AuthManagerFromConfigLive` that reads `pinHash` from `DaemonConfigRef` at construction time and calls `auth.setPinHash(hash)`. But `setPinHash` is also called dynamically via IPC (`handleSetPinHash` in Task 8). The AuthManager instance is constructed once by the Layer. The IPC handler (Task 8) updates both `auth.setPinHash(hash)` AND `Ref.update(configRef, ...)`. This means the AuthManager's internal state and the Ref can diverge if only one is updated. | `src/lib/auth.ts:132-134`, Plan Task 8 `handleSetPinHash` | Decision needed: Should AuthManager read pinHash from the Ref on every `authenticate()` call (reactive), or is the dual-write approach (update both AuthManager internal state and Ref) acceptable? The dual-write is fragile but simpler. The reactive approach would require wrapping AuthManager or modifying it. |
| 9 | Fragile Code | Amend Plan | **Task 3: Layer finalizers vs imperative drain().** Plan says "Layer finalizers handle cleanup automatically when `daemonRuntime.dispose()` is called." This is correct for `KeepAwakeLive` (has `Effect.addFinalizer`), and the background fibers in VersionChecker/StorageMonitor/PortScanner are `forkScoped` (interrupted on scope close). However, the imperative `drain()` methods also do things like clear internal timers and flush pending work. The Layer versions use `Effect.forkScoped` which interrupts the fiber -- this is equivalent to drain for these services. But the plan should explicitly verify that `instanceManager.drain()` and `registry.drain()` are NOT being deleted in Task 3 (they're separate, converted in Tasks 6-7). | `daemon-main.ts:779-780` | Amend plan Step 4 to clarify: only delete drain calls for `versionChecker`, `storageMonitor`, and `scanner`. Keep `instanceManager.drain()` and `registry.drain()` until Tasks 6-7 convert them. The plan text currently says "remove" the three drain calls which is correct, but should add an explicit note to preserve the other two. |
| 10 | Fragile Code | Accept | **Task 3: Event loop monitor replacement.** Plan says "replace with comment: Event loop monitoring handled by Layer" but no Layer replacement is defined in Task 3. The design doc mentions `EventLoopMonitorLive` but no task creates it. | Plan Task 3 Step 3 | Minor: the event loop monitor is a debugging tool (`setInterval` with `.unref()`). Deleting it without replacement is acceptable but should be a conscious decision. |
| 11 | Non-Strict Typing | Accept | **`bgParts` array uses `any` type.** `daemon-layers.ts:354` uses `Array<Layer.Layer<any, any, never>>` with a biome-ignore comment. The plan's Task 3 adds items to this array without changing the typing approach. | `src/lib/effect/daemon-layers.ts:354` | Existing pattern -- not introduced by this plan. |
| 12 | Missing Wiring | Amend Plan | **Task 4: No `AuthManagerFromConfigLive` wired into `makeDaemonLive`.** Plan Step 2 creates the Layer factory in `auth-middleware.ts` but does not show where it gets composed into `makeDaemonLive` in `daemon-layers.ts`. Currently, `auth-middleware.ts:14-16` has `makeAuthManagerLive(auth)` which takes an imperative instance. The plan needs a step to replace the `Layer.succeed(AuthManagerTag, auth)` call in `daemon-main.ts:1111` with `AuthManagerFromConfigLive` in the Layer composition. | `src/lib/effect/daemon-main.ts:1111`, `src/lib/effect/daemon-layers.ts:332` | Add a step to wire `AuthManagerFromConfigLive` into `makeDaemonLive` (or into the composition in `daemon-main.ts`), replacing the existing `Layer.succeed(AuthManagerTag, auth)`. This requires `DaemonConfigRefLive` (Task 1) to be composed first. |
| 13 | Missing Wiring | Amend Plan | **Task 4: No `CrashCounterLive` wired into `makeDaemonLive`.** Plan Step 3 creates the Layer but does not show where it is composed. Currently, `CrashCounterTag` is required by `runStartupSequence` (daemon-startup.ts:199) but is presumably provided by the imperative bridge somewhere. The plan needs to show where `CrashCounterLive` is added to the Layer composition. | `src/lib/effect/daemon-startup.ts:199` | Add explicit step: compose `CrashCounterLive` into `makeDaemonLive` in `daemon-layers.ts`. |

## No issues found in:

(All assigned categories had findings.)
