# Test Coverage Blind-Spot Audit — Smithers

## 1. Executive Summary

The suite is **broad but shallow on the exact guarantees Smithers sells**: crash-safety, deterministic replay, the human-in-the-loop trust boundary, and security/auth. Line coverage is misleadingly high across most packages (often 90–100%), but that number is hiding three structural problems: (1) the highest-consequence code is *executed but not asserted*, (2) failure/rollback branches are uncovered even where happy paths are dense, and (3) the e2e fault matrix **simulates** the very recovery behaviors it claims to prove.

The five most dangerous gaps:

1. **No fault case kills and restarts a real engine process.** `e2e/harness/killProcess.ts` is built and unit-tested but imported by **zero** fault cases; all "crash recovery" (case01–06) is modeled by writing a stale heartbeat into a hand-built `:memory:` table. The product's #1 guarantee — a run survives SIGKILL and resumes from its last checkpoint — is verified nowhere against the real engine.
2. **Concurrent seq allocation is untested, and the Postgres path is racy by construction.** `insertEventWithNextSeq` / `insertSignalWithNextSeq` (`packages/db/src/adapter.js:2071-2126`, `1829-1868`) are never raced; the non-`bun:sqlite` (Postgres) fallback allocates `lastSeq+1` with **no transaction or row lock**, so two writers silently drop one event via `insertIgnore`. This corrupts the event-log ordering backbone for replay, live-stream, and reconnect-after-seq.
3. **The auth trust boundary has large unexercised branches.** An entire auth mode (`trusted-proxy`, `gateway.js:3204-3228`) is untested; the JWT verifier's rejection branches (alg-confusion/`none`, signature tamper, issuer, nbf, skew — `gateway.js:943-985`) are unexercised; token revocation is untested; and the WS upgrade applies **no Origin check** for token/jwt/none modes (`gateway.js:2339-2364`), allowing a drive-by page to gain `['*']` control of a default-mode gateway.
4. **Durable error serialization can crash on the error path.** `errorToJson` (`packages/errors/src/errorToJson.js:18-23`) copies `cause`/`details` by raw reference; a circular cause or BigInt detail throws inside the `JSON.stringify(errorToJson(...))` that the engine writes to the event log (`engine.js:4367,4423`). An error while recording an error corrupts the durable log — verified to throw today.
5. **Time-travel/rewind's failure-then-no-mutation invariant is unasserted.** `timeTravel` VCS-restore failure (`timetravel.js:140-165`) and `revertToAttempt`'s post-VCS-revert DB deletion outside any transaction (`revert.js:38-67`) have no test proving a failed VCS revert leaves frames/nodes untouched. A reorder regression would destroy durable state while the working copy stays at HEAD.

**Overall:** the arithmetic/decision helpers, buildCommand/argument construction, and read-mostly CLI commands are genuinely well tested. The control-plane core — concurrency, recovery, HITL completion, and security — is where green tests give false confidence.

---

## 2. Systemic Gaps (themes across many packages)

These cross-cutting patterns matter more than any single finding.

- **Concurrency / TOCTOU races are untested almost everywhere.** Read-then-write without a lock or atomicity assertion recurs in: db (`insertEventWithNextSeq`, `updateClaimedRun`, HITL status guards), engine (`isRunHeartbeatFresh`, EventBus `seq++` fallback), server (duplicate `startRun` for same runId), time-travel (`timeTravel`/`retryTask` have no single-flight lock unlike `jumpToFrame`), sandbox (concurrency-limit check-then-act), accounts (`addAccount` read-modify-write lost update), scorers (no unique constraint → duplicate rows on retry). Most have ~100% line coverage and still ship the race.

- **Failure/rollback branches are uncovered even where happy paths are dense.** Compensation-itself-fails (scheduler saga), spawn-failure claim rollback (cli supervisor), VCS-revert-then-DB-mutation rollback (time-travel `revert.js`/`retry-task.js`), transaction rollback on migration 0013 (`schema-migrations.js:283-295`), summarizer non-transactional delete-then-save (memory), sandbox cleanup-error swallowing. The pattern: the "and then it failed partway" path is the one that corrupts durable state, and it's the one with no test.

- **High line coverage masks unasserted behavior.** Reported 100%-line packages (errors, accounts, gateway, components, devtools, vcs, time-travel hardened path) carry real blind spots because lines *execute* under happy-path tests without assertions. errors/accounts/devtools/scorers gaps were confirmed by direct probes, not coverage.

- **Security trust boundaries verify config delivery, not enforcement.** e2e case23 (network-policy) asserts the proxy *config* reaches a fake provider but never blocks a disallowed host; case22 (secret-injection) targets the sandbox, but `sandbox` `secretBindings` injection, openapi LLM-header-override of injected auth (`_helpers.js:203-206`), and the review proxy's `srk_` no-spend-cap path are all untested.

- **The e2e fault matrix simulates rather than injects.** 19/27 fault cases reimplement the engine contract against a hand-built `:memory:` schema; `killProcess` and `freezeSqliteLock` injectors exist but are used by 0 cases. Whole fault *classes* are absent: disk-full/torn-WAL/truncated-DB corruption, schema-migration-during-a-live-run resume, multi-run cross-tenant interference, cross-process clock skew (`skewClock` only patches the test process), and the `latency.json` budget is asserted by **zero** tests despite the README claiming regressions fail.

- **Replay/idempotency invariants are not pinned.** Replayed/duplicate lifecycle events resurrect terminal runs (devtools `DevToolsRunStore.js`), `saveMessage` is non-idempotent (crashes on UNIQUE PK on re-save), event/signal replay-dedup is untested (db `2075-2085`), ratio scorer sampling uses unseeded `Math.random()` (non-deterministic across replay), and the tagged-error durable round-trip loses numeric fields (NaN→null).

- **`.js`/`.ts` and cross-package contract drift is unguarded.** protocol error-code arrays vs type unions, the duplicate DevTools wire types in protocol vs devtools, and the divergence between `extract.js` (production) and `dom/extract.js` (published subpath that silently drops fork/aspects) have no parity test.

---

## 3. Critical & High Blind Spots (ordered by risk)

### Theme A — Crash recovery is simulated, never real (e2e-meta, db, engine)

- **No real engine kill+restart anywhere.** `e2e/faults/case01-06` use `corruptHeartbeat` on a fake table; `killProcess.ts` is imported by 0 cases. *Bug it ships:* a half-written checkpoint on SIGKILL, an in-flight node re-executed on resume, or a torn WAL read as valid — none catchable. *Test:* spawn the engine (`Bun.spawn`) on an on-disk DB, kill at node 2 with SIGKILL, start a fresh engine on the same DB, assert the run reaches `finished` exactly-once.
- **Concurrent seq allocation, gapless/monotonic (db `adapter.js:2071-2126`).** *Bug:* two writers compute the same `MAX(seq)+1`; one event silently dropped. *Test:* `Promise.all` of ~50 `insertEventWithNextSeq` on one runId → assert returned seqs are `[0..49]`, `getLastEventSeq===49`, history length 50.
- **Postgres seq fallback has no transaction (db `adapter.js:2087-2097`, `1854-1868`).** *Bug:* TOCTOU under pglite/Postgres silently drops an event/signal via `insertIgnore`. *Test:* via the pglite harness, fire ~20 concurrent `insertEventWithNextSeq` → assert no rows dropped.
- **`isRunHeartbeatFresh` boundary (engine `engine.js:1937`, `RUN_HEARTBEAT_STALE_MS=30_000`).** The exported gate deciding resume-vs-takeover has no unit test. *Bug:* boundary flip → crashed run stuck forever, or split-brain double-resume. *Test:* truth table — fresh at `now-29_999`, exactly `now-30_000`, not at `now-30_001`; never fresh for `waiting-approval`/`failed`/null heartbeat.
- **`updateClaimedRun` CAS (db `adapter.js:1036-1068`).** The write half of crash-resume claim is uncovered. *Bug:* split-brain returning engine clobbers a resumer's state. *Test:* matching owner+heartbeat → true+patched; wrong owner/advanced heartbeat → false+no mutation.

### Theme B — Auth & trust boundary (server, gateway, gateway-react, review)

- **`trusted-proxy` mode entirely untested (server `gateway.js:3204-3228`).** *Bug:* skipped origin allow-list accepting forged `x-user-*`, or default-scope fallback granting `['*']` when upstream omits scopes — full privilege escalation. *Test:* boot with `mode:'trusted-proxy'`, assert disallowed Origin→401, allowed Origin+headers→exact scopes, missing scope header→`defaultScopes`.
- **JWT rejection branches (server `gateway.js:943-985`).** alg-confusion/`none`, signature tamper, wrong issuer, nbf-not-active, skew — all unexercised. *Bug:* classic JWT auth bypass. *Test:* forge `alg:'none'`/`HS384`→rejected; flip a signature byte→fail; wrong `iss`/`nbf=now+3600`→reject.
- **WS upgrade has no Origin gate for token/jwt/none (server `gateway.js:2339-2364`).** *Bug:* drive-by page opens a WS to a localhost/LAN gateway and gets role `operator`/`['*']`. *Test:* drive upgrade with an unexpected Origin and pin the intended policy (reject, or explicitly assert "none mode is deliberately open").
- **Token revocation untested (server `gateway.js:3144-3153`).** *Bug:* a revoked token keeps working. *Test:* past-`revokedAtMs`→401 "Token has been revoked"; future-`revokedAtMs`→authenticates.
- **Per-method `requiredScope` only checked as "some valid scope" (gateway `rpc/index.ts:559-1058`).** *Bug:* `hijackRun`/`rewindRun` silently downgraded to `run:write`/`run:read` → under-privileged takeover/destructive rewind, all tests still green. *Test:* table-driven exact-scope assertion for all 27 methods.
- **`reset()` cross-account purge unasserted (gateway-react `createGatewayCollections.ts:532-544`).** *Bug:* `persistenceStore.clearAll()` dropped → next session hydrates the prior account's runs/approvals/memory from disk (cross-account leak). *Test:* spy `clearAll`, open a stream, `reset()`, assert `clearAll()` once and a fresh upstream handle on re-subscribe.
- **`srk_` API-key proxy runs inference with no spend cap (review `handleAnthropic.ts:92-95`).** *Bug:* unbounded billed inference, mis-attributed repo. *Test:* mint an `srk_` key, POST with `x-api-key`, assert 200, real upstream key used, and a `usage_events` row with the right repo/pr.

### Theme C — Human-in-the-loop completion & replay (cli, pi-plugin, devtools)

- **Supervisor approval-decided-while-detached resume (cli `supervisor.js:351-429`).** The durable HITL completion path (human approves while no engine runs; supervisor wakes the run) is unexercised; the existing e2e tests the *opposite* (not-resumed). *Bug:* approved gates never make progress. *Test:* seed a `waiting-event` run with a decided approval + dead pid + stale heartbeat; `supervisorPollEffect` → asserts `RunAutoResumed`.
- **Supervisor spawn-failure rollback (cli `supervisor.js:220-243,303-326,392-414`).** *Bug:* spawn throws, claim not released → run permanently stuck (looks owned, no engine). *Test:* `spawnResumeDetached` throws → assert `skippedCount===1`, claim restored, second poll retries.
- **DevToolsClient approve/deny/signal untested (pi-plugin `DevToolsClient.ts:426-456`).** *Bug:* flipped `approved`, dropped `nodeId`/`iteration`, or wrong RPC method → approve/deny the wrong attempt with no audit row. *Test:* assert the captured RPC body equals `{method:'approvals.decide', params:{...,approved:true,note}}` and `auditRowId` is extracted.
- **MCP `rewind_run` confirm gate (cli `semantic-tools.js:1389-1395`).** *Bug:* dropped `!input.confirm` check → an LLM destroys run history in one tool call. *Test:* without `confirm`→`INVALID_INPUT` + no mutation; with `confirm:true`→destructive jump runs.
- **MCP `resolve_approval` success + re-decision guard (cli `semantic-tools.js:1164-1208`).** Only error cases tested. *Bug:* re-deciding an already-granted approval (replay-unsafe). *Test:* approve once→`approved`; second call→`INVALID`/no-op.
- **Replayed/duplicate lifecycle events resurrect terminal state (devtools `DevToolsRunStore.js:89-160`).** Confirmed: `RunStarted` after `RunFinished` flips back to `running`; `NodeStarted` after `NodeFinished` un-finishes the task; `waiting-approval` never clears on resume (`:161-177`). *Bug:* inspector shows finished runs as live on reconnect-after-seq. *Test:* feed terminal-then-earlier sequences and replay twice; assert monotonic terminal status.
- **Pi RPC error/abort settle paths (agents `runRpcCommandEffect.js:237-291,330-348`).** *Bug:* an aborted/refused turn falls through to `finalize()` treated as success. *Test:* feed `{response,prompt,success:false}` and `turn_end stopReason:'aborted'` → assert typed `AGENT_CLI_ERROR`.

### Theme D — Time-travel / VCS durability (time-travel, vcs)

- **`timeTravel` VCS-restore failure no-mutation (time-travel `timetravel.js:140-165`).** *Test:* inject `revertToJjPointer→{success:false}`; assert `result.success===false`, frames/attempts/nodes byte-for-byte unchanged.
- **`revertToAttempt` post-VCS DB deletion outside a transaction (time-travel `revert.js:38-67`).** *Bug:* frame-deletion fails after VCS revert lands → working copy rewound, DB still has post-cutoff frames → un-resumable divergence, no `needs_attention`. *Test:* inject failing `deleteFramesAfter`; assert error surfaced and VCS rolled back or run flagged.
- **`captureWorkspaceSnapshot` ↔ `revertToJjPointer` round-trip never exercised with a real commitId (vcs `jj.js:154,173`).** The producer/consumer durability contract behind crash-recovery is untested end-to-end. *Test:* capture, mutate files, `restore --from <capturedCommitId>`, assert content matches.

### Theme E — Durable serialization on the error path (errors)

- **`errorToJson` not JSON-safe (errors `errorToJson.js:18-23`).** Confirmed to throw on circular `cause`/BigInt `details`; consumed by `JSON.stringify` on the failed-task write path. *Test:* circular cause and BigInt details → `expect(() => JSON.stringify(errorToJson(e))).not.toThrow()`.
- **Tagged-error round-trip loses numeric fields (errors `toTaggedErrorPayload.js:36-48`).** `Number(undefined)→NaN→null` on `attempt`/`timeoutMs`, read back by retry/backoff logic. *Test:* round-trip `TaskTimeout`/`TaskHeartbeatTimeout` with missing numerics through `JSON.parse(JSON.stringify(...))` and assert a defined contract.

---

## 4. Medium / Low Blind Spots

| Area | File | Untested behavior | Risk |
|---|---|---|---|
| scheduler | `scheduleTasks.js:411-443` | saga `onFailure:'fail'` + compensation-itself-fails | M |
| scheduler | `makeWorkflowSession.js:113-152` | `__timerUntil` ISO + `h`/`m`/bare-ms parse on resume | M |
| scheduler | `:835-848` / `:640-657` / `:683-709` | orphan-recovery no-op; continue-as-new quiescence defer; requireStableFinish rerender | M |
| driver | `WorkflowDriver.js:574-579` | `drainInflight()` barrier (no writes after terminal) | M |
| driver | `:506-541` / `child-process.js:153-182` | deadline-fires-while-inflight; throwing onStdout/onStderr; late chunks | M |
| db | `adapter.js:2075-2085` / `1675-1691` / `schema-migrations.js:283-295` | event/signal replay-dedup; HITL status-guard idempotency; migration 0013 rollback | M |
| db | `dialect.js:44-200` | placeholder/DDL translation string-machine (`?` in literals/comments) | M |
| server | `gateway.js:2768-2877` / `2079-2099` | duplicate concurrent `startRun`; webhook explicit-runId to terminal/non-waiting run | M |
| gateway-client | `createGatewayCollection.ts:265-301` / `SmithersGatewayClient.ts:545-585` / `createElectricCollection.ts:223-355` | stream error+reconnect; `ext.stream.error`; Electric onError/markReady fallback | M |
| gateway-react | `createGatewayCollections.ts:139-147` / `useSyncSubscription.ts:46-51` / `useGatewayRunEvents.ts:44-48` | offline/reconnect status; unmount abort/no-leak; `afterSeq` cursor | M |
| agents | `BaseCliAgent.js:939-949,405-455` / `KimiAgent.js:238-337` | kimi `AGENT_SESSION_LOST`/`discardResumeSession`; `.stream()` adapter (0%); kimi interpreter | M |
| memory | `Summarizer.js:72-79` / `MemoryStoreLive.js:235-248` | non-transactional delete-then-save; `saveMessage` non-idempotent | M |
| scorers | `run-scorers.js:123` / `aggregate.js` | unclamped NaN/Inf/out-of-range scores; missing-`score` drops row + swallows error | M |
| openapi | `_helpers.js:203-206,219-224` | LLM header-param overrides injected auth; non-2xx returned as success | M |
| sandbox | `process-runner.js:263-271` / `execute.js:771-778` | `/workspace` volume-override guard; cleanup-error swallowing | M |
| components | `Sandbox.js:17-53` / `Worktree.js:15` | `env`/`allowNetwork`/`egress`/`branch`/`baseBranch` descriptor passthrough (only `meta` echo asserted) | M |
| protocol | `errors/index.js` vs `*.ts` | error-code array ↔ union drift; devtools wire-type duplication | M |
| react-reconciler | `reconciler.js:472,186-197` | render-time throw / error boundary; specific-child removal | M |
| observability | `createSmithersOtelLayer.js:11-21` / `trackEvent.js` | OTLP layer (0%) disabled/enabled; Sandbox*/RunContinuedAsNew/AgentEvent metrics | M |
| review | `parseUsageFromJson.ts` / `handleAdminUsage.ts` / `verifyOidc.ts:96-118` | non-SSE $0 metering; admin-usage auth (0%); OIDC alg/sig/issuer reasons | M |
| smithers-pack | `.smithers/package.json` / `quality.ts` / `files.ts` | `test` script runs 2/10 files; release publish/approval gate; `safeJoin` traversal | M |
| graph | `extract.js:168-178` / `validateForkSources.js:61-95` / `utils/xml.js:7-35` | `defaultIteration` resume fallback; transitive fork cycle via `dependsOn`; canonicalizeXml order-independence | M |

Low-severity (pure-logic, narrow blast radius): scheduler `approvalOnDeny` skip/continue & `approvalTimedOut` race guard; engine `assertResumeDurabilityMetadata` defensive branches & EventBus ordering; driver UTF-8 truncation & RetryBackoff-abort; vcs `workspaceList -T` empty-stdout, `revertToJjPointer` empty pointer, env scrubbing; usage TTLs/provider routing/corrupt-cache/duplicate-label; accounts 0600-on-overwrite, orphan temp-file, corrupt-file mutation, secret-in-error-message; openapi PUT/PATCH/head, body-serialization fallbacks; time-travel `jumpToFrame` finally/broadcast failure & `updateRewindAuditRow` no-fromFrameNo; devtools out-of-order `FrameCommitted` & `addNode` index clamp; cli `formatStatusExitCode`, `find-db` boundary refusal, `resumeRunDetached` argv, `snapshot-hook` bad-ack/timeout, `json-args`; smithers-pack `ShipTickets`/`VerifiableGoals`/`LoopUntilScored`/`SqlitePromptCache`/push-fence.

---

## 5. Coverage Heatmap Commentary

- **Weakest by real risk (not file ratio): e2e-meta and smithers-pack.** e2e-meta confirmed only 7/12 but the misses are structural — recovery is simulated, injectors unused, latency budgets unasserted, four security/durability cases `test.skip`-ed. smithers-pack ships 135 workflows with ~3 tested and a `test` script that silently excludes 8/10 existing test files; its incident-driven push-safety fence and release publish gate are untested.
- **db, server, cli, review: broad files, deep blind spots.** All confirmed near-complete (9–10 of their findings). The gaps are concentrated in concurrency (db seq/CAS, server duplicate run), security (server auth modes, review admin/srk_), and HITL/recovery (cli supervisor + MCP). High file count ≠ guarantee coverage.
- **driver, protocol, errors — flagged low by file ratio, and the audit confirms the risk is real but nuanced.**
  - **driver**: ~84% line / near-100% per-file, but concentrated on the spawn primitive; the crash-safety control logic (drainInflight barrier, mid-flight cancel, deadline-while-inflight) is happy-path-only. **Real risk confirmed.**
  - **protocol**: reported 100% line, but that's *import-presence* coverage of two `.js` files — no contract/parity/drift/round-trip behavior is asserted. The risk is contract drift, not dead lines. **Real but different in kind.**
  - **errors**: perfect 100% line, yet the two durability guarantees (JSON-safe `errorToJson`, lossless tagged round-trip) are broken *today* and unasserted. **Real and confirmed by probe — the clearest example of "green by line, gapped by guarantee."**
- **Strong and trustworthy:** engine (durability snapshot/EventBus/restoreWorkspace are assertion-dense), agents (buildCommand/auth/OAuth), components (rendering), gateway-client resilience (real-socket reconnect/resume), memory CRUD/namespace, scorers parsing. Gaps here are specific branches, not whole files.

---

## 6. Recommended Next Steps (ordered punch-list)

1. **Real engine kill+restart e2e.** Wire the existing `e2e/harness/killProcess.ts` into a new case: `Bun.spawn` engine on disk, SIGKILL mid-node, fresh engine resumes, assert finished + exactly-once. Highest-leverage single test in the repo.
2. **db seq concurrency + Postgres fallback.** Race `insertEventWithNextSeq`/`insertSignalWithNextSeq` (bun:sqlite gapless/monotonic) and the pglite path (no dropped rows). Add the replay-dedup assertion (`2075-2085`) in the same file.
3. **Server auth boundary suite.** `trusted-proxy` mode, JWT rejection branches, token revocation, and a WS-upgrade Origin test pinning the intended cross-origin policy. Cheap (pure logic + connect path), highest security ROI.
4. **errors durable serialization.** Circular-cause/BigInt `errorToJson` JSON-safety and the tagged-error numeric round-trip — both already-broken, both one-file fixes that protect the crash path.
5. **Supervisor HITL + spawn-rollback.** Two `supervisor-core.test.js` cases: approval-decided-while-detached resume, and `spawnResumeDetached` throw → claim released.
6. **MCP destructive/HITL gates.** `rewind_run` confirm gate (no-confirm → no mutation) and `resolve_approval` success + re-decision guard.
7. **Time-travel failure invariants.** `timeTravel` VCS-restore-failure no-mutation and `revertToAttempt` post-VCS DB-failure divergence; plus the vcs capture→restore round-trip with a real commitId.
8. **`isRunHeartbeatFresh` + `updateClaimedRun`** unit truth tables (the resume-vs-takeover liveness gate and its CAS write half).
9. **devtools terminal-monotonicity** — replayed/duplicate lifecycle events must not resurrect finished runs/tasks or strand `waiting-approval`.
10. **Per-method `requiredScope` table test** (gateway) and the `requiredScope`-exactness for the 27 RPC methods.
11. **gateway-react `reset()`/`clearAll` cross-account purge** + subscription unmount-abort assertion.
12. **Fix the smithers-pack `test` script** to run all 10 files and add a gate that fails when a `.smithers` test file is unmatched by the glob — this unblocks already-written, currently-dead tests.

Items 1–4 address the executive-summary dangers directly; 5–8 close the recovery/HITL/time-travel core; 9–12 are high-leverage, low-cost wins that turn green-by-line packages into green-by-guarantee.

---

## 7. Appendix — All 188 Confirmed Blind Spots (by area)

Each was adversarially verified as genuinely untested anywhere in the repo (cross-package, e2e/faults, evals). Severity is the verifier-calibrated value.

**Totals:** 13 high · 75 medium · 100 low

### accounts (6)

- **[LOW] Concurrent addAccount/removeAccount races silently drop writes (no locking on read-modify-write)** — `/Users/williamcory/smithers4/packages/accounts/src/addAccount.js:34-51` _(concurrency-race)_  
  - Untested: addAccount and removeAccount perform an unsynchronized read-modify-write: readAccounts() -> mutate in memory -> writeAccounts() (atomic rename of the whole file). No test exercises two interleaved mutations. If two processes/async calls each readAccounts() the same base state and then writeAccounts(), the second rename clobbers the first writer's account entirely (lost update). The temp file name `${path}.tmp.${process.pid}.${Date.now()}` is also not collision-safe within a single process if two writes land in the same millisecond.  
  - Risk: Smithers runs many agents/CLIs concurrently and the wizard plus programmatic API can both touch ~/.smithers/accounts.json. A lost-update race means a user adds account B, B silently vanishes because a concurrent add of A rewound the file — a freshly-registered credential disappears with no error. This is the exact 'concurrent access' risk the audit calls out, and it is completely unasserted.  
  - Test: In a temp SMITHERS_HOME, run Promise.all of multiple addAccount() calls (distinct labels) and assert listAccounts() contains ALL of them (or, if last-writer-wins is the accepted contract, document and assert that explicitly). Add a second test interleaving addAccount('a') and removeAccount('b') and assert no unrelated account is lost. Also assert two same-millisecond writes do not collide on the temp path.
- **[LOW] writeAccounts overwriting a pre-existing world-readable accounts.json is not asserted to re-tighten to 0600** — `/Users/williamcory/smithers4/packages/accounts/src/writeAccounts.js:13-23` _(security-trust-boundary)_  
  - Untested: The only permission test (writeAccounts produces a mode-0600 file) writes to a non-existent path. There is no test where accounts.json already exists with loose perms (e.g. 0644 or 0666) before the write. writeAccounts creates the tmp at 0600 and renames over the target, then chmods the target to 0600 — but the rename/chmod ordering and the case of an attacker-pre-created file are unverified. There is also no test that the orphaned `.tmp` file (which contains plaintext API keys) is created at 0600 and/or cleaned up.  
  - Risk: This file stores raw API keys in plaintext; the 0600 guarantee is the package's primary secret-leak defense. If a regression made writeAccounts skip the final chmod (or preserve the source file's mode on some platform), keys would silently become world-readable. A pre-existing 0644 file getting overwritten is a realistic upgrade/migration scenario. Untested = a secret-leak regression could ship green.  
  - Test: Pre-create accounts.json at mode 0666, call addAccount(), then statSync(...).mode & 0o777 === 0o600. Separately, simulate a crash by intercepting renameSync to throw and assert that any leftover `${path}.tmp.*` file is mode 0600 (no world-readable key spill), and ideally that writeAccounts cleans up its temp file on rename failure.
- **[LOW] Crash between temp-write and rename: orphaned plaintext-key temp file is never cleaned up or asserted** — `/Users/williamcory/smithers4/packages/accounts/src/writeAccounts.js:16-21` _(crash-safety-durability)_  
  - Untested: writeAccounts writes the full registry (including apiKey plaintext) to a `.tmp` file, then renameSync. If the process is killed (engine kill / machine restart — core smithers failure modes) between writeFileSync and renameSync, the .tmp file is left behind permanently with no cleanup on next run. No test covers a failed/interrupted rename, leftover-temp accumulation, or that the real accounts.json is left intact (atomicity) when the rename never happens.  
  - Risk: Smithers' whole premise is surviving kills/restarts. The atomic-rename pattern protects the destination, but leaked .tmp files accumulate API keys in cleartext under ~/.smithers and are never garbage-collected, and the atomicity guarantee (destination unchanged on partial write) is unverified. A regression that wrote in place instead of via temp would corrupt the registry on a mid-write kill and no test would catch it.  
  - Test: Monkeypatch/stub renameSync to throw after the temp file is written; assert (a) the original accounts.json is byte-identical to its pre-call contents (atomicity preserved), (b) addAccount throws rather than half-writing, and (c) ideally that the temp file is removed (or document that startup must sweep stale `*.tmp.*`).
- **[LOW] addAccount/removeAccount/listAccounts behavior on a corrupt existing accounts.json is unasserted** — `/Users/williamcory/smithers4/packages/accounts/src/addAccount.js:34` _(error-path)_  
  - Untested: Only readAccounts is tested against a corrupt file. addAccount(), removeAccount(), getAccount(), and listAccounts() all call readAccounts() first, so on a corrupt/malformed file they will throw ACCOUNTS_FILE_INVALID — but none of these is tested. More importantly, there is no assertion that addAccount does NOT overwrite/destroy a corrupt-but-recoverable file (it should fail closed and leave the bad file for the user to inspect, not clobber it).  
  - Risk: If a user's accounts.json gets partially corrupted, the desired behavior is to surface a clear error and preserve the file, not silently overwrite it (which would destroy other valid accounts the parser choked on). A regression where addAccount caught the parse error and wrote a fresh single-account file would silently wipe a user's other credentials. That data-loss path is untested.  
  - Test: Write a corrupt accounts.json, call addAccount()/removeAccount()/getAccount()/listAccounts() and assert each throws ACCOUNTS_FILE_INVALID; then assert the on-disk file is unchanged (not clobbered) after the failed addAccount.
- **[LOW] Secret keys can leak into thrown error messages / no assertion that they do not** — `/Users/williamcory/smithers4/packages/accounts/src/parseAccountsFile.js:78,addAccount.js:26` _(security-trust-boundary)_  
  - Untested: The 'must set configDir or apiKey, never both' error in parseAccountsFile (line 78) and addAccount (line 26) include the label and provider but the test only matches the generic message. There is no negative assertion that a raw apiKey value is never interpolated into any thrown SmithersError message or into logs. The both-set branch in addAccount specifically fires when an apiKey is present alongside configDir.  
  - Risk: Error messages frequently get logged, surfaced to the gateway/UI, or sent to telemetry. If a future edit interpolated `${account.apiKey}` into a validation error (easy mistake given the message already mentions provider/label), a secret would leak to logs. Pinning a test that asserts the secret string never appears in the thrown message prevents that regression from ever shipping.  
  - Test: Call addAccount({provider:'claude-code', configDir:'/x', apiKey:'sk-SECRET-LEAK'}) and parseAccountsFile of the equivalent JSON; catch the error and assert error.message does NOT contain 'sk-SECRET-LEAK'. Repeat for any path that has access to apiKey.
- **[LOW] addAccount drops a non-string addedAt and stale/invalid configDir variants are not round-trip tested** — `/Users/williamcory/smithers4/packages/accounts/src/parseAccountsFile.js:88-93` _(edge-case)_  
  - Untested: parseAccountsFile silently coerces non-string model/addedAt/configDir/apiKey to undefined (lines 89-92). No test covers a file where addedAt or model is present but the wrong type (e.g. number), nor that addAccount preserving addedAt works when the on-disk addedAt is malformed. The whitespace-only configDir on an API provider, and an apiKey that is a non-string for an API provider (rejected at line 83-84) vs accepted-empty, also lack edge assertions.  
  - Risk: Silent type-coercion can mask data-corruption: a file hand-edited (or written by an older/newer version) with addedAt as a number would lose its timestamp without warning, and addAccount's replace-preserves-addedAt logic would then mint a new timestamp, quietly rewriting audit history. Low severity but it is the kind of silent data-loss the audit asks to surface.  
  - Test: parseAccountsFile a file with addedAt:12345 (number) and assert the resulting account.addedAt is undefined (documenting the coercion), then addAccount-replace it and assert the behavior is intentional. Add a case asserting a non-string apiKey on an api provider throws.

### agents (6)

