From 4868bc465bbf5c19ceb3d70ad50fcbdf74429a8c Mon Sep 17 00:00:00 2001 From: Operator Bot Date: Fri, 7 Aug 2026 00:03:44 +0100 Subject: [PATCH] Finding F20260806-63E2D28A: planner emitted child item(s) --- .operator/data/findings/F20260806-63E2D28A.md | 4 +- .operator/data/tasks/T20260806-3A261A44.md | 86 ++++++++++++ .operator/data/tasks/T20260806-A9673078.md | 126 ++++++++++++++++++ 3 files changed, 214 insertions(+), 2 deletions(-) create mode 100644 .operator/data/tasks/T20260806-3A261A44.md create mode 100644 .operator/data/tasks/T20260806-A9673078.md diff --git a/.operator/data/findings/F20260806-63E2D28A.md b/.operator/data/findings/F20260806-63E2D28A.md index c859699..8277e6c 100644 --- a/.operator/data/findings/F20260806-63E2D28A.md +++ b/.operator/data/findings/F20260806-63E2D28A.md @@ -2,12 +2,12 @@ id: F20260806-63E2D28A kind: finding title: Daemon's top-level cycle error boundary discards the error object entirely — a crashed cycle is invisible -status: pending +status: in-progress priority: 2 source: code-quality#FINDING-001 created_at: '2026-08-06T20:45:49Z' +started_at: "2026-08-06T23:03:44Z" --- - **Severity**: high **Priority**: 2 **Files Affected**: 2 diff --git a/.operator/data/tasks/T20260806-3A261A44.md b/.operator/data/tasks/T20260806-3A261A44.md new file mode 100644 index 0000000..5256888 --- /dev/null +++ b/.operator/data/tasks/T20260806-3A261A44.md @@ -0,0 +1,86 @@ +--- +id: T20260806-3A261A44 +kind: task +title: Finalize the cycle execution row as failed when Engine.runOnce throws +status: pending +priority: 2 +created_at: '2026-08-06T23:03:44Z' +parent_id: F20260806-63E2D28A +--- + +# Finalize the cycle execution row as failed when `Engine.runOnce` throws + +## Problem + +`Engine.runOnce` (`engine/engine/engine.ts:118-197`) opens a cycle execution +row with `status: "running"` at `engine.ts:139` and finalizes it at +`engine.ts:185`, but has no try/catch between them. Any throw from +`cycleHistory.start` (KV/SQLite write), `enumerateRepos` (`engine.ts:154`), +`guard.acquire` (`engine.ts:221`, SQLite write), or an event-bus handler +propagates out and skips the finalize entirely. + +Result: the `executions/cycle-*` row stays `running` forever with no error +text, so the App UI shows a cycle stuck in-flight. Because the caller's +boundary is also lossy, there is currently no second source of truth for what +failed. + +This is inconsistent with the sibling path in the same file: workspace-prep +failures at `engine.ts:284-325` are caught, logged at ERROR, and written as a +synthetic `executions/workspace-prep-*` row finalized `status: "failed"` with +the error text. The cycle row deserves the same treatment — and the errors +that escape `runOnce` are precisely the *unexpected* ones, since the expected +ones are already handled. + +Violates the `intelligence/rules/migration.md` §Quality Gates invariant +"Every stage execution produces an `executions/{id}` KV entry" in spirit: a +row that never reaches a terminal status is not an observable record. + +## Solution + +Wrap the body of `runOnce` (from after `cycleHistory.start` through the +existing finalize) so that a throw: + +1. Logs ERROR via `this.deps.log?.error` with the message, the `.cause` + chain, `traceId`, and the elapsed `durationMs` — matching the shape at + `engine.ts:287-289`. +2. Calls `cycleHistory.finalize({ finishedAt, durationMs, status: "failed", + error: message, successScore: 0, summary: ... }, ctx)` — the + `ExecutionFinalizeFields` contract at + `engine/pipeline/primitives/execution-history.ts:101-122` already carries + `error` and `successScore`. +3. Wraps that finalize in its own best-effort try/catch so an observability + write failure never masks the original error — mirror the existing + `} catch { /* best-effort */ }` at `engine.ts:321-324`. +4. Rethrows the original error unchanged, so the daemon boundary and the + `--once` exit code behaviour are preserved. + +Do not change the success path: the `status: failed === 0 ? "completed" : "failed"` +computation at `engine.ts:188` stays as-is. + +## Regression test + +In `engine/engine/engine.test.ts`, add a test named for the bug scenario, +e.g. `"finalizes the cycle execution row as failed when the cycle throws"`: +build an `Engine` with the existing `FakeKV` (`engine.test.ts:59`) and an +`enumerateRepos` (or `guard.acquire`) that rejects; assert `runOnce` rejects +AND that the `executions/cycle-*` row in `kv.rows` has `status: "failed"` +with non-empty `error`. The lookup pattern at `engine.test.ts:427` +(`[...kv.rows.entries()].find(([key]) => key.startsWith("executions/..."))`) +is the model. The test must fail on pre-fix code (the row is `running`). + +## Affected Files + +- `engine/engine/engine.ts:118-197` — wrap the cycle body; on throw, ERROR-log and finalize the cycle row as `failed` with the error text, then rethrow +- `engine/engine/engine.test.ts` — regression test asserting the cycle row reaches `failed`, using the existing `FakeKV` + +## Acceptance Criteria + +- A cycle that throws leaves its `executions/{id}` row in `status: "failed"` with the error text populated, not `running` +- The original error still propagates out of `runOnce` unchanged — the daemon boundary and the `--once` exit-code path are unaffected +- A failure inside the error-path `finalize` does not mask the original error +- The regression test fails against the current no-try/catch `runOnce` +- An ERROR log line is added for the cycle failure carrying message, `.cause` chain, `traceId`, and `durationMs`; the existing INFO lines at `engine.ts:128`, `156`, `192` are preserved +- Coverage on `engine/engine/engine.ts` stays >=90% +- `engine.ts` stays within the 300-line cap on code lines (logging, comments, and JSDoc are excluded from the count) +- `npm run typecheck && npm run lint && npm test` passes from the repo root + diff --git a/.operator/data/tasks/T20260806-A9673078.md b/.operator/data/tasks/T20260806-A9673078.md new file mode 100644 index 0000000..9fdc2b8 --- /dev/null +++ b/.operator/data/tasks/T20260806-A9673078.md @@ -0,0 +1,126 @@ +--- +id: T20260806-A9673078 +kind: task +title: Log daemon cycle failures and scheduler job rejections instead of discarding them +status: pending +priority: 2 +created_at: '2026-08-06T23:03:44Z' +parent_id: F20260806-63E2D28A +--- + +# Log daemon cycle failures and scheduler job rejections instead of discarding them + +## Problem + +The daemon's outermost error boundary destroys every failure it catches. + +1. `engine/daemon/daemon.ts:196` is a bare `} catch {` — the thrown error is + never bound, so message, stack, and `.cause` are lost. The only surviving + side effects are `this.health.recordCycle(false)` and a status-line flag, + which reduce an arbitrary failure to the single word `failure`. +2. The class could not log it even if the error were bound: the logger + dependency is typed `{ info: (msg: string) => void }` at `daemon.ts:51`, + so `.warn` / `.error` are structurally unreachable — even though the + composition root already passes the full `Logger` at `engine/entry.ts:541`. + Every neighbouring branch (`daemon.ts:117`, `122`, `125`, `168`, `210`) + logs via `this.log?.info`; the failure path is the sole unlogged one. +3. `engine/daemon/scheduler.ts:40` discards the job callback's rejection the + same way, and `IntervalScheduler` holds no logger at all, so it cannot + back-stop the boundary above it. Its skipped-tick branch + (`scheduler.ts:36`) is likewise an unlogged decision, unlike the + equivalent guard at `daemon.ts:167-168` whose comment states + "a dropped tick is a DECISION, never silent." + +In daemon mode the process keeps ticking every `cycleIntervalMs` and failing +invisibly for as long as it is left running. In `--once` mode +(`engine/entry.ts:621-628`) it exits 1 printing only `Cycle complete: failure`, +giving CI/cron callers a red exit with zero diagnostics. + +Violates `intelligence/rules/typescript.md` §REQUIRED (Observability) — +"Every catch-and-continue branch MUST warn with the reason" and "ERROR — +every failure, with full error including `.cause`" — and the §Quality Gates +observability BLOCKER in `intelligence/rules/migration.md`. + +## Solution + +### 1. Widen the `Daemon` logger dependency + +Change `daemon.ts:51` from `{ info: (msg: string) => void }` to the full +`Logger` from `../logging/logger.js` (the layer graph allows +`daemon/ → logging/`; `entry.ts:541` already passes exactly that). Keep it +optional (`log?: Logger`) so existing tests that omit it still compile. + +### 2. Bind and log the cycle error + +Replace the bare catch at `daemon.ts:196` with `catch (err)` and emit an +ERROR line before `this.health.recordCycle(false)`, carrying: +- the error message, +- the full `.cause` chain (walk `err.cause` and join, do not stop at depth 1), +- `cycle: this.cycleCount`, `traceId: ctx.traceId`, and the cycle duration + (capture `Date.now()` at the top of the `try`). + +Follow the shape already used at `engine/engine/engine.ts:287-289` +(`log?.error(msg, { traceId, repoId, errorCode, durationMs })`). + +### 3. Give `IntervalScheduler` a logger + +Add an optional constructor parameter `log?: Logger` to `IntervalScheduler`. + +**Note**: `this.scheduler = new IntervalScheduler()` at `daemon.ts:37` is a +class-field initializer and runs *before* the constructor parameter +properties are assigned, so `this.log` is `undefined` there. Move the +instantiation into the constructor body next to the existing +`this.health = new HealthMonitor(...)` at `daemon.ts:54` and pass `this.log`. +`IntervalScheduler` is same-layer (`daemon/`), so this does not conflict with +the "composition root is `entry.ts` only" rule — the `new` is already there today. + +Then in `schedule()`: +- `scheduler.ts:36` — log WARN naming `job.id` before returning from a + skipped tick (unexpected-but-non-fatal; mirrors `daemon.ts:167-168`). +- `scheduler.ts:40` — bind the error and log ERROR naming `job.id`, the + message, and the `.cause` chain. Keep the swallow (the daemon must not + crash from a job error) but replace the justifying comment with one that + states the error is now reported. + +### 4. Regression tests + +In `engine/daemon/daemon.test.ts`, add a test named for the bug scenario, +e.g. `"logs an ERROR with the cause chain when the engine cycle throws"`: +construct a `Daemon` with a fake logger capturing `error` calls and an +`engine.runOnce` that rejects with +`new Error("outer", { cause: new Error("inner") })`; assert the ERROR line +contains both `"outer"` and `"inner"`. The existing fake at +`daemon.test.ts:16-20` (`makeEngine`) and the log fakes used at +`daemon.test.ts:198`, `221`, `246`, `276` are the patterns to follow — extend +the log fake to capture `warn`/`error`, not just `info`. + +In `engine/daemon/scheduler.test.ts`, add two tests: +- `"logs an ERROR naming the job id when a scheduled callback rejects"` — + tighten the existing `"catches callback errors without crashing"` + (`scheduler.test.ts:90-104`) into a separate assertion, or add alongside it. +- `"logs a WARN when a tick is skipped because the previous run is still in flight"` + — reuse the fake-timer + slow-callback setup already at + `scheduler.test.ts:~60-88`. + +Both must fail against the pre-fix code (they will: no logger exists to assert on). + +## Affected Files + +- `engine/daemon/daemon.ts:51` — widen `log?` from `{ info }` to `Logger` +- `engine/daemon/daemon.ts:37,54` — move `new IntervalScheduler()` into the constructor body, pass `this.log` +- `engine/daemon/daemon.ts:182-199` — capture cycle start time; replace `} catch {` with `catch (err)` + ERROR log carrying message, `.cause` chain, cycle number, traceId, duration +- `engine/daemon/scheduler.ts:21-53` — add optional `log?: Logger` constructor param; WARN on the skipped-tick branch (line 36); bind + ERROR-log the job rejection (line 40) +- `engine/daemon/daemon.test.ts` — regression test for the cause-chain ERROR line; extend the log fake to capture `warn`/`error` +- `engine/daemon/scheduler.test.ts` — regression tests for the job-rejection ERROR and the skipped-tick WARN +- `engine/entry.ts:541` — verify only; already passes the full `Logger`, no change expected + +## Acceptance Criteria + +- A `Daemon` whose `engine.runOnce` rejects with an `Error` carrying a `.cause` produces an ERROR log line containing both the message and the cause; the test fails against the current bare `} catch {` +- An `IntervalScheduler` job whose callback rejects produces an ERROR log naming the job id, and a tick skipped by the `running` guard produces a WARN; both tests fail against pre-fix code +- `Daemon`'s injected logger type exposes `error`; no catch block in `engine/daemon/**` discards its error binding +- The daemon still does not crash on a cycle failure — `recordCycle(false)`, the status-line flag, and the `finally` block behaviour are unchanged +- Coverage on `engine/daemon/daemon.ts` and `engine/daemon/scheduler.ts` stays >=90% +- `daemon.ts` and `scheduler.ts` stay within the 300-line cap (logging and comments are excluded from the count — do not strip comments to fit) +- `npm run typecheck && npm run lint && npm test` passes from the repo root +