- **[HIGH] Pi RPC error/abort/prompt-failure settle paths in runRpcCommandEffect are untested** — `packages/agents/src/BaseCliAgent/runRpcCommandEffect.js:237-242,251-291,330-348` _(error-path)_  
  - Untested: Coverage leaves uncovered: the prompt-response failure branch (type:"response", command:"prompt", success:false -> kill with the server's error, 237-242); the message_end/turn_end stopReason==="error"||"aborted" branches that set promptResponseError and surface message.errorMessage (251-258, 269-271, 280-281); and the child `close` handler's promptResponseError and non-zero exit branches (336-343). Existing run-rpc-command-effect-timers.test.js only covers timer cleanup via stdin-unavailable and a throwing extension handler; pi-support.test.js only sends successful turn_end events (no stopReason error/aborted, no success:false).  
  - Risk: PiAgent is a human-in-the-loop RPC engine. These are the exact paths that translate an upstream model error or a user-aborted/refused turn into a typed AGENT_CLI_ERROR the engine can act on. If, say, the stopReason==="aborted" branch silently fell through to finalize() (treating an aborted turn as success with empty/partial text), a denied or interrupted human-gated task would be persisted as a completed result — a correctness and HITL-integrity bug. Termination/cleanup of the detached process group on these error settles is also unverified.  
  - Test: Extend the fake-child harness in run-rpc-command-effect-timers.test.js: (1) write a `{type:"response",command:"prompt",success:false,error:"quota exceeded"}` line and assert the effect fails with that message; (2) write a `{type:"turn_end",message:{role:"assistant",stopReason:"error",errorMessage:"model error"}}` and assert failure carries "model error"; (3) write a `{type:"message_end",message:{role:"assistant",stopReason:"aborted"}}` then close with code 0 and assert the result is a failure (not a silent success); (4) close with a non-zero code and non-empty stderr and assert the stderr text is surfaced.
- **[MEDIUM] Kimi session-loss retry signal (AGENT_SESSION_LOST + discardResumeSession) is never exercised** — `packages/agents/src/BaseCliAgent/BaseCliAgent.js:939-949` _(crash-safety-durability)_  
  - Untested: When a kimi run exits non-zero and prints `kimi -r <uuid>` (kimi's broken-session hint) to stderr or the error text, BaseCliAgent fails with SmithersError code AGENT_SESSION_LOST carrying failureRetryable:true, discardResumeSession:true, and kimiSessionId:<uuid>. No test ever runs a binary named exactly `kimi` that exits non-zero with that stderr — the kimi tests (kimi-oauth-refresh.test.js, proactive-auth-detection.test.js) only run stubs that `exit 0`, and the proactive-auth FakeCliAgent uses a random bin name so `commandSpec.command === "kimi"` is never true. The session-id capture regex, the retryable flag, and the discardResumeSession contract are all unasserted.  
  - Risk: This is the unit that makes a corrupted kimi session recoverable: the engine relies on discardResumeSession:true to DROP the broken resume id and start a fresh session on retry. If a refactor changed the code to failureRetryable:false, omitted discardResumeSession, or broke the `kimi -r <uuid>` regex (e.g. tightened the hex length), every kimi run that hit a mid-stream crash would either retry the same corrupt session forever (deterministic re-crash) or be marked permanently failed — a silent durability regression that no test would catch.  
  - Test: In a kimi-session-loss.test.js: create a FakeCliAgent subclass whose buildCommand returns command:"kimi" and benignStderrPatterns matching kimi's, backed by a fake `kimi` binary that writes `To resume this session: kimi -r 1234abcd-...\n` to stderr and exits 1. Call generate({resumeSession:"old-id"}) and assert the rejection has code AGENT_SESSION_LOST, details.failureRetryable === true, details.discardResumeSession === true, and details.kimiSessionId matches the printed uuid. Add a negative case: a non-kimi command with the same stderr must stay AGENT_CLI_ERROR.
- **[MEDIUM] BaseCliAgent.stream() and the synthetic StreamTextResult adapter are entirely untested** — `packages/agents/src/BaseCliAgent/BaseCliAgent.js:405-455,1199-1208` _(missing-e2e-scenario)_  
  - Untested: No test in the suite calls `.stream()` on any agent (grep for `.stream(` over tests/ returns nothing). Coverage shows lines 405-453 (buildStreamResult), 382-398 (asyncIterableToStream), and 1199-1206 (the stream() method) at 0%. The textStream/fullStream readable streams, the text-start/text-delta/text-end framing, and the usage/finishReason promise plumbing of the AI-SDK StreamTextResult shape are never asserted.  
  - Risk: `stream()` is a public AgentLike method consumed by the AI SDK and by smithers ui live rendering. A bug in buildStreamResult — e.g. fullStream never closing, emitting a text-delta with empty text, or asyncIterableToStream not rebinding Symbol.asyncIterator (which it does manually, a fragile spot) — would deadlock or corrupt every streaming consumer, yet ship green. The default emptyUsage()/buildStreamResult fallbacks (when result has no usage/response) are also exercised only on this path.  
  - Test: Add base-cli-agent-stream.test.js using a fake CLI (same fixture style as claude-support.test.js) that emits a known answer. Call agent.stream(), then: collect textStream into a string and assert it equals the answer; iterate fullStream and assert the ordered part types are [text-start, text-delta(text=answer), text-end]; await result.usage/finishReason/response and assert they resolve (not hang). Add a case where the CLI emits no usage to cover the emptyUsage() default.
- **[MEDIUM] Kimi stream-json output interpreter (assistant/tool/onExit events) has no test** — `packages/agents/src/KimiAgent.js:238-337` _(happy-path-only)_  
  - Untested: KimiAgent.createOutputInterpreter() (parseLine for role:assistant with tool_calls, role:tool results, the started/completed event emission with resume:issuedSessionId, and onExit ok/error mapping) is at 71% func coverage with lines 238-337 uncovered. Every other adapter's interpreter (Claude, Codex, Pi, OpenCode, Amp, Antigravity) is asserted by feeding NDJSON through a fake CLI with onEvent; kimi is only ever invoked in text/exit-0 mode in the OAuth and auth tests.  
  - Risk: The interpreter is what surfaces kimi tool-call/assistant events to the live event log and, crucially, attaches resume:issuedSessionId to the started/completed events — the session id the engine persists for resume-after-restart. A regression in event shape or a dropped resume field would break kimi run resumption and the live UI for kimi specifically, with no failing test. The onExit error branch (mapping non-zero exit + stderr to a completed{ok:false}) is the kimi-specific failure surface and is also unasserted.  
  - Test: Add a kimi-interpreter test using the makeFakeClaude-style fixture renamed to `kimi`, emitting JSONL: an assistant line with tool_calls, a tool-result line, then exit 0. Capture options.onEvent events and assert: a started event with engine:"kimi" and resume === the issued session id; an action event for the tool call (kind from toolKindFromName, title=name); a tool-result action; and a completed event with ok:true, answer=last assistant content. Add an exit-1 case asserting completed{ok:false} carries the stderr text.
- **[MEDIUM] classifyNonRetryableAgentError does not take precedence over structured JSON error extraction for stream-json adapters** — `packages/agents/src/BaseCliAgent/BaseCliAgent.js:917-951` _(error-path)_  
  - Untested: On non-zero exit, when outputFormat is json/stream-json, the error text is taken from extractErrorFromJsonPayload(stdout) (a `{type:"error",error:{...message}}` line) BEFORE classifyNonRetryableAgentError runs. The existing auth-classifier tests (proactive-auth-detection.test.js) all use a default (text) FakeCliAgent that emits auth errors on stderr; none drives a stream-json agent that emits an auth error inside a JSON `error` event on stdout. So the path where a 401/invalid-auth message arrives only via the structured JSON channel (the real shape Claude/Codex emit in stream-json) and must still be reclassified as non-retryable AGENT_CONFIG_INVALID is unverified.  
  - Risk: Claude Code and Codex run in stream-json by default. If a CLI reports `401 invalid_authentication` only inside a stream-json `{type:"error"}` event, the structuredError path must feed classifyNonRetryableAgentError so the engine stops retrying. If extraction returned the message but classification was skipped (or extractErrorFromJsonPayload changed shape), the run would retry a hard auth failure to exhaustion — wasted turns and a degraded failure message — with no test catching it.  
  - Test: Add a FakeCliAgent whose buildCommand returns outputFormat:"stream-json" and a fake binary that prints `{"type":"error","error":{"message":"Error code: 401 invalid_authentication"}}` to stdout and exits 1. Assert generate() rejects with code AGENT_CONFIG_INVALID and details.failureRetryable === false. Add a sibling case proving a benign JSON error (e.g. ECONNRESET) stays AGENT_CLI_ERROR / retryable.
- **[LOW] createHttpTool error paths (timeout abort, 204/205 no-content, non-JSON content-type, non-ok status) are untested** — `packages/agents/src/http/createHttpTool.js:68-86,123-136` _(edge-case)_  
  - Untested: http-tool.test.js asserts only the happy POST path (201 + JSON body + bearer/basic/header auth applied). Uncovered branches: timeoutMs -> AbortController.abort() (lines 68-71, 84), the 204/205 -> null body short-circuit (124-125), the non-JSON content-type fall-through returning raw text (134-135), and serializeBody for string/Blob/FormData inputs vs the JSON default. The basic/header auth variants in applyAuth (97-101) are also not asserted (only bearer is).  
  - Risk: This is the agent's generic REST escape hatch invoked across an LLM trust boundary; a hang on timeout (if the AbortController wiring regressed) or a JSON.parse throw on an empty/non-JSON 204 response would surface to the model as an opaque tool crash rather than a structured result. basic-auth base64 and custom-header auth carry secrets, so an off-by-one in applyAuth would leak/garble credentials silently.  
  - Test: Extend http-tool.test.js against the existing Bun.serve fixture: (1) a route that sleeps past a small timeoutMs and assert execute rejects/aborts rather than hanging; (2) a 204 route and assert body === null; (3) a text/plain route and assert body is the raw string; (4) assert basic auth produces `Basic ${btoa(user:pass)}` and a `header` auth sets the named header. Drive each through the served endpoint (no fetch mocking).

### cli (10)

- **[HIGH] Supervisor approval-decided resume path (waiting-event run with a decided approval while detached) has no unit test** — `apps/cli/src/supervisor.js:351-429 (processApprovalDecidedCandidateEffect, pollEffect lines 470-485)` _(crash-safety-durability)_  
  - Untested: The entire processApprovalDecidedCandidateEffect branch and its wiring in pollEffect. The supervisor resumes a `waiting-event` run whose approval was DECIDED while the run was detached (no engine alive). supervisor-core.test.js never exercises this; supervisor-e2e.test.js inserts a `waiting-approval` run with status 'requested' and asserts it is NOT resumed (line 269/294) — the opposite case. No test creates a `waiting-event` run + a DECIDED approval (status 'approved'/'denied' via insertOrUpdateApproval with decidedAtMs set) and asserts it gets claimed, spawned, and emits RunAutoResumed. runHasDecidedApprovalEffect (line 108) and the requireStale/expectedStatus:'waiting-event' claim are likewise unasserted.  
  - Risk: This is the durable human-in-the-loop completion path: a human approves a gate while no engine is running, and the supervisor must wake the run so it makes progress. If this branch regressed (wrong expectedStatus, requireStale dropping the run, runHasDecidedApprovalEffect returning false on a real decided approval, or the approvalSlots math starving it after timer resumes), approved runs would silently hang forever with the approval already granted — the worst HITL failure: the user said yes and nothing happens. e2e/faults restart-waiting-approval covers a restart-then-approve flow but not this per-unit detached-decision resume in the supervisor poll.  
  - Test: In supervisor-core.test.js: insert a run with status 'waiting-event', heartbeatAtMs older than staleThreshold, runtimeOwnerId of a dead pid; insert a node 'review' in waiting-event; insertOrUpdateApproval with status 'approved', decidedAtMs set. Run supervisorPollEffect with isPidAlive:()=>false and a recording spawnResumeDetached. Assert resumedCount===1, the run is in `resumed`, and a RunAutoResumed event was emitted for it. Add a negative variant: same run but approval status 'requested' (undecided) → asserts NOT resumed. Add a starvation variant: maxConcurrent=1 with one stale run + one approval-decided run → approval run is rate-limited (RunAutoResumeSkipped reason 'rate-limited').
- **[HIGH] Supervisor spawn-failure rollback (release claim after spawnResumeDetached throws) is never tested** — `apps/cli/src/supervisor.js:220-243, 303-326, 392-414 (Effect.try spawn + releaseRunResumeClaimEffect on Left)` _(error-path)_  
  - Untested: Every resume branch claims the run, then spawns the detached resume process inside Effect.try; if spawn throws it must releaseRunResumeClaimEffect to roll the claim back so the next poll can retry. No supervisor test makes spawnResumeDetached throw — all test doubles return a pid. The compensating releaseRunResumeClaimEffect call and the resulting 'skipped' result are completely unasserted.  
  - Risk: If spawn fails (e.g. PROCESS_SPAWN_FAILED: bun missing, fork limit, EAGAIN) and the claim is NOT released, the run stays claimed by a supervisor that never started an engine — it becomes permanently stuck: the claim makes it look owned/fresh so future polls skip it, yet no engine is running. This is a durable liveness bug exactly in the crash-recovery path the supervisor exists to provide. A regression that drops or mis-targets the release (wrong claimOwnerId, wrong restore heartbeat) would never be caught.  
  - Test: Run supervisorPollEffect on a stale run with spawnResumeDetached:()=>{throw new Error('spawn boom')}. Assert resumedCount===0, skippedCount===1, no RunAutoResumed event, and that the run's runtimeOwnerId/heartbeatAtMs were restored to the pre-claim values (claim released), so a second poll with a working spawn re-attempts and succeeds (resumedCount===1).
- **[MEDIUM] MCP time-travel/durability handlers (fork_run, replay_run, rewind_run, time_travel) are only verified to EXIST, never executed** — `apps/cli/src/mcp/semantic-tools.js:1315-1411 (fork_run, replay_run, rewind_run) and the time_travel handler` _(api-contract)_  
  - Untested: semantic-tools-unit.test.js only asserts SEMANTIC_TOOL_NAMES contains 'fork_run'/'replay_run'/'rewind_run'/'time_travel' (lines 346-352). No test calls these handlers and asserts behavior: replay_run's restoreVcs + vcsRestored/vcsPointer/vcsError shaping, fork_run's branch/snapshot return + run summary, time_travel routing, or that the returned runId is a real forked run. Only list_snapshots and restore_checkpoint (failure case) handlers are actually invoked.  
  - Risk: These are the MCP/LLM entry points to the deterministic-replay and fork/branch guarantees. An LLM driving smithers via the semantic MCP server is the primary consumer. A regression in argument plumbing (e.g. replay_run not forwarding restoreVcs or inputOverrides, fork_run dropping resetNodes, a renamed underlying forkRun field) would ship undetected because the contract is exercised only through the CLI surface, not the MCP one. The output schema (resultSchema) could also drift from the handler's returned shape with no test catching it.  
  - Test: Using the in-memory makeHarness in semantic-tools-unit.test.js, seed a run with frames/checkpoints, then call('fork_run',{parentRunId,frameNo,resetNodes,branchLabel}) and assert structuredContent.ok, a non-empty runId distinct from parent, branch/snapshot present, and a getRun on the new runId succeeds. Call('replay_run',{...,restoreVcs:true}) and assert vcsRestored is a boolean and vcsPointer/vcsError shaping matches the output schema. Validate each result against its declared outputSchema with .parse().
- **[MEDIUM] MCP rewind_run confirm=true destructive gate is untested** — `apps/cli/src/mcp/semantic-tools.js:1389-1395` _(security-trust-boundary)_  
  - Untested: rewind_run throws INVALID_INPUT unless input.confirm===true, then performs a destructive jumpToFrameRoute (deletes later frames, invalidates diffs, reverts sandboxes). No test asserts that calling rewind_run WITHOUT confirm returns the INVALID_INPUT error and performs no mutation. The CLI confirm path (runRewindOnce) is tested in rewind-unit.test.js/rewind.test.js, but that is a separate code path; the MCP guard is independent and unverified.  
  - Risk: This is the only guard preventing an LLM (which calls MCP tools autonomously) from destroying run history without explicit confirmation. If a refactor dropped or inverted the !input.confirm check, an agent could irreversibly delete frames/attempts and revert worktrees on a single tool call — a destructive trust-boundary failure. e2e covers rewind-reverts-vcs but not the MCP confirm gate specifically.  
  - Test: In semantic-tools-unit.test.js, call('rewind_run',{runId,frameNo:1}) with confirm omitted/false and assert isError true, error.code 'INVALID_INPUT', and that jumpToFrameRoute was NOT invoked (no frames deleted — assert run state unchanged). Then call with confirm:true on a seeded run and assert the destructive path runs and returns the result/run shape.
- **[MEDIUM] find-db anchor-preference and project-boundary refusal are untested** — `apps/cli/src/find-db.js:50-71 (anchor preference, CLI_DB_NOT_FOUND when anchor DB absent, multiple-db stderr warning)` _(security-trust-boundary)_  
  - Untested: cli-find-db.test.js / find-db-unit.test.js only cover the simple upward walk, not-found, and timeout. Untested: (1) when a `.smithers/` anchor exists, the anchor's smithers.db is preferred even if a stray smithers.db sits closer to cwd; (2) when an anchor exists but its DB hasn't been created, findSmithersDb THROWS CLI_DB_NOT_FOUND instead of falling back to a parent/stray DB (the project-boundary refusal at lines 57-59); (3) the multi-candidate stderr 'Warning: multiple smithers.db ... Using/Ignored' message (lines 62-69).  
  - Risk: Every read/write CLI command resolves its DB through this function. If the anchor-preference or boundary-refusal regressed, a command run inside project A could silently operate on project B's smithers.db (a parent monorepo DB, or a stray sibling) — approving the wrong run, reading the wrong events, or writing crons into the wrong control plane. That is a cross-project trust-boundary leak with no diagnostic, since the warning is also untested.  
  - Test: Build a temp tree: parent/smithers.db plus parent/proj/.smithers/ (anchor) with NO smithers.db. Assert findSmithersDb(parent/proj) throws CLI_DB_NOT_FOUND (does NOT return parent/smithers.db). Then create parent/proj/smithers.db and assert it is chosen over parent/smithers.db even though parent's is higher up. Separately, place two smithers.db along the walk with an anchor and capture process.stderr to assert the 'Warning: multiple smithers.db' message names the chosen and ignored paths.
- **[MEDIUM] resumeRunDetached resume-claim argv construction is unverified** — `apps/cli/src/resume-detached.js:16-37` _(crash-safety-durability)_  
  - Untested: resumeRunDetached builds the `up ... --resume --run-id <id> -d --force` argv plus the optional claim args (--resume-claim-owner, --resume-claim-heartbeat, --resume-restore-owner, --resume-restore-heartbeat). No test asserts the exact argv produced for a given claim, including the conditional inclusion of restoreRuntimeOwnerId/restoreHeartbeatAtMs (skipped when null/undefined) and stringification of numeric heartbeats. cli-flag-validation.test.js only tests the receiving side's rejection of --resume-claim-owner without --resume-claim-heartbeat.  
  - Risk: This is the producer half of the supervisor's crash-recovery claim handoff: the supervisor claims a stale run, then the detached child must restore the exact prior owner/heartbeat so the claim transfers atomically. If the argv were malformed (wrong flag name, dropping a value, passing a number where a string is expected, or including --resume-restore-owner with a null value), the resumed engine would mis-restore the claim and could double-run or fail to take ownership — a concurrency/durability hazard. The producer side is currently only exercised, if at all, by full integration, never asserted directly.  
  - Test: Inject a fake child_process.spawn (as cron-command-scheduler.test.js does) and call resumeRunDetached('/wf.tsx','run-1',{claimOwnerId:'supervisor:x',claimHeartbeatAtMs:123,restoreRuntimeOwnerId:'pid:9:o',restoreHeartbeatAtMs:456}); assert the captured args array equals the expected sequence including stringified '123'/'456'. Add a variant with restoreRuntimeOwnerId:null and restoreHeartbeatAtMs:null and assert those two flags are OMITTED entirely. Also assert detached:true, stdio:'ignore', cwd=dirname of resolved workflow.
- **[LOW] MCP resolve_approval success path and replay-unsafe re-decision guard are untested** — `apps/cli/src/mcp/semantic-tools.js:1164-1208` _(security-trust-boundary)_  
  - Untested: semantic-tools-unit.test.js only tests the error cases (no match line 703, wrong iteration 710, ambiguous 717). The SUCCESS path — single pending approval matched, approveNode/denyNode actually called, returned approval status 'approved'/'denied', and run summary rebuilt — is never asserted. Critically, there is no test that calling resolve_approval a SECOND time on an already-decided approval returns the 'no match' error rather than re-deciding (relies on listAllPendingApprovals excluding decided rows).  
  - Risk: resolve_approval is a destructive HITL action exposed to LLMs. Without a success-path test, a regression that fails to persist the decision, returns the wrong status, or (worse) re-decides an already-granted approval would ship silently. The replay-unsafe-approval guarantee (an already-granted approval must not be re-triggered) is covered by e2e at the engine level but not at this MCP entry point, which is a distinct trust boundary an agent can reach.  
  - Test: Seed one pending approval. call('resolve_approval',{action:'approve',runId,nodeId,iteration}) → assert ok, approval.status 'approved', decidedAtMs set, run summary returned, and the DB approval row is now decided. Then call resolve_approval again with identical filters → assert isError with code INVALID_INPUT ('No pending approval matched'), proving an already-granted approval cannot be re-approved via MCP.
- **[LOW] formatStatusExitCode mapping (run status -> process exit code) lacks a focused unit test** — `apps/cli/src/index.js:239-251 (formatStatusExitCode)` _(api-contract)_  
  - Untested: The pure mapping finished->0, waiting-*->3, cancelled->2, everything-else(failed)->1 is used for process.exitCode at 6 call sites but is not unit-tested. Integration tests assert waiting->3 (integration.test.js:248, ask-human.e2e:60) and cancelled->2 is touched once (agents-account-cli.e2e:66), but there is no assertion that a FAILED run yields exit 1 distinct from cancelled (2), nor a single table-driven test pinning the full mapping.  
  - Risk: This mapping is the CLI's machine contract for scripting/CI: `smithers up` returning 0 vs 1 vs 2 vs 3 is how automation decides pass/fail/blocked. A regression (e.g. failed runs returning 0, or cancelled collapsing to 1, or a new status falling through to a wrong bucket) would silently break every CI pipeline and supervisor loop that branches on exit code, with no test failing. Because it is a trivially pure function, the absence of a direct test is a real gap, not a triviality.  
  - Test: Export or test formatStatusExitCode directly with a table: finished->0, waiting-approval/waiting-event/waiting-timer->3, cancelled->2, failed->1, and an unknown/other status->1. Additionally add one integration assertion that a workflow that fails produces process exitCode 1 (distinct from a cancelled run's 2).
- **[LOW] snapshot-hook fail-open does not cover malformed-ack and a successful-with-spool-still-absent boundary** — `apps/cli/src/snapshot-hook.js:38-39, 82-84 (bad-ack parse branch; spool only on !result.ok)` _(edge-case)_  
  - Untested: snapshot-hook.test.js covers: no socket, happy forward, and connect-failure spool. Untested: (1) the server returns a non-JSON / garbage line so requestSnapshot resolves {ok:false,error:'bad-ack'} — the hook should still exit 0 AND spool a gap with reason 'bad-ack'; (2) the timeout branch (server accepts the connection but never replies within timeoutMs) producing reason 'timeout' and a spooled gap; (3) malformed hook JSON on stdin (the catch at line 67) still produces a well-formed payload with toolName undefined.  
  - Risk: This hook is the Tier-1 durability checkpoint at every tool boundary; its contract is 'never block the agent, but make every miss visible via a spooled gap'. The bad-ack and timeout paths are exactly the partial-failure modes most likely in production (engine busy, socket half-open). If a regression caused those to neither exit 0 nor spool, durability misses would be invisible — silently losing crash-recovery checkpoints. Connect-failure is tested but the in-protocol failures are not.  
  - Test: Start a real snapshot server (createSnapshotServer) whose onHook returns a value that serializes oddly, or write raw non-JSON to the socket, and assert runSnapshotHookOnce exits 0 and spool received reason 'bad-ack'. Add a server that delays its reply beyond timeoutMs and assert reason 'timeout' is spooled with exit 0. Add a case with stdin='{not json' and assert payload.toolName is undefined and a snapshot is still requested.
- **[LOW] json-args parseJsonInput fail() callback path is not unit-tested** — `apps/cli/src/json-args.js:48-59 (parseJsonInput) and readJsonArgumentPayload empty-stdin error (line 18-20)` _(error-path)_  
  - Untested: json-args.js has no direct test. The `--input -`/1MB-limit/malformed-JSON behaviors are covered at the CLI integration level (cli-flag-validation.test.js:103-145), but parseJsonInput's `fail({code,message,exitCode:4})` adapter — used by commands/MCP that route JSON errors through a fail callback — is not exercised, nor is the specific 'stdin was empty' INVALID_JSON error from readJsonArgumentPayload, nor that a SmithersError's code is preserved (vs defaulting to INVALID_JSON) when passed to fail.  
  - Risk: parseJsonInput is the shared error-mapping seam for JSON payload args across commands; the integration test exercises one consumer (--input) but not the fail()-callback contract (correct code/exitCode 4 propagation, SmithersError code preservation). A regression that swallowed the original error code or used the wrong exit code would degrade scriptable error handling without any unit test catching it.  
  - Test: Unit test parseJsonInput('{bad', 'label', fail) and assert fail was called with code 'INVALID_JSON', exitCode 4, message containing 'Invalid JSON for label'. Test readJsonArgumentPayload('-', 'label') with an empty stdin (fd 0 stubbed) throws SmithersError code INVALID_JSON 'stdin was empty'. Test that a thrown SmithersError with a custom code is surfaced verbatim by parseJsonInput's fail callback rather than coerced to INVALID_JSON.

### components (5)

- **[MEDIUM] Sandbox env/volumes/ports/allowNetwork/reviewDiffs/autoAcceptDiffs passthrough is unasserted (only egress + allowNested are checked)** — `packages/components/src/components/Sandbox.js:17-53 (tested at tests/composite-coverage.test.jsx:531-548)` _(security-trust-boundary)_  
  - Untested: Sandbox.js forwards `env` (where secrets are injected), `volumes`, `ports`, `allowNetwork`, `reviewDiffs`, `autoAcceptDiffs`, `memoryLimit`, `cpuLimit`, `workspace`, `image`, `command` onto the host element, which graph/src/dom/extract.js:473-514 lifts into `config`/the sandbox factory and into `meta.__sandboxConfig`. The single Sandbox test (composite-coverage.test.jsx:531-548) asserts only `egress`, `allowNested`, and `runtime===undefined`. There is NO assertion that `env`, `allowNetwork`, `volumes`, `ports`, `reviewDiffs`, or `autoAcceptDiffs` survive rendering onto the descriptor.  
  - Risk: These are exactly the network-policy and secret-injection knobs (secret-injection-no-leak, network-policy-allow-deny). If a prop is dropped/typo'd in Sandbox.js, `allowNetwork` could default to false-but-actually-true, `egress` allowlist could vanish (open egress), or injected `env` secrets could be silently lost — and the only e2e coverage exercises a real provider, so a pure component-level regression in the prop wiring ships undetected. autoAcceptDiffs flipping to true would auto-accept agent diffs without review (diff-review-mode bypass).  
  - Test: Render Sandbox with `allowNetwork={false}`, `env={{SECRET:"x"}}`, `egress={{httpsProxy:...}}`, `volumes=[...]`, `ports=[...]`, `reviewDiffs autoAcceptDiffs={false}` and assert on the extracted descriptor's sandbox config/meta: e.g. `expect(task.meta.__sandboxConfig.env).toEqual({SECRET:"x"})`, `expect(task.meta.__sandboxAllowNetwork).toBe(false)`, `expect(task.meta.__sandboxAutoAcceptDiffs).toBe(false)`. Include a default-omitted case asserting allowNetwork defaults to false.
- **[LOW] Worktree branch/baseBranch propagation onto nested task descriptors is unasserted at the unit level** — `packages/components/src/components/Worktree.js:15 (tested at tests/worktree.test.jsx:8-21)` _(regression-risk)_  
  - Untested: Worktree.js puts `branch` and `baseBranch` on the host element; graph/src/dom/extract.js:272-274,330-331,427-428,569-570 lifts them onto every nested task descriptor as `worktreeBranch`/`worktreeBaseBranch`. The render-level worktree test (worktree.test.jsx) asserts `worktreeId` and `worktreePath` on nested tasks but never `worktreeBranch`/`worktreeBaseBranch`. The only baseBranch coverage is a heavy git-runtime integration test (worktree-hardcoded-main.test.jsx) gated on `hasGit()` (skipped on CI boxes without git).  
  - Risk: baseBranch is the sync target for rewind-reverts-vcs and worktree pointer integrity (file-vcs-pointer-integrity). If the component drops branch/baseBranch from the host element, the engine silently falls back to main/origin/main — syncing against the wrong base, which corrupts diffs and can clobber a worktree branched off `release`. On a git-less CI box the integration test is skipped, so nothing guards the wiring.  
  - Test: In worktree.test.jsx render `<Worktree branch="unit/x" baseBranch="release"><Task/></Worktree>` via SmithersRenderer and assert the nested task descriptor: `expect(t.worktreeBranch).toBe("unit/x")` and `expect(t.worktreeBaseBranch).toBe("release")`. Add a nested-worktree case asserting the innermost branch/baseBranch wins, mirroring the existing innermost-path test.
- **[LOW] ApprovalGate auto-approve else-branch payload shape never validated against the approval output schema** — `packages/components/src/components/ApprovalGate.js:33-44 (tested at tests/composite-coverage.test.jsx:481-484)` _(edge-case)_  
  - Untested: When `when` is false, ApprovalGate renders a static Task with `{approved:true, note:"auto-approved", decidedBy:null, decidedAt:null}`. The default approvalDecisionSchema (Approval.js:22-30) requires `decidedAt: z.string().datetime().nullable()` — null is allowed, so this passes, but no test runs this static payload through schema validation. composite-coverage.test.jsx:484 only asserts `staticPayload` `toMatchObject({approved:true})`; it does not validate the full shape against the schema, nor exercise a run that persists it.  
  - Risk: If the default decision schema is tightened (e.g. decidedAt made non-nullable, or decidedBy required) the auto-approve branch would start failing schema validation at write time — turning a no-human-needed gate into a hard run failure — and no component test would catch it because the static payload is never schema-checked. This is the auto-approve half of the approval gate (the replay-unsafe-approval / scope-denial siblings cover the human half).  
  - Test: Run a workflow with `<ApprovalGate when={false} output={outputs.gate} request={{title}}/>` to `finished` and assert the persisted row equals `{approved:true, note:"auto-approved", decidedBy:null, decidedAt:null}` validated against the default decision schema. Add a unit assertion that `approvalDecisionSchema.safeParse(autoPayload).success === true` so a schema tightening that breaks auto-approve fails loudly.
- **[LOW] Approval/HumanTask custom outputSchema rejection path (buildDecisionPayload note:null fallback) is uncovered** — `packages/components/src/components/Approval.js:90-93` _(error-path)_  
  - Untested: Coverage shows Approval.js lines 92-93 uncovered. buildDecisionPayload returns `{...base, note:null}` only when the stored approval has no string note AND `outputSchema.safeParse(base)` FAILS — i.e. a caller-supplied `outputSchema` that requires a `note` key. All existing tests use the default decision schema (where base parses successfully) or supply a note. The custom-outputSchema decision path is never resolved with a missing note.  
  - Risk: Workflows that pass a stricter `outputSchema` to Approval (requiring note/extra fields) hit an unverified branch; a bug there could emit a decision object that fails the user's own schema at write time, hard-failing an approved gate. Error/fallback shaping in the durable HITL decision path should be asserted, not left to incidental coverage.  
  - Test: Seed an approved approval with `note:null` and render `<Approval outputSchema={z.object({approved:z.boolean(), note:z.string(), decidedBy:z.string().nullable(), decidedAt:z.string().nullable()})} .../>`; resolve computeFn under withTaskRuntime and assert it returns a value where the `note:null` fallback was applied (exercising lines 92-93), then assert it round-trips through the supplied outputSchema as expected (or surfaces a clear validation error rather than a silent bad write).
- **[LOW] WaitForEvent/Signal onTimeout as a callback (not the "fail"/"continue" string) is never asserted to propagate** — `packages/components/src/components/WaitForEvent.js:24,32 and Signal.js:30` _(error-path)_  
  - Untested: WaitForEvent forwards `onTimeout` to `__smithersOnTimeout`/meta with a default of "fail". Signal forwards `props.onTimeout`. The Signal/Timer tests cover blocking/resume and async, but no test asserts a non-default `onTimeout` (e.g. a custom handler or "continue"/fallback string) reaches the host element / descriptor. The Poller `onTimeout` cases (poller-component.test.jsx) cover Poller's own loop, not the WaitForEvent/Signal external-wait timeout wiring.  
  - Risk: If onTimeout wiring drops to the "fail" default silently, a Signal/WaitForEvent that the author configured to resume-with-fallback on timeout would instead hard-fail the run (restart-waiting-event sibling). The default-fail behavior is the safe one, so a regression here degrades to surprising run failures rather than data loss, but it is still an unguarded contract.  
  - Test: Render `<WaitForEvent event="e" onTimeout="continue"/>` and `<Signal id="s" schema={z...} onTimeout={...}/>` via the renderer; assert the descriptor/host props carry the configured onTimeout (e.g. `__smithersOnTimeout === "continue"`), and a separate case asserting the default is "fail" when omitted.

### db (9)

- **[HIGH] Postgres seq-allocation fallback allocates seq without a transaction — racy and untested** — `packages/db/src/adapter.js:2087-2097` _(concurrency-race)_  
  - Untested: When the client does not expose exec/query/run (the Postgres / non-bun:sqlite dialect path), insertEventWithNextSeq (2087-2097) and insertSignalWithNextSeq (1854-1868) fall back to `lastSeq = getLastSeq() ?? -1; seq = lastSeq+1; insertIgnore(...)` with NO surrounding transaction or row lock. The postgres dialect test (db-postgres-dialect.test.js) never calls either *WithNextSeq function, so this fallback is entirely untested on Postgres.  
  - Risk: Read-then-write seq allocation without a transaction is a classic TOCTOU race: two concurrent Postgres writers can both read lastSeq=N and both write seq=N+1, and because the row is inserted via insertIgnore (ON CONFLICT DO NOTHING on the PK run_id+seq), one event/signal would be SILENTLY DROPPED rather than appended. On the Postgres-backed cloud deployment this silently loses events from the durable log with no error — the worst kind of durability bug, and there is no test even asserting basic sequential correctness on Postgres for these functions.  
  - Test: Using the existing pglite harness from db-postgres-dialect.test.js, insert two distinct events via insertEventWithNextSeq sequentially and assert seqs 0,1 and getLastEventSeq===1 (baseline). Then fire ~20 concurrent insertEventWithNextSeq and assert no rows are dropped: listEventHistory length === 20 and seqs are unique and gapless. (If this fails it documents the existing race; at minimum add the sequential test so a regression is caught and the race is tracked.)
- **[MEDIUM] insertEventWithNextSeq has no unit test for concurrent monotonic seq allocation (the BEGIN IMMEDIATE allocator)** — `packages/db/src/adapter.js:2071-2126` _(concurrency-race)_  
  - Untested: insertEventWithNextSeq is entirely uncovered in the db package (coverage 2071-2126). The only caller-side test (engine/tests/sql-message-storage-contract.test.js) inserts two events SEQUENTIALLY and asserts seq 0 then 1. There is no test that races N concurrent insertEventWithNextSeq calls on the same runId and asserts the resulting seqs are gapless, unique, and monotonic — which is the entire reason the function wraps a `BEGIN IMMEDIATE; SELECT MAX(seq)+1; INSERT; COMMIT` block guarded by acquireTransactionTurn().  
  - Risk: The event log seq is the ordering backbone for live streaming, listEvents(afterSeq), WS reconnect-afterseq, and deterministic replay. If two writers ever computed the same MAX(seq)+1 (e.g. a regression that drops BEGIN IMMEDIATE, removes acquireTransactionTurn, or changes it to a deferred BEGIN), the result is duplicate seqs or a UNIQUE violation that silently corrupts log ordering. A reordering/gap bug would ship undetected because no test exercises the contended path the function exists to protect.  
  - Test: Open one shared bun:sqlite Database, build a SmithersDb, insertRun. Fire Promise.all of ~50 insertEventWithNextSeq calls with distinct payloads on the same runId. Assert: returned seqs sorted === [0..49] (gapless, unique, no duplicates), getLastEventSeq === 49, and listEventHistory length === 50 with strictly increasing seq. Repeat across 2 SmithersDb instances sharing the same Database to mimic two code paths racing.
- **[MEDIUM] Event/signal replay-dedup path (identical row returns existing seq) is untested** — `packages/db/src/adapter.js:2075-2085` _(crash-safety-durability)_  
  - Untested: Both insertEventWithNextSeq (2075-2085) and insertSignalWithNextSeq (1833-1852) begin with a SELECT for an existing row matching (runId, timestampMs/receivedAtMs, type/signalName, payloadJson, ...) and, if found, return that existing seq instead of allocating a new one. No test inserts the same event/signal twice and asserts the second call is a no-op that returns the SAME seq (rather than appending a duplicate with a new seq).  
  - Risk: This dedup is the replay-safety mechanism for the durable event log: when the engine replays/resumes after a crash, re-emitting an already-persisted event must NOT double-append the log. If this branch breaks (e.g. the WHERE predicate stops matching on one column, or NULL handling for correlationId/receivedBy changes), replay would append duplicate events/signals, inflating seqs and corrupting live-stream diffs and signal delivery — exactly a 'replaying re-triggers a side effect' class of bug. It is completely unguarded by tests.  
  - Test: insertEventWithNextSeq with a fixed {runId, timestampMs, type, payloadJson}; capture seq. Call again with the identical row; assert returned seq === first seq and listEventHistory length unchanged (1). For signals: insert the same signal twice (and a variant with correlationId=null and receivedBy=null) and assert idempotency; then insert a DIFFERENT payload and assert it gets a new seq.
- **[MEDIUM] updateClaimedRun optimistic-concurrency guard (resume after crash) has no unit test** — `packages/db/src/adapter.js:1036-1068` _(crash-safety-durability)_  
  - Untested: updateClaimedRun is fully uncovered (1036-1068). It performs a compare-and-set UPDATE guarded by `runtime_owner_id = ? AND COALESCE(heartbeat_at_ms,-1) = COALESCE(?,-1)` and returns boolean success. No test asserts: it returns true and patches when the owner+heartbeat match; returns false (and does NOT mutate) when another owner has taken over or the heartbeat has advanced; handles the empty-patch early-return (returns true, no UPDATE); and the camelToSnake assignment building for multi-key patches.  
  - Risk: This is the write half of the crash-resume claim protocol (claimRunForResume is the read half and IS tested). After a resumer claims a stale run, every subsequent run mutation must go through updateClaimedRun so a returning original engine (split-brain) cannot clobber state. If the CAS predicate regresses (e.g. heartbeat NULL-coalescing breaks, or it stops returning false on mismatch), two engines could both write to the same run — the concurrent-resume-vs-supervisor scenario, at the db primitive level. The e2e fault covers the integration but not this unit's CAS true/false semantics or the empty-patch branch.  
  - Test: insertRun with runtime_owner_id='ownerA', heartbeat=100. updateClaimedRun({runId, expectedRuntimeOwnerId:'ownerA', expectedHeartbeatAtMs:100, patch:{status:'finished'}}) → expect true and getRun.status==='finished'. Then updateClaimedRun with expectedRuntimeOwnerId:'ownerB' (wrong owner) → expect false and getRun unchanged. Then with stale expectedHeartbeatAtMs → expect false. Finally with patch:{} → expect true and no error.
- **[MEDIUM] Human-request answer/cancel/reopen status-guard idempotency (HITL) is untested for the lost-update race** — `packages/db/src/adapter.js:1675-1691` _(concurrency-race)_  
  - Untested: answerHumanRequest, cancelHumanRequest, and reopenHumanRequest each constrain their UPDATE to a specific source status (`status = 'pending'` for answer/cancel, `status = 'answered'` for reopen). The existing test (db-adapter-coverage.test.js:104) only walks the happy sequence answer→reopen→cancel. There is NO test that a guarded transition is a no-op when the row is in the wrong state: answering an already-cancelled or already-expired request, double-answering (second answer must not overwrite the first decision), or cancelling an already-answered request.  
  - Risk: Approval/human-in-the-loop decisions are durable and must be write-once: a late answer arriving after the request was cancelled-on-timeout (or a duplicate answer from a retried RPC) must not silently overwrite the recorded decision or resurrect a closed gate. The status guard is the only thing preventing this, and its negative path is unverified — a regression dropping `AND status = 'pending'` would let a stale/duplicate human decision win, which is a trust-boundary failure for HITL.  
  - Test: insertHumanRequest (pending). cancelHumanRequest(id); then answerHumanRequest(id, ...) → assert getHumanRequest.status is still 'cancelled' and responseJson is null (no overwrite). Separately: answerHumanRequest twice with different payloads → assert the row retains the FIRST answer (second is a no-op). And: expireStaleHumanRequests then answerHumanRequest → assert status remains 'expired'.
- **[MEDIUM] FK-rebuild migration 0013 rollback-on-failure path is uncovered (destructive migration crash safety)** — `packages/db/src/schema-migrations.js:283-295` _(crash-safety-durability)_  
  - Untested: rebuildRunOwnedForeignKeyTable wraps the destructive table rebuild (RENAME → CREATE → copy valid rows → DROP legacy) in `BEGIN IMMEDIATE` with a catch that ROLLBACKs and rethrows (283-291), plus a `finally` that re-enables foreign_keys. Coverage shows 283-285,287 uncovered. The happy upgrade and orphan-drop counting are tested (migrations.test.js v0.19 case), but the failure branch — where the INSERT/copy throws mid-rebuild — is never exercised, so it is unproven that a failed migration leaves the original data recoverable (legacy table intact, no half-built table, foreign_keys restored).  
  - Risk: Migration 0013 is marked destructive and physically renames/recreates the frames/node_diffs/time_travel tables. If the rollback path is broken (e.g. ROLLBACK silently swallowed, or the legacy table already dropped before failure), a migration that fails on a user's real DB (disk full, constraint surprise) could destroy run history with no recovery — catastrophic for a durable control plane upgrading in place. The post-rebuild `PRAGMA foreign_key_check` throw (439) is also untested.  
  - Test: Build a v0.19-shaped DB (createV019RunOwnedSchema). Inject a failure into the copy step (e.g. pre-create a conflicting legacy table name, or stub createTableStatementFor to produce DDL that makes the INSERT fail). Run ensureSmithersTables and expect it to throw; then assert the original _smithers_frames table still has its rows, no orphan __legacy_fk_ table remains, PRAGMA foreign_keys is back ON, and the 0013 migration row was NOT recorded (so a retry will re-attempt).
- **[MEDIUM] Postgres DDL/placeholder translation has no direct unit tests (string-machine correctness)** — `packages/db/src/dialect.js:44-200` _(edge-case)_  
  - Untested: translatePlaceholders, translateDdl, columnType, jsonExtractText, and beginTransactionSql have zero direct test references; they are only exercised indirectly when the pglite postgres integration test runs (and CI runs on a box without browsers but pglite may still load). Coverage: dialect.js 71.82% lines, with uncovered ranges in the placeholder state machine (string/comment skipping at 58-69,75-76,87-88,106-113) and columnType (136-143). The `?`→`$n` rewriter must NOT rewrite `?` inside single quotes, double-quoted identifiers, or `--`/`/* */` comments — and those exact skip branches are the uncovered lines.  
  - Risk: Every Postgres write goes through translatePlaceholders. A bug that rewrites a `?` inside a string literal or comment to `$n` (or miscounts the index) produces a malformed query or, worse, binds parameters to the wrong positions — silent data corruption on the cloud Postgres backend. translateDdl's ordered regex (AUTOINCREMENT before INTEGER→BIGINT) is similarly load-bearing and only covered via end-to-end table creation, so a subtle reorder bug would not be isolated.  
  - Test: Pure unit tests on dialect.js: translatePlaceholders(POSTGRES, "WHERE a=? AND b='lit?eral' AND c=? -- ?comment\n AND d=?") → assert only the real placeholders became $1,$2,$3 in order and the `?` in the literal/comment are untouched. translateDdl(POSTGRES, "id INTEGER PRIMARY KEY AUTOINCREMENT, n INTEGER, b BLOB, r REAL") → assert BIGSERIAL PRIMARY KEY (not BIGINT...AUTOINCREMENT), BIGINT, BYTEA, DOUBLE PRECISION. jsonExtractText for both dialects with a quote-containing key (escaping).
- **[LOW] getRawNodeOutput silently swallows all query errors (returns null on failure)** — `packages/db/src/adapter.js:1254-1271` _(error-path)_  
  - Untested: getRawNodeOutput and hasPhysicalTable both end with `.pipe(Effect.catchAll(() => Effect.succeed(null/false)))`, so any underlying read failure (locked DB, malformed table, query error) is converted to 'no output' / 'table missing' with no log and no distinction from a genuine empty result. db-getRawNodeOutput.test.js exists but there is no test asserting what happens when the read genuinely fails (e.g. the boolean-column coercion throws, or the table name is malformed) — the swallow branch is untested.  
  - Risk: A node's persisted output is what replay/review reads back to decide whether work is already done. If a transient read error (e.g. SQLITE_BUSY that escaped retry, or a coercion bug) is silently swallowed as null, the engine may conclude a finished node has no output and re-run it — re-triggering side effects on replay, or the review/diff surface shows empty output for a node that actually produced data. The error-collapsing branch hides exactly the failures an operator needs to see.  
  - Test: Drive getRawNodeOutput against a state that forces the inner read to throw (e.g. corrupt/incompatible row that breaks coerceRawBooleanColumns, or close the underlying sqlite handle mid-call) and assert the current behavior (returns null) is intentional vs. should surface; at minimum add a test asserting a genuine empty table returns null AND that a real row round-trips with booleans coerced, so a future change that conflates the two is caught. Consider asserting a warn is logged on the catchAll path.
- **[LOW] listRunAncestry recursive CTE (continue-as-new lineage) has no unit test for depth ordering and limit** — `packages/db/src/adapter.js:896-913` _(missing-e2e-scenario)_  
  - Untested: listRunAncestry (896-913) walks parent_run_id via a recursive CTE and is uncovered at the db unit level. No test builds a parent→child→grandchild run chain and asserts the returned rows are ordered by depth ASC, that depth values are correct (0 for self, increasing toward ancestors), and that the limit param truncates the chain. The e2e continue-as-new-lineage fault covers the integration but not the db query's ordering/limit semantics.  
  - Risk: Lineage is how the UI and CLI reconstruct a continue-as-new chain for time-travel/fork. If the CTE join direction or depth ordering regressed (e.g. ORDER BY depth dropped, or the WHERE parent_run_id IS NOT NULL termination changed), the lineage view would show runs out of order or stop early, breaking fork-from-ancestor navigation. It is a self-contained, easily-unit-testable query left to integration-only coverage.  
  - Test: insertRun for r1 (parent_run_id null), r2 (parent r1), r3 (parent r2). listRunAncestry('r3') → assert rows are [{run_id:r3,depth:0},{run_id:r2,depth:1},{run_id:r1,depth:2}] in that order. Then listRunAncestry('r3', 2) → assert only depth 0 and 1 returned. Also assert listRunAncestry on a run with no parent returns just itself at depth 0.

### devtools (5)

- **[MEDIUM] Run-level waiting-approval / waiting-timer status is set but never cleared when the node resumes (inspector stuck blocked)** — `packages/devtools/src/DevToolsRunStore.js:161-177` _(happy-path-only)_  
  - Untested: NodeWaitingApproval sets run.status='waiting-approval' and NodeWaitingTimer sets run.status='waiting-timer', but no handler ever resets run.status back to 'running' when the node resumes (NodeFinished/NodeStarted/NodeRetrying for that node, or an approval-decided event). I confirmed empirically: after RunStarted -> NodeWaitingApproval -> NodeFinished on the same node, run.status is still 'waiting-approval'. No test asserts the run leaves the waiting state after the gate clears.  
  - Risk: This is the inspector-never-idle / human-in-the-loop guarantee. After an operator approves a gate and the run continues to completion, the devtools read-model still reports the run as permanently 'waiting-approval' (or 'waiting-timer'). An inspector/UI built on this would show a live, progressing (or even finished) run as eternally blocked on a human, masking real progress and real new blocks. The task explicitly calls out approval gates and inspector-never-idle as severe.  
  - Test: In DevToolsRunStore.test.ts: feed RunStarted -> NodeWaitingApproval(n1) -> NodeFinished(n1); assert run.status transitions back to 'running' (not 'waiting-approval'). Same for NodeWaitingTimer -> NodeStarted/NodeFinished. Also assert that after RunFinished the run is 'finished' even if it had been waiting-approval. (If current code does not reset, this test will fail and expose the bug.)
- **[MEDIUM] Replayed/duplicate lifecycle events flip a terminal run or task back to a non-terminal state (stale-completion)** — `packages/devtools/src/DevToolsRunStore.js:89-160,114-133` _(crash-safety-durability)_  
  - Untested: processEngineEvent overwrites status unconditionally with no monotonic terminal guard. Confirmed empirically: RunStarted after RunFinished sets run.status back to 'running' while finishedAt stays set (inconsistent zombie state); NodeStarted after NodeFinished resets a task from 'finished' to 'started'. No test feeds a terminal-then-earlier sequence or replays the same lifecycle twice.  
  - Risk: On WS reconnect-after-seq, fork/replay, or supervisor takeover, the engine event log is commonly re-fed from an earlier seq. Without terminal monotonicity the inspector resurrects already-completed runs/tasks as live — the classic stale-completion class. A finished run would render as running with a stale finishedAt, and a completed task would show as in-progress, breaking idle detection and time-travel display correctness.  
  - Test: Feed RunStarted->RunFinished->RunStarted and assert status stays 'finished' (and finishedAt unchanged). Feed NodeStarted->NodeFinished->NodeStarted and assert task stays 'finished'. Then replay the entire event array twice through a fresh store and assert the final state is identical to a single pass (idempotent replay).
- **[LOW] Out-of-order FrameCommitted regresses frameNo to a stale value** — `packages/devtools/src/DevToolsRunStore.js:106-108` _(concurrency-race)_  
  - Untested: FrameCommitted assigns run.frameNo = event.frameNo with no max/monotonic guard. Confirmed: FrameCommitted(5) followed by FrameCommitted(2) leaves run.frameNo=2. No test feeds frames out of order or duplicated.  
  - Risk: frameNo is the seq cursor the live-stream/inspector uses to decide what to fetch next and to detect rewind vs forward progress. A late/duplicate/out-of-order FrameCommitted (possible on reconnect replay or interleaved producers) would rewind the cursor and could cause the consumer to re-request or mis-order frames, or display the run as having moved backward when it has not.  
  - Test: Feed FrameCommitted frameNo=5 then frameNo=2; assert run.frameNo stays 5 (monotonic), or — if regression is intended for rewind — document and assert the exact intended semantics. Also feed a duplicate FrameCommitted(5) and assert no adverse effect.
- **[LOW] applyDelta silently clamps a negative/out-of-range addNode index instead of rejecting it** — `packages/devtools/src/applyDelta.js:86-94` _(edge-case)_  
  - Untested: addNode validates that index is a finite number but then Math.max(0, Math.min(op.index, len)) clamps any out-of-range value. Confirmed: index=-5 inserts at position 0. Existing tests only cover the rejection of non-numeric index, never the clamping of a valid-but-out-of-range (negative or > length) numeric index.  
  - Risk: diffSnapshots is supposed to emit ascending, exact final indices (a documented invariant the reorder regression tests depend on). If a producer or a future diff change emits a wrong negative/overflow index, applyDelta would silently place the node at the wrong sibling position rather than failing loud, corrupting tree order in the inspector without any error surfacing.  
  - Test: applyDelta with addNode index=-5 and with index=999 on a 2-child parent; assert the resulting child ordering matches the clamped position AND lock in the intended contract (either it clamps deterministically to 0/length — assert exact order — or it should throw; pick one and assert it so a regression is caught).
- **[LOW] diffSnapshots is not tested for a node that moves to a new parent while keeping the same id/shape, nor for cycles/missing-children robustness** — `packages/devtools/src/diffSnapshots.js:149-203` _(edge-case)_  
  - Untested: The 'moved' branch (fromEntry.parentId !== toEntry.parentId) is exercised only via sibling reorders within one parent; a true cross-parent move (same id, same type/name/depth) is not asserted to round-trip, and the topLevelAddIds parentId-null filter / depth-tie sort fallback is the one uncovered line (99.35%). diffSnapshots also assumes well-formed inputs (children present, no duplicate ids) with no test for malformed/duplicate-id trees.  
  - Risk: Cross-parent moves are common when the engine restructures a subtree (e.g., loop/subflow re-parenting). If the remove+add ordering for a move is wrong, the inspector tree silently desyncs from the engine tree — and because diffSnapshots emits the transport for every reconnect, a desync is durable, not transient. Duplicate-id input (a plausible producer bug) would currently overwrite Map entries and silently drop nodes.  
  - Test: Add a diff case where child id 3 moves from parent A to parent B with unchanged shape (I verified this round-trips today — lock it in). Add a case where depth-3 grandchildren move up to depth-1 (forces the depth-tie sort path). Add a defensive test feeding a tree with two nodes sharing an id and assert diffSnapshots either throws or produces a deterministic, documented result.

### driver (6)

- **[MEDIUM] drainInflight() barrier invariant not asserted — a task settling DURING drain can still report to the session** — `packages/driver/src/WorkflowDriver.js:574-579` _(concurrency-race)_  
  - Untested: drainInflight() exists to enforce the durability invariant documented in its own JSDoc: 'no executor writes after the run is terminal' (called before Failed and ContinueAsNew exits). The two existing tests (ctx-utils.test.js:659, :693) only assert that after the run resolves `driver.inflightTasks.size === 0`. They never assert that a task whose executor settles AFTER the terminal decision was taken does NOT get reported via session.taskCompleted/taskFailed, nor that settledTasks accumulated during the drain are discarded (line 578 `this.settledTasks.length = 0`). The while-loop re-checking `inflightTasks.size > 0` (to catch tasks started mid-drain) is also unasserted.  
  - Risk: If a regression let a late-settling task call session.taskCompleted after the run is Failed/ContinuedAsNew, an executor would write output/VCS state into a run the engine already considers terminal — corrupting the durable record and breaking continue-as-new lineage and replay (a completion appears in a run that 'already ended'). Line coverage is 100% here yet the actual ordering guarantee is unverified.  
  - Test: Build a driver whose taskCompleted decision is Failed while a second slow task is still in flight. Spy on session.taskCompleted/taskFailed. Resolve the slow task only AFTER run() returns Failed. Assert: (a) the slow task's settle never produced a taskCompleted/taskFailed call after the terminal decision, (b) settledTasks is empty, (c) inflightTasks.size === 0. Repeat for the ContinueAsNew path. Also: start a task inside the executor of a draining task to assert the while-loop drains second-generation in-flight work.
- **[MEDIUM] Deadline-elapsed-without-completion fallback (timer/retry fires while siblings in flight) is unasserted** — `packages/driver/src/WorkflowDriver.js:506-541,584-594` _(edge-case)_  
  - Untested: handleWait, when work is in flight (line 585), keeps the wait's deadline and calls nextCompletionDecision(deadlineMs) so a Timer/RetryBackoff elsewhere still fires on time. If that deadline elapses before any task settles, nextCompletionDecision falls through (settledTasks empty) to either re-submit lastGraph for a fresh decision (line 538-539) or return {status:'waiting-event'} when lastGraph is null (line 541). No test injects a deadline that elapses first while an in-flight task is still pending — the existing out-of-order test always resolves a task. The Timer-reason deadline computation `reason.resumeAtMs - Date.now()` (line 592) is also never exercised in the in-flight branch.  
  - Risk: This is a scheduling-boundary durability path: a retry backoff or timer must fire even when unrelated tasks are still running. If the deadline race (Promise.race with sleepWithAbort at line 518) or the lastGraph re-submit regressed, a timer/retry could be silently swallowed (run hangs forever) or fire late — exactly the kind of bug that makes a run never resume. Line coverage shows these lines as among the few uncovered in WorkflowDriver.js.  
  - Test: Driver with one never-settling in-flight task and lastGraph set; call handleWait({_tag:'Timer', resumeAtMs: Date.now()+20}). Assert it re-submits lastGraph (spy session.submitGraph) after ~20ms rather than blocking on the task. Repeat with RetryBackoff{waitMs:20}. Then set lastGraph=undefined and assert the deadline path returns {status:'waiting-event'}.
- **[MEDIUM] spawnCaptureEffect: onStdout/onStderr callbacks that throw, and chunks arriving after settle, are unhandled-path untested** — `packages/driver/src/child-process.js:153-182,99-118` _(error-path)_  
  - Untested: onStdout/onStderr (lines 166,181) are invoked synchronously inside the data handler with no try/catch. If a caller's callback throws, it propagates into the stream 'data' event — potentially crashing or leaving timers uncleared. Tests only use push-to-array callbacks that never throw. Also untested: a data chunk arriving AFTER finalize/kill has set settled=true (the handler still does resetIdle() and string concat work, and still calls onStdout) — i.e. ordering of late chunks vs a fired timeout. The idle-timer generation guard (idleGeneration, lines 93,104-108) that prevents a stale idle timer from killing after a reset is only indirectly covered by the 'idle timer resets' happy test, never by a test that forces an out-of-order stale timer fire.  
  - Risk: A throwing observer callback turning into an unhandled stream error would crash the process running the agent — the same class of bug the EPIPE test guards against on stdin, but unguarded on stdout/stderr. Late chunks after settle invoking onStdout could deliver output to a caller who already saw the run resolve, confusing live-streaming consumers.  
  - Test: Run with onStdout that throws on first chunk; assert the Effect still settles (the process completes/fails) and no uncaughtException fires (mirror the stdin EPIPE test's uncaughtException listener). Separately, run a process that emits a chunk shortly after a timeout would fire, and assert onStdout is not invoked after the PROCESS_TIMEOUT failure / that settled prevents double-resume.
- **[LOW] spawnCaptureEffect truncation can split a multi-byte UTF-8 sequence — no test asserts byte-cap safety on non-ASCII** — `packages/driver/src/child-process.js:16-23,157-165` _(edge-case)_  
  - Untested: truncateToBytes slices a UTF-8 Buffer at an arbitrary byte offset (buf.subarray(0,maxBytes).toString('utf8')). All truncation tests use single-byte ASCII ('A'/'B'/sentinels). A cap that lands mid-codepoint of a multi-byte char (emoji, CJK, accented) yields a replacement char and, more importantly, the byte-length-after-decode-then-reencode can differ. No test feeds non-ASCII output near the cap, nor asserts the resulting string is still valid/decodable and within the byte cap.  
  - Risk: CLI agents routinely emit UTF-8 (file paths, prompts, emoji in logs). If truncation corrupts the boundary, downstream JSON.parse of agent output (the whole point of capture) can throw on mojibake even when the JSON would otherwise be valid up to the cap, or a partial codepoint inflates byte length past maxOutputBytes. This is an error-path the tests claim to cover ('truncated JSON parse fails predictably') but only for ASCII.  
  - Test: Spawn a process that writes a long string of multi-byte chars (e.g. '日'.repeat(N) or emoji) with a small maxOutputBytes whose cap lands mid-codepoint. Assert Buffer.byteLength(result.stdout) <= maxOutputBytes and that result.stdout has no unintended trailing partial bytes (or document/handle the replacement-char behavior explicitly). Repeat for truncateKeep:'tail'.
- **[LOW] RetryBackoff sleep interrupted by abort, and getNextDecision-vs-lastGraph fallback ordering, only happy-path tested** — `packages/driver/src/WorkflowDriver.js:612-624` _(error-path)_  
  - Untested: handleWait's RetryBackoff branch (no in-flight work) sleeps via sleepWithAbort then checks `signal.aborted` (line 614) to cancel, else prefers session.getNextDecision (617), else submitGraph(lastGraph) (620), else returns waiting-timer (623). The existing test (ctx-utils.test.js:745) only covers waitMs:0 with getNextDecision present. Untested: (a) abort firing DURING the backoff sleep → cancelRun; (b) the submitGraph(lastGraph) fallback when getNextDecision is absent but lastGraph exists; (c) the terminal `waiting-timer` return when neither exists.  
  - Risk: Retry-with-backoff is a core scheduling guarantee. If the abort-during-sleep check regressed, a cancel issued during a backoff window would be ignored and the run would resume a task the user cancelled. The lastGraph fallback is the resume path when the session lacks getNextDecision — a regression there means a backoff never re-derives a decision and the run stalls.  
  - Test: Driver with no in-flight tasks; call handleWait({_tag:'RetryBackoff', waitMs:50}) and abort the signal at ~10ms; assert result is {status:'cancelled'}. Second: session without getNextDecision but with lastGraph set; assert handleWait(RetryBackoff) re-submits lastGraph. Third: no getNextDecision and no lastGraph; assert {status:'waiting-timer'}.
- **[LOW] loadCreateSession failure path (scheduler import/factory missing) throws unhelpfully — untested** — `packages/driver/src/WorkflowDriver.js:156-170,380-383` _(error-path)_  
  - Untested: initializeSession falls back to loadCreateSession() which dynamic-imports the scheduler and returns null if the import throws or makeWorkflowSession is absent (lines 163-169). When null, initializeSession throws the 'requires a WorkflowSession or createSession' error (line 382). The happy 'loads the default scheduler session factory' test (ctx-utils.test.js:542) only exercises the success branch; the null/throw branch is never tested.  
  - Risk: Minor, but this is the bootstrap error a user hits when the scheduler dep is misconfigured; an unasserted error message can silently degrade (e.g. become a confusing TypeError) and the catch-swallow at line 163 could mask a real import error. Low severity because it is a startup misconfiguration, not a runtime durability path.  
  - Test: Construct a driver with session:undefined and createSession:undefined in an environment where loadCreateSession returns null (or inject createSession that returns a non-WorkflowSession non-Effect value), and assert initializeSession rejects with the explicit 'requires a WorkflowSession or createSession' message rather than a generic TypeError.

### e2e-meta (7)

- **[HIGH] No fault case actually kills and restarts a real engine process — 'crash recovery' is simulated by writing a stale heartbeat into a fake table** — `e2e/faults/case01-kill-engine-mid-task.test.ts (and case02/03/04/05/06 via corruptHeartbeat); harness/killProcess.ts (used only by harness/killProcess.test.ts)` _(crash-safety-durability)_  
  - Untested: killProcess() is built and unit-tested but imported by ZERO fault cases. Every crash-recovery case (01,02,03,04,05,06) models 'engine died' by calling corruptHeartbeat(db, runId, 'stale') against a hand-built `:memory:` _smithers_runs table, then asserts deriveRunState() returns 'stale' and a hand-written takeover SQL CAS picks a single winner. No test starts a real engine in a child process, SIGKILLs it mid-task, restarts the engine against the same on-disk DB, and asserts the run resumes from its last checkpoint with no lost/duplicated work.  
  - Risk: The product's #1 guarantee is 'a run survives engine kill and resumes from its last checkpoint.' A simulated stale-heartbeat row proves only that deriveRunState classifies a number; it cannot catch a real recovery bug: a half-written checkpoint on SIGKILL, the restarted engine re-executing the in-flight node, a WAL frame lost on abrupt exit, or the supervisor never being triggered by a real stale heartbeat. All of those would ship green.  
  - Test: Add a real cross-process case: spawn the engine (Bun.spawn) running a multi-node workflow against an on-disk DB, wait for node 2 to start (poll the events table), killProcess(child,'SIGKILL'), then start a fresh engine/supervisor against the same DB path and assert (a) the run reaches finished, (b) each node's output row appears exactly once, (c) no duplicate ToolCallStarted for the in-flight node, (d) recovery happened within the heartbeat-stale SLO.
- **[MEDIUM] freezeSqliteLock (DB-wedge / slow-disk injector) is built and unit-tested but never used by any fault case — engine behavior under a locked/slow DB is untested** — `e2e/harness/freezeSqliteLock.ts (used by harness/freezeSqliteLock.test.ts only; 0 fault cases)` _(crash-safety-durability)_  
  - Untested: freezeSqliteLock holds a BEGIN IMMEDIATE reserved lock to block writers (the harness comment explicitly says 'matching the engine's degraded-but-not-dead behavior under a wedged DB'). No fault case injects it into a running engine. There is no scenario for: heartbeat writes blocked by a held lock, checkpoint/frame inserts timing out under SQLITE_BUSY, the engine emitting a stale heartbeat purely because the DB was wedged (false-positive takeover), or a slow-disk (fsync stall) extending a write past the heartbeat SLO.  
  - Risk: A wedged or slow durability store is one of the most common real-world failures (noisy-neighbor disk, fsync stall, another process holding the lock). If the engine treats SQLITE_BUSY as a crash and a supervisor takes over while the original owner is alive-but-blocked, you get a split-brain double-resume — exactly the failure the takeover CAS is supposed to prevent, but the CAS is only tested against simulated rows, not against a real lock-induced stall.  
  - Test: Start a real engine on an on-disk DB; once running, freezeSqliteLock(dbPath, 2000ms) to block writers mid-task; assert the engine either (a) retries the write and continues without losing the heartbeat lease, or (b) cleanly surfaces an unhealthy 'db-wedged' reason distinct from engine-heartbeat-stale; and critically assert a supervisor does NOT take over a run whose owner is merely blocked on the lock (no split-brain).
- **[MEDIUM] network-policy 'deny' half is never enforced — case23 asserts proxy CONFIG delivery, not that a disallowed request is actually blocked** — `e2e/faults/case23-network-policy-allow-deny.test.ts:64 (provider id 'iron-proxy-fake' whose run() just inspects request.egress)` _(security-trust-boundary)_  
  - Untested: The case is titled allow/deny but only verifies the egress proxy config (httpsProxy, CA cert path, secretBindings) is handed to a FAKE provider, and that the harness's own HTTPS_PROXY is restored. No request is actually made through a real egress proxy, and nothing asserts that a disallowed host is blocked while an allowed host succeeds. The 'deny' behavior — the entire security point — is absent.  
  - Risk: Network policy is a trust-boundary guarantee: a compromised/confused agent must not be able to exfiltrate to an arbitrary host. Verifying config delivery proves nothing about enforcement; a bug where the proxy is configured but allow/deny rules are inverted or not applied would ship green. There is no positive (allowed host reachable) or negative (denied host refused) assertion against a real proxy.  
  - Test: Stand up a real loopback egress proxy that allows host A and denies host B; run a real sandbox task that fetches both; assert the request to A succeeds and the request to B is refused at the proxy (connection refused / 403), and that the denial is surfaced to the run. Separately assert the agent cannot bypass the proxy by hitting a raw IP.
- **[MEDIUM] Missing fault class: durability-store corruption (disk-full / torn WAL / partial-write / truncated DB) on the recovery path** — `e2e/ (no case; harness has corruptHeartbeat for a single field but no file/WAL corruption injector)` _(crash-safety-durability)_  
  - Untested: The harness can corrupt a single heartbeat column but cannot inject: a disk-full ENOSPC during a checkpoint/frame write, a torn/partial WAL frame from a SIGKILL mid-fsync, a truncated or zero-length DB file, or a -wal/-shm left behind by an abrupt exit. No case opens a DB that crashed mid-transaction and asserts the engine recovers (WAL replay) or fails safe rather than reading torn state.  
  - Risk: A SIGKILL mid-write is the realistic version of 'engine kill', and SQLite's safety depends on WAL replay working and on the engine not committing a half-written checkpoint. If a torn frame is read as a valid checkpoint, replay reproduces corrupt state — silently violating deterministic replay. Disk-full mid-run is also common in CI/cloud sandboxes and must fail the run cleanly, not corrupt the log.  
  - Test: (a) ENOSPC: run a real engine on a tiny/full tmpfs and assert a checkpoint write failure marks the run failed with a clear error and leaves the prior checkpoint intact (resumable). (b) Torn WAL: kill the engine mid-transaction (SIGKILL during a large frame insert), reopen the DB, and assert WAL recovery yields the last fully-committed checkpoint and the run resumes deterministically — no partial node output, no phantom event seq.
- **[LOW] Missing fault class: schema migration / version upgrade during a live run (resume after the engine binary or DB schema is upgraded)** — `e2e/ (no case exercises ensureSmithersTables/migrations against a run paused under an older schema)` _(regression-risk)_  
  - Untested: No case starts a run, pauses it (waiting-approval/event/timer), applies a forward migration / opens the DB with a newer ensureSmithersTables, and then resumes. Nothing asserts that a checkpoint/frame/event written by version N is readable and resumable by version N+1, or that a migration mid-flight does not corrupt in-flight run state.  
  - Risk: Durable runs outlive deploys — a long-running run will frequently be resumed by a newer engine/schema than the one that started it. A migration that drops/renames a column an in-flight run depends on, or a serialized-checkpoint format change, would break resume for exactly the long-lived runs the product exists to protect, and no test guards the cross-version boundary.  
  - Test: Start a run and park it at an approval gate, snapshot the on-disk DB. Apply the latest migrations (or simulate N+1 by adding the next migration), reopen with the current adapter, resume, and assert the run finishes correctly with outputs intact. Add a negative: a deliberately incompatible checkpoint surfaces a clear migration error rather than silent corruption.
- **[LOW] Latency budgets are defined but asserted by zero tests — README claims regressions fail, but reconnect/suite-wall-time regressions are silently uncaught** — `e2e/budgets/latency.json (reconnectAfterSeqMaxMs, perPRSuiteWallTimeMaxMs, nightlySoakWallTimeMaxMs); referenced by no test (only loadBudget('memory') is called)` _(regression-risk)_  
  - Untested: loadBudget('latency') is never called. case09 (reconnect-afterseq) does not assert reconnectAfterSeqMaxMs (5000ms). No test enforces perPRSuiteWallTimeMaxMs or nightlySoakWallTimeMaxMs. README.md explicitly says 'memory and latency ceilings ... regressions fail; they are not just recorded' — the latency half is false.  
  - Risk: Reconnect latency is a user-facing SLO for the live control plane (a dropped WS that takes 30s to replay-from-afterSeq is a broken UX and a sign of an O(N) replay regression). With no assertion, a reconnect/replay slowdown ships unnoticed, and the documented guarantee is misleading.  
  - Test: In a real-gateway reconnect test, measure wall time from WS drop to fully caught-up (lastSeq == authoritative max) and assert < latency.reconnectAfterSeqMaxMs. Optionally add a suite-level afterAll that records total wall time and fails if it exceeds perPRSuiteWallTimeMaxMs, fulfilling the README's claim.
- **[LOW] Clock-skew fault only affects the test process; child-process / cross-node clock skew (the realistic distributed-clock case) is untested** — `e2e/harness/skewClock.ts:9 (comment: a child process via spawnSync will NOT see the skew); used only by case08-inspector-never-idle` _(edge-case)_  
  - Untested: skewClock monkey-patches Date.now only in the current process and the harness explicitly notes child processes don't inherit it. The realistic failure — engine and supervisor on hosts with skewed clocks disagreeing about whether a heartbeat is stale, or a forward/backward NTP jump mid-run — is not tested. Only case08 uses it, and only in-process for inspector idle detection. Backward clock jumps (heartbeat 'in the future' beyond the corruptHeartbeat 'future' nudge) and their effect on stale-threshold math are not exercised against the real takeover path.  
  - Risk: Heartbeat-staleness, timer firing, and continue-as-new lineage all depend on time comparisons across processes. If the engine and supervisor disagree on 'now' by more than the stale threshold, you get either premature takeover (split-brain) or a never-detected dead engine. A backward NTP step could make a fresh heartbeat look stale. None of these cross-process clock disagreements are covered.  
  - Test: Run engine in a child process with an injectable clock offset (env var the engine honors for now()); set the supervisor's clock +60s relative to the engine; assert the supervisor does NOT prematurely take over a healthy run (i.e. staleness is robust to bounded skew). Add a backward-jump variant: heartbeat written, clock steps back, assert the run is not misclassified stale.

### engine (3)

- **[MEDIUM] isRunHeartbeatFresh (the resume-vs-takeover liveness gate) has no unit test** — `packages/engine/src/engine.js:1937 (isRunHeartbeatFresh); RUN_HEARTBEAT_STALE_MS=30_000 at :853` _(concurrency-race)_  
  - Untested: isRunHeartbeatFresh(run, now) is exported (re-exported from index.js) and is the decision used at engine.js:4624 to throw RUN_STILL_RUNNING (refuse a non-forced resume) when an existing 'running' run looks alive. Its full boolean truth table is unasserted: status must be exactly 'running' (a waiting-* or failed run is never 'fresh' even with a recent heartbeat); heartbeatAtMs must be a number (null/undefined => not fresh); and the staleness boundary is now - heartbeatAtMs <= 30_000 (exactly 30s = fresh, 30_001ms = stale). grep shows no test references isRunHeartbeatFresh at all.  
  - Risk: This function decides whether two processes can both drive the same run. If 'fresh' wrongly returns true for a stale/dead run, a crashed run can never be resumed without --force (stuck forever). If it wrongly returns false (e.g. boundary flipped to <, or status check dropped so a waiting-approval run counts as fresh), a second supervisor can take over a run whose original owner is still alive → split-brain double execution, duplicated side effects, corrupted attempt rows. The e2e concurrent-resume-vs-supervisor covers the wired path but not this exported gate's boundary, and exported API surface should have its contract pinned.  
  - Test: Unit test isRunHeartbeatFresh directly: fresh=true for {status:'running', heartbeatAtMs: now-29_999}; fresh at exactly now-30_000; NOT fresh at now-30_001; NOT fresh for {status:'waiting-approval', heartbeatAtMs: now} and {status:'failed', heartbeatAtMs: now}; NOT fresh for {status:'running', heartbeatAtMs: null}; NOT fresh for null/undefined run. Pin the 30s boundary so a constant change is caught.
- **[LOW] assertResumeDurabilityMetadata: defensive/missing-hash/legacy/VCS-root branches and the must-NOT-throw path are unasserted** — `packages/engine/src/engine.js:1802-1853 (assertResumeDurabilityMetadata)` _(error-path)_  
  - Untested: engine-internals.test.js asserts only a single throwing case (everything changed at once), and durability.test.jsx covers 'workflow file changed' and 'imported module changed' via integration. Untested branches: (1) the happy path where existing == current metadata must NOT throw (no test proves a legitimate resume is allowed); (2) the storedDurabilityVersion >= DURABILITY_METADATA_VERSION branch that pushes 'workflow module graph unavailable' / 'workflow entry hash unavailable' when a hash is missing on either side (engine.js:1820-1831); (3) the legacy else-if branch (storedDurabilityVersion < current version) that compares existingRun.workflowHash to current.entryWorkflowHash (engine.js:1833-1835); (4) VCS-root-changed in isolation (engine.js:1836-1840), including the resolve()-normalization so /repo and /repo/ compare equal but /repo vs /repo2 mismatch.  
  - Risk: This is the trust/durability gate that refuses to resume a run when the code or repo it ran against changed — a wrong resume here replays decisions against a different program/worktree, producing silently incorrect state (the whole point of deterministic replay). A regression that (a) throws on a legitimate matching resume would brick every resume; (b) fails to push a mismatch when a hash is missing/legacy would allow an unsafe resume to proceed. Only the all-changed case is locked; the missing-hash and legacy-version and VCS-only paths can each break independently and ship undetected.  
  - Test: Add unit cases on __engineInternals.assertResumeDurabilityMetadata: (a) identical metadata + matching stored durability config => does not throw; (b) stored version >= current with existingRun.workflowHash absent => throws mentioning 'workflow module graph unavailable'; with storedEntryWorkflowHash absent => 'workflow entry hash unavailable'; (c) stored version 0/legacy config => compares existingRun.workflowHash vs current.entryWorkflowHash and throws 'workflow entry file changed' on diff; (d) only vcsRoot differs (/repo vs /repo2) => throws 'VCS root changed', and /repo vs /repo/ => does NOT throw (resolve-normalized).
- **[LOW] EventBus.emitEventQueued ordering under slow/interleaved persists, and non-atomic seq++ fallback under concurrency** — `packages/engine/src/events.js:111-121 (enqueuePersist) and :126-153 (persistDb seq fallback, this.seq++)` _(concurrency-race)_  
  - Untested: Two adjacent behaviors are unasserted. (1) Event LOG/DB ordering: emitEventQueued emits synchronously but chains persistence through this.persistTail (a promise tail). No test proves that if event A's persist is slow, event B still persists AFTER A (ordering preserved) — events-bus/events-advanced/events-persist-log-append only assert append-as-lines, queued-failure-surfaces-once, and parallel emitEventWithPersist (which is the unqueued path). (2) Seq fallback: when the db exposes only insertEvent/insertEventEffect (no insertEventWithNextSeq), persistDb stamps seq = this.seq++ locally. There is a test that DB-assigned seq does not advance local seq, but none that the local-seq fallback assigns gapless, unique, monotonic seqs across many emits — and this.seq++ is read-modify-write that can collide if two emitEvent effects run concurrently against the same bus.  
  - Risk: The gateway/live-stream contract depends on a strict, gapless event seq ordering for replay scrubbing, reconnect-after-seq, and frame application. If queued persists can reorder (A lands after B), or if the local-seq fallback can hand out duplicate/colliding seqs under concurrency, the durable event log and the live stream disagree, breaking time-travel/reconnect correctness — a subtle corruption that no current assertion would surface (the e2e reconnect/seq tests run against the insertEventWithNextSeq path, not the local fallback).  
  - Test: (1) Ordering: build EventBus with a db whose insertEventEffect resolves on a controllable delay; emitEventQueued(A) with a long delay then emitEventQueued(B) fast; await flush(); assert the db received A before B. (2) Seq fallback: build EventBus({db}) where db only has insertEventEffect that records rows; fire N emitEvent calls (some via Promise.all to force concurrency); assert the recorded seqs are exactly 0..N-1 with no duplicates or gaps, proving the this.seq++ fallback is collision-free (or, if it is intentionally single-threaded, document/assert that emitEvent must not be called concurrently).

### errors (4)

- **[MEDIUM] errorToJson output is not guaranteed JSON-serializable; circular `cause` or BigInt `details` crash the durable error-persistence path** — `packages/errors/src/errorToJson.js:18-23` _(crash-safety-durability)_  
  - Untested: errorToJson copies error.cause and error.details by raw reference into the returned object (lines 19,21 for SmithersError; 30,32 for EngineError). No test feeds a SmithersError/EngineError whose cause is circular or whose details contain a BigInt/function/undefined and then asserts the result is safe for JSON.stringify. I verified by execution: `JSON.stringify(errorToJson(new SmithersError('INTERNAL_ERROR','boom',undefined,{cause:circular})))` throws 'Converting circular structure to JSON', and `details:{n:10n}` throws 'Do not know how to serialize a BigInt'.  
  - Risk: Every consumer wraps this in JSON.stringify and writes it to the durable event log: engine.js lines 4367, 4423 (`errorJson: JSON.stringify(errorToJson(effectiveError))`) and server/index.js line 501. These calls happen ON THE ERROR PATH while recording a failed task. If errorToJson returns a non-serializable graph (trivially produced when a real failure's cause is an http client error, an effect fiber, or a circular Node object), the stringify throws inside the error handler, the failure is never durably recorded, and the run's crash-safety/resume story breaks at the worst possible moment (recording the very failure it must survive). This ships undetected because the unit never exercises non-trivial cause/details.  
  - Test: In errors-core.test.js add: (1) build a SmithersError with a circular cause object and assert `expect(() => JSON.stringify(errorToJson(e))).not.toThrow()` (drives a fix toward safe serialization, e.g. JSON.stringify with a circular-safe replacer or cause flattening); (2) details containing a BigInt and a function value, assert the stringified output is produced and the BigInt/function are represented (not thrown); (3) an EngineError whose context contains a circular object, same assertion. Inject the exact shapes a real driver/http failure produces.
- **[LOW] Tagged-error durable round-trip silently corrupts when required numeric fields are absent (NaN -> null -> not-equal on restore)** — `packages/errors/src/toTaggedErrorPayload.js:36-38,44-48` _(crash-safety-durability)_  
  - Untested: toTaggedErrorPayload coerces with Number(error.attempt)/Number(error.timeoutMs)/etc. When a field is missing/undefined, Number(undefined)=NaN, and JSON.stringify serializes NaN as null. fromTaggedErrorPayload then reconstructs with that null. No test exercises a TaskTimeout/TaskHeartbeatTimeout whose numeric fields are undefined through a real JSON.parse(JSON.stringify(payload)) cycle. I verified: `toTaggedErrorPayload(new TaskTimeout({message:'m',nodeId:'n'}))` yields `{...,attempt:null,timeoutMs:null}` and the restored error has attempt=null, timeoutMs=null instead of the original undefined.  
  - Risk: single-runner.js (engine effect boundary) calls toTaggedErrorPayload to ship the error across the runner boundary and fromTaggedErrorPayload to reconstruct it. The reconstructed error is what retry/backoff/scheduling logic reads (attempt count, timeoutMs). A null/NaN attempt or timeoutMs can break retry-attempt accounting or backoff math after a resume, and the deserialized error no longer equals what was raised — a replay-fidelity violation. The existing round-trip tests only use fully-populated payloads, so this lossiness is invisible.  
  - Test: Add a parametric round-trip test that does `const restored = fromTaggedErrorPayload(JSON.parse(JSON.stringify(toTaggedErrorPayload(instance))))` for each tagged case INCLUDING instances constructed with missing/undefined numeric fields, and assert restored deep-equals a well-defined expected (decide and assert the contract: either reject/throw on missing required fields, or preserve them as undefined rather than null). Also assert toTaggedErrorPayload never emits NaN values.
- **[LOW] fromTaggedError default/foreign-_tag branch and message-coercion are unasserted** — `packages/errors/src/fromTaggedError.js:19,60-62` _(error-path)_  
  - Untested: fromTaggedError has a `default: return undefined` branch (line 60) for objects that have a string _tag but an unrecognized one — but the tests only feed null and plain Error (line 325), never `{_tag:'SomethingForeign', message:'x'}`. Also the message fallback `typeof payload.message === 'string' ? payload.message : String(payload._tag)` (line 19) is never tested with a non-string message. I verified toTaggedErrorPayload similarly coerces an object message to '[object Object]' with no test.  
  - Risk: fromTaggedError is the front door of toSmithersError and errorToJson for any duck-typed `{_tag,...}` value crossing process/RPC boundaries (gateway, MCP, child runs). An attacker- or bug-supplied foreign _tag, or a non-string message, must be handled deterministically. If the default branch regressed (e.g. someone made it throw or return a partial), foreign tagged payloads from an untrusted boundary could crash normalization or inject a bogus code — and the current suite would not catch it.  
  - Test: Add tests: fromTaggedError({_tag:'NotAKnownTag', message:'x'}) === undefined; fromTaggedError({_tag:'InvalidInput', message:{obj:1}}) produces a SmithersError whose summary is a defined string (assert the coercion contract); toTaggedErrorPayload of a tagged instance with a numeric/object message asserts the String() coercion result explicitly.
- **[LOW] errorToJson + SmithersError(includeDocsUrl:false) interaction and the EngineError-cause-is-tagged path are untested** — `packages/errors/src/errorToJson.js:13-35` _(edge-case)_  
  - Untested: Two combinations are not asserted: (1) errorToJson of a SmithersError created with `{includeDocsUrl:false}` — does the persisted `message` then omit the docs URL while `summary`/`docsUrl` still carry it, and is that the intended durable shape? No test crosses these. (2) errorToJson copies error.cause raw (line 20/31) even when the cause is itself a tagged-error object; only the TOP-LEVEL error is normalized via fromTaggedError, nested tagged causes are left un-normalized in the `cause` field. I verified a nested tagged cause is emitted verbatim, and a self-referential tagged cause makes the subsequent JSON.stringify throw (ties into finding #1).  
  - Risk: The persisted error message is what humans and the UI read after a crash. If includeDocsUrl:false silently changes what gets durably written vs what summary holds, log/UI consistency drifts. Leaving nested tagged causes un-normalized means replayed/inspected errors have inconsistent shapes depending on nesting depth, and (per finding #1) a tagged cause that is circular is a concrete stringify crash.  
  - Test: Add: errorToJson(new SmithersError('INVALID_INPUT','Bad',undefined,{includeDocsUrl:false})) and assert message has no docs URL but docsUrl field still equals ERROR_REFERENCE_URL (lock the contract). Add a SmithersError whose cause is a tagged-error object and assert how errorToJson represents nested causes; pair with a stringify-safety assertion.

### gateway (5)

- **[MEDIUM] The requiredScope on each of the 27 RPC definitions is never asserted exactly — a wrong scope on a mutating/admin method would ship undetected** — `packages/gateway/src/rpc/index.ts:559-1058 (GATEWAY_RPC_DEFINITIONS); tests/rpc-contract.test.ts:150-162` _(security-trust-boundary)_  
  - Untested: The only assertion on per-definition requiredScope is `expect(GATEWAY_SCOPE_VALUES).toContain(definition.requiredScope)` — it merely checks the value is *some* valid scope. There is no test pinning each method to its CORRECT scope. submitApproval->approval:submit, hijackRun->run:admin, rewindRun->run:admin, cancelRun->run:write, cronCreate->cron:write, deleteTicket->ticket:write, streamDevTools->observability:read are never individually asserted at the definition level (only getRequiredScopeForGatewayMethod is spot-checked for a few legacy aliases, which resolves through the definition but is tested only for runs.create/streamDevTools/etc., not for the full map).  
  - Risk: This is the package's central trust boundary. If a refactor or typo set hijackRun.requiredScope to 'run:write' (or rewindRun to 'run:read'), an under-privileged token could perform operator takeover or a destructive VCS rewind, and every existing test would still pass because the scope is still a valid enum value. A scope downgrade on a mutating RPC is exactly the kind of silent privilege escalation a contract test exists to prevent.  
  - Test: Add a table-driven test asserting the exact expected requiredScope for ALL 27 methods, e.g. `const EXPECTED: Record<GatewayRpcMethod, GatewayScope> = { launchRun: 'run:write', hijackRun: 'run:admin', rewindRun: 'run:admin', submitApproval: 'approval:submit', cancelRun: 'run:write', deleteTicket: 'ticket:write', streamDevTools: 'observability:read', ... }` then `for (const d of GATEWAY_RPC_DEFINITIONS) expect(d.requiredScope).toBe(EXPECTED[d.method])` and assert `Object.keys(EXPECTED).length === GATEWAY_RPC_DEFINITIONS.length` so a newly added method without an expected scope fails the test.
- **[MEDIUM] Legacy 'read' grant -> account:read / memory:read / prompt:read / score:read mappings are unasserted** — `packages/gateway/src/auth/scopes.ts:67-80 (legacyAccessImplies 'read' case); tests/rpc-contract.test.ts:182-197,270-281` _(security-trust-boundary)_  
  - Untested: legacyAccessImplies('read', required) returns true for run:read, cron:read, account:read, memory:read, prompt:read, score:read, ticket:read, observability:read. Tests only assert the read->observability:read and read->ticket:read mappings. account:read, memory:read, prompt:read, score:read (which gate listAccounts — the secret-redaction surface — and memory/prompt/score reads) are never asserted (grep: 0 refs). Similarly legacyAccessImplies('execute') -> signal:submit is never asserted (no execute+signal test).  
  - Risk: listAccounts gates the account registry whose API keys are redaction-sensitive. If the legacy 'read' case dropped 'account:read' (or a refactor reordered the ||), a legacy 'read' token would lose access to listAccounts silently, or worse a mis-edit could grant a token that should not reach a read family. Because these are OR-chained string equalities, omission/typo is invisible without per-scope assertions. signal:submit-via-execute being untested means a legacy execute token's ability to send signals to a waiting run is unverified.  
  - Test: Assert each legacy mapping explicitly: `expect(hasGatewayScope(['read'],'account:read','listAccounts')).toBe(true)` and same for memory:read/prompt:read/score:read; `expect(hasGatewayScope(['execute'],'signal:submit','submitSignal')).toBe(true)`. Also assert a NEGATIVE: `expect(hasGatewayScope(['read'],'run:write','launchRun')).toBe(false)` to pin that 'read' never crosses into a write family.
- **[LOW] Four special-cased read methods (frames.list, frames.get, attempts.list, attempts.get) have zero assertions despite 100% line coverage** — `packages/gateway/src/rpc/index.ts:1086 (getRequiredScopeForGatewayMethod); tests/rpc-contract.test.ts:173` _(api-contract)_  
  - Untested: getRequiredScopeForGatewayMethod special-cases `runs.diff || frames.list || frames.get || attempts.list || attempts.get` to return 'run:read' on a single line. Only `getRequiredScopeForGatewayMethod('runs.diff')` is asserted, so the four time-travel/frame/attempt methods are never individually checked. grep confirms 0 references to frames.list/frames.get/attempts.list/attempts.get in the test file. Line coverage shows them 'covered' only because they sit on the same `||` expression the runs.diff assertion executes.  
  - Risk: These are the read-side methods for time-travel/frame inspection and attempt history. If someone mistyped 'frames.get' as 'frame.get' or dropped attempts.* from the chain, those methods would silently resolve to undefined scope, which hasGatewayScope treats as 'grant nothing' for name-grants and could either lock out a legitimate read or (via the broad legacy 'read' grant) behave inconsistently with the others. A scoped-auth check that returns undefined for a real read method is a contract regression no current test catches.  
  - Test: Extend the legacy-scope test to assert each: `expect(getRequiredScopeForGatewayMethod('frames.list')).toBe('run:read')` and likewise for frames.get, attempts.list, attempts.get. Add a guard test that iterates the documented special-method list and asserts none returns undefined.
- **[LOW] OpenAPI generator (generate-openapi.ts) has no unit test; only the CI --check gate guards it, and the gate can be defeated by re-running generate** — `packages/gateway/scripts/generate-openapi.ts (entire file); no test references it` _(regression-risk)_  
  - Untested: buildOpenApiDocument, buildPath, errorSchema/successSchema/requestFrameSchema, and the hand-rolled toYaml/scalarToYaml serializer have zero unit tests. The package test script runs only `bun test tests` (the contract test); the generator is validated solely by `check:openapi` (a string diff of the committed openapi.yaml). That gate only proves the file matches current code output — it does NOT prove the output is correct, and any drift is 'fixed' by regenerating, so a wrong-but-consistent doc passes. The custom YAML emitter (scalarToYaml regex quoting, empty-object/array handling, key escaping) is exactly the kind of code that silently emits invalid YAML for an edge-case string.  
  - Risk: openapi.yaml is the published external contract consumers code-generate against. A serializer bug (e.g. a scope description containing a colon/newline, or an error description with a YAML-special char) could emit malformed or semantically-wrong YAML that round-trips through check:openapi but breaks downstream codegen. There is also no test asserting that every GATEWAY_RPC_DEFINITION produces a /v1/rpc/<method> path or that each definition.errors code maps to the right httpStatus in the responses block.  
  - Test: Add tests/generate-openapi.test.ts that imports buildOpenApiDocument and asserts: (1) one path per definition with operationId === method and security scope === requiredScope; (2) every declared error code appears under its GATEWAY_RPC_ERRORS[code].httpStatus key; (3) round-trip toYaml output parses back (use a YAML parser or assert structural invariants) and that a scope description containing ':' or '\n' is correctly quoted. Optionally snapshot a single representative path.
- **[LOW] hasGatewayScope empty/whitespace-only grant filtering and empty grants array are unasserted** — `packages/gateway/src/auth/scopes.ts:103 (normalizeScope + .filter(Boolean)) and 98-127` _(edge-case)_  
  - Untested: hasGatewayScope normalizes each granted scope (trim) and filters out falsy entries before matching. No test exercises an empty grants array (`hasGatewayScope([], 'run:read', 'getRun')` should be false), a whitespace-only token (`['   ']` should NOT grant anything and must not be mis-trimmed into a match), or a token list containing '' alongside a real scope. The trim is only indirectly hit via `[' * ']`.  
  - Risk: A subtle bug where an empty or whitespace token slipped through (or where '*' detection ran before trim) could either deny a legitimately-scoped caller or, in the inverse failure mode, treat a blank grant as a match. Auth predicates should have explicit deny-by-default assertions for the empty/blank input boundary.  
  - Test: Assert `hasGatewayScope([], 'run:read', 'getRun') === false`, `hasGatewayScope(['   '], 'run:read', 'getRun') === false`, `hasGatewayScope(['', 'run:read'], 'run:read', 'getRun') === true` (blank ignored, real scope honored), and `hasGatewayScope([' run:read '], 'run:read', 'getRun') === true` (trim of a real scope still matches).

### gateway-client (7)

- **[MEDIUM] createGatewayCollection live-stream error + reconnect path is never asserted (test helper exists but is unused)** — `packages/gateway-client/src/sync/createGatewayCollection.ts:265-301 (openStreamLoop catch + reconnect), :234-263 (applyFrame/enqueueFrame error)` _(error-path)_  
  - Untested: The stream loop's catch block — classify thrown error as auth (onAuthError, return) vs transient (onError, backoff+reconnect), reset of `attempt` to 0 on a frame after a reconnect, and `shouldReconnect` gating (threw || reconnectOnGracefulEnd || default streamDevTools) — has no test. The test file's `controllableStreamTransport` even defines a `fail(cause)` helper to inject a stream error, but NO test ever calls `.fail(...)`. Errors thrown inside applyFrame/frameToWrites that route through `enqueueFrame`'s `.catch(handleError)` are also unasserted.  
  - Risk: This is the gateway's live-streaming durability guarantee at the collection layer: if a streamRunEvents/streamDevTools error mis-classifies (e.g. a transient drop treated as auth and silently returning, or an auth error retried in a hot loop), a live run UI either freezes on stale state forever or busy-reconnects against a rejecting gateway. A regression here ships invisibly because every happy-path test still passes. The auth-vs-transient branch is precisely the kind of subtle logic that breaks on refactor.  
  - Test: Using controllableStreamTransport: (1) call stream.fail(new Error('boom')) mid-stream and assert the collection reconnects (opens.length increments after backoff) and resumes with afterSeq = last seen frame.seq; (2) stream.fail an error whose message matches /^UNAUTHORIZED/ and assert onAuthError fires exactly once and the loop does NOT reconnect (opens.length stays 1); (3) inject a frameToWrites that throws and assert onError is called and the collection stays ready (does not crash the sync chain); (4) assert attempt resets to 0 (next backoff is base, not escalated) after a successful frame following a reconnect.
- **[MEDIUM] streamExtension error-frame (ext.stream.error) and post-drop tail loss are untested** — `packages/gateway-client/src/SmithersGatewayClient.ts:545-585 (streamExtension)` _(error-path)_  
  - Untested: The extension stream test asserts only the happy path (initial + one filtered live frame, unrelated streamId dropped). Untested: (a) a GATEWAY_EXTENSION_STREAM_ERROR frame whose streamId matches must THROW a GatewayRpcError carrying the frame's code/message; (b) an error frame for a DIFFERENT streamId must be ignored (not throw); (c) a clean WS close mid-stream ends the iterator gracefully with the connection closed (finally runs); (d) the documented 'resume after a drop is the caller's responsibility' contract — there is no test proving a re-subscribe with a fresh afterSeq actually resumes, nor that a drop without afterSeq silently loses the tail.  
  - Risk: Extensions stream live data (logs tails, etc.) over the same gateway WS. If the error-frame branch regresses, a server-side extension failure would be silently swallowed and the consumer would hang on a dead stream instead of surfacing the error; or worse, an error frame intended for another subscriber's streamId could throw into the wrong iterator (cross-subscriber leakage). The streamId filter on the error path (line 573) is a trust boundary that is asserted only on the success path, not the error path.  
  - Test: Extend the FakeWebSocket extension test: emit a GATEWAY_EXTENSION_STREAM_ERROR frame with the matching streamId and assert the async iterator rejects with a GatewayRpcError of that code/message; emit an error frame with a non-matching streamId and assert it is ignored (iterator keeps yielding). Add a test that closes the socket after the initial frame and asserts the iterator completes (done:true) and connection.close() ran.
- **[MEDIUM] createElectricCollection onError and load-failure markReady fallback are never exercised** — `packages/gateway-client/src/sync/createElectricCollection.ts:223-226 (handleError), :348-355 (loadShapeStream catch + ready fallback), :347 (subscribe onError)` _(error-path)_  
  - Untested: No test drives an error: (a) the ShapeStream subscribe's onError callback (handleError -> config.onError) is never invoked in tests; (b) the catch around loadShapeStream() — when the dynamic import or ShapeStream construction throws — and its CRITICAL fallback that flips `ready=true; markReady()` so the collection is never stranded in a non-ready state, is untested; (c) handleError's `if (signal.aborted) return` guard (no onError after unmount) is unverified.  
  - Risk: The cloud-Electric path backs live surfaces (e.g. /memory). If loadShapeStream throws (missing optional dep, bad shapeUrl) and the markReady fallback regresses, collection.preload() never resolves and the UI hangs on a loading spinner forever — a silent, total failure of that surface. The aborted-signal guard prevents onError firing after unmount (a React state-update-after-unmount / ghost-state hazard); without a test, a refactor could fire onError post-abort.  
  - Test: Make the fake ShapeStream's subscribe call onError(new Error('shape boom')) and assert config.onError receives it. Add a test where loadShapeStream rejects (mock.module to throw) and assert collection.status becomes 'ready' (preload resolves) and onError fired — proving the never-strand fallback. Add a test that aborts the collection (unmount) then pushes an error and asserts onError is NOT called.
- **[MEDIUM] SmithersGatewayConnection: stale/duplicate response-id handling and frame ordering after error are unasserted** — `packages/gateway-client/src/SmithersGatewayConnection.ts:146-172 (handleMessage), :104-125 (events drains queue after close)` _(concurrency-race)_  
  - Untested: Untested branches: (a) a response frame for an id NOT in `pending` (already-resolved, or a stale/duplicate res from a flapping reconnect) is silently ignored — line 148 `if (!pending) return` — with no test, so a regression could re-reject an already-settled promise or throw; (b) a malformed `{type:'res', id}` frame whose id matches a pending request rejects that specific request with invalidFrameError (lines 159-165) — distinct from the unmatched-id case, untested; (c) events() must DRAIN buffered event frames after the socket closes (`while (!this.closed || this.queue.length > 0)`, line 112) so seq-ordered frames queued before close are not lost — only the close-and-error cases are tested, not 'frames queued then close, all still delivered in order'.  
  - Risk: Event-log ordering and not-losing-already-buffered-frames is core to the live-streaming guarantee. On a reconnect a server may resend a response for a request the client already abandoned; mishandling a stale/duplicate id could crash the connection or double-settle. The drain-after-close behavior is what guarantees no live frame between the last shift and the close is dropped — a subtle off-by-one in the loop condition would silently lose the final events of a run.  
  - Test: (1) Receive a res frame for an unknown id and assert no throw and pending stays empty. (2) requestRaw, then receive a res with matching id but missing `ok`/`error` shape, assert that request rejects with INVALID_GATEWAY_RESPONSE. (3) Push two valid event frames (seq 1, 2) into the connection, then dispatch the socket 'close' event, then start iterating events() and assert BOTH frames are yielded in order before the iterator completes.
- **[LOW] getKeyFromRaw-omitted delete fallback (key via getKey(mapRow(raw))) is documented but untested** — `packages/gateway-client/src/sync/createElectricCollection.ts:252-258 (the def.getKeyFromRaw ? ... : fallback IIFE)` _(edge-case)_  
  - Untested: Every Electric delete test uses the memoryFacts def, which DEFINES getKeyFromRaw, so only that branch runs. The fallback branch — when a def omits getKeyFromRaw and the delete key must be derived via getKey(mapRow(raw)) — is never executed, including the documented failure mode where mapRow returns undefined on a PK-only value and the delete is therefore (correctly) skipped.  
  - Risk: The whole point of getKeyFromRaw is that the fallback SILENTLY DROPS deletes for any collection def whose mapRow requires non-PK columns (the comment at lines 64-69 calls this out as the row-stranded-forever bug). A new collection def added without getKeyFromRaw would reintroduce exactly that durability bug — live deletes never applying, rows stranded — and no test guards against shipping such a def. The fallback's own correctness (key derived when mapRow happens to succeed on PK-only input) is also unverified.  
  - Test: Construct an ElectricCollectionDef with getKeyFromRaw omitted and a mapRow that (a) succeeds on PK-only input — assert a PK-only delete IS applied via the fallback; and (b) a second def whose mapRow returns undefined on PK-only input — seed a row, push a PK-only delete, and assert the row is (per the documented limitation) NOT removed, codifying the trap so a future def author is forced to add getKeyFromRaw.
- **[LOW] createGatewayCollection maxRows eviction interacting with live deletes and the insert-onto-existing collapse is only happy-path tested** — `packages/gateway-client/src/sync/createGatewayCollection.ts:150-204 (applyWrites: maxRows overflow + insert->update collapse + delete-of-absent skip)` _(edge-case)_  
  - Untested: The maxRows test only pushes monotonically increasing inserts. Untested: (a) maxRows eviction when the batch also contains a DELETE of a key already counted (the `deleted`/`liveKeys` bookkeeping at 156-191 that must not double-delete and must not evict a key already deleted); (b) an `insert` whose key already exists collapsing to an `update` only when the value differs (lines 172-177); (c) a `delete` for a key NOT in the collection being skipped (line 159) so no phantom delete write is emitted; (d) `upsert` mapping to insert-vs-update based on collection.has (line 166-168).  
  - Risk: maxRows bounds memory under a long-lived event stream (the n5-subscribers-bounded-memory concern at the unit level). If overflow eviction picks the wrong keys (keySort ordering) or collides with an in-batch delete, the bounded-event-log collection could either grow unbounded (leak) or evict the newest events (lose the run's tail). The insert->update collapse prevents spurious re-render churn; a regression silently degrades UI performance and could resurrect stale values.  
  - Test: Push a batch via frameToWrites that mixes inserts pushing the count over maxRows AND a delete of an existing key; assert the surviving keys are exactly the highest-sorted maxRows and no key is written twice. Separately, insert a key, then push another insert with the same key but different value and assert it becomes one update (collection value changes); push an identical insert and assert no write occurs (collection.get unchanged reference / no churn).
- **[LOW] Electric reconcile loses a since-deleted key if must-refetch arrives but the fresh snapshot never closes (no snapshot-end / up-to-date)** — `packages/gateway-client/src/sync/createElectricCollection.ts:327-345 (must-refetch sets inSnapshotPhase=true; reconcile only on snapshot-end/up-to-date)` _(edge-case)_  
  - Untested: must-refetch re-entering the snapshot phase is tested ONLY when followed by a clean snapshot-end. Untested: must-refetch followed by buffered change messages that are then interrupted by a SECOND must-refetch (server rotates twice) — pending is cleared and the phase stays snapshot, but the cache-hydrated stale keys are not reconciled until a terminator finally arrives. Also untested: the collection's behavior while stuck mid-resnapshot (rows remain stale but present) is unspecified by any assertion.  
  - Risk: Shape rotation under churn (a busy Electric server) can emit back-to-back must-refetch controls. If the buffering/clear logic regresses (e.g. dropping inSnapshotPhase=true on the second must-refetch), the eventual reconcile would NOT run and a row deleted in Postgres stays visible — the exact persistence/stale-cache bug the reconcile was built to fix, but only under double-rotation.  
  - Test: Seed two rows via an initial snapshot-end. Push [must-refetch], then a partial insert (no terminator), then [must-refetch] again, then a fresh snapshot omitting one seed terminated by snapshot-end. Assert the omitted seed is reconciled out and the surviving row remains — proving double-rotation still reconciles exactly once at the final boundary.

### gateway-react (7)

- **[HIGH] reset() does not assert stream/query collection teardown or persistence clearAll (cross-account leak guard)** — `packages/gateway-react/src/sync/createGatewayCollections.ts:532-544 (reset)` _(security-trust-boundary)_  
  - Untested: tests/sync/sync.test.ts:902 calls registry.reset() and only asserts connection().status === 'idle'. It never asserts that (a) the per-run streamed collections / stream handles / query handles are actually cleaned up (cleanup() called, maps cleared so a later subscribe re-opens fresh), nor (b) that persistenceStore.clearAll() is invoked. The reset path explicitly exists to drop the prior account's cached rows on sign-out / remote-mode swap (line 540-542 comment), but no test wires a persistence store and asserts clearAll fired.  
  - Risk: This is a trust-boundary: if reset() stops calling persistenceStore.clearAll() (or clears the in-memory maps but leaves the SQLite/OPFS cache), the NEXT session hydrates the previous account's runs/approvals/memory facts from disk on first render — a cross-account data leak that the no-flash hydrate path makes invisible until someone notices another user's runs. Also, if stream handles are not cleaned up, an old account's WS keeps streaming after sign-out.  
  - Test: Build createGatewayCollections with a fake persistence store (spy on clearAll). Open a run stream + a list collection, then call reset(): assert clearAll() was called exactly once, and that re-requesting the same run stream returns a NEW handle / re-opens the upstream (opens.length grows), proving the prior handle was torn down. Assert connection back to idle.
- **[MEDIUM] Connection observer offline streak + reconnect recovery (markOffline / reconnectingSince) is never asserted** — `packages/gateway-react/src/sync/createGatewayCollections.ts:139-147 (createConnectionObserver), src/sync/useGatewayConnectionStatus.ts` _(error-path)_  
  - Untested: The only connection-status test (tests/sync/sync.test.ts:867) covers idle->online and online->unauthorized. The offline path is completely untested: a non-auth transport rejection should call markOffline() and set reconnectingSince to the first-failure epoch; a subsequent success should call markOnline() and clear reconnectingSince. Also untested: markConnecting() only flips from `idle` (line 140), so after an offline streak a recovery jumps offline->online WITHOUT ever passing through `connecting`, and reconnectingSince must reset to undefined on recovery.  
  - Risk: useGatewayConnectionStatus is what the UI uses to show 'reconnecting since X' banners. A regression where markOffline forgets to seed reconnectingSince, or where a recovery fails to clear it (so the banner sticks forever), or where the dedupe in set() (line 129, which compares both status AND reconnectingSince) drops a legitimate transition, would ship undetected. This is the live-streaming/gateway reconnect guarantee surfaced to the operator.  
  - Test: Drive useGatewayConnectionStatus with a transport whose rpc rejects with a non-auth Error (e.g. 'ECONNRESET'): assert status becomes 'offline' and reconnectingSince is a number. Then make the next rpc resolve and assert status flips to 'online' and reconnectingSince is undefined. Assert the recovery did NOT emit a 'connecting' state. Separately assert two consecutive offline failures keep the SAME reconnectingSince (streak not reset on each failure).
- **[MEDIUM] Subscription teardown on unmount is never asserted (upstream abort / no leaked open)** — `packages/gateway-react/src/sync/useSyncSubscription.ts:46-51, src/sync/createGatewayCollections.ts:98 (RUN_GC_TIME=0)` _(concurrency-race)_  
  - Untested: Every test calls harness.unmount() for cleanup but NONE asserts the consequence: that unmounting the last subscriber aborts the upstream stream (the fake transport records options.signal but no test checks signal.aborted after unmount), and that re-mounting opens exactly one fresh stream rather than leaking. The gcTime:0 / 'last observer aborts the upstream' contract documented at useSyncSubscription.ts:14-16 is asserted nowhere at the unit level. N-subscribers-shared-one-socket multiplexing (the per-key collection id) is also only implied, never asserted (mount two probes on the same runId, assert opens.length stays 1).  
  - Risk: This is the per-unit basis for the e2e 'ghost-state-on-unmount' and 'n5-subscribers-bounded-memory' guarantees. A regression where gcTime drifts off 0, or where useLiveQuery's deps array stops keying on `enabled`, would leave WS connections (and unbounded frame buffers) alive after navigation — exactly the bounded-memory / no-ghost-state durability promise. The e2e covers the integrated case but cannot localize a regression to this hook.  
  - Test: Mount useGatewayRunStream('r1'); capture the transport's recorded signal. Unmount and assert signal.aborted === true (after gcTime:0 teardown settles). Mount two probes on the same runId and assert opens.length === 1 (shared socket). Unmount one and assert NOT aborted; unmount the second and assert aborted. Mount-unmount-mount and assert exactly two distinct opens, no leak.
- **[MEDIUM] useGatewayRunEvents afterSeq resume cursor is untested** — `packages/gateway-react/src/useGatewayRunEvents.ts:44-48` _(missing-e2e-scenario)_  
  - Untested: The afterSeq option filters `sorted.filter(row => row.seq > afterSeq)` — the resume cursor a consumer uses after a reconnect/replay to only render events past what it already showed. The single useGatewayRunEvents test (tests/sync/sync.test.ts:746) passes no afterSeq, so the boundary (strictly greater-than: seq===afterSeq excluded; afterSeq applied BEFORE the maxEvents cap; lastHeartbeat also filtered by afterSeq) is never asserted.  
  - Risk: afterSeq is the event-log ordering/seq resume primitive surfaced to React. An off-by-one (>= vs >) would re-render an already-shown event (duplicate) or, combined with the maxEvents cap, silently drop a window of events on resume after a WS drop+reconnect. That maps directly to the live-streaming 'reconnect-afterseq' guarantee but at the hook level it is unverified.  
  - Test: Push events seq 1..10 (plus a heartbeat at seq 5) into the runEvents collection, render useGatewayRunEvents('r', { afterSeq: 5 }), and assert events are exactly seq 6..10 (seq 5 excluded), and lastHeartbeat is undefined if the only heartbeat was at seq<=afterSeq. Then assert afterSeq is applied before the cap by setting maxEvents:3 and afterSeq:5 -> events === [8,9,10].
- **[LOW] Error path of useGatewayRunEvents / useGatewayRunTree (live.isError -> error + streaming:false) untested** — `packages/gateway-react/src/useGatewayRunEvents.ts:58-59, src/sync/useGatewayRunTree.ts:48-54` _(error-path)_  
  - Untested: Both hooks surface `error` only when the underlying live query errors (live.isError). No test drives a stream/devtools source into an error state, so the branches that build `new Error('Run event stream failed.')` / `new Error('Failed to load run tree.')` and set streaming:false (RunEvents) are never exercised. RunTree's isLoading computation (Boolean(runId) && !live.isReady && nodes.length===0) edge — a runId present but no nodes yet vs. error — is also unverified.  
  - Risk: If the WS errors mid-run, the UI relies on these hooks reporting error + streaming=false to show a 'stream failed, retrying' state instead of a frozen-but-apparently-live tree. A regression where isError stops mapping to error (e.g. tanstack renames the flag) would make a dead stream look healthy — the operator believes a long-running agent is still streaming when it is silently disconnected.  
  - Test: Make the fake transport's stream throw after one frame for the runEvents/devtools scope; render the hook and assert error is a defined Error and (for RunEvents) streaming===false. Also assert the previously-received frames remain rendered (error does not blank the buffer).
- **[LOW] syncSource:'electric' selection + silent fallback-to-gateway on missing config is untested** — `packages/gateway-react/src/sync/createGatewayCollections.ts:209-210, 489-498 (memoryFacts electric branch)` _(edge-case)_  
  - Untested: createGatewayCollections supports syncSource:'electric' to back memoryFacts from an ElectricSQL shape, with a deliberate degrade: if syncSource is 'electric' but no `electric` config is supplied, electricConfig is null and memoryFacts silently falls back to the gateway collection (lines 209-210, 490). Neither the electric-engaged path (electricCollection built, no INVALIDATE pulse wired) nor the misconfigured-fallback path is tested. useGatewayMemoryFacts is only tested over the gateway source.  
  - Risk: Cloud mode is a real deployment target. If the fallback condition regresses (e.g. requires only syncSource without checking electric presence), a cloud build with a missing/typo'd shape URL would throw at render instead of degrading to the working gateway path, taking down the whole UI. Conversely, if electricCollection accidentally got an INVALIDATE pulse wired, invalidate() would fire spurious RPCs against data Electric already pushes.  
  - Test: Call createGatewayCollections({ client, syncSource:'electric' }) with NO electric config and assert memoryFacts() returns the gateway-backed collection (e.g. invalidate(memoryFacts) triggers a listMemoryFacts RPC). Then with a valid electric config, assert memoryFacts() builds the electric collection and that invalidate() is a no-op for it (no RPC fired).
- **[LOW] withPersistence write-through delete + cold-seed snapshot path untested through the registry** — `packages/gateway-react/src/sync/persistence/withPersistence.ts:94-118` _(crash-safety-durability)_  
  - Untested: persistence.test.ts covers cold-seed (write-through inserts), warm hydrate, and corrupt-row handling. It does NOT cover: (a) the write-through DELETE branch (line 97-98) — when a live reconcile removes a row, store.delete must fire so the next reload does not resurrect a deleted run/approval; (b) the cold-cache seed-from-snapshot block (lines 110-117) when hydrated===false; (c) the subscribeChanges write-through surviving a thrown store.put without breaking the live UI (the catch at line 103). The store CRUD is unit-tested in isolation but the change-stream wiring that keeps the cache consistent on deletes is not.  
  - Risk: The no-flash reload guarantee is only safe if the cache stays consistent. If the delete write-through regresses, a run that was cancelled/cleaned up server-side reappears as a ghost on the next reload (stale durable state shown as live). This is the persistence side of the durability story — replay/reload must reflect the last real snapshot, not a half-stale one.  
  - Test: Over a real SQLite store + real collection (as in persistence.test.ts), deliver a live snapshot of [r1,r2], let it write through, then deliver [r1] (r2 removed) and assert store.read(collectionId) no longer contains r2. Separately, wrap a store whose put() throws and assert the live collection still updates (the UI is not broken by a cache-write failure).

### graph (6)

- **[MEDIUM] defaultIteration resume/replay fallback in getRalphIteration is never tested** — `packages/graph/src/extract.js:168-178 (and packages/graph/src/dom/extract.js:117-127)` _(crash-safety-durability)_  
  - Untested: getRalphIteration falls back to opts.defaultIteration (not 0) when a Ralph id is absent from the ralphIterations map. No test in packages/graph passes defaultIteration at all (grep of tests/ returns nothing). The Map branch (map.get(id) ?? fallback) and the Record branch both depend on this fallback, and neither is exercised for the not-in-map case.  
  - Risk: defaultIteration is the resume/replay path: engine.js (lines 5630-5638) computes it from the max persisted node/ralph iteration on opts.resume and threads it via render opts (5712-5713). On resume after a crash inside a Ralph loop, every task's nodeId is suffixed by its ralph iteration (the @@id=N loop scope). If this fallback regressed (e.g. fell back to 0, or coerced a stored 0 incorrectly), a resumed run would re-derive nodeIds at iteration 0, re-execute already-completed loop iterations and/or write durable rows under wrong keys — silently corrupting checkpoint state with no test catching it.  
  - Test: In extract.test.jsx (and dom-extract.test.jsx): extract a workflow with a <Ralph id="loop"> containing a task, calling extractGraph(root, { defaultIteration: 7 }) with NO ralphIterations entry for 'loop'; assert tasks[0].iteration === 7 and ralphId === 'loop'. Add a second case where ralphIterations has a DIFFERENT ralph id so the target ralph still falls through to defaultIteration. Add a Map case where the ralph IS present with value 0 and defaultIteration=5, asserting iteration stays 0 (Map.get(id) ?? fallback must honor a stored 0).
- **[MEDIUM] Transitive fork cycle through dependsOn (not just fork->fork) is not asserted** — `packages/graph/src/validateForkSources.js:61-95,107-108` _(edge-case)_  
  - Untested: canReach() unions dependsOn edges AND fork edges for cycle detection, but fork-extract.test.jsx only covers direct self-fork and fork<->fork. There is no test for a cycle closed through a dependsOn chain (e.g. A dependsOn B, B forks A) or a multi-hop chain (A dependsOn B, B dependsOn C, C forks A). The DFS multi-hop traversal and the dependsOn/fork edge union are reachable by the simple case (so line coverage shows 100%) but the mixed-edge cycle behavior is never verified.  
  - Risk: A fork edge that closes a cycle via dependsOn can deadlock the scheduler at runtime (fork waits for a snapshot of a task that transitively depends on the forker). The whole point of validateForkSources is to fail fast at build time instead. A regression that only walked fork edges (not dependsOn) in canReach would let these cycles through, and 100% line coverage would hide it because the existing fork->fork test still passes.  
  - Test: Case 1: agentTask('a', { dependsOn: ['b'] }), agentTask('b', { fork: 'a' }) -> expect TASK_FORK_CYCLE. Case 2 (multi-hop): a dependsOn b, b dependsOn c, agentTask('c', { fork: 'a' }) -> expect TASK_FORK_CYCLE. Case 3 (no cycle control): a dependsOn b, agentTask('c', { fork: 'a' }) with c independent -> expect no throw.
- **[LOW] fork referencing a source inside a Ralph loop scope is not tested (loop-scope stripping in validateForkSources)** — `packages/graph/src/validateForkSources.js:11-14,42,99-104` _(edge-case)_  
  - Untested: validateForkSources strips the @@loop-scope suffix via logicalId() so fork/dependsOn (authored as logical ids) resolve against the scoped nodeIds present in the graph. fork-extract.test.jsx only tests flat (unscoped) ids; there is no test where the fork source is a task inside a <Ralph> (nodeId becomes 'src@@loop=N') and a forking agent task references logical id 'src'.  
  - Risk: Fork seeds an agent session from the latest completed snapshot of a task id — a core time-travel/lineage primitive. The logicalId() stripping is the only thing letting a fork target a task that lives inside a loop. If stripping broke (e.g. indexOf('@@') off-by-one, or presentLogicalIds built from scoped ids without stripping), every fork pointing into a loop would spuriously throw TASK_FORK_SOURCE_NOT_FOUND and deadlock the workflow at graph-build time — or, worse, a non-stripped match could let an invalid fork through. None of this is asserted.  
  - Test: Build root = <Ralph id="loop"> containing agentTask('src'), and a sibling agentTask('user', { fork: 'src' }); render with ralphIterations {loop: 2}. Assert no throw and that the forking task's forkSource === 'src' even though the source nodeId is 'src@@loop=2'. Add a negative case: fork: 'does-not-exist' inside the same loop throws TASK_FORK_SOURCE_NOT_FOUND.
- **[LOW] dom/extract.js silently drops fork/forkSource, aspects, groundTruth, and context — divergence from the production extractor is untested** — `packages/graph/src/dom/extract.js:700-864 (smithers:task branch)` _(regression-risk)_  
  - Untested: The two extractors diverge: root extract.js (extractGraph, the production path resolved by react-reconciler) extracts forkSource, calls validateForkSources, and extracts aspects/groundTruth/context; dom/extract.js (extractFromHost, exported as the public @smithers-orchestrator/graph/dom/extract subpath, used by engine and react-reconciler roundtrip tests) extracts NONE of these (grep for fork/aspects/groundTruth/context in dom/extract.js is empty). No test asserts this divergence or guards against the legacy path being wired into a code path that needs fork validation.  
  - Risk: dom/extract.js is a published subpath import. Any consumer (or a future migration step per the TODO at its top) that routes extraction through it gets a graph with no fork lineage, no fork-cycle validation, and no aspect budgets — meaning a <Task fork=...> would be silently treated as a plain task (wrong session seeding for time-travel) and token/latency budgets would be unenforced. Because the production path is the other file, this gap can rot undetected until something switches extractors.  
  - Test: Add an extractor-parity test (parameterized over both extractFromHost-from-dom and extractGraph) feeding a workflow with <Task fork> + __aspects + groundTruth + context, asserting either identical descriptor output OR an explicit, documented difference. At minimum, add a test that documents dom/extract.js does not populate forkSource/aspects so an accidental rewire is caught.
- **[LOW] canonicalizeXml is not tested for the replay-frame determinism property (insertion-order-independent hash) it underpins** — `packages/graph/src/utils/xml.js:7-35` _(crash-safety-durability)_  
  - Untested: xml-utils.test.js checks that props are sorted and that a single node canonicalizes identically twice, but never asserts the cross-object determinism guarantee: two element nodes built with the SAME props in DIFFERENT insertion orders (and structurally-identical children) produce byte-identical canonicalizeXml output. The element-level `element.props ?? {}` undefined-props branch is also not exercised.  
  - Risk: engine.js (line 5191-5192) hashes canonicalizeXml(graph.xml) with sha256 to fingerprint each durable driver frame for time-travel/replay; equal hashes mean 'same frame' (dedup) and unequal means a new persisted frame. If canonicalization were not order-independent (e.g. a future change removed sortProps or stopped recursing children), two renders of the same workflow would hash differently, exploding the frame log and breaking frame-scrub/replay equivalence — or conversely a too-lossy canonicalization could collapse distinct frames. The hashing-stability contract is never asserted at the unit level.  
  - Test: Construct nodeA and nodeB as the same tree but with props inserted in reversed key order and children arrays containing structurally-equal-but-separately-allocated nodes; assert canonicalizeXml(nodeA) === canonicalizeXml(nodeB). Add a case with element.props === undefined asserting it serializes as {} . Add a negative case asserting a changed child text yields a different string (and thus a different hash).
- **[LOW] dom/extract.js Subflow/Sandbox computeFn runtime error paths are entirely uncovered** — `packages/graph/src/dom/extract.js:349-361 (subflow computeFn), 446-491 (sandbox computeFn runtime/workflow validation)` _(error-path)_  
  - Untested: Coverage shows lines 349-359 and 458-489 of dom/extract.js uncovered. The subflow computeFn throwing WORKFLOW_EXECUTION_FAILED when a child run is not 'finished', and the sandbox computeFn's unsupported-runtime IIFE throw plus the 'missing workflow definition' throw, are not invoked by any dom test (the equivalent sandbox 'missing workflow definition' path IS tested via the unknown-runtime case at dom-extract.test.jsx:345, but the subflow failure path and the unsupported-runtime branch are not).  
  - Risk: These computeFns are how the legacy extractor wires child-workflow and sandbox execution; their guard clauses (status !== finished -> throw, unsupported runtime -> throw) are the only place malformed subflow/sandbox declarations fail loudly rather than executing with undefined inputs. Since this is the legacy path with no production importers, severity is low, but if it is ever re-activated these throws would be unproven.  
  - Test: In dom-extract.test.jsx: build a <Subflow> descriptor and stub loadRuntimeModule so executeChildWorkflow returns { status: 'failed' }; await task.computeFn() and expect rejection with code WORKFLOW_EXECUTION_FAILED. Build a <Sandbox> with runtime: 'not-a-runtime' and a valid workflow def; await computeFn() and expect 'Unsupported sandbox runtime'.

### memory (7)

- **[MEDIUM] saveMessage is not idempotent — deterministic re-save on resume/replay crashes on UNIQUE PK** — `packages/memory/src/store/MemoryStoreLive.js:235-248 (saveMessageEffect)` _(crash-safety-durability)_  
  - Untested: saveMessage does a bare db.insert(...).values(...) with no onConflict handling. No test saves a message twice with the same id. Probe confirmed the second save throws `SmithersError: memory saveMessage: UNIQUE constraint failed: _smithers_memory_messages.id`, leaving count=1. setFact uses onConflictDoUpdate, but saveMessage does not, and that asymmetry is never asserted.  
  - Risk: On crash-resume, deterministic replay, or fork/restore, an agent that re-emits the same message (message ids are frequently derived deterministically, e.g. from run/node/seq) will re-call saveMessage with an id that already exists. That raises a DB_WRITE_FAILED instead of being a safe no-op, turning a recoverable resume into a hard failure — a direct violation of the 'resume correctly from last checkpoint / replay must not re-trigger side effects' guarantee. A test would lock in whether the contract is 'upsert' or 'throw'; today neither is pinned.  
  - Test: saveMessage twice with identical id (different content). Assert the defined contract: either it upserts (count stays 1, content updated — replay-safe) or it surfaces a typed SmithersError with a stable code that callers can treat as 'already saved'. Same for the saveMessageEffect path. If upsert is intended, this test fails today and exposes the bug.
- **[MEDIUM] Summarizer delete-then-save is non-transactional — data-loss window if summary write fails** — `packages/memory/src/Summarizer.js:72-79 (processEffect)` _(crash-safety-durability)_  
  - Untested: processEffect calls deleteMessagesEffect(oldMessages) and THEN saveMessageEffect(summary) as two independent writeEffect operations inside the Effect.gen — not wrapped in a db.transaction (unlike deleteThread, which is). No test injects a failure between the delete and the summary save, and no test crashes the process between them.  
  - Risk: If the summary insert fails (DB_WRITE_FAILED, UNIQUE collision on the summary id, or process/sandbox kill) after the old messages are already deleted, those messages are permanently lost with no summary to replace them — irreversible memory corruption. This is precisely the durability invariant deleteThread was hardened against (see its transaction comment), yet the Summarizer reintroduces the same hazard untested.  
  - Test: Wrap the real store so saveMessage throws once after deleteMessages succeeds (or close the sqlite handle between the two). Run Summarizer.process and assert that EITHER the old messages are still present (operation atomic / rolled back) OR a recoverable error is raised that preserves the originals — never the state where old messages are gone and no summary exists.
- **[LOW] Summarizer wraps agent.run failure as raw UnknownException, not a typed SmithersError** — `packages/memory/src/Summarizer.js:70 (Effect.tryPromise(() => agent.run(prompt)))` _(error-path)_  
  - Untested: agent.run is invoked via Effect.tryPromise with NO catch mapper, so a failing/throwing/timeout agent surfaces as Effect's UnknownException rather than a SmithersError (confirmed by probe: '(FiberFailure) UnknownException'). Every test uses a happy mock agent that returns {text:...}; no test exercises an agent that throws, rejects, or hangs.  
  - Risk: All other failures in this package are normalized to SmithersError with codes (DB_QUERY_FAILED/DB_WRITE_FAILED) so the engine/observability can classify and retry them. An un-normalized UnknownException from the LLM call breaks that contract: schedulers/retry policies that match on SmithersError codes won't recognize it, and the error message leaks internal Effect framing. A regression that swallows or mis-types LLM-failure handling would ship undetected.  
  - Test: Summarizer({ run: async () => { throw new Error('LLM down') } }).process(store) over a thread with >2 messages. Assert it rejects with a typed SmithersError (stable code), and assert no messages were deleted (combines with the atomicity finding). Repeat with an agent that returns a non-string/non-{text}/non-{output} value to cover extractSummary's JSON.stringify fallback branch (uncovered line 38).
- **[LOW] No thread referential integrity — orphan messages persist and deleteThread can't clean them** — `packages/memory/src/store/MemoryStoreLive.js:235-248 (saveMessage) / 216-229 (deleteThread)` _(edge-case)_  
  - Untested: The messages table has no FK to threads, and saveMessage never checks the thread exists. Probe confirmed saveMessage({threadId:'no-such-thread'}) silently succeeds and the orphan is listable. deleteThread only deletes messages matching that threadId, so orphans are unreachable garbage. No test covers saving to a missing thread or asserts whether it should be rejected.  
  - Risk: After a thread is deleted (or never created) due to a partial write/replay, message saves keyed to that thread accumulate as invisible, unbounded, ungarbage-collectable rows — a slow memory/storage leak with no GC path (TtlGarbageCollector only handles facts). It also means message counts/history can silently diverge from thread state. The intended contract (reject vs. allow) is undefined and unasserted.  
  - Test: saveMessage to a threadId that was never created; assert the defined behavior (reject with SmithersError, or document+test the orphan and ensure a cleanup path exists). Also: create thread, save messages, deleteThread, then re-save a message with the old threadId — assert it does not silently resurrect an orphaned thread's history.
- **[LOW] Store-level listThreads and deleteMessages are exported but never directly exercised** — `packages/memory/src/store/MemoryStoreLive.js:199-211 (listThreadsEffect), 290-297 (deleteMessagesEffect)` _(happy-path-only)_  
  - Untested: grep of tests/ shows zero direct references to store.listThreads / store.deleteMessages / listThreadsEffect. They are only reached transitively through processors. deleteMessages' empty-array short-circuit (returns Effect.succeed(0) without a DB call) and its changes ?? rowsAffected ?? 0 return-count fallback are not asserted at the store level; listThreads ordering (createdAtMs) and multi-namespace aggregation are not asserted.  
  - Risk: These are part of the public MemoryStore contract relied on by Summarizer/TokenLimiter. The deleteMessages([]) short-circuit specifically: a regression that drops it would issue a `DELETE ... IN ()` (malformed/whole-table delete on some drivers). Untested, that could silently wipe a thread's messages. The rowsAffected fallback is only tested for deleteExpiredFacts, not deleteMessages, so a driver returning rowsAffected (not changes) would make TokenLimiter's 'deleted N' accounting wrong undetected.  
  - Test: Direct store.deleteMessages(threadId, []) → assert returns 0 and leaves all messages intact (no DB mutation). store.deleteMessages with a mix of existing and non-existing ids → assert return count equals only the rows actually removed. store.deleteMessages on a driver stub returning {rowsAffected:n} → assert n. listThreads across two namespaces → assert ordering by createdAtMs and that both namespaces' threads appear.
- **[LOW] deleteThread transaction failure/atomicity is documented but not tested** — `packages/memory/src/store/MemoryStoreLive.js:216-229 (deleteThreadEffect)` _(crash-safety-durability)_  
  - Untested: deleteThread runs message-delete + thread-delete in db.transaction specifically so a failure on the second write can't orphan messages (per its own comment). Tests only cover the success path (thread+messages gone). No test injects a failure on the second statement to prove the first is rolled back, and the broken-store error-wrapping suite stops before asserting partial-state cleanup.  
  - Risk: The transaction is the durability guarantee here. If a refactor accidentally moved either delete outside the transaction (or a driver doesn't honor the rollback), a failed deleteThread could leave a thread with no messages or messages with no thread — exactly the corruption the code claims to prevent — and the suite would stay green.  
  - Test: Wrap the real bun:sqlite db so the second delete (threads) throws mid-transaction; call deleteThread and assert it rejects AND that the messages are still present (rollback worked) — i.e. no partial deletion. Verify both rows survive an injected mid-transaction failure.
- **[LOW] deleteExpiredFacts TTL boundary (expiry == now) and ttl=0 semantics under-pinned** — `packages/memory/src/store/MemoryStoreLive.js:302-307 (deleteExpiredFactsEffect)` _(edge-case)_  
  - Untested: The expiry predicate is strict `updatedAtMs + ttlMs < now`. Tests rely on real sleep(1-10ms) so the exact boundary (updatedAtMs + ttlMs === now → must NOT be deleted) is never deterministically asserted, and facts with ttlMs IS NULL surviving is only implied. Because nowMs is mockable (scheduler/nowMs), this boundary could be pinned exactly but isn't.  
  - Risk: An off-by-one flip of `<` to `<=` would prematurely delete a fact at its exact expiry instant; a flip the other way would never expire a 0-ttl fact. With time-based sleeps the test would pass either way. For a control plane where memory TTL governs what context survives across runs, premature deletion silently drops state.  
  - Test: Freeze nowMs (via scheduler/nowMs override or a fixed-clock db wrapper). Insert facts whose updatedAtMs+ttlMs is exactly now, just before now, just after now, and one with ttlMs=null. Run deleteExpiredFacts and assert ONLY the strictly-past ones are removed; the boundary fact and the null-ttl fact survive.

### observability (6)

- **[MEDIUM] OTLP exporter layer construction is 0% covered: disabled/missing-collector path and enabled path both unexercised** — `apps/observability/src/createSmithersOtelLayer.js:11-21` _(error-path)_  
  - Untested: createSmithersOtelLayer is at 0% function coverage. Neither branch is tested: (a) when resolved.enabled is false it must return Layer.empty (no exporter, no FetchHttpClient), and (b) when enabled it must build Otlp.layerJson with baseUrl=resolved.endpoint, serviceName, and the tracerContext span-storage hook, provided FetchHttpClient.layer. createSmithersObservabilityLayer (createSmithersObservabilityLayer.js, also 0%) is likewise untested including the installLogger=false branch that must omit logger layers and the logFormat->logger mapping (resolveLogger json/pretty/string/logfmt).  
  - Risk: This is the exact 'exporter failure / missing collector behavior' the audit targets. If the disabled guard regresses (e.g. someone removes the early Layer.empty return), every process that imports the layer without SMITHERS_OTEL_ENABLED would attempt to stand up an OTLP exporter and hammer a non-existent collector at http://localhost:4318, adding latency/errors to runs that never opted into telemetry. Conversely, if the enabled branch silently drops the tracerContext hook, smithers spans stop being captured into smithersTraceSpanStorage and span correlation breaks — with zero test signal.  
  - Test: Assert createSmithersOtelLayer({enabled:false}) === Layer.empty (or builds to an empty layer that provides no Tracer). With enabled:true, build the layer in an Effect scope and assert it composes without throwing and that resolveSmithersObservabilityOptions is honored (endpoint/serviceName propagate). For createSmithersObservabilityLayer: assert installLogger:false produces a layer with no Logger.replace (e.g. by checking the merged layer set length / behavior), and that each logFormat yields a layer that builds. No real collector needed — only layer construction, not flushing.
- **[MEDIUM] trackEvent: Sandbox*, RunContinuedAsNew, and AgentEvent completed/usage/retry branches are untested** — `apps/observability/src/metrics/trackEvent.js` _(happy-path-only)_  
  - Untested: Coverage shows large untested regions of trackEvent (lines ~267-317 = all Sandbox* cases, 338-346 = RunContinuedAsNew carriedStateBytes/ancestryDepth, 502-507/519-550 = AgentEvent 'started'/'completed' session counters, agentErrorsTotal on ok===false, recordAgentUsageMetrics token extraction, hasAgentRetrySignal). Existing effect-metrics-track tests assert RunStarted/Finished, TokenUsageReported, OpenApi, and AgentEvent 'action' only, and most other cases are 'does not throw' with no metric-delta assertion.  
  - Risk: RunContinuedAsNew feeds continue-as-new lineage metrics (ancestry depth / carried-state bytes) — a durability/lineage signal the audit flags as core. AgentEvent 'completed' drives agent session success/failure counters and token totals, and the active-sandbox gauge increments on SandboxCreated and decrements on SandboxCompleted; an unbalanced +/- (e.g. decrement without the runtime tag) would leak the smithers.sandbox.active gauge upward forever. None of these specific deltas are asserted, so a sign error or wrong tag would ship undetected.  
  - Test: Extend effect-metrics-track.test.js with before/after snapshot deltas for: SandboxCreated then SandboxCompleted balancing sandbox.active to 0 with matching runtime tag and status; RunContinuedAsNew asserting runs.continued_total+1, active runs -1, runs.carried_state_bytes histogram count+1, and runs.ancestry_depth recorded when provided; AgentEvent 'completed' with ok:false asserting agent_sessions_total{status=failed}+1 and agent_errors_total+1; AgentEvent 'completed' with usage asserting agent_tokens_total{kind=input/output/...} increments via recordAgentUsageMetrics.
- **[LOW] emitOtelLogRecord severity-to-logger routing is untested** — `apps/observability/src/emitOtelLogRecord.js:11-19` _(error-path)_  
  - Untested: emitOtelLogRecord routes a record to logErrorAwait/logWarningAwait/logInfoAwait based on record.severity. There is no test that an ERROR record calls the error path, a WARN record the warning path, and the default (INFO and any unexpected value) the info path, nor that record.body/attributes/category are forwarded unchanged.  
  - Risk: This is the final hop that actually ships agent-trace and agent-session records (with their redaction metadata) into the Effect log pipeline that the OTLP layer exports. A miswire (e.g. ERROR records logged at INFO) would silently downgrade error visibility in Loki/Grafana, and is invisible to the builder-level tests that only check inferCanonicalSeverity/inferSessionSeverity in isolation.  
  - Test: Using setSmithersLogRunner to inject a capturing runner (the pattern already in tests/effect-logging.test.js), call emitOtelLogRecord('agent-trace', {severity:'ERROR',body:'b',attributes:{x:1}}) and assert the effect captured carries Error level + the body/attributes/span(category); repeat for WARN and INFO and for an unknown severity falling through to info.
- **[LOW] updateAsyncExternalWaitPending floor-at-zero clamp untested (durable ask-human/event blocking gauge)** — `apps/observability/src/metrics/updateAsyncExternalWaitPending.js:9-14` _(edge-case)_  
  - Untested: This exported function (0% coverage) maintains the smithers.external_wait.async_pending gauge per kind ('approval'|'event') using a module-level counter clamped with Math.max(0, current+delta). Untested: that two increments then three decrements floor at 0 (not -1), that the gauge actually reflects the clamped value, and that 'approval' and 'event' counters are independent.  
  - Risk: This gauge is how operators see how many runs are durably blocked on ask-human/approval or external events — a core human-in-the-loop/durability signal. If the clamp or the per-kind keying regresses, the gauge could go negative or cross-contaminate kinds, making 'is anything waiting on a human?' unreadable. The prometheus test only sets the gauge directly via Metric.set and never drives it through this updater, so the clamp logic has no coverage.  
  - Test: Effect.runSync a sequence: updateAsyncExternalWaitPending('approval', +1) x2, then -1 x3, snapshot the externalWaitAsyncPending gauge tagged kind=approval and assert value===0 (not negative); interleave 'event' updates and assert the two kinds track independently.
- **[LOW] renderPrometheusMetrics: no assertion that default-catalog seeding does not duplicate or overwrite live metric lines, and NaN/Inf number formatting unverified** — `apps/observability/src/renderPrometheusMetrics.js:190-205,23-33` _(api-contract)_  
  - Untested: After rendering live snapshots, renderPrometheusMetrics seeds defaultPrometheusMetricLines for every catalog metric only when registry entry has zero lines (lines 190-195). There is no test that a catalog metric which IS live keeps only its live lines (no zero-valued default lines appended), nor that a metric absent from the snapshot emits exactly the default 0 lines (with correct histogram _bucket/_sum/_count scaffolding and defaultLabels expansion). The formatPrometheusNumber NaN/+Inf/-Inf branches (lines 26-31) and bigint counter rendering are also unasserted.  
  - Risk: Prometheus rejects a scrape if a metric name appears with duplicate/contradictory lines or a malformed value. If the 'metric.lines.length===0' guard regresses, every active counter would also get a spurious '<name> 0' line, corrupting the scrape and silently dropping the whole endpoint from Prometheus. The histogram-state path emitting NaN/Inf (from an empty Effect histogram) must serialize as the Prometheus tokens, not 'undefined'.  
  - Test: Increment a known catalog counter, render, and assert its name appears exactly once with the live value and NOT also with a trailing ' 0' default line; for a catalog histogram that was never updated, assert the output contains its _bucket{le=+Inf}, _sum, and _count default 0 lines and the defaultLabels expansion. Add a unit test (export or via a crafted snapshot) that formatPrometheusNumber(NaN)==='NaN', (+Inf)==='+Inf', and a bigint counter renders as a plain integer.
- **[LOW] updateCurrentCorrelationContext in-place-mutation shim is 0% covered** — `apps/observability/src/correlation.js:31-44` _(concurrency-race)_  
  - Untested: The legacy compatibility shim updateCurrentCorrelationContext mutates the current correlation context object IN PLACE via Object.assign, unlike the FiberRef-based core version. It is at 0% coverage. Untested: the no-current-context no-op, that a held reference observes the mutation (the documented intentional semantics), and that an invalid/empty patch (mergeCorrelationContext returns undefined) is a no-op rather than clobbering.  
  - Risk: In-place mutation of shared correlation state is exactly the kind of thing that silently cross-contaminates log annotations between concurrently-running fibers/runs if the no-op guards regress. Because logging.js builds annotations from getCurrentCorrelationContext(), a bad patch leaking runId/nodeId across runs would misattribute logs/traces — a subtle correctness bug with no current test signal.  
  - Test: Assert updateCurrentCorrelationContext(patch) with no active context is a no-op (does not create a context). Within a withCorrelationContext scope, capture the context object reference, call the shim with a partial patch, and assert the same object now reflects merged values; then call with an empty/invalid patch and assert the object is unchanged (no fields dropped).

### openapi (8)

- **[MEDIUM] LLM-controlled header parameter can override/strip the injected auth secret (trust-boundary)** — `packages/openapi/src/tool-factory/_helpers.js:203-206` _(security-trust-boundary)_  
  - Untested: In executeRequest the header object is built as `{ ...buildAuthHeaders(options), ...headerParams }` — so model-supplied header *parameter* values (headerParams, derived from spec-declared `in: header` params and filled from LLM tool args) are spread AFTER the injected auth headers. If a spec declares a header parameter named `Authorization` (or, for apiKey-in-header auth, a header param whose name equals options.auth.name), the LLM-provided value WINS and replaces/strips the operator-injected secret. No test asserts precedence between injected auth headers and LLM-controlled header params. The existing escaping test only checks that a newline in a benign `X-Request-Id` is rejected by fetch.  
  - Risk: Smithers' security guarantee is 'secret injection without leak / LLM trust boundary'. An LLM (or prompt-injected content) could set the Authorization header param to an attacker token, or to empty/garbage, defeating the configured credential or pinning requests to a token the model controls — silently, with no test catching the regression. This is the per-unit analog of the e2e secret-injection-no-leak case and is currently unverified at the unit level.  
  - Test: Build a spec with a header parameter literally named `Authorization` (and a second variant matching an apiKey header name). Create tools with options.auth = bearer token X. Mock fetch, execute with args providing the Authorization header param = 'attacker'. Assert that init.headers.Authorization still equals `Bearer X` (i.e. operator auth must win), or document+test the intended precedence. Repeat for apiKey-in-header where auth.name collides with a header param.
- **[LOW] Non-2xx HTTP responses (401/403/429/500) are returned as successful tool results — error path untested** — `packages/openapi/src/tool-factory/_helpers.js:219-224` _(error-path)_  
  - Untested: executeRequest never inspects `response.ok` / response.status. A 401, 403, 429, or 500 with a JSON body is parsed and returned exactly like a 200, with no { error: true } marker and no status surfaced to the agent. The only failure tests in the suite are fetch *rejections* (network error). No test exercises a resolved-but-non-OK Response.  
  - Risk: The agent driving these tools cannot distinguish 'created the resource' from 'got 403 Forbidden' / '429 rate limited' / '500 server error' — it sees a normal object result. This can cause the LLM to proceed as if a side-effecting call succeeded (e.g. treat a 500 on createPet as success), corrupting downstream workflow logic. For a durable control plane orchestrating long-running side-effecting calls, swallowing HTTP error status is a real correctness hazard that ships undetected because nothing asserts non-2xx behavior.  
  - Test: Mock fetch to resolve `new Response(JSON.stringify({code:403,message:'denied'}), {status:403, headers:{'content-type':'application/json'}})`. Execute a tool and assert the result reflects the failure (e.g. contains the status / an error flag), not a bare success object. Add cases for 401, 429, and a 500 with text/plain body. This both documents and locks in the intended contract.
- **[LOW] PUT and PATCH operations are declared supported but never extracted/executed in any test** — `packages/openapi/src/types.js:20 / src/extractOperations.js:26-50` _(happy-path-only)_  
  - Untested: HTTP_METHODS = ['get','post','put','delete','patch']. Tests exercise only get/post/delete. No test extracts a `put` or `patch` operation, builds its schema, or executes it (verifying init.method === 'PUT'/'PATCH' and body serialization). Coverage shows the loop line as hit, but the put/patch branches' end-to-end behavior is never asserted.  
  - Risk: PUT/PATCH are the most common body-carrying update verbs. A regression in method casing (operation.method.toUpperCase()), body serialization for PATCH, or extraction order would pass the suite. update/replace semantics are exactly the side-effecting calls a durable agent must get right; shipping a broken PATCH undetected corrupts user data.  
  - Test: Add a spec fixture with `put /pets/{id}` and `patch /pets/{id}` (both with JSON requestBody). Assert extractOperations returns them, then mock fetch and execute: assert init.method === 'PUT' / 'PATCH', body === JSON.stringify(args.body), and the path param substituted.
- **[LOW] head/options/trace operations are silently dropped — no test documents the exclusion** — `packages/openapi/src/types.js:20` _(edge-case)_  
  - Untested: HTTP_METHODS omits head/options/trace, so an operation defined only under those verbs yields zero tools. No test asserts this behavior (e.g. a spec with only a `head` operation producing an empty tool set, or that a path with get+head only surfaces get).  
  - Risk: Silent omission means a spec author who relies on a HEAD/OPTIONS operation gets no tool and no error/warning. While usually acceptable, the absence of a test means changing HTTP_METHODS (adding/removing a verb) would not be caught, and the intentional-vs-accidental nature of the omission is undocumented.  
  - Test: Add a spec whose only operation is `head /status` and assert extractOperations returns [] and createOpenApiToolsSync returns {}. Add a path with both `get` and `head` and assert only the get-derived tool exists.
- **[LOW] Pre-loaded spec object missing the 'openapi' field (e.g. Swagger 2.0) — rejection path untested in both loaders** — `packages/openapi/src/loadSpecSync.js:19-21 / src/loadSpecEffect.js:21-28` _(error-path)_  
  - Untested: Both loaders, when given an object input lacking an 'openapi' key (e.g. a Swagger 2.0 object with `swagger: '2.0'`, or any plain object), throw/fail with the dedicated Swagger-2.0 guidance message. Coverage explicitly flags loadSpecSync:19-21 and loadSpecEffect:21-28 as uncovered. No test passes an object without `openapi`.  
  - Risk: This is the single most likely real-world misuse (handing a Swagger 2.0 doc straight in). The error message is the only thing telling a user/agent what went wrong. An untested error branch can silently regress (wrong message, or worse, falling through to treat a 2.0 spec as 3.x and generating broken tools) with no signal. Note loadSpecEffect must FAIL (not throw synchronously) and wrap via toSmithersError — a regression to a synchronous throw would break Effect callers and is uncaught.  
  - Test: loadSpecSync({swagger:'2.0', paths:{}}) should throw matching /missing an 'openapi' field/. Effect.runPromise(loadSpecEffect({swagger:'2.0', paths:{}})) should reject (not throw synchronously) with the same message and be a SmithersError tagged 'openapi load spec'.
- **[LOW] Non-object / non-array request body serialization fallbacks are untested (multipart/urlencoded/raw)** — `packages/openapi/src/tool-factory/_helpers.js:99-111,124-135,141-167` _(edge-case)_  
  - Untested: Coverage flags these lines uncovered. Untested: (a) buildFormData/buildUrlEncodedBody when an array property is present (append per-item — lines 99-102, 123-126); (b) the fallback where the body is NOT a plain object (e.g. a string or Blob) so it is appended under the literal key 'body' (109-111, 133-135); (c) buildRawBody (141-144) and the raw-body branch of serializeRequestBody (166-167) for non-JSON, non-form media types (e.g. application/octet-stream, text/csv) — the body is JSON.stringified-or-passed-through and Content-Type set to the media type.  
  - Risk: These are the file-upload / binary / CSV / form-array paths agents hit for real integrations (e.g. multipart with multiple file fields, octet-stream uploads). A serialization bug here sends a malformed body to a real API with no test guarding it — e.g. an array form field collapsing to '[object Object]', or a raw binary body being JSON.stringified. The multipart test only covers scalar string fields, never an array field or a Blob.  
  - Test: (1) multipart spec with an array-typed property: execute with body.tags=['a','b'] and assert FormData.getAll('tags')===['a','b']. (2) urlencoded with an array field — assert repeated key. (3) A spec with requestBody content-type 'application/octet-stream' (or 'text/csv'): execute with a string body and assert init.headers['Content-Type']==='application/octet-stream' and init.body is the raw string (not JSON-quoted), plus a Blob body passed through unchanged.
- **[LOW] options.headers vs injected auth precedence is unspecified and untested (auth-override / header clobber)** — `packages/openapi/src/tool-factory/_helpers.js:42-44` _(security-trust-boundary)_  
  - Untested: buildAuthHeaders does Object.assign(headers, options.headers) AFTER setting the auth header, so a user-supplied options.headers.Authorization overrides the configured auth credential. There is also no test of what happens when both auth and custom headers are supplied together. The 'applies custom headers' test uses a distinct header name (X-Custom) and the 'applies bearer auth' test supplies no custom headers — the interaction is never exercised.  
  - Risk: Whether explicit options.headers should override structured auth is a real contract decision. Without a test, the behavior can flip silently. More importantly, if static config and dynamic auth disagree, an operator could unknowingly null out their bearer token by also passing a headers map, or vice versa — a credential-handling surprise that should be pinned by a test.  
  - Test: Create tools with auth=bearer 'A' AND options.headers={Authorization:'Bearer B'}; execute and assert which wins (documenting the intended contract). Also assert that supplying both auth and an unrelated custom header preserves both (Authorization=Bearer A AND X-Custom present).
- **[LOW] executeToolEffect / execute error handling: SmithersError-shaped failures and message contents not asserted** — `packages/openapi/src/tool-factory/_helpers.js:236-251,332-345` _(error-path)_  
  - Untested: executeToolEffect wraps execution in Effect with metric increments (calls/errors/duration) and the createToolFromOperation execute() catch returns { error, message, status:'failed' }. Tests assert error===true and a message on a network reject, but never assert: (a) the error metric is actually incremented on failure, (b) that a non-Error thrown value (e.g. a string, or a SmithersError) is rendered with a sensible message rather than 'undefined'/'[object Object]', (c) that the returned message does not leak the auth token even when the underlying error embeds the request.  
  - Risk: The catch reads e?.message ?? String(error). A thrown plain object or a TaggedError without .message would surface 'undefined' or a useless string to the agent. And if any error path ever stringifies the RequestInit (which carries the Authorization header), the secret could leak into the tool result returned to the model — directly violating the secret-no-leak boundary. None of this is asserted.  
  - Test: Mock fetch to reject with a plain string and a SmithersError; assert the returned message is meaningful (not 'undefined'). Add a test that the auth token value never appears in the returned error message/serialization. Assert openApiToolCallErrorsTotal increments on failure (via a Metric snapshot) to lock the observability contract.

### pi-plugin (8)

- **[HIGH] Human-in-the-loop mutations approve/deny/signal in DevToolsClient are entirely untested** — `packages/pi-plugin/src/runtime/DevToolsClient.ts:426-456` _(happy-path-only)_  
  - Untested: DevToolsClient.approve, deny, and signal have NO direct test (coverage shows lines 407-454 uncovered). Untested: the RPC method name ('approvals.decide'), the exact payload sent (runId/nodeId/iteration/approved:true|false/note for approve+deny; signalName/data/correlationId for signal), the default iteration=0, and that the returned auditRowId is extracted from the gateway response. Only cancel() and resume() exercise the rpc/auditRowId path.  
  - Risk: These are the approve/deny gates of the human-in-the-loop trust boundary. A regression that flips `approved` (approve sending false), drops `nodeId`/`iteration` (approving the wrong attempt), or sends the wrong RPC method would silently approve/deny the wrong thing or fail to record the audit row — and nothing in CI would catch it. The /smithers-approve command and RunInspector 'a'/'d' keys route through exactly these methods.  
  - Test: With the existing httpFixture helper, call client.approve('run-x','node-a',2,'note') and assert the captured RPC body equals {method:'approvals.decide', params:{runId:'run-x',nodeId:'node-a',iteration:2,approved:true,note:'note'}} and that the returned object is {auditRowId:'<id-from-response>'}. Repeat for deny (approved:false) and the default iteration=0 path. For signal, assert params {runId,signalName,data,correlationId} and data defaults to {} when omitted.
- **[MEDIUM] extension.ts plugin lifecycle has only source-string grep tests, no behavioral coverage** — `packages/pi-plugin/src/extension.ts:374-612` _(error-path)_  
  - Untested: Coverage is 11% funcs / 13% lines. Untested behaviors: session_start clearing a stale pollInterval before re-arming; the polling timer self-disarming (clearInterval + pollInterval=undefined) when updateStatusBar throws on a stale ctx after reload/fork; session_shutdown disconnecting every tracked store and clearing the runs map; the smithers-approve command filtering waiting nodes and surfacing approve/deny failures via ctx.ui.notify; the smithers-cancel confirm/abort and error-notify path; trackRun returning the existing run (not double-connecting) for a known runId; and smithers-run parsing runId from JSON-vs-text MCP output. The only tests are extensionUiInterop.test.ts which greps the source for 'setStatus'/'normalizeState' strings.  
  - Risk: This is the actual integration surface of the plugin. The code comments explicitly warn that a stale timer can crash the Pi process and that ctx goes stale after reload/newSession/fork. A regression removing the try/catch self-disarm, or the clearInterval on re-arm, would leak timers and crash long-lived Pi sessions — exactly the durability/long-running guarantee. None of it is asserted; grep tests pass even if the handler bodies are gutted.  
  - Test: Construct a fake ExtensionAPI capturing registered handlers/commands. Fire session_start, then make ctx.ui.setStatus throw and advance/trigger the interval callback; assert the interval is cleared (no further calls) instead of throwing. Fire session_start twice and assert the first interval was cleared. Fire session_shutdown after trackRun seeded a run and assert store.disconnect was called and runs map is empty. Invoke the smithers-approve handler with a fake ctx whose ui.select returns a waiting node + 'Approve', stub client.approve to throw, and assert ctx.ui.notify('error') is called and no success notify.
- **[MEDIUM] RunInspector keyboard handlers (approve/deny/cancel/rewind/return-to-live) untested** — `packages/pi-plugin/src/views/RunInspector.ts:68-117,176-234` _(happy-path-only)_  
  - Untested: Coverage 50% funcs / 39% lines: handleInput and all the action handlers are uncovered. Untested: 'a'/'d' calling client.approve/deny with the selected node's runId/nodeId/iteration; 'a'/'d' notifying 'No task node selected' when selectedNode has no task; 'w' (rewind) gating on store.isRewindEligible and notifying when ineligible; 'c' cancel; 'l'/end returnToLive; focus cycling and routing input to scrubber vs inspector vs tree; and error-notify when a mutation throws.  
  - Risk: This is the interactive operator path for approving gates and rewinding a live run from the inspector TUI. A bug binding 'a' to deny, or rewindDisplayedFrame not checking isRewindEligible (allowing rewind of a finished run, an unsafe time-travel side effect), would let an operator irreversibly mutate the wrong run. selectedTask() silently returning undefined on a ghost/non-task node must surface a warning, not a no-op or crash.  
  - Test: Build a DevToolsStore with a selected task node and a fake DevToolsClient spying approve/deny/cancel. Drive inspector.handleInput('a') and assert client.approve called with the node's runId/nodeId/iteration and onNotify('Approved...'). handleInput('d') -> deny. With no selectable task, assert onNotify('No task node selected.','warning') and no client call. With mode=live (not historical), handleInput('w') -> onNotify warns and store.rewind NOT called; with a historical frame on a live run, assert store.rewind(displayedFrameNo,true) is invoked. Make client.approve reject and assert onNotify('error').
- **[MEDIUM] Delta baseSeq-mismatch and applyDelta-throw resync paths in DevToolsStore are untested** — `packages/pi-plugin/src/runtime/DevToolsStore.ts:603-641` _(crash-safety-durability)_  
  - Untested: applyDeltaToLiveState has three guarded branches that are not asserted: (1) a delta whose baseSeq !== liveSnapshot.seq must be DROPPED and trigger requestResync (a fresh stream); (2) applyDelta() throwing (corrupt/invalid delta) must be swallowed and also trigger requestResync rather than crash; (3) a delta arriving while awaitingSnapshotAfterGapResync is true must be ignored. The behavior tests only cover a clean snapshot->delta sequence and the gapResync-keeps-old-tree case.  
  - Risk: This is the live-stream ordering/seq integrity guarantee. If a stale or out-of-order delta (baseSeq mismatch) were applied instead of dropped, the displayed tree would diverge from the durable run state — silent corruption of what the operator sees while making approve/rewind decisions. If a malformed delta threw without the catch+resync, the consumeStream loop would surface it as a stream error and the store could get stuck instead of self-healing via a fresh snapshot.  
  - Test: With a fakeClient streamDevTools, feed snapshot(seq:2) then a delta with baseSeq:5 (mismatch); assert the tree is unchanged, the delta is not applied, and the store re-subscribes (a second streamDevTools call with afterSeq undefined). Feed a delta that applyDelta will reject (e.g. removeNode for a missing id forcing InvalidDeltaError) and assert no throw escapes and a resync is requested. Feed gapResync then a delta and assert the delta is dropped (awaitingSnapshotAfterGapResync) until a fresh snapshot arrives.
- **[MEDIUM] DevToolsClient WS auth token and subscribe params on connect are not asserted** — `packages/pi-plugin/src/runtime/DevToolsClient.ts:331-342,494-502` _(security-trust-boundary)_  
  - Untested: For the WS streamDevTools path, no test asserts the connect frame carries auth:{token: apiKey} when an apiKey is configured (or omits auth when not). The HTTP rpc path's Bearer header is asserted only for getDevToolsSnapshot; approve/deny/signal/rewind Bearer forwarding is not. Also untested: SeqOutOfRange when afterSeqCursor is undefined falls through to assertOk (no infinite gapResync) and that connect's 5s challenge timeout (PI_GATEWAY_TIMEOUT) fires when no connect.challenge arrives.  
  - Risk: This is the auth trust boundary for the gateway control plane. If the apiKey stopped being forwarded on the WS connect (or in mutation RPCs), an authenticated gateway would reject the subscription/mutation, or worse, a token could be silently dropped and the operator would think they were connected securely. The connect.challenge timeout is the only guard against hanging forever on a non-Smithers WS endpoint.  
  - Test: Extend the WebSocketServer fixture to capture the connect frame and assert params.auth === {token:'srk_x'} when DevToolsClient is built with apiKey, and that auth is undefined otherwise, plus subscribe:[runId]. For the HTTP path, assert the Authorization:Bearer header on an approve() call. Add a fixture that sends connect.challenge but never replies / never sends challenge and assert PI_GATEWAY_TIMEOUT after the configured window.
- **[MEDIUM] Stale banner reveal timer correctness (delay, recovery cancels, run-switch resets) is only partially tested** — `packages/pi-plugin/src/runtime/DevToolsStore.ts:696-730` _(concurrency-race)_  
  - Untested: One test asserts the banner becomes visible after a drop. Untested: that markStreamHealthy on reconnect within the delay window cancels the pending timer so the banner never shows (no spurious 'gateway disconnected' flash on a quick reconnect); that the staleSince-identity guard in the timer callback prevents a banner from a previous disconnect epoch firing after recovery+re-disconnect; and that connect() to a new runId clears the timer/staleSince.  
  - Risk: The stale banner is the operator's signal that the displayed tree may be diverged from durable state — a false-positive banner during normal reconnect erodes trust, and a stuck banner after recovery hides that data is live again. The timer callback compares staleSince timestamps to avoid cross-epoch firing; a regression there would surface the wrong banner state at exactly the moment an operator is deciding whether to trust the tree for an approval/rewind.  
  - Test: Drive a store with a short staleBannerDelayMs: simulate stream interrupt (markConnectionInterrupted), then before the delay elapses apply a healthy event (markStreamHealthy); assert isStaleBannerVisible stays false and staleSince is cleared. Simulate interrupt -> wait past delay -> banner true -> recover -> interrupt again quickly and assert the old timer doesn't toggle the banner for the stale epoch. Call connect(newRunId) while banner is visible and assert it resets staleSince/isStaleBannerVisible.
- **[LOW] auditRowId extraction is only tested for one shape (result.audit_row_id); 5 key aliases, 6 nested containers, and array recursion untested** — `packages/pi-plugin/src/runtime/DevToolsClient.ts:78-179` _(edge-case)_  
  - Untested: The auditRowId helper recognizes 6 key spellings (auditRowId/audit_row_id/auditId/audit_id/auditLogId/audit_log_id), recurses through 6 nested containers (result/data/mutation/ack/payload/meta) and arrays, and returns undefined when absent. Tests only exercise payload.result.audit_row_id (via resume). The camelCase keys, top-level keys, the other containers, array-of-results, and the undefined fallback are unverified.  
  - Risk: The audit row id is the durable receipt that an approve/deny/cancel/rewind mutation was recorded — the inspector toast and lastAuditRowId surface it to the operator as proof of a human decision. If the gateway returns auditRowId (camelCase) inside ack/meta and the extractor misses it, the operator sees 'Rewound to frame N.' with no audit reference even though one exists, undermining the human-in-the-loop audit trail. The empty-string guard (id.length>0) is also untested.  
  - Test: Unit-test the mutation methods (or export auditRowId) against responses: {auditRowId:'x'} (top-level camelCase), {ack:{audit_id:'y'}}, {meta:{auditLogId:'z'}}, [{}, {data:{audit_row_id:'a'}}] (array recursion), {auditRowId:''} -> undefined, and {} -> undefined. Assert each mutation returns the expected auditRowId or undefined.
- **[LOW] streamEvents/SmithersPiHttpClient.events on non-ok or missing body is untested** — `packages/pi-plugin/src/api/SmithersPiHttpClient.ts:56-62` _(error-path)_  
  - Untested: The events() SSE generator returns silently (no error, no events) when res.ok is false or res.body is null. The api.test.ts only covers the happy stream and SmithersPiHttpClient.test.ts covers malformed-frame skipping and reader release — none assert the !ok/!body early return, nor multi-line SSE frames where 'data:' is not the first line, nor a chunk split across the read boundary mid-JSON.  
  - Risk: This is the programmatic live-event consumer (exported in index.ts). A 401/404/500 from the events endpoint yields an empty stream that looks identical to 'run finished, no events' — a consumer would silently believe a run produced nothing rather than surfacing an auth/HTTP failure. Buffer-split handling (buffer = parts.pop()) is the correctness core of SSE reassembly and is only implicitly exercised.  
  - Test: Stub fetch to return {ok:false,status:401} and assert the events() generator yields nothing and does not throw (documenting the silent behavior — and consider whether it should throw). Stub a body that splits a single JSON payload across two chunks ('data: {"n":' then '1}\n\n') and assert exactly one {n:1} is yielded. Add an SSE frame with a comment line before the data line and assert it still parses.

### protocol (3)

- **[MEDIUM] No test asserts the runtime error-code arrays match their TypeScript type unions (.js vs .ts drift)** — `packages/protocol/src/errors/index.js + src/errors/{DevToolsErrorCode,JumpToFrameErrorCode,NodeDiffErrorCode,NodeOutputErrorCode}.ts` _(api-contract)_  
  - Untested: Each error code set is hand-maintained TWICE: once as a runtime array in errors/index.js (e.g. DEVTOOLS_ERROR_CODES) and once as a string-literal union in the matching .ts file (e.g. DevToolsErrorCode). The existing test (protocol-contracts.test.js) only hard-codes the array contents inline; it never compares the array to the type union. tsup builds dts-only, so it does NOT regenerate these .js arrays — nothing keeps them in sync with the .ts unions. They are byte-equal today purely by manual discipline.  
  - Risk: This is the wire contract for the gateway/devtools RPC error channel. If someone adds a code to the union but not the array (or vice-versa), TypeScript still compiles everywhere (consumers narrow on the type, the runtime emits/validates against the array), and the test stays green because it only pins the array. A server could emit a 'WorkingTreeDirty'-style code the client's type never models, or a client could exhaustively switch on a union member the runtime never sends — silent contract fork shipped undetected. Order also matters (the existing test asserts exact-order equality of the arrays) yet nothing pins union order to array order.  
  - Test: Add a type-level + runtime parity test: import each *_ERROR_CODES array and assert `[...ARRAY] as const satisfies readonly ErrorCodeUnion[]` AND `type guard: every union member appears in the array`. Concretely, write a compile-time check `const _: Record<DevToolsErrorCode, true> = Object.fromEntries(DEVTOOLS_ERROR_CODES.map(c=>[c,true]))` (fails to typecheck if array is missing a member) plus a runtime test asserting array length === number of union members for all four sets. Repeat for the other three. This catches add-to-one-not-the-other drift.
- **[MEDIUM] Duplicate wire-type definitions in packages/protocol and packages/devtools have no drift guard** — `packages/protocol/src/devtools/{DevToolsDelta,DevToolsDeltaOp,DevToolsSnapshot}.ts vs packages/devtools/src/{DevToolsDelta,DevToolsDeltaOp,DevToolsSnapshotV1}.ts` _(api-contract)_  
  - Untested: The DevToolsDelta, DevToolsDeltaOp and DevToolsSnapshot wire types exist as a second, independent copy inside packages/devtools (the package that actually applies deltas and builds snapshots). I diffed them: DevToolsDelta.ts and DevToolsDeltaOp.ts are byte-identical across the two packages, and DevToolsSnapshot.ts differs only by the type name (DevToolsSnapshot vs DevToolsSnapshotV1). Nothing imports the protocol copy into devtools, and no test asserts the two copies are structurally equal.  
  - Risk: packages/devtools/src/applyDelta.js mutates the live run tree from these ops, and the gateway serializes the protocol copy onto the wire. If a 5th delta op (e.g. moveNode) or a new DevToolsNode field is added to one copy but not the other, the producing side emits ops/fields the consuming side's apply logic silently ignores or mishandles — the exact 'replaying must reproduce state exactly / live tree diverges' class of bug. Two source-of-truth copies of a wire schema with zero cross-check is a structural durability/replay hazard.  
  - Test: Add a test that imports BOTH copies and asserts structural equivalence via a type-assignability check in each direction (`const a: ProtocolDelta = {} as DevtoolsDelta; const b: DevtoolsDelta = {} as ProtocolDelta`) so a field added to only one side fails to compile. Additionally, a runtime sample-message round-trip: construct a DevToolsDelta with one op of each kind and assert applyDelta (from devtools) accepts it without throwing, ensuring the op-kind set is in sync.
- **[LOW] DEVTOOLS_PROTOCOL_VERSION=1 is asserted, but no test pins it as the value embedded in every wire message's `version` field** — `packages/protocol/src/devtools.js (DEVTOOLS_PROTOCOL_VERSION) + src/devtools/{DevToolsEvent,DevToolsDelta,DevToolsSnapshot}.ts (version: 1)` _(api-contract)_  
  - Untested: DEVTOOLS_PROTOCOL_VERSION is a standalone constant (=1), and every wire message type independently hard-codes a literal `version: 1` field. The test asserts the constant equals 1 but never ties the constant to the message-shape literal. There is no test that a constructed DevToolsEvent/Delta/Snapshot carries `version === DEVTOOLS_PROTOCOL_VERSION`, and no negative test that a consumer rejects `version !== DEVTOOLS_PROTOCOL_VERSION` (that rejection logic lives in devtools applyDelta.js:50 and is unguarded at the protocol layer).  
  - Risk: When the protocol bumps to v2, a developer can bump DEVTOOLS_PROTOCOL_VERSION but forget the literal `version: 1` in one message type, or vice-versa. A snapshot stamped with the wrong version would be accepted/rejected inconsistently across the gateway and the time-travel/replay path, where version mismatch is the gate that protects against applying an incompatible delta to a snapshot. The constant exists precisely so producers/consumers agree; nothing enforces they use it.  
  - Test: Add a test: construct a minimal DevToolsSnapshot, DevToolsDelta, and both DevToolsEvent variants, and assert `msg.version === DEVTOOLS_PROTOCOL_VERSION` for each (compile-time: type `version: typeof DEVTOOLS_PROTOCOL_VERSION`). Also assert that bumping the literal without bumping the constant (or vice versa) is impossible by deriving the literal type from the constant in the type defs, and document that consumers must compare against the exported constant rather than a hard-coded 1.

### react-reconciler (4)

- **[MEDIUM] Render-time component throw / error-boundary behavior is completely untested** — `packages/react-reconciler/src/reconciler.js:472 (createContainer onUncaughtError/onCaughtError/onRecoverableError) and :479-484 (render)` _(error-path)_  
  - Untested: SmithersRenderer.render() calls updateContainerSync + flushSyncWork with reconciler.defaultOnUncaughtError/defaultOnCaughtError/defaultOnRecoverableError wired in. No test renders a component that throws during render, throws inside an error boundary, or produces a recoverable error. There is no assertion on what render() does when the React tree throws — whether it rejects, leaves container.root in a half-mutated state, or silently swallows.  
  - Risk: A workflow's build() can throw (bad props, a thrown SmithersError from useCtx outside a Workflow, an exception in a compute child evaluated during render). If the synchronous flush throws and leaves container.root partially mutated, the very next render() (a re-render frame) reconciles against corrupted host state — producing a wrong graph or an unrecoverable renderer instance. For a durable control plane that re-renders every frame, a render that throws must fail loudly and leave the container in a defined state; nothing asserts that contract.  
  - Test: Render a function component that throws synchronously; assert render() rejects (or surfaces the error) rather than resolving with a stale/partial graph. Render with useCtx() (from src/context.js) outside any Workflow provider and assert the SmithersError('CONTEXT_OUTSIDE_WORKFLOW') propagates out of render(). Then render a valid tree on the SAME renderer instance and assert it still produces a correct graph (the renderer is not poisoned by the prior throw).
- **[LOW] removeChildFromContainer with a specific child (the `if (child)` branch) is untested** — `packages/react-reconciler/src/reconciler.js:186-197 (removeChildFromContainer)` _(edge-case)_  
  - Untested: reconciler-coverage.test.jsx:104 calls removeChildFromContainer(container) with NO child argument, exercising only the `else { roots.length = 0 }` branch. The `if (child) { const idx = roots.indexOf(child); if (idx>=0) roots.splice(idx,1) }` branch — removing one specific top-level root while leaving others — has zero coverage, and the post-removal refreshContainerRoot/materializeRoot transition (multi-root fragment collapsing back to a single root, or to null) is never asserted.  
  - Risk: When a top-level Fragment shrinks from 3 roots to 1, React calls removeChildFromContainer(container, child) per removed root, then materializeRoot must collapse roots.length===1 back to a bare element (not a smithers:fragment wrapper). If indexOf misses or materializeRoot mis-collapses, the extracted graph's shape changes (extra/missing smithers:fragment wrapper) and the engine sees a different workflow structure than authored — a regression that would ship undetected.  
  - Test: Render a Fragment with 3 top-level smithers:task roots [a,b,c]; assert root.tag==='smithers:fragment' with 3 children. Re-render the Fragment with only [a]; assert getRoot() is a single smithers:task with id 'a' (fragment wrapper collapsed) and container.roots.length===1. Re-render to [a,c] and assert order is preserved and 'b' is gone.
- **[LOW] Multiple SmithersDevTools instances chaining/overwriting hook handlers is untested** — `packages/react-reconciler/src/devtools/SmithersDevTools.js:214-283 (start) and :262-281 (_cleanup)` _(concurrency-race)_  
  - Untested: Only single-instance start/stop is tested (devtools-stop.test.js). When instance A.start() then B.start() run, B captures A's handler as previousRootHandler and installs its own. A.stop() is a no-op (guard `hook.onCommitFiberRoot === installedRootHandler` fails because B overwrote it), and B.stop() restores A's handler. The start() idempotency guard (`if (this._active) return this`, line 215) is also never asserted. No test exercises two live instances or double-start.  
  - Risk: Real usage can layer devtools (e.g. a global instrumenter plus a per-run inspector). If stop() ordering wrongly deletes a still-active sibling's handler, live commit streaming to a UI/inspector silently dies — a subscriber stops receiving frames with no error. This is the kind of bounded-subscriber/live-stream regression the project guards at the gateway level but not at this fiber-hook level.  
  - Test: Install a clean hook. A=new SmithersDevTools(); A.start(). B=new SmithersDevTools(); B.start(). Fire hook.onCommitFiberRoot with a workflow fiber and assert B.tree updates. B.stop() and assert A's handler is restored AND A.tree updates on the next commit. Separately, call A.start() twice and assert the second call is a no-op (handler reference unchanged, no double-registration). Verify A.stop() after B overwrote it does not clobber B's handler.
- **[LOW] extractSerializableProps object-collapsing branches (agent/Zod/generic Object) and agent-array name join are not directly asserted** — `packages/react-reconciler/src/devtools/SmithersDevTools.js:58-125 (extractSerializableProps, extractTaskInfo)` _(happy-path-only)_  
  - Untested: The serialization safeguards are untested at the unit level: object props with modelId/model → '[Agent: …]' (line 74-76), props with `shape` → '[ZodSchema]' (line 78-79), other objects → '[Object]' (line 81-82), function props → '[Function]' (line 68-69), and the agent-ARRAY name join 'a → b' in extractTaskInfo (line 108-112). Existing tests assert nodeId/kind/label/depth but never that a Zod schema or an Agent object prop is collapsed to a stable label rather than serialized whole.  
  - Risk: These collapses are a trust/leak boundary for the devtools snapshot: a snapshot is forwarded to inspectors/UIs and may be persisted. If a real Zod schema, Drizzle table, or Agent config (which can carry API keys/credentials in nested fields) is NOT collapsed, the full object — potentially with secrets — leaks into the snapshot stream. Also unbounded objects bloat snapshot memory under N subscribers. A regression that drops the `[Agent:]`/`[ZodSchema]` guard would silently start leaking.  
  - Test: Build a fiber with memoizedProps containing { agent: {modelId:'claude-x', apiKey:'secret'}, schema: zod.object({...}), table: {/* drizzle */}, cb: ()=>{}, prompt:'hi' }; run fiberToNode and assert props.agent === '[Agent: claude-x]' (no apiKey present anywhere in the serialized output), props.schema === '[ZodSchema]', props.table === '[Object]', props.cb === '[Function]', props.prompt === 'hi'. Add an agent-array case ([{modelId:'a'},{modelId:'b'}]) and assert task.agent === 'a → b'.

### review (10)

- **[MEDIUM] srk_ API-key path through the Anthropic proxy runs inference with NO spend cap and is entirely untested** — `apps/review/src/server/proxy/handleAnthropic.ts:92-95 (and authenticateProxyRequest.ts:71-74)` _(security-trust-boundary)_  
  - Untested: Every handleAnthropic test authenticates with a session token. The `auth.kind === 'api-key'` branch (lines 92-95: repo = auth.repos[0] ?? auth.owner, pr = 0, spendCapUsd left at POSITIVE_INFINITY, sessionHash = null) and the corresponding lookupApiKey branch in authenticateProxyRequest (72-74) are never exercised by the proxy. recordUsage with sessionHash=null (no reservation guard) is likewise unasserted via the proxy.  
  - Risk: An operator srk_ key can proxy unbounded Anthropic inference: there is no spend cap (Infinity) and no per-call reservation. A regression that, e.g., mis-attributes the repo, mis-reads repos[], or accidentally honored a cap of 0 could either bill silently to api.anthropic.com or hard-block all operator traffic — and no test would catch it. Cost-control is a stated core guarantee (per-session runaway brake).  
  - Test: Mint an srk_ key (handleAdminKeys) with repos:['octo/widgets'], call POST /anthropic/v1/messages with x-api-key=srk_..., assert 200, assert the upstream got the real ANTHROPIC_API_KEY (not the srk_ key), and after metering assert a usage_events row with repo='octo/widgets', pr=0 and a non-session cost recorded. Add a second case with repos:[] asserting repo falls back to owner. Add a case asserting NO 402 ever fires for api-key auth regardless of prior spend.
- **[MEDIUM] Non-streaming usage parser (parseUsageFromJson) is never invoked — silent $0 metering for non-SSE responses** — `apps/review/src/server/proxy/parseUsageFromJson.ts:8-18` _(error-path)_  
  - Untested: 0% coverage. All proxy metering tests use content-type text/event-stream, which routes to parseUsageFromSse. The JSON branch in handleAnthropic (contentType not including 'text/event-stream' -> parseUsageFromJson) and the parser's null-on-missing-model / null-on-parse-failure behavior are unasserted.  
  - Risk: Non-streaming Messages responses are common (tool calls, small completions). If this parser regresses (e.g. returns null when usage is present, or reads the wrong field), every non-streaming call records cost $0, so the spend cap never trips and dashboards understate spend — the runaway-cost brake fails open with no failing test.  
  - Test: Unit: parseUsageFromJson on a valid {model,usage:{input_tokens,output_tokens}} returns the summary; on a body with no model returns null; on invalid JSON returns null; on missing usage returns {input:0,output:0}. Integration: serve a fixture with content-type application/json and a usage object, POST through the session, assert a usage_events row with kind='messages' and the expected cost and that spent_usd incremented.
- **[MEDIUM] GET /api/admin/usage endpoint has zero tests — admin auth gate and spend-data exposure unverified** — `apps/review/src/server/admin/handleAdminUsage.ts:19-51` _(security-trust-boundary)_  
  - Untested: 0% function coverage. No test hits /api/admin/usage at all: not the 401 unauthorized gate, not the 405 method gate, not the daily-aggregation SELECT/JSON shape.  
  - Risk: This endpoint returns per-repo per-model spend across all customers. If the timingSafeStringEqual auth check regressed (or ADMIN_TOKEN handling broke), cross-customer spend/usage would be readable by anonymous callers, undetected. handleAdminRepos/Keys have auth tests; usage was missed.  
  - Test: (1) GET without Authorization -> 401. (2) GET with a wrong bearer -> 401. (3) Seed two repos' usage_events across two UTC days, GET with the correct bearer, assert 200 and that days[] is grouped by day/repo/model, ordered day DESC, with inputTokens/outputTokens/costUsd summed. (4) POST -> 405.
- **[MEDIUM] handleWalkthroughs auth-failure, empty-body, size-limit, and session/srk_ auth paths untested** — `apps/review/src/server/walkthroughs/handleWalkthroughs.ts:31-38` _(security-trust-boundary)_  
  - Untested: The only test uses the legacy Bearer REVIEW_PUBLISH_TOKEN. Uncovered: the !legacyOk -> authenticateProxyRequest fallback (lines 33-34), the 401 when no credential is valid, the 400 empty-body guard, the 413 >25MB guard, and publishing with a session token or srk_ key (the documented preferred credentials).  
  - Risk: Anyone who can publish a walkthrough can persist arbitrary HTML served from review.jjhub.tech under /w/<id>. If the auth fallback or 401 path regressed, an unauthenticated attacker could store HTML on the trusted domain (the x-robots-tag noindex only blocks crawlers, not direct links). The 413/empty guards protect against R2 abuse / cost. None is asserted.  
  - Test: POST /api/walkthroughs with: (a) no auth -> 401; (b) a valid action session token via x-api-key -> 201 and body served back; (c) a valid srk_ key -> 201; (d) empty body with valid auth -> 400; (e) a >25MB body -> 413. Assert the legacy-token path still works alongside.
- **[MEDIUM] handleSessions api-key minting branch: repo-authorization (403) and missing repo/pr (400) untested** — `apps/review/src/server/sessions/handleSessions.ts:67-80` _(security-trust-boundary)_  
  - Untested: handleAdmin.test.ts mints a session with an srk_ key whose repos list MATCHES the requested repo. Uncovered: 403 'api key not authorized for repo' when record.repos is non-empty and excludes body.repo (line 76-77); the wildcard case (record.repos.length === 0 allows any repo); 400 'missing repo' (70-71) and 400 'missing pull request number' (73-75).  
  - Risk: This is the authorization scope for operator keys. A regression in the `record.repos.length > 0 && !record.repos.includes(...)` check would let a key scoped to repo A mint sessions for repo B (consuming B's quota and proxying inference billed to B). Authorization-denial is exactly the kind of branch that silently inverts.  
  - Test: Mint an srk_ key scoped to ['octo/widgets']. POST /api/sessions with apiKey + repo='octo/other' -> 403. With repos:[] (wildcard) + any registered repo -> 200. With apiKey but no repo -> 400. With apiKey + repo but no/<=0 pr -> 400.
- **[MEDIUM] verifyOidc malformed/unsupported-alg/bad-signature/wrong-issuer reasons not asserted at the unit level** — `apps/review/src/server/sessions/verifyOidc.ts:96-118` _(security-trust-boundary)_  
  - Untested: verifyOidc.test.ts asserts only the 'unknown-key' reason. handleSessions integration tests cover wrong-key->401, wrong-audience, and expired, but the distinct reason codes 'malformed' (parts!=3 / unparseable header/payload), 'unsupported-alg' (alg!=RS256), 'bad-signature' (valid key, tampered payload), and 'wrong-issuer' are never individually asserted.  
  - Risk: These are the OIDC trust boundary. The 'unsupported-alg' check is the classic alg-confusion defense (e.g. alg:'none' or HS256); a regression that fell through to verify would accept forged tokens. 'bad-signature' vs 'unknown-key' must stay distinct so a tampered-but-known-kid token is rejected. Mapping each to 401 is asserted nowhere for these specific paths.  
  - Test: Unit verifyOidc with: a 2-part token -> {ok:false,reason:'malformed'}; a header with alg:'none'/'HS256' -> 'unsupported-alg'; a token signed by the right key but with one payload byte flipped -> 'bad-signature'; valid signature but iss changed -> 'wrong-issuer'. Each using the rsaKeypair/serveJwks helpers already present.
- **[MEDIUM] Codex subscription secret materialization (auth.json with 0o600) is untested** — `apps/review/action/src/runAction.ts:87-97` _(security-trust-boundary)_  
  - Untested: resolveInferenceEnv (the pure decision) is tested, but the side effect in runAction that writes CODEX_AUTH_JSON to <codexHome>/auth.json with mode 0o600 and points CODEX_HOME at an isolated dir is not. runAction is a top-level script (runs on import), so the secret-write logic is not extracted into a testable function.  
  - Risk: This writes a repo owner's ChatGPT credential to disk on a CI runner. If the mode (0o600) or the isolated-CODEX_HOME choice regressed — e.g. writing to the real ~/.codex or with world-readable perms — the secret could leak to other steps/processes on the runner. Secret-injection-without-leak is a core trust guarantee.  
  - Test: Extract a `materializeCodexAuth(workspace, authJson, codexHome?)` helper and unit-test that it: creates the dir, writes auth.json containing exactly authJson, the file's mode is 0o600 (statSync(...).mode & 0o777 === 0o600), and it never writes outside the chosen codexHome (defaults under the workspace, not homedir).
- **[LOW] handleAnthropic upstream failure paths (502 on fetch throw, no-body upstream passthrough) untested** — `apps/review/src/server/proxy/handleAnthropic.ts:107-124` _(error-path)_  
  - Untested: Lines 114 (catch -> jsonError(502,'upstream fetch failed')) and 118-123 (upstream.body == null -> read text and return with upstream status) are uncovered. No test injects an upstream that throws or returns a bodyless response.  
  - Risk: When api.anthropic.com is down or returns a non-streaming error with no body, the proxy must degrade gracefully (502 vs throwing, or relaying the upstream error status/content-type). A regression here turns a transient upstream blip into a 500/unhandled rejection in the action, failing the user's PR review for no good reason.  
  - Test: (1) fetchUpstream that throws -> assert 502 with detail. (2) fetchUpstream returning new Response(null,{status:429,headers:{'content-type':'application/json'}}) or a bodyless error -> assert the returned status mirrors upstream and no usage row is written (no body to meter).
- **[LOW] fetchOidcToken error paths (missing env, non-2xx, missing value) untested** — `apps/review/action/src/fetchOidcToken.ts:15-33` _(error-path)_  
  - Untested: No test for fetchOidcToken: the missing-env guard (no ACTIONS_ID_TOKEN_REQUEST_URL/TOKEN -> instructive throw), the non-ok HTTP response throw, the missing/empty `value` throw, and the audience query-param construction (& vs ? join).  
  - Risk: It is testable (accepts a fetchImpl). A regression in the audience param or the error messaging would make the OIDC handshake fail with a confusing error in CI, or request a token for the wrong audience (which the server then rejects as wrong-audience), wasting a CI run with a misleading reason.  
  - Test: Set the two env vars and inject a fetchImpl: assert the requested URL includes audience=smithers-review (and uses & when requestUrl already has a query). Unset env -> rejects with the id-token:write hint. fetchImpl returning {ok:false,status:500} -> rejects 'HTTP 500'. fetchImpl returning {} (no value) -> rejects 'missing value'.
- **[LOW] publishWalkthrough success path and ~/.smithers-review.json config fallback untested** — `apps/review/src/cli/publishWalkthrough.ts:5-48` _(happy-path-only)_  
  - Untested: 75% function coverage. Only the 'no publish URL' guard is tested (with a mock fetch that is asserted NOT called). The config-file fallback (loadPublishConfig reading publishUrl/publishToken from ~/.smithers-review.json when env is unset), the trailing-slash stripping, the successful POST returning data.url, the non-ok HTTP error, and the 'response had no url' error are unasserted (the live e2e is skip-by-default).  
  - Risk: This is the client that uploads the walkthrough through the session credential. A regression in the config-file fallback or URL normalization would break publishing on developer machines (env-less) while CI (env-set) stays green, and the error-detail truncation guards against leaking large server bodies.  
  - Test: Against a Bun.serve fixture: write ~/.smithers-review.json (via homeDir option) with publishUrl/publishToken, no env -> assert it POSTs to <url>/api/walkthroughs with Bearer <token> and returns the share url. Trailing slash in url is stripped. Fixture returning 500 -> rejects 'publish failed: HTTP 500'. Fixture returning 201 with {} -> rejects 'response had no url'.

### sandbox (6)

- **[MEDIUM] cleanup/provider.cleanup errors are swallowed in finally — no test that a cleanup failure does not mask the real error or surface** — `packages/sandbox/src/execute.js:771-778 (finally: transportCall(...).catch(()=>undefined); provider.cleanup(...).catch(()=>undefined))` _(crash-safety-durability)_  
  - Untested: The finally block calls transport cleanup(handle) and provider.cleanup(request), each wrapped in .catch(()=>undefined). No test injects a cleanup that throws to assert: (a) the original success/failure result is preserved (cleanup error does not override it), and (b) a throwing cleanup does not crash the run. provider.cleanup is only tested on the happy path (it succeeds).  
  - Risk: If a future refactor removes the .catch, a flaky cleanup (e.g. rm on a locked dir, docker rm racing) would convert a successful run into a failure or mask the real error — breaking durability/idempotent teardown. Swallowing is a deliberate design choice that must be pinned by a test, otherwise the opposite (silently hiding a cleanup that should be retried/observed) can also regress unnoticed.  
  - Test: Provider test where provider.cleanup rejects and run() succeeds: assert executeSandbox still resolves with the outputs and getSandbox status is the real status (not affected). Second test: transport handle whose cleanup throws (fake executor) — assert the run's success/failure result is unchanged and no rejection escapes.
- **[MEDIUM] Docker volume mount-override guard (assertDockerVolumeDoesNotOverrideRuntimeMount) is an untested security branch** — `packages/sandbox/src/effect/process-runner.js:263-271, invoked at :420` _(security-trust-boundary)_  
  - Untested: dockerArgs rejects volumes whose container path is /workspace, /workspace/*, /result, or /result/* (preventing a config from shadowing the read-only request mount or the result mount). No test exercises this branch — the only docker-volume test uses container '/cache' (the allowed case).  
  - Risk: If this guard regresses, a malicious or buggy sandbox config could remap /workspace to a writable host path (escaping the ro-bind isolation) or redirect /result to exfiltrate, defeating the sandbox trust boundary. The allow path is tested; the deny path is not, so the deny could be silently broken.  
  - Test: dockerArgs('run', { ...handle, volumes:[{host:'/tmp/evil', container:'/workspace'}] }) -> toThrow('may not override'); repeat for '/workspace/sub', '/result', '/result/sub'; and a positive control that '/result-sibling' (prefix-but-not-subpath) is allowed.
- **[MEDIUM] Sandbox subprocess orphan-cleanup on engine/sandbox kill is unverified for docker (no --die-with-parent equivalent)** — `packages/sandbox/src/effect/process-runner.js:306-349 (bubblewrap --die-with-parent) vs :393-434 (dockerArgs has no analogous teardown); socket-runner/http-runner cleanup` _(crash-safety-durability)_  
  - Untested: bubblewrapArgs includes --die-with-parent (verified only via a generic --ro-bind/--unshare assertion, NOT specifically asserting --die-with-parent presence). dockerArgs has 'run --rm' but no --init / parent-death linkage, so on engine kill the docker child can be orphaned. The two crash-cleanup cases are explicitly test.skip in sandbox-isolation.test.js (require real bwrap). No unit-level assertion pins that bubblewrap requests --die-with-parent and docker requests --rm so a refactor cannot silently drop parent-death cleanup.  
  - Risk: kill-sandbox-engine-alive and kill-engine-mid-task e2e cover the engine/run layer, but the per-runtime arg that actually causes the child process to die with the engine is unguarded. Dropping --die-with-parent (bubblewrap) or --rm (docker) would leave orphaned sandbox processes/containers consuming resources and potentially continuing privileged work after the controlling run is gone — a crash-safety regression that ships green.  
  - Test: Assert bubblewrapArgs(...) contains '--die-with-parent' explicitly; assert dockerArgs(...) contains '--rm'. Add a static assertion (or doc-test) that docker cleanup() rm's the request path, and pin that cleanup is invoked in executeSandbox's finally for both runtimes (see the transport-failure test above).
- **[LOW] caCertPath-only egress (NODE_EXTRA_CA_CERTS sourced from caCertPath without caCertPem) is untested** — `packages/sandbox/src/egress.js:150-153 (caCertPath ?? (caCertPem ? options.caCertPath ?? WORKSPACE_PATH : undefined))` _(edge-case)_  
  - Untested: All egress env tests use caCertPem (which maps NODE_EXTRA_CA_CERTS to the workspace ca.crt path). The branch where the caller supplies caCertPath directly (egress.caCertPath set, caCertPem absent) — yielding NODE_EXTRA_CA_CERTS=<that path> verbatim — is never asserted. Also the options.caCertPath override argument to sandboxEgressEnv is untested.  
  - Risk: A regression in the precedence (caCertPath taking priority over the derived workspace path, or the options override) would silently point the sandbox at the wrong CA bundle, breaking TLS interception/egress trust for any user who pre-stages the cert by path instead of inline PEM.  
  - Test: sandboxEgressEnv({ caCertPath:'/etc/ssl/custom-ca.crt' }) -> { NODE_EXTRA_CA_CERTS:'/etc/ssl/custom-ca.crt' }; sandboxEgressEnv({ caCertPem }, { caCertPath:'/override/ca.crt' }) -> uses '/override/ca.crt'; and writeSandboxEgressFiles is a no-op when only caCertPath (not caCertPem) is set.
- **[LOW] provider.run throwing (rejecting) — as opposed to returning a malformed result — is not exercised** — `packages/sandbox/src/execute.js:238-245 (runSandboxProvider), 478 (await runSandboxProvider)` _(error-path)_  
  - Untested: Provider failure tests only cover providers that RESOLVE with a bad/incomplete result (missing status, bad diffBundle, no applier). No test has provider.run reject with an error. The catch path that records SandboxFailed, marks status failed, and still runs provider.cleanup in finally for a thrown provider is unasserted.  
  - Risk: A provider (remote sandbox API) throwing on a network/timeout error is the common failure. If the catch/finally regresses for the thrown case (e.g. cleanup not called, sandbox left 'shipped' not 'failed'), a failed remote sandbox would look stuck/active and could trip the concurrency limit for subsequent runs.  
  - Test: registerSandboxProvider with run: async()=>{ throw new Error('remote 503') } and a cleanup spy; assert executeSandbox rejects, getSandbox status==='failed', SandboxFailed emitted, and provider.cleanup was invoked with the request.
- **[LOW] Concurrency-limit check is read-then-act with no atomicity test for two sandboxes racing the same run** — `packages/sandbox/src/execute.js:381-390 (listSandboxes -> count active -> upsert)` _(concurrency-race)_  
  - Untested: The limit is enforced by listSandboxes() then counting active rows then upsert — a classic check-then-act. Only the already-at-capacity (sequential) case is tested. Two executeSandbox calls for the same runId starting concurrently (both read count = max-1, both proceed) is untested. The concurrent-resume-vs-supervisor e2e is about resume, not two sandboxes in one run racing the cap.  
  - Risk: Under concurrent <Sandbox> nodes in one run, the cap can be exceeded (both pass the check before either upserts), defeating SMITHERS_MAX_CONCURRENT_SANDBOXES and over-subscribing host resources. A blind spot for resource-exhaustion DoS.  
  - Test: Seed (max-1) active sandboxes, fire two executeSandbox calls for the same runId via Promise.all with a provider that awaits a shared barrier, and assert no more than max end up active (or document/accept that the cap is best-effort by asserting the actual behavior so a future tightening is detectable).

### scheduler (9)

- **[MEDIUM] Durable timer resume via absolute __timerUntil (ISO timestamp) and parseDurationMs h/m units are untested** — `packages/scheduler/src/makeWorkflowSession.js:113-152 (timerResumeAtMs / parseDurationMs)` _(crash-safety-durability)_  
  - Untested: Only __timerDuration:'5s' is exercised (service-branches timerFired test -> resumeAtMs 6000). The absolute-deadline branch __timerUntil (Date.parse of an ISO string), the duration units 'h' and 'm', the bare-number 'ms' default, the invalid/non-finite fallthrough (returns nowMs), and the empty/garbage parseDurationMs returning null are all uncovered (lines 116-126, 142-150).  
  - Risk: Timer resume time is a durability primitive: after a process/machine restart the engine recomputes resumeAtMs from the persisted descriptor meta. If __timerUntil parsing regresses (e.g. timezone/format change makes Date.parse NaN, falling through to nowMs), a timer that should fire hours from now fires immediately on restart — or a relative 'h'/'m' timer resolves to the wrong instant. restart-waiting-timer e2e covers one path at integration but does not pin the absolute-deadline vs relative-duration math per unit, nor the unit table.  
  - Test: Unit-test the resume math through the public surface: descriptor with meta.__timer + meta.__timerUntil set to a fixed future ISO string, nowMs fixed; submitGraph -> assert Wait Timer resumeAtMs === Date.parse(thatString). Add cases for __timerDuration '2h' (nowMs + 7200000), '30m', '500' (bare ms), and a malformed value (resumeAtMs === nowMs). Then timerFired and assert Finished with firedAtMs.
- **[MEDIUM] recoverOrphanedTasks idempotent no-op path (OrphanRecovery Wait) is untested** — `packages/scheduler/src/makeWorkflowSession.js:835-848` _(crash-safety-durability)_  
  - Untested: The existing test only covers the case where in-progress work exists, gets requeued, and decide() returns Execute (count>0 path). The branch where count===0 AND decide() returns Wait — i.e. a second recovery sweep after the first already requeued everything, or recovery called on a run with nothing in-progress — which must return {_tag:'Wait', reason:{_tag:'OrphanRecovery', count:0}} is uncovered (lines 846-847).  
  - Risk: Orphan recovery runs on engine restart after a crash and is expected to be safe to call repeatedly. The no-op branch is the idempotency guarantee: calling recover twice must not re-execute already-requeued tasks or mislabel the wait reason. A regression that returns ExternalTrigger or re-dispatches here would either spin or double-run tasks on every restart, undetected. concurrent-resume-vs-supervisor e2e covers contention but not the count===0 idempotent sweep result shape.  
  - Test: Submit a graph with a single task, drive it to a waiting state (e.g. waiting-event with no in-progress tasks), then call recoverOrphanedTasks() and assert decision === {_tag:'Wait', reason:{_tag:'OrphanRecovery', count:0}}. Separately, after a normal recovery that requeues N tasks, call recoverOrphanedTasks() again with nothing in-progress and assert the OrphanRecovery count:0 wait (proves idempotency).
- **[MEDIUM] Saga onFailure:'fail' fatal path and compensation-itself-fails path are untested** — `packages/scheduler/src/scheduleTasks.js:411-443` _(error-path)_  
  - Untested: scheduleTasks/saga only tests onFailure:'compensate' scheduling the first compensation, and the session-level test only tests successful compensation. Uncovered: onFailure:'fail' (sets fatalError 'Saga <id> failed' immediately, no compensation — line 412-413), the compensationFailed branch where a compensation task itself fails and the saga returns {terminal:false} (lines 424-429), and onFailure:'compensate-and-fail' setting fatalError after successful compensation (lines 440-441). The buildPlanTree test constructs onFailure:'compensate-and-fail' but never schedules it.  
  - Risk: Saga is the VCS/side-effect-rollback primitive. If onFailure:'fail' stops setting fatalError, a failed saga silently completes as success; if 'compensate-and-fail' stops re-raising after compensation, rollback runs but the run is wrongly reported finished; if a failing compensation isn't held as non-terminal, the run can finish with a half-rolled-back side effect (money charged, worktree dirty). None of these would be caught.  
  - Test: scheduleTasks unit: saga onFailure:'fail' with one finished + one failed action -> assert fatalError==='Saga <id> failed' and no compensation in runnable. saga onFailure:'compensate-and-fail' with all compensations finished -> assert fatalError set AND terminal. saga where a compensation task is in state 'failed' -> assert result indicates non-terminal (compensation not complete) and no fatalError yet. Mirror at least 'fail' through makeWorkflowSession and assert decision._tag==='Failed' with code SCHEDULER_ERROR.
- **[MEDIUM] requireStableFinish deadlock-rerender-then-fail and stable-finish rerender are untested** — `packages/scheduler/src/makeWorkflowSession.js:683-709` _(edge-case)_  
  - Untested: The DEPENDENCY_DEADLOCK failure is tested only with requireStableFinish unset (service-branches deadlock test). When requireStableFinish is true, the scheduler must instead give a reactive graph one chance to mount a missing producer: emit ReRender once (recording lastDeadlockSignature), and only fail with DEPENDENCY_DEADLOCK if the same mounted signature recurs (lines 683-693). Likewise the requireStableFinish finish path that emits ReRender once before Finished when the mounted signature changed (lines 704-709) is uncovered.  
  - Risk: This is the guard against spuriously failing/finishing reactive workflows whose producers mount on a later render pass. A regression that fails immediately (skipping the one-rerender grace) would break legitimate conditional/lazy graphs; one that never fails would hang the run forever. The deadlock diagnostic is also the main operator-facing error for deps/needs mistakes.  
  - Test: makeWorkflowSession({requireStableFinish:true}). Submit a graph with a task whose dependsOn points at a not-yet-mounted producer; complete the prerequisite so nothing is in-progress; assert first decision is ReRender. Re-submit the SAME graph (same mountedSignature) and assert the second decision is Failed code DEPENDENCY_DEADLOCK. Separately assert: when the second submit mounts the missing producer (signature changes), it does NOT fail. Add a stable-finish case where the final settle emits ReRender once then Finished on a stable signature.
- **[MEDIUM] continueAsNewEvery quiescence deferral (do-not-tear-down-mid-flight) is not asserted at the unit level** — `packages/scheduler/src/makeWorkflowSession.js:640-657` _(concurrency-race)_  
  - Untested: The happy quiescent case (continueAsNewEvery:1 -> loop-threshold transition at quiescence) is tested. The deferral guard `if (wantsContinueAsNew && (hasInProgress || schedule.pendingExists)) continue;` (lines 644-646) — which suppresses the continue-as-new handoff while sibling tasks are still in-progress or pending — is not directly exercised; coverage shows the continue/defer line region partially uncovered.  
  - Risk: Run-level continue-as-new tears down the run to start a fresh lineage; doing so while sibling tasks are mid-flight loses or duplicates that work. The deferral is the safety interlock for that. A regression that drops the hasInProgress/pendingExists check would issue a ContinueAsNew while a sibling is executing, orphaning it across the lineage boundary. continue-as-new-lineage e2e validates lineage but does not pin the mid-flight deferral decision in isolation.  
  - Test: Construct a graph with a Ralph (continueAsNewEvery:1) whose loop body completes while an UNRELATED sibling task is still in-progress (and/or a pending dependent exists). After completing the loop body, assert the decision is NOT ContinueAsNew (it should be Wait/Execute) — i.e. the handoff is deferred. Then complete/settle the sibling so the run is quiescent and assert the next decision IS the loop-threshold ContinueAsNew transition.
- **[LOW] approvalOnDeny 'skip' and 'continue' branches are completely untested** — `packages/scheduler/src/makeWorkflowSession.js:384-405 (applyApprovalResolution)` _(edge-case)_  
  - Untested: Only the approve path (state->pending, key added to approvals) and the default deny->failed path are asserted (workflowSession-service-branches approvalTimedOut, prune test). The two other deny dispositions are never exercised: approvalOnDeny==='skip' (sets state to 'skipped') and approvalOnDeny==='continue' (sets state to 'finished' AND writes an output row equal to the ApprovalResolution object). Neither is reached by any test (coverage shows lines 391-399 uncovered).  
  - Risk: These are durable HITL decisions. The 'continue' branch records the denial resolution as the task's output, which then flows into downstream tasks and into replay/time-travel snapshots. A regression that (a) fails the run on a 'skip'-policy denial, (b) forgets to record the 'continue' output, or (c) writes the wrong output shape would corrupt every downstream consumer and silently diverge on replay — with zero test signal. The e2e approval-scope-denial case covers scope at integration but not these per-unit state/output transitions.  
  - Test: Build a session with one needsApproval task at descriptor.approvalOnDeny='skip'; submitGraph -> Wait Approval; call approvalResolved(nodeId, {approved:false}); assert decision is Finished/Execute-of-next and getTaskStates().get(key)==='skipped'. Repeat with approvalOnDeny='continue' and assert state is finished AND that the recorded output (via a downstream task that depends on it, or via the Finished result.output) equals the resolution object {approved:false,...}. Also assert the approve path adds to approvals so a later prune/re-mount test (already exists) stays meaningful.
- **[LOW] Compute (non-agent) non-retryable error classification not exercised through the session** — `packages/scheduler/src/makeWorkflowSession.js:158-181 (isRetryableFailure) and 472-475 (isTransientSessionFailure agent gate)` _(error-path)_  
  - Untested: Retry classification is tested only for agent tasks: details.failureRetryable=false and AGENT_CONFIG_INVALID fail without backoff; ordinary AGENT_CLI_ERROR retries (workflowSession-retry). Untested: a NON-agent (compute) task failing with INVALID_OUTPUT / HEARTBEAT_PAYLOAD_NOT_JSON_SERIALIZABLE / HEARTBEAT_PAYLOAD_TOO_LARGE must NOT retry (line 177-179), while the same codes on an agent task SHOULD remain retryable. Also untested: the isTransientSessionFailure 'keep alive for agent' suppression (unhandledFailureDecision lines 472-475) for an exhausted-but-transient agent failure with no continueOnFail — only the parallel-sibling variant is covered.  
  - Risk: Misclassifying a deterministic compute failure (bad output, oversized heartbeat) as retryable wastes the full retry budget on a guaranteed-to-fail task and delays the surfaced error; misclassifying a transient agent stream drop as fatal kills a recoverable run. These are the boundaries of the retry policy and have no direct unit assertion for compute tasks.  
  - Test: Session with a compute descriptor (no agent), retries:3, and taskFailed error code 'INVALID_OUTPUT' -> assert decision is Failed (no RetryBackoff). Same descriptor but with agent set -> assert Wait RetryBackoff. Add a single-agent-task case (no sibling) with retries:0 failing SESSION_ERROR and assert it is held (not Failed) per the isTransientSessionFailure agent gate, distinct from the existing parallel-sibling test.
- **[LOW] submitGraph / hotReloaded buildPlanTree throw paths (NESTED_LOOP, DUPLICATE_ID) -> failedDecision are untested at the session boundary** — `packages/scheduler/src/makeWorkflowSession.js:714-722 and 793-802 (try/catch around markGraph)` _(error-path)_  
  - Untested: buildPlanTree's throws (Nested <Ralph>, Duplicate Ralph id) are tested directly against buildPlanTree, but the session-level try/catch that converts a markGraph/buildPlanTree exception into a Failed EngineDecision (failedDecision('submitGraph'/'hotReloaded')) is uncovered (lines 719-720, 799-800). The catch is the contract that a malformed graph yields a structured Failed decision rather than throwing out of Effect.sync.  
  - Risk: If the catch regresses, an authoring error (e.g. a duplicate Ralph id introduced on a hot reload) throws synchronously out of the session method instead of returning a Failed decision, crashing the engine loop rather than failing the run cleanly. hotReloaded is especially exposed because it runs on live edits.  
  - Test: submitGraph with a graph whose xml nests two <smithers:ralph> with the same id (or nested ralph) and assert the returned decision is {_tag:'Failed'} with a SESSION_ERROR (not a thrown exception). Repeat by first submitting a valid graph then hotReloaded with a duplicate-id graph and assert a Failed decision is returned and the session remains usable.
- **[LOW] approvalTimedOut early-return when task is no longer waiting-approval (already resolved) is untested** — `packages/scheduler/src/makeWorkflowSession.js:751-768` _(concurrency-race)_  
  - Untested: Only the timeout->failed path is tested. The guard at line 757-759 — if the task's state is no longer 'waiting-approval' (it was already approved/denied via a racing approvalResolved), approvalTimedOut must be a no-op that just re-decides — is uncovered. Same shape exists for timerFired's already-fired guard (line 783).  
  - Risk: Approval timeout and the operator's manual decision can race (timeout fires just as the approve RPC lands). Without the guard asserted, a regression could let a late timeout overwrite an already-granted approval as failed — the classic replay-unsafe-approval / double-resolution bug, but in the live timing window. replay-unsafe-approval e2e covers replay re-trigger, not the live timeout-vs-resolve race in the session unit.  
  - Test: Submit a needsApproval task -> Wait Approval. Call approvalResolved(nodeId,{approved:true}) (state becomes pending/in-progress). Then call approvalTimedOut(nodeId) and assert it does NOT transition the task to failed (state stays consistent with the granted approval) and returns a normal decide() result, not a Failed-by-timeout. Mirror for timerFired after the timer already fired.

### scorers (6)

- **[MEDIUM] Custom scorer scores are persisted and aggregated UNCLAMPED / unvalidated (NaN, Infinity, out-of-range)** — `packages/scorers/src/run-scorers.js:123 (row.score = result.score) and src/aggregate.js:62-77` _(api-contract)_  
  - Untested: runSingleScorerEffect copies result.score directly into the DB row with no Number.isFinite / 0-1 clamp. llmJudge.js clamps to [0,1] and coerces non-finite to 0, but createScorer (and therefore schemaAdherence/latency/any user scorer) does NOT. No test feeds an out-of-range (e.g. 5 or -3), NaN, or Infinity score from a createScorer scorer through runScorersBatch -> aggregateScores. I confirmed empirically: a scorer returning {score:5} produces aggregate {mean:5,max:5}; {score:NaN} returns score:NaN and then silently vanishes from aggregation.  
  - Risk: The ScoreResult contract says score is 'between 0 and 1' and downstream gating (e2e case27 'blocks-downstream') compares score >= threshold. A buggy custom scorer emitting 1.5, -0.2, NaN, or Infinity silently corrupts mean/stddev/p50 dashboards and can spuriously pass or fail a threshold gate that protects a destructive node. A NaN score poisons AVG and can make a whole scorer's aggregate row disappear, hiding regressions. This ships undetected because no unit asserts invariant enforcement.  
  - Test: In run-scorers.test.js: register a createScorer returning {score: 5}, {score: -1}, {score: NaN}, and {score: Infinity}; run runScorersBatch with a real in-memory SmithersDb; assert either the persisted/returned score is clamped to [0,1] (and non-finite coerced to 0) OR that a SCORER_INVALID_RESULT error is raised. Then call aggregateScores and assert mean/min/max/p50 stay within [0,1] and no scorer row silently disappears.
- **[MEDIUM] Missing/non-number `score` in a ScoreResult silently drops the row and swallows the DB error (split-brain with ScorerFinished event)** — `packages/scorers/src/run-scorers.js:65-136 (no ScoreResult shape validation; insert error caught by catchAll at lines 170/188)` _(crash-safety-durability)_  
  - Untested: No test covers a scorer that returns a result object WITHOUT a numeric score (e.g. {reason:'x'}). I confirmed: runScorersBatch returns {s:{reason:'no score'}} (score undefined), emits ScorerStarted+ScorerFinished, but the insertScorerResult fails the NOT NULL real `score` column and the error is swallowed by Effect.catchAll, leaving 0 persisted rows. The caller's returned map and the event log both say 'finished/succeeded' while the DB has nothing.  
  - Risk: This is a durability/consistency bug masquerading as success: the event stream and any live UI show ScorerFinished, but the durable record is missing, so on replay/aggregation the score is gone. A scorer author who forgets to return `score` gets no error, no log of the persistence failure surfaced to them, and silently lost data — exactly the kind of silent durability hole this control plane must not have.  
  - Test: runScorersBatch with a createScorer returning {reason:'oops'} (no score) against a real SmithersDb; assert that EITHER a SCORER_FAILED/SCORER_INVALID_RESULT error path runs (ScorerFailed emitted, scorersFailed incremented) OR the row persists with a defined score. Assert event log and DB row count agree (no Finished-event-without-row). Add a variant where insertScorerResult rejects mid-batch and assert the failure is observable (logged/returned), not silently mapped to null indistinguishable from a passing scorer.
- **[LOW] Ratio sampling uses Math.random() — non-deterministic across replay/time-travel** — `packages/scorers/src/run-scorers.js:21-33 (shouldRun, case 'ratio': Math.random() < rate)` _(edge-case)_  
  - Untested: shouldRun's ratio branch is tested only by monkeypatching Math.random to fixed values to hit run/skip (run-scorers.test.js:97-141). There is no test that the run/skip decision is stable across a replay/fork of the same node+attempt. Because the decision is pure Math.random with no seeding off (runId,nodeId,iteration,attempt), replaying a checkpoint can flip a previously-skipped scorer to run (or vice versa).  
  - Risk: Smithers' core guarantee is deterministic replay/time-travel reproducing state exactly. A ratio-sampled scorer that ran on the original execution but is skipped on replay (or runs again, creating a NEW row) breaks reproducibility, double-counts in aggregates, and can change a threshold-gating decision between original and forked runs. This is a replay-determinism gap specific to this unit (the e2e replay cases do not exercise ratio sampling).  
  - Test: Add a test asserting shouldRun (or runScorersBatch) is deterministic for a fixed (runId,nodeId,iteration,attempt): call twice with identical context and assert identical run/skip and identical persisted-row count. If the implementation is meant to be seeded, assert two contexts differing only in attempt can differ but same context always matches. This will fail today and motivate seeding the sampler off the durable identity instead of global Math.random.
- **[LOW] No unique constraint + scorer re-run on retry produces duplicate rows that double-count aggregates** — `packages/db/src/internal-schema/smithersScorers.js (PK is `id` UUID only; no unique on runId/nodeId/iteration/attempt/scorerId) consumed by src/run-scorers.js:113-134 and src/aggregate.js` _(regression-risk)_  
  - Untested: Every scorer insert uses crypto.randomUUID() as id, and the schema has no uniqueness over (runId,nodeId,iteration,attempt,scorerId). No test runs the SAME scorer twice for the same node/attempt (the retry/idempotency scenario) and checks aggregate behavior. Retrying a task re-invokes runScorers with the same identity and appends a second row.  
  - Risk: Node retries are a first-class control-plane behavior. If a node's scorers run, the node fails and retries (or a resume re-runs scoring), aggregateScores will count the score twice, skewing mean/count/p50 and any threshold decision built on them. Because aggregate.test.js only inserts hand-made distinct rows, this real duplication path is invisible to the suite.  
  - Test: runScorersBatch twice with identical ctx (same runId/nodeId/iteration/attempt) for one scorer; then aggregateScores and assert count is 1 (idempotent upsert) rather than 2 — or, if intentional, add a test pinning the documented dedup/latest-wins semantics so a future change can't silently double-count.
- **[LOW] runScorersAsync fire-and-forget: one failing scorer must not abort sibling scorers or lose their rows** — `packages/scorers/src/run-scorers.js:166-173 (runScorersAsync, Effect.all unbounded+discard)` _(concurrency-race)_  
  - Untested: runScorersAsync is only tested with a single scorer (success, persistence-fail-does-not-throw, finished-event). There is no async test with MULTIPLE scorers where one throws, asserting the others still run AND persist their rows. The batch path tests failing+working together (run-scorers.test.js:142-167) but the live/async path — the one used during real execution — does not.  
  - Risk: Live scoring runs concurrently and fire-and-forget; if a thrown scorer or a discard:true semantics could interrupt or drop siblings, live scores would be silently missing during real runs (the most common code path) while batch tests stay green. Isolation under concurrency is exactly the kind of property a high line-coverage number hides.  
  - Test: runScorersAsync with three scorers (one throws synchronously in score, one rejects, one succeeds) against a real in-memory adapter; await persistence settling; assert the two healthy scorers each produced exactly one persisted row and a ScorerFinished event, the failing one produced ScorerFailed + scorersFailed increment, and no sibling row was lost.
- **[LOW] aggregateScores p50/stddev/median edge cases unverified against single-element and degenerate distributions through the real query path** — `packages/scorers/src/aggregate.js:62-101 (computeMedian/computeStddev; scoresByScorer fallback ?? [] at line 63)` _(edge-case)_  
  - Untested: computeStddev returns 0 for length<=1 and computeMedian returns 0 for empty, but the line-63 path where aggRows has a scorer that yields ZERO score rows in the second query (scoresByScorer.get(id) === undefined -> []) producing p50=0,stddev=0 while count>0 is only hit via a hand-built mockAdapter, never via a real DB inconsistency. Single-score real-DB p50 (should equal that score, not 0) is not asserted through aggregateScores end-to-end.  
  - Risk: If the two SQL queries ever diverge (e.g. a WHERE/ORDER mismatch or a NULL score filtered by one query but not the other), a scorer could report count>0 with p50/stddev silently 0, masking real distribution data on dashboards. Low severity because current queries share the WHERE clause, but it is an untested invariant coupling two queries.  
  - Test: Insert a single real score (e.g. 0.42) and assert aggregateScores returns count:1, p50:0.42 (not 0), stddev:0. Add a NULL-score row directly via SQL and assert it is either excluded consistently from BOTH count and the score list (no count/p50 divergence).

### server (8)

- **[HIGH] trusted-proxy auth mode is entirely untested (origin allow-list, trusted-header parsing, default-scope fallback)** — `packages/server/src/gateway.js:3204-3228 (authenticateRequest trusted-proxy branch)` _(security-trust-boundary)_  
  - Untested: No test sets `auth: { mode: 'trusted-proxy' }`. The entire branch is unexercised: the allowedOrigins check, the trustedHeaders triple-destructure (x-user-id/x-user-scopes/x-user-role, lowercased), the scope-string split, and crucially the fallback `scopes = [...(this.auth.defaultScopes ?? ['*'])]` when no scopes header is present.  
  - Risk: trusted-proxy is the production deployment shape behind an SSO/ingress proxy. A regression could (a) skip the origin allow-list and accept forged x-user-* headers from any origin, or (b) silently grant `['*']` scope when the upstream forgot to send x-user-scopes — a full privilege escalation. Because nothing asserts the allow-list rejects a disallowed/missing origin, a broken `allowedOrigins.includes(origin)` check would ship green.  
  - Test: Boot a gateway with mode:'trusted-proxy', allowedOrigins:['https://app.example'], trustedHeaders default. Assert: (1) request with Origin not in list -> 401 UNAUTHORIZED 'Origin is not allowed'; (2) allowed origin + x-user-id/x-user-scopes/x-user-role headers -> connect ok with exactly those scopes/role/userId; (3) allowed origin but NO scopes header -> scopes equal configured defaultScopes (and assert it is NOT silently '*' unless defaultScopes says so); (4) custom trustedHeaders array is honored case-insensitively.
- **[HIGH] JWT verifier rejection branches unexercised: alg-confusion, signature tamper, wrong issuer, nbf-not-active, clock-skew** — `packages/server/src/gateway.js:943-985 (verifyJwtToken)` _(security-trust-boundary)_  
  - Untested: Only a valid HS256 token and an invalid-audience token are tested. Untested branches: header.alg !== 'HS256' (alg-confusion / 'none'/HS384 downgrade) -> 'Unsupported JWT algorithm'; signature mismatch via timingSafeEqual; iss !== config.issuer; nbf with `now + skew < nbf` -> 'JWT is not active yet'; clockSkewSeconds boundary around exp/nbf; missing three segments; non-JSON header/payload.  
  - Risk: verifyJwtToken is the sole gate for jwt-mode auth and is pure logic (cheap to test). An alg-confusion bug (e.g. accidentally accepting alg:'none' or not pinning HS256) is a classic JWT auth bypass; a broken issuer check lets tokens from any HS256 signer in; a broken nbf/skew lets not-yet-valid or just-expired tokens through. None of these would be caught today.  
  - Test: Unit-test verifyJwtToken directly (it is module-internal; export it or test via connect): forge tokens with alg:'none' and alg:'HS384' -> rejected 'Unsupported JWT algorithm'; flip one signature byte -> 'JWT signature verification failed'; wrong iss -> 'JWT issuer did not match'; nbf = now+3600 -> 'JWT is not active yet'; exp = now-1 with clockSkewSeconds=120 -> accepted (within skew) and exp=now-200 -> rejected; token with two segments -> 'JWT must have three segments'.
- **[HIGH] WS upgrade performs no Origin check for token/jwt/none auth modes (cross-origin connect not gated)** — `packages/server/src/gateway.js:2339-2364 (server.on('upgrade')) and 3043-3104 (handleConnect)` _(security-trust-boundary)_  
  - Untested: The upgrade handler only enforces maxConnections; Origin is never checked at the upgrade boundary. allowedOrigins is consulted ONLY inside the trusted-proxy auth branch. There is no test asserting cross-origin behavior of the WS endpoint for the default mode (none) or token/jwt modes. For default `mode: none`, any browser origin completes connect and receives role 'operator' with scopes ['*'].  
  - Risk: A drive-by webpage in a victim's browser can open a WS to a localhost/LAN gateway running with default (no auth) and gain full ['*'] control (launchRun, cancelRun, rewindRun) — a CSRF-style cross-origin takeover. The e2e list mentions 'reject cross-origin PTY upgrades' but that targets the CLI's PTY path; there is no unit/integration assertion that the *gateway's* RPC WS upgrade rejects or constrains a foreign Origin, and no test documents that 'none' mode is intentionally localhost-only.  
  - Test: Drive the WS upgrade with an Origin header set to an unexpected site. Assert the intended policy explicitly: either (a) the upgrade is rejected (e.g. 403) when an allowedOrigins/CSRF guard is configured, or (b) if 'none' mode is deliberately unauthenticated, add a test that pins this and that connect with a hostile Origin yields no privileged scope. At minimum, add a regression test that allowedOrigins is enforced at upgrade for token/jwt modes, not only trusted-proxy.
- **[MEDIUM] Token revocation (revokedAtMs) path is untested** — `packages/server/src/gateway.js:3144-3153 (authenticateRequest token-mode revoked branch)` _(security-trust-boundary)_  
  - Untested: Only `expiresAtMs` is asserted (v1-contract 'expired-token'). The separate `revokedAtMs <= Date.now()` branch returning UNAUTHORIZED 'Token has been revoked' with the refresh hint is never exercised, nor is the case where revokedAtMs is in the future (token still valid).  
  - Risk: Revocation is the operator's emergency kill-switch for a leaked token. If a refactor coalesced expiry/revocation or inverted the comparison, a revoked token could keep working with no failing test. The distinct error message/hint is also part of the RPC contract operators rely on.  
  - Test: token-mode with two grants: one with revokedAtMs = Date.now()-1, one with revokedAtMs = Date.now()+60_000. Assert the past-revoked token -> 401 UNAUTHORIZED message 'Token has been revoked' with details.refresh; the future-revoked token still authenticates successfully.
- **[MEDIUM] Gateway-level duplicate/concurrent startRun for the same runId is unguarded and untested** — `packages/server/src/gateway.js:2768-2877 (startRun), 4207-4233 (launchRun accepts caller-supplied runId)` _(concurrency-race)_  
  - Untested: launchRun lets the caller pass an explicit runId, and startRun unconditionally overwrites runRegistry/activeRuns and spawns a second runWorkflow with no check for an already-active runId. resumeRunIfNeeded guards with activeRuns.has(runId), but the direct launchRun/startRun path does not. No test fires two concurrent launchRun calls (or webhook-start racing a manual launchRun) with the same runId.  
  - Risk: Two engines driving the same runId concurrently is precisely the double-execution hazard a durable control plane must prevent. Even though the engine has a durable lease, the gateway losing track of the first record (overwritten in the Maps) means the first run's abort handle is leaked and cleanup/event attribution can corrupt. A regression that removes the engine lease would also be invisible here. The concurrent-rpc test only covers concurrent READS.  
  - Test: Issue Promise.all of two launchRun calls with the same explicit runId (and separately: a webhook delivery that startRuns while a manual launchRun targets the same id). Assert exactly one engine drives the run (one record in activeRuns, one run.started), the loser returns a conflict/idempotent response, and the abort handle of the surviving run is the one stored — i.e. cancelRun afterward actually aborts the live run.
- **[MEDIUM] Webhook explicit runIdPath targeting a terminal or non-waiting run is not covered** — `packages/server/src/gateway.js:2079-2099 (findMatchingWebhookRuns) and 2049-2072 (runWaitsForSignal)` _(error-path)_  
  - Untested: Tests cover correlation-match delivery and the no-match->start-new-run path, but not the explicitRunId branch where the named run exists yet is finished/failed/cancelled (filtered out at 2084-2088) or is not actually waiting for that signalName/correlationId (runWaitsForSignal returns false). The signalName-mismatch and correlationId-mismatch rejection inside runWaitsForSignal (2063-2068) are also only hit indirectly.  
  - Risk: A webhook that names a specific runId via runIdPath must not deliver a signal to a completed run or wake the wrong run. A regression that dropped the terminal-status guard would resurrect or mis-signal finished runs (a durability/replay-safety violation). Currently nothing asserts the explicit-runId path refuses a terminal target.  
  - Test: Configure a webhook with signal.runIdPath. (1) POST naming a runId whose status is 'finished' -> matchedRunIds empty, started new run (or no-op per config), no signal delivered to the finished run. (2) POST naming a waiting run but with a correlationId that does not match its waitForEvent snapshot -> not matched. (3) POST naming a waiting run with a different signalName -> not matched.
- **[LOW] feedOutOfProcessRunEvents drain/seq-cleanup branches under shared DB are not asserted** — `packages/server/src/gateway.js:2503-2583 (pollOutOfProcessRunEvents / feedOutOfProcessRunEvents)` _(edge-case)_  
  - Untested: gateway.test.jsx asserts the bridge streams persisted events for detached runs and does NOT replay for in-process runs. But specific branches are unasserted: the terminal-run pagination loop (rows.length === PAGE_LIMIT keeps draining), adding a run to outOfProcessEventBridgeDrainedRuns once terminal, and the GC of drainedRuns/lastFedSeq when a runId disappears from listRuns (lines 2526-2535). The transition where a run becomes in-process mid-poll (runRegistry.has -> mark drained, delete lastFedSeq, 2538-2542) is also not pinned.  
  - Risk: These guards prevent (a) replaying the same persisted events twice once a run is adopted in-process (double-delivery to subscribers / wrong stateVersion) and (b) unbounded growth of lastFedSeq/drainedRuns maps for completed runs (a memory leak in a long-lived gateway). A regression in the dedup or GC logic would re-emit events or leak memory without any failing test.  
  - Test: Shared-DB gateway: persist >PAGE_LIMIT (e.g. 600) events for a terminal detached run, run one poll, assert every event delivered exactly once and the run is in drainedRuns. Then simulate the run being removed from listRuns on the next poll and assert its lastFedSeq/drainedRuns entries are cleared. Separately, register a run in runRegistry between polls and assert no persisted-copy replay occurs and lastFedSeq for it is deleted.
- **[LOW] serve.js auth token comparison uses plain === (non-timing-safe) and its header-parsing variants are untested** — `packages/server/src/serve.js:113-129 (auth middleware)` _(security-trust-boundary)_  
  - Untested: serve.test.js asserts a missing token -> 401 and that Bearer / x-smithers-key with the correct token pass. It does not test wrong-token rejection paths thoroughly (e.g. a Bearer header with a near-miss token, an Authorization value without the 'Bearer ' prefix treated verbatim, case variants), and there is no assertion about constant-time comparison. The gateway.js HTTP auth header edge cases ARE tested, but serve.js uses a separate, simpler `===` comparison that is not.  
  - Risk: serve.js is the per-run HTTP control surface; its token check is `smithersKey === authToken` / `token === authToken`, which is timing-variable and a separate code path from the gateway's. A divergence (e.g. an empty configured token, or prefix-handling bug) could weaken auth on the per-run server without any failing test. Lower severity because it is a single-run local surface, but it is still an auth boundary.  
  - Test: With authToken set: assert a wrong Bearer token -> 401; an Authorization value equal to the token WITHOUT 'Bearer ' prefix is accepted (documenting the verbatim behavior); x-smithers-key with a wrong value -> 401. Consider switching the comparison to timingSafeEqual and adding a test that empty/whitespace tokens never authenticate.

### smithers-pack (8)

- **[HIGH] release-content publish/approval gate (quality.ts) has zero unit tests; eval only covers dryRun=true** — `.smithers/lib/release-content/quality.ts (runDeterministicChecks, enforceQualityGate) + .smithers/evals/release-content.jsonl` _(security-trust-boundary)_  
  - Untested: runDeterministicChecks/enforceQualityGate encode the entire 'do-not-publish-unreviewed-or-unsupported-marketing' policy: banned-phrase + superlative detection, claimLedger validation (unknown id → blocker, allowedInMarketing=false → blocker, no sources → blocker, requireClaimLedger with zero claimIds → major), and the publish guards `publish && skip.scoring` (blocker), `publish && !dryRun && skip.approval && !allowUnreviewedPublish` (blocker). The only eval cases (release-content.jsonl) all set dryRun=true/publish=false; the string 'approv' never appears in the eval. The actual publish path (publish=true, dryRun=false) and every blocker branch are unasserted.  
  - Risk: package.json ships `marketing:thread:write` = `--input {dryRun:false,publish:true,allowUnreviewedPublish:true,skip:{approval:true}}` — the exact escape hatch this gate protects. If a refactor flips a boolean (e.g. drops the `!allowUnreviewedPublish` condition, or treats an unknown/disallowed claimId as non-blocking), unsupported or fabricated marketing claims publish to the repo with no test catching it. This is a trust boundary: it gates real, externally-visible side effects.  
  - Test: Pure-logic unit tests on runDeterministicChecks: (1) banned phrase in changelog/tweet/blog → blocker; (2) claimId not in ledger → blocker; (3) claimId with allowedInMarketing=false → blocker; (4) claimId with empty sources → blocker; (5) requireClaimLedger=true + zero claimIds → major; (6) publish && skip.scoring → blocker; (7) publish && !dryRun && skip.approval && allowUnreviewedPublish=false → blocker, and that flipping allowUnreviewedPublish=true removes exactly that blocker. On enforceQualityGate: asserts it THROWS when any blocker present, when score missing while !skip.scoring, and when score.score < minScore. Add eval rows with publish=true/dryRun=false: one denied-approval and one allowUnreviewedPublish asserting publishResult.published correctly true/false.
- **[HIGH] release-content side-effecting publish + approval-marker gate (files.ts) untested: safeJoin traversal guard, publishFiles dryRun/overwrite, hasApprovedMarketingContent** — `.smithers/lib/release-content/files.ts (safeJoin:14, publishFiles:260, hasApprovedMarketingContent:296)` _(security-trust-boundary)_  
  - Untested: safeJoin is a path-traversal guard ('Refusing to write outside repository root') with a subtle prefix check (normalizedRoot with trailing sep) — never tested with `../` or sibling-dir (root + '-evil') inputs. publishFiles returns early on dryRun (no writes) and respects per-channel skip flags + output.overwrite; none of these branches asserted. hasApprovedMarketingContent is the gate release.tsx uses to decide whether a major release may proceed — it returns ok:false on missing marker, unparseable JSON, or approved!==true; none asserted.  
  - Risk: safeJoin is the only thing stopping a malicious/garbled repoRelativePath (e.g. from LLM-produced output paths) from writing outside the repo — a classic LLM-trust-boundary. A regression in the prefix logic (e.g. startsWith(root) without the trailing-sep normalization) would let `/repo-evil/x` pass. hasApprovedMarketingContent failing open (returning ok:true on a parse error) would let an unapproved major release publish. publishFiles ignoring dryRun would write files during a preview.  
  - Test: safeJoin: assert it throws for '../escape', for an absolute path outside root, and for a sibling dir whose name extends root (root='/a/repo', path resolving to '/a/repo-evil'); assert it returns the joined abs path for a normal nested path. publishFiles: with dryRun=true assert published=false and files=[]; with skip.publishChangelog=true assert changelog not written; with overwrite=false over an existing file assert it throws. hasApprovedMarketingContent: tmp dir with no marker → ok:false; marker {approved:false} → ok:false; corrupt JSON → ok:false (fails closed); {approved:true} → ok:true.
- **[MEDIUM] Pack's `test` script runs only 2 of 10 test files — extract-prompt + ship-pipeline + ultragrill tests never execute in CI** — `.smithers/package.json (scripts.test)` _(regression-risk)_  
  - Untested: scripts.test = `bun test ./tests/open-code-review.test.ts ./tests/open-code-review-ui.e2e.test.ts`. Eight test files that exist on disk are excluded: tests/ship-pipeline-run.e2e.test.ts, tests/ship-pipeline-ui.e2e.test.tsx, tests/ultragrill.e2e.test.ts, and the 5 components/extract-prompt/*.test.ts files. Root CI runs `pnpm -r --no-bail test`, so these never run. The gate scripts/check-smithers-test-script.mjs only inspects packages/smithers, not .smithers, so nothing flags the omission.  
  - Risk: Authors wrote real unit tests (PromptCache caching, stakesToThreshold, readLatestScore, rctfCompletenessScorer) and two real e2e suites, but a regression in any of that code ships green because CI never invokes them. Dead test files rot — they may already not compile/pass and nobody knows. This is the meta-blind-spot: the appearance of coverage without execution.  
  - Test: Add a repo gate (extend check-smithers-test-script.mjs or add a new check) that fails if any *.test.* file under .smithers is not matched by the pack's `test` script glob. Simplest fix: change scripts.test to `bun test` (discover all) or explicitly enumerate all 10 files, then ensure they pass on a clean CI box (seed fake agent, skip browser-only). Assert the discovered-test-file set equals the executed set.
- **[MEDIUM] ShipTickets queue discovery + per-ticket feedback/done scoping logic untested (only happy-path superset e2e)** — `.smithers/components/ShipTickets.tsx (discoverTickets:50, parseTitle:42, buildFeedback:82)` _(edge-case)_  
  - Untested: discoverTickets walks ticketsDir (depth>4 cutoff, skips dotfiles/README/non-.md, derives slug via rel.replace(/[/\\]/g,'__')) — slug-collision and nested-dir cases unasserted. buildFeedback scopes validate/review by `${slug}:` nodeId prefix across a SHARED review table and computes done = validationPassed && anyReviewApproved; the prefix filter, the validate===undefined early return, and the rejected-review feedback aggregation are unasserted. The ship-pipeline e2e drives one ticket with an all-approved superset fixture, so the rejection/feedback and multi-ticket-scoping branches never run.  
  - Risk: ShipTickets ships work serially and merges to the base branch. If buildFeedback's slug-prefix filter is wrong (e.g. slug 'a' matching 'ab:review:'), one ticket's rejection could mark a different ticket done, merging unreviewed code. If discoverTickets' slug derivation collides for nested tickets, two tickets share a worktree/branch. These are durability/correctness bugs in the merge gate that the single-ticket happy-path e2e cannot surface.  
  - Test: Unit test discoverTickets against a tmp dir: nested dirs (assert slug uses '__'), a README.md and a dotfile (excluded), depth>4 (excluded), stable id sort. Unit test buildFeedback with a fake ctx exposing outputs.review and outputMaybe: (a) validate undefined → {feedback:null,done:false}; (b) two slugs 'a' and 'ab', a rejected review on 'ab' → assert slug 'a' done stays false and feedback null (no cross-slug leak); (c) validation failed → feedback contains 'VALIDATION FAILED'; (d) validate passed + one approved review → done:true.
- **[MEDIUM] Push-safety fence (audit-burndown / verify-push-safety) logic untested despite being incident-driven** — `.smithers/workflows/audit-burndown.tsx (push-fence/oracle:96-98,265-272) + .smithers/workflows/verify-push-safety.tsx (pushStateClean/verdict)` _(security-trust-boundary)_  
  - Untested: These workflows exist because an unattended run pushed ~11 commits to shared origin/main. The protection is deterministic: a baseline captures origin/main SHA at start; the oracle flags roguePushDetected on any `burndown/*` origin branch or origin/main drift and halts at an approval; verify-push-safety computes pushStateClean = no burndown branches && mainInSync && typecheckGreen && lintGreen. None of this detection logic is unit-tested (no test references either workflow), and it is not in the e2e fault list.  
  - Risk: This is a real, previously-exploited trust boundary: the difference between 'agent commits locally' and 'agent corrupts shared main'. If the drift comparison or the burndown-branch detection regresses (e.g. compares the wrong SHA field, or pushStateClean's && chain drops a clause), a rogue push goes undetected and the run reports done. There is no automated proof the fence actually trips.  
  - Test: Extract the deterministic fence predicate into a pure helper and unit-test it: given baseline origin SHA X, current origin SHA Y!=X → roguePushDetected true; given a non-empty `burndown/*` branch list → true; clean → false. For verify-push-safety, unit-test pushStateClean over a gate fixture matrix (each of originHasBurndownBranches/mainInSync/typecheckGreen/lintGreen toggled) asserting it is true only when all clauses hold. Optionally a faults-style e2e that injects a simulated origin/main drift and asserts the run halts at oracle-fix approval rather than completing.
- **[LOW] VerifiableGoals ticket-file writer untested: frontmatter escaping, index padding, empty-goals failure** — `.smithers/components/VerifiableGoals.tsx (write task:73-111)` _(error-path)_  
  - Untested: The write task throws 'goals task produced no output' when goals is missing, pads index to 4 digits, and emits frontmatter with title via JSON.stringify but slug/dependsOn raw-interpolated. None of: the no-goals throw path, the filename `${index}-${slug}.md` shape, the dependsOn `[a, b]` rendering, or unescaped special chars in slug are tested. ShipTickets.discoverTickets then re-parses these files, so the writer↔reader contract is the queue's integrity boundary and is only exercised by the single-ticket e2e.  
  - Risk: If the writer emits a slug with a slash or colon, or malformed frontmatter, discoverTickets' parseTitle/slug derivation silently drifts and the downstream pipeline ships the wrong or duplicate tickets. The missing-output throw is the only guard against writing an empty queue; if it regresses, the pipeline runs zero tickets and reports success.  
  - Test: Call the write task body with a fake ctx returning two tickets (one slug with a special char, one with dependsOn) into a tmp dir; assert files named 0001-*.md/0002-*.md, frontmatter title JSON-quoted, dependsOn rendered, and that discoverTickets reading the same dir round-trips slug+title. Separately, ctx with no goals output → assert it throws 'goals task produced no output'.
- **[LOW] LoopUntilScored gating logic (pure, branchy) has no test** — `.smithers/components/LoopUntilScored.tsx (until computation:62-65)` _(edge-case)_  
  - Untested: until = hasScore ? currentScore>=threshold : (onPending==='stop'). The four cases — score>=threshold (stop), score<threshold (continue), score null with onPending='continue' (keep looping), score null with onPending='stop' (terminate) — plus the typeof-number guard (NaN/undefined) are unasserted. This is pure logic with no backend dependency.  
  - Risk: This wrapper is the documented reusable score-gating loop primitive (used by extract-prompt). If onPending='continue' ever short-circuits to until=true on a null score, a quality loop exits before producing a passing artifact; if the >= flips to >, a loop never terminates at exactly-threshold and burns iterations/budget. Both are silent correctness/cost bugs.  
  - Test: Render LoopUntilScored (or extract the until computation into a tested pure helper) and assert the Loop's `until` prop: score=0.8 threshold=0.7 → true; score=0.5 → false; score=null onPending='continue' → false; score=null onPending='stop' → true; score=NaN → treated as no-score path. Assert idPrefix produces `${idPrefix}:loop`.
- **[LOW] SqlitePromptCache (durable cache) has no test; upsert/keys/missing-dir paths unverified, sibling caches are tested** — `.smithers/components/extract-prompt/SqlitePromptCache.ts` _(crash-safety-durability)_  
  - Untested: Unlike MemoryPromptCache and MarkdownPromptCache (which have *.test.ts), SqlitePromptCache has no test. Untested: ON CONFLICT upsert (set twice on same key updates not duplicates), get-miss returns undefined, delete, keys(), and lazy db() creating the parent dir + table on first use. Also note table name is string-interpolated into SQL (`${this.table}`) with no identifier validation.  
  - Risk: This is a durable cache that survives restarts; if upsert regresses to plain INSERT it throws on the second write of a key, breaking batch reruns. The interpolated table name is an injection surface if ever fed untrusted input. Its two sibling cache implementations are tested, so the gap is an inconsistency that likely just means this variant was overlooked.  
  - Test: Against a tmp .db path: set(k,v) then get(k) equals v; set(k,v2) then get(k) equals v2 and keys() length is 1 (upsert, not duplicate); get(missing) undefined; delete(k) then get undefined; constructing with a path under a non-existent dir then set() succeeds (db() mkdirs). Add a guard test asserting a table name with quotes/spaces is rejected or safely quoted.

### time-travel (8)

- **[HIGH] timeTravel: VCS-restore failure branch (returns success:false without mutating DB) is never asserted** — `packages/time-travel/src/timetravel.js:140-165` _(error-path)_  
  - Untested: When restoreVcs is true and revertToJjPointer returns { success:false }, timeTravel emits TimeTravelFinished{success:false}, returns early, and MUST NOT delete frames / cancel attempts / reset nodes. Coverage shows lines 144-164 are 0% covered. Every existing timetravel test either passes restoreVcs:false or runs against a jj mock/real repo that succeeds; the failure-then-no-mutation invariant is unverified.  
  - Risk: If a future refactor reorders the DB mutation before the VCS-success check (or drops the early return), a failed jj revert would still truncate frames and reset nodes — destroying durable state while the working copy stays at HEAD. That is exactly the rewind-corruption class this control plane must never ship, and nothing would catch it.  
  - Test: Inject revertToJjPointer (via mock.module or a seam) to return { success:false, error:'boom' }. Call timeTravel(restoreVcs:true) on a seeded run. Assert result.success===false, result.vcsRestored===false, the error propagates, AND that frames/attempts/nodes are byte-for-byte unchanged from before the call (snapshot listFrames/listAttempts/getNode before and after).
- **[MEDIUM] revertToAttempt: VCS revert succeeds but DB frame-deletion failure leaves VCS/DB diverged with no rollback or needs_attention marker** — `packages/time-travel/src/revert.js:38-67` _(crash-safety-durability)_  
  - Untested: revertToAttempt reverts the jj working copy first (line 38), then deletes frames recorded after the attempt start (lines 56-65) OUTSIDE any transaction. There is no test where listFrames/deleteFramesAfter throws (or the process is killed) AFTER the VCS revert has already succeeded. revert.test.js only covers attempt-not-found, missing-jjPointer, and event emission; vcsRestoreSuccess.test.js covers only the all-success path.  
  - Risk: If frame deletion fails after the VCS revert lands, the working copy is rewound but the DB still has the post-cutoff frames/nodes (which remain 'finished'). The run is now durably inconsistent: a resume will replay against a working tree that no longer matches the frame log, and unlike jumpToFrame there is no markRunNeedsAttention, no audit row, and no rollback of the VCS pointer. A ship-blocking divergence could go completely undetected.  
  - Test: Seed a run with an attempt that has a jjPointer and frames after startedAtMs. Inject a failing adapter whose deleteFramesAfter (or listFrames) rejects. Assert: (a) the function surfaces the error rather than returning success:true, (b) either the VCS pointer is rolled back OR the run is flagged needs_attention, and (c) frames are not left half-deleted. Document the divergence as the regression guard.
- **[MEDIUM] timeTravel and retryTask have no single-flight lock — concurrent rewind/retry on the same run can interleave DB mutations** — `packages/time-travel/src/timetravel.js:184; packages/time-travel/src/retry-task.js:156` _(concurrency-race)_  
  - Untested: jumpToFrame acquires acquireRewindLock and rejects a concurrent caller with Busy (tested in jumpToFrame.test.ts and rewindLock-concurrent.test.ts). timeTravel() and retryTask() acquire NO lock and write NO audit/recovery marker. There is no test exercising two timeTravel() calls (or timeTravel racing retryTask, or racing an engine resume) on the same runId concurrently.  
  - Risk: Two overlapping rewinds on one run can read frames, both compute a cutoff, and both run deleteFramesAfter/updateRun, producing lost-update or double-cancel of attempts and a run flipped to 'running' twice with inconsistent node state. The e2e 'concurrent-resume-vs-supervisor' covers the engine resume path, not these time-travel units, so this race is unguarded at the unit level.  
  - Test: Start two timeTravel() promises (and one timeTravel vs one retryTask) on the same seeded runId with an injectable gate that holds the first inside its withTransaction, then assert the final DB state is consistent (no duplicate cancellations, frames truncated exactly once, run status sane). If the intended contract is that callers must serialize, add an assertion that documents/forces that contract (e.g., a Busy rejection) rather than silent interleave.
- **[LOW] retryTask transaction atomicity: mid-transaction failure rollback is untested** — `packages/time-travel/src/retry-task.js:156-203` _(crash-safety-durability)_  
  - Untested: retryTask performs attempt-cancel, output-row delete, node upsert-to-pending, and run status update inside one withTransaction. forkRun (fork-recovery.test.js) and jumpToFrame (rewindRollback.test.ts) both have explicit mid-write rollback tests; retryTask has none. The retry-task.e2e tests only cover the all-success and pre-flight-rejection (node-not-found, running-without-force) cases.  
  - Risk: If updateNode/deleteOutputRow/updateRun throws partway (e.g., a dropped output table, a write conflict), the test suite would not catch a non-atomic outcome — some nodes reset to pending while others stay finished and the run status half-updated, leaving an un-resumable or double-running state. The transaction guarantee is asserted nowhere.  
  - Test: Seed a finished run with several reset-eligible nodes; inject a failing seam in the loop (e.g., drop one node's outputTable so deleteOutputRowEffect throws, or wrap the adapter so updateRunEffect rejects). Assert the whole reset rolled back: node states, attempt states, and run.status are identical to before the failed retryTask call.
- **[LOW] jumpToFrame: terminal audit-write failure in the finally block (turns success into RewindFailed) is uncovered** — `packages/time-travel/src/jumpToFrame.js:998-1013` _(error-path)_  
  - Untested: If updateRewindAuditRow/writeRewindAuditRow throws in the finally block, the code logs 'audit write failed' and, when there is no prior error, manufactures a RewindFailed error even though the durable DB mutation already committed. Coverage shows 998-1011 uncovered. No test injects an audit-write failure after a successful jump.  
  - Risk: This is a surprising failure-after-success: the rewind actually happened (frames truncated, event committed) but the caller receives RewindFailed and may retry or mark the run broken, double-applying or confusing the operator. Worse, the audit row may be left as in_progress, which recoverInProgressRewindAudits would later flip the run to needs_attention even though the rewind succeeded.  
  - Test: Wrap the adapter so the audit client's UPDATE throws only for the terminal update (after the in_progress insert). Run a normal successful jumpToFrame and assert the documented behavior: either it still resolves ok (preferred, since the mutation committed) or it throws RewindFailed AND the audit row is not left in_progress. Pin whichever contract is intended so a regression is caught.
- **[LOW] jumpToFrame: emitEvent in-memory broadcast failure must not fail the RPC — path uncovered** — `packages/time-travel/src/jumpToFrame.js:839-854` _(error-path)_  
  - Untested: After the durable TimeTravelJumped event commits, an optional emitEvent broadcast is attempted; a throw there is caught and only warn-logged (the durable row lets subscribers reconcile by seq). Coverage shows 840-852 uncovered — no test passes an emitEvent that throws.  
  - Risk: If a refactor lets a broadcast failure escape, a rewind that already committed durably would be reported as failed to the caller, breaking the live-streaming/gateway contract that durable state is the source of truth and in-memory broadcast is best-effort.  
  - Test: Call jumpToFrame with emitEvent: async () => { throw new Error('bus down'); } on a seeded run. Assert result.ok===true, the TimeTravelJumped row exists in _smithers_events, and the audit result is 'success' (the broadcast failure is swallowed).
- **[LOW] updateRewindAuditRow: no-fromFrameNo update branch is untested** — `packages/time-travel/src/updateRewindAuditRow.js:27-34` _(happy-path-only)_  
  - Untested: updateRewindAuditRow has two SQL branches: with fromFrameNo (tested in rewindAuditHelpers.test.ts) and without it (lines 27-34, function coverage 62.5%). The without-fromFrameNo update is never exercised directly.  
  - Risk: The else branch writes result+duration without touching from_frame_no. If it regressed (e.g., wrong column list or accidentally nulling from_frame_no), the rate-limit/audit data would silently corrupt the recorded source frame, and no test would notice.  
  - Test: Insert an in_progress audit row with a known fromFrameNo, call updateRewindAuditRow({ id, result:'success', durationMs:5 }) (omitting fromFrameNo), then read back via listRewindAuditRows and assert result/durationMs updated while fromFrameNo is preserved unchanged.
- **[LOW] forkRun does not copy VCS tags to the child, so a later replay(restoreVcs) of the fork has no tag to restore** — `packages/time-travel/src/fork/forkRunEffect.js:78-158; packages/time-travel/src/vcs-version/rerunAtRevisionEffect.js:13-24` _(regression-risk)_  
  - Untested: forkRun copies the snapshot's vcsPointer into the child run row and child snapshot, but writes NO row into _smithers_vcs_tags for the child. replayFromCheckpoint(restoreVcs:true) calls rerunAtRevision against the PARENT runId/frameNo, but no test asserts what happens when a fork is itself replayed with restoreVcs, nor that the child's VCS lineage is recoverable. replay-edges and time-travel-replay only test restoreVcs:false for forks.  
  - Risk: If a user forks a run and then time-travels/replays the fork with VCS restore, rerunAtRevision finds no vcs_tag for the child (it returns restored:false silently) — the working copy is NOT restored even though the call appears to succeed, breaking VCS integrity for nested forks. This silent restored:false is exactly the kind of gap that ships as a 'replay didn't restore my files' bug.  
  - Test: Tag a parent frame's VCS, fork from it, then call replayFromCheckpoint(restoreVcs:true) on the CHILD run with a mocked jj seam. Assert either the child VCS is actually restored (tag lineage copied) or that vcsRestored:false is explicitly surfaced as an error/warning the caller can act on — pin the intended behavior so a silent no-op is caught.

### usage (6)

- **[MEDIUM] refreshIntervalMs per-provider soft TTLs (claude 180s, codex 60s) are never asserted** — `packages/usage/src/getUsageForAccounts.js:14-20` _(edge-case)_  
  - Untested: Coverage flags lines 14-17 uncovered. Existing cache tests only use kimi (default 30s) and the claude HARD floor; no test pins the SOFT refresh interval boundaries: claude-code 180_000, codex 60_000, default 30_000. The boundary condition age < interval (strict <) vs age == interval is also unasserted.  
  - Risk: These intervals exist specifically because "the Claude usage endpoint 429s aggressively below 180s". If someone edits the numbers or flips the comparison (<= vs <), the package could re-probe Claude every 30s and get the account 429-banned, or cache forever. A scheduling boundary regression in the rate-limit-protection logic is exactly the kind of bug that should be unit-pinned.  
  - Test: Seed the cache with a codex report fetchedAt T. At nowMs=T+59_999 assert stale:true (cache hit); at T+60_001 assert a fresh probe (stale:false). Repeat for claude-code at 179_999 vs 180_001 (without --fresh) and default(kimi) at 29_999 vs 30_001. Also test the exact-boundary instant to lock the strict-less-than semantics.
- **[LOW] probeFor provider routing is untested for every real provider (only kimi/default exercised)** — `packages/usage/src/getAccountUsage.js:31-49` _(api-contract)_  
  - Untested: The switch that routes each AccountProvider to its adapter (claude-code->claudeOauthUsage, codex->codexWhamUsage, anthropic-api->anthropicHeaderUsage, openai-api->openaiHeaderUsage, antigravity/gemini-api->googleUsage) is only covered for the kimi and default arms (usage.test.js "kimi degrades to none"). Coverage shows getAccountUsage.js branch lines 33-42 uncovered. A comment says this switch must "mirror accountToProviderEnv in the accounts package" but nothing asserts the mapping.  
  - Risk: A mis-wired case (e.g. routing codex to claudeOauthUsage, or a new provider falling through to default) would send the wrong credentials/User-Agent to the wrong endpoint or silently report "not supported" for a supported provider, and no unit test would catch it. The accounts-package alignment the comment promises is unverified, so a drift between the two switches ships undetected.  
  - Test: For each provider, inject a fetch mock and a temp configDir/apiKey, call getAccountUsage, and assert the URL/headers correspond to that provider's adapter (e.g. anthropic-api hits count_tokens with x-api-key; codex sends ChatGPT-Account-Id; antigravity/gemini-api return the google "none" error without any fetch). Also assert antigravity routes to googleUsage (it is currently never asserted).
- **[LOW] Corrupt/invalid usage-cache file is not tested for graceful recovery (crash-during-write durability)** — `packages/usage/src/usageCache.js:29-39` _(crash-safety-durability)_  
  - Untested: readUsageCache wraps JSON.parse in try/catch and validates version===1 and entries is a non-array object, falling back to an empty cache. Coverage shows lines 34-35 (the catch fallthrough) uncovered. No test feeds it: truncated/corrupt JSON (the realistic outcome of a crash mid-write on a non-atomic FS or a partially-flushed temp), wrong version, entries being an array, or entries being null. The atomic rename path (write tmp -> rename) and its 0600 mode are also not directly asserted.  
  - Risk: This cache lives next to accounts.json under SMITHERS_HOME. A corrupt cache from an interrupted write or disk-full must degrade to a cold cache, never throw, or the whole `smithers usage` / gateway usage panel crashes on startup. Since the package's value is robust accounting across process restarts, the corrupt-cache recovery path is a core durability guarantee that is currently unexercised.  
  - Test: Write a cache file with (a) truncated JSON `{"version":1,"entries":{` , (b) `{"version":2,"entries":{}}`, (c) `{"version":1,"entries":[]}`, (d) `null`; assert readUsageCache returns {version:1,entries:{}} for each and never throws. Separately assert writeUsageCache produces a file with mode 0600 and that a pre-existing tmp file or read-only target is handled (the rename atomicity claim).
- **[LOW] Duplicate account labels collide in the cache (last-write-wins, silent data loss)** — `packages/usage/src/getUsageForAccounts.js:45-66` _(concurrency-race)_  
  - Untested: The cache is keyed solely by account.label (decisions read cache.entries[account.label]; writes do cache.entries[report.accountLabel] = {report}). Nothing asserts behavior when two accounts in the input array share a label but differ in provider/configDir. They would share one cache slot: account B's report overwrites account A's, and on the next run A reads B's cached numbers.  
  - Risk: If labels are not guaranteed unique upstream, one account would display another account's quota/usage — a cross-account accounting leak and a correctness violation in the exact thing this package reports. Even if labels are meant to be unique, there is no assertion or guard, so a duplicate (easy to create) silently corrupts the cache rather than erroring.  
  - Test: Call getUsageForAccounts with two accounts sharing label "work" but different providers (claude-code and codex). Assert the documented/intended behavior: either both produce distinct cache entries (keyed by label+provider) or the function rejects duplicate labels — currently it does neither, so a test pins the collision and forces a decision.
- **[LOW] parseDurationSeconds microsecond units (us/µs) and ns rounding are untested; misparse would shift reset times** — `packages/usage/src/parseDurationSeconds.js:35-37` _(edge-case)_  
  - Untested: Coverage flags lines 35-36 (the us/µs case) uncovered. Tests cover ms, s, m, h and the seconds-as-plain-number path, but never us, µs (note the unicode micro sign), or ns, nor a compound like "1h2m3s4ms5us6ns", nor overflow-ish values. The OpenAI reset header is fed straight into new Date(nowMs + seconds*1000) in parseOpenAiRateLimitHeaders, so a parse bug here produces a wrong absolute resetsAt.  
  - Risk: OpenAI sends Go-duration reset strings; if a sub-second unit is dropped or the unicode µs branch silently fails, the computed reset timestamp drifts and the "resets in" column lies (e.g. shows "now" or a wrong future time). It is low severity because the magnitude is tiny, but the µs unicode branch is genuinely unverified and a regex edit could break it.  
  - Test: Assert parseDurationSeconds("5us")===5e-6, parseDurationSeconds("5µs")===5e-6 (unicode), parseDurationSeconds("100ns")===1e-7, and a compound "1h2m3s4ms" === 3723.004; plus a malformed-unit-order case like "3s2m" to confirm it still parses left-to-right contiguous segments and returns a number, not undefined.
- **[LOW] openaiHeaderUsage: non-429 non-ok response WITH rate-limit headers (returns headers, not none) is untested** — `packages/usage/src/openaiHeaderUsage.js:44-55` _(error-path)_  
  - Untested: openaiHeaderUsage has a distinct branch from anthropicHeaderUsage: on !res.ok it returns source:"none" ONLY when windows.length===0 (line 52-53), otherwise it returns source:"headers" with whatever windows parsed. Tests cover 401 and 200; coverage shows lines 45-50,53,56-57 uncovered. The case of e.g. a 500/400 that still carries x-ratelimit-* headers (return headers) vs one with no headers (return none) is unasserted, as is the 429-with-retry-after path for OpenAI specifically.  
  - Risk: This asymmetry between the two adapters is subtle and easy to break in refactor. If the !res.ok-with-headers branch regresses, OpenAI usage on a degraded-but-rate-limit-emitting endpoint silently becomes "not supported", or conversely a true error is reported as live headers. The OpenAI 429 retry-after formatting is also entirely unexercised.  
  - Test: Mock fetch to return 500 with x-ratelimit-limit-requests/remaining headers; assert source:"headers" and the parsed window. Mock 500 with NO rate-limit headers; assert source:"none" with the "returned 500 with no rate-limit headers" error. Mock 429 with retry-after; assert source:"headers" and error containing "retry after".

### vcs (5)

- **[LOW] workspaceList: `-T` exiting 0 with EMPTY stdout returns [] and never falls back to human parsing** — `packages/vcs/src/jj.js:251 (workspaceList)` _(edge-case)_  
  - Untested: Tests cover -T success with names, the human fallback only when -T EXITS NON-ZERO (jj-workspace.test.js:318 returns exit 2), and the all-fail [] case (exit 1). There is no test for `jj workspace list -T` exiting 0 but printing nothing (or only blank lines). In that branch the code returns [] from the structured path and never reaches the human-output fallback at line 263.  
  - Risk: A jj version that accepts the `-T 'name ++ "\n"'` template but emits no output (template syntax drift, a locale/quoting quirk, or a transient empty result) would make workspaceList report ZERO workspaces while the repo actually has several. Callers (worktree management, cleanup, pointer integrity) could then treat existing agent worktrees as absent — re-creating over live work or skipping cleanup. The masking is silent because exit code is 0.  
  - Test: Fake-jj test: `workspace list -T ...` → echo nothing, exit 0; a subsequent `workspace list` (no -T) → echo real rows, exit 0. Assert workspaceList returns the human-parsed rows (i.e. the empty structured result must trigger fallback), OR if [] is the intended contract, assert it explicitly and add a code comment so the behavior is a decision, not an accident.
- **[LOW] workspaceAdd leaves no assertion on worktree-pointer integrity after all syntax attempts fail** — `packages/vcs/src/jj.js:198 (workspaceAdd)` _(edge-case)_  
  - Untested: workspaceAdd rm's the target path once, then tries up to 4 different `jj workspace add` argv forms in a loop. Tests assert the final {success:false} + a non-empty error string (jj-workspace.test.js:272, jj-real-repo.test.js:203). Nothing asserts the on-disk / jj-metadata state after a multi-attempt failure: whether a partially-created workspace dir or a dangling jj workspace entry is left behind, and whether re-running workspaceAdd recovers (the pre-check forget at line 210 only forgets entries named `${name}:`).  
  - Risk: Worktree pointer integrity is a stated VCS guarantee. If attempt N partially registers a workspace then fails, the loop continues issuing more `add` calls against an inconsistent state, and the next workspaceAdd's stale-forget pre-check may not match the partial entry's exact format — leaving a permanently broken workspace that blocks the agent's worktree. The 'partial state may exist' hint string is returned but the actual partial-state recovery path is untested.  
  - Test: Fake-jj test where the first `add` form creates the dir and registers a workspace entry but exits non-zero (simulating a partial add), later forms also fail. Assert the returned error, then call workspaceAdd again and assert the pre-check forget+rm cleans the partial entry so the retry can succeed (or assert it deterministically fails with a recoverable message). Inject via the workspace-list stdout format to verify the forget pre-check matches the partial entry.
- **[LOW] revertToJjPointer / restore commands have no test for empty or malformed pointer input** — `packages/vcs/src/jj.js:173 (revertToJjPointer)` _(error-path)_  
  - Untested: revertToJjPointer(pointer, cwd) builds `jj restore --from <pointer>` with no validation. The engine passes latest.jjCommitId, which is read from a DB row (restoreWorkspace.js:64) and could be undefined/empty/whitespace if a checkpoint row was written partially (crash mid-write) or migrated. There is no test for pointer === '' / undefined / whitespace, and getJjPointer can legitimately return null which a caller might coerce.  
  - Risk: On crash recovery the pointer comes from durable state that itself may be torn by the crash being recovered from. `jj restore --from ''` could either error usefully or, worse, resolve to a surprising default revset and silently rewrite the working copy to the wrong state — defeating the recovery. The function must fail safe (success:false with a clear error) on a degenerate pointer; this is currently unasserted.  
  - Test: Unit tests: revertToJjPointer('') and revertToJjPointer('   ') against a fake jj that records the argv it received; assert either an early guarded failure (no destructive `restore` issued) or a clean {success:false, error} — and crucially assert the working copy is NOT mutated. Mirror for the real-repo path with an empty pointer.
- **[LOW] No test for concurrent captureWorkspaceSnapshot calls racing on the same working copy** — `packages/vcs/src/jj.js:154 (captureWorkspaceSnapshot)` _(concurrency-race)_  
  - Untested: captureWorkspaceSnapshot does two sequential jj calls: step 1 (`log -r @`) forces a working-copy snapshot, step 2 (`--ignore-working-copy operation log`) reads the latest op id and relies on it describing step 1's snapshot. No test exercises two captureWorkspaceSnapshot invocations (or a capture concurrent with an agent write) interleaving against the same repo. The 'same snapshot' invariant assumes nothing else snapshots between step 1 and step 2.  
  - Risk: The engine can capture durability checkpoints while the agent is still mutating files, and the supervisor/resume paths can run concurrently with a live attempt (cf. the concurrent-resume-vs-supervisor fault case). If a second snapshot or an agent write lands between step 1 and step 2, the operationId returned can belong to a DIFFERENT working-copy state than the commitId — pairing a commitId with a mismatched operationId in the durable checkpoint, which could misdirect later op-log-based restore/forensics. This race is invisible to the current sequential tests.  
  - Test: Real-jj concurrency test: launch N captureWorkspaceSnapshot(dir) Effects concurrently (and/or interleave a file write between the two internal jj calls via a fake-jj that signals a write on the first call). Assert every returned {commitId, operationId} pair is self-consistent (the operationId's recorded commit equals commitId), or document and guard the race. Inject by making the fake `log` step touch a file that the fake `operation log` step inspects.
- **[LOW] runJj inherits the full process environment into the jj subprocess with no scrubbing test** — `packages/vcs/src/jj.js:41 (runJj)` _(security-trust-boundary)_  
  - Untested: runJj builds Command.make(...) with only cwd customized; the spawned jj inherits the parent process env wholesale. No test asserts which environment variables cross into the jj subprocess (e.g. that injected secrets / API keys are or are not present). The secret-injection-no-leak fault case covers the agent sandbox boundary, not the jj helper specifically.  
  - Risk: jj runs hooks and can invoke git, which can run user-configured hooks/credential helpers; whatever env Smithers holds (provider API keys, tokens) is exposed to those child processes. There is no test pinning the trust boundary for jj invocations, so a future change that, say, passes a secrets-laden env or fails to scrub one could leak credentials to git hooks/credential helpers undetected.  
  - Test: Fake-jj test that dumps `env` to a temp file; set a sentinel secret env var in the parent, run runJj, and assert the boundary contract — either the sentinel is intentionally absent (scrubbed) or its presence is an explicit, documented decision. At minimum lock the current behavior so a regression is visible.
