# Codebase Review Remediation — TRD

## Status

**Draft** — 2026-03-22

## Overview

Systematic remediation of issues identified in the [2026-03-20 codebase audit](../superpowers/specs/2026-03-20-codebase-review.md). The audit found **3 P0 critical issues**, **13 P1 high issues**, and **14 P2 medium issues** spanning the server (Express/Bedrock/Redis/Prisma), client (React/Vite), and shared `@coda/core-api` package. Additionally, 5 large files were flagged for splitting and 2 instances of duplicate infrastructure were identified.

Since the audit, **6 issues have been resolved** (3 P1, 4 P2 per the [todos tracker](../superpowers/todos.md)), and 1 P0 (`fail()` masking) appears to have been fixed in code but is not yet marked complete in tracking. This TRD documents the remaining remediation work, prioritized by risk.

**This is a risk management document.** The unresolved P0 issues have direct cost implications (wasted Bedrock API spend), safety implications (masked errors hiding production failures), and reliability implications (new model families silently losing extended thinking).

## Goals

| Priority | Target                              | Timeline                                |
| -------- | ----------------------------------- | --------------------------------------- |
| P0       | Fix all critical issues             | This sprint (1 week)                    |
| P1       | Fix all high issues                 | Over 2 sprints (4 weeks)                |
| P2       | Fix medium issues opportunistically | As touched or bundled with related work |

## Issue Inventory

### P0 — Critical

#### P0-1: No server-side abort on client disconnect

- **File:** `server/src/routes/stream-handler.ts`
- **Status:** Open
- **Description:** When the browser navigates away or the SSE connection drops mid-stream, `converseWithTools()` continues running to completion: all remaining Bedrock API calls, tool executions, Redis writes, and DB persistence proceed for a response no one will consume.
- **Impact:** Direct financial waste. A single abandoned 15-round conversation with extended thinking can cost $0.50-2.00 in Bedrock tokens. Across concurrent users, this compounds. Additionally, tool executions (Snowflake queries, GraphQL calls) consume backend resources for no purpose.
- **Root cause:** The orchestrator (`server/src/ai/orchestrator.ts`) does not accept an `AbortSignal`. The stream handler does not wire `res.on('close')` to any cancellation mechanism.
- **Proposed fix:**
  1. Add an `AbortSignal` parameter to `converseWithTools()` options.
  2. Check `signal.aborted` before each Bedrock API call and before each tool execution round.
  3. Pass `signal` through to the Bedrock provider's `client.send()` call (AWS SDK supports `abortSignal`).
  4. In `stream-handler.ts`, create an `AbortController`, wire `res.on('close', () => controller.abort())`, and pass `controller.signal` to `converseWithTools()`.
  5. On abort, call `persister.fail()` to mark the DB record as abandoned.
- **Effort:** 3-4 hours (orchestrator + stream-handler + provider + tests)
- **Dependency:** Partially overlaps with the [stream module refactor plan](../superpowers/plans/2026-03-20-stream-module.md), but can and should be done independently as a hotfix. The stream module refactor can adopt the signal pattern later.

#### P0-2: `stream-persister.fail()` masks DB errors

- **File:** `server/src/db/coda/stream-persister.ts:56-58, 303-304`
- **Status:** Likely resolved (code fix present, tracking not updated)
- **Description:** Originally, `assertBegun()` threw if `begun` was false. If `persister.begin()` itself threw (e.g., DB connection failure), the catch block in `stream-handler.ts` would call `persister.fail()`, which would throw from `assertBegun()`, masking the original database error. Masked errors are invisible in logs and monitoring — the team would see "begin() must be called first" instead of the actual connection failure.
- **Current state:** The code at line 303-304 now reads `if (!this.begun) return;` — the exact fix prescribed by the audit. However, the [todos tracker](../superpowers/todos.md) still lists this as "(Still open.)". This TRD recommends:
  1. Verify the fix is covered by tests (confirmed: `stream-persister.test.ts` line 466 has `assertBegun guard` tests).
  2. Mark as resolved in the todos tracker.
- **Effort:** 0.5 hours (verification + tracking update only)

#### P0-3: `supportsThinking()` regex is fragile

- **File:** `server/src/ai/providers/bedrock/provider.ts:330-334`
- **Status:** Open
- **Description:** The regex `/claude-(?:sonnet|opus|haiku)-(\d+)/` only matches three model families. When Anthropic ships a new family name (e.g., `claude-nova-5`), thinking silently disables — no error, no log, just degraded output quality. Additionally, `parseInt("4-6", 10)` returns `4` by accident (stops at the hyphen), which works today but is a latent parsing bug.
- **Impact:** Every new Claude model family requires a code change to enable thinking. If the deployment misses this, users get worse responses without any signal that something is wrong. This is a silent degradation — the hardest kind to detect.
- **Proposed fix:**
  1. Short-term: replace the regex with a lookup table or allowlist-based approach. Use the `models` DB table (see "Model capabilities to DB" todo) to store thinking support as a column.
  2. Immediate: broaden the regex to `/claude-[\w]+-(\d+)/` and fix the version parsing to extract the major version correctly (e.g., split on `-`, take the first numeric segment).
  3. Add a log warning when `supportsThinking()` returns `false` for any model ID containing `claude` — makes silent failures visible.
- **Effort:** 2 hours (regex fix + warning log + tests). The DB migration is a separate, larger effort tracked in todos.

### P1 — High

#### P1-1: `toolResults.find()` is O(n^2) in orchestrator loop

- **File:** `server/src/ai/orchestrator.ts:301`, `server/src/ai/selection-emitter.ts:50`
- **Status:** Open
- **Description:** Inside the tool-result processing loop, `allToolResults.find(r => r.toolUseId === toolUse.toolUseId)` performs a linear scan per tool use. With 8-12 tools per round across 15 rounds, this is O(n^2).
- **Proposed fix:** Build a `Map<string, ToolResult>` once per round from the tool results array. Replace `.find()` with `.get()`.
- **Effort:** 1 hour
- **Note:** The original audit also flagged `auto-batch.ts:66` and `stream-handler.ts:232`, but these may have been addressed as part of the file-splits-dedup plan. Verify before fixing.

#### P1-2: `searchSchema()` has no result cache

- **File:** `server/src/ai/tools/graphql/search.ts` (formerly `schema-index.ts:447-490`)
- **Status:** Open
- **Description:** `searchSchema()` iterates approximately 11,500 entries per call with no caching. The same schema queries repeat within a single conversation (e.g., the model searches for "account" multiple times).
- **Proposed fix:** Add an LRU cache (e.g., `lru-cache` package or a simple `Map` with max-size eviction) keyed by the normalized query string. TTL of 5 minutes or until schema re-introspection.
- **Effort:** 2 hours

#### P1-3: `usageByModel` memo recomputes on every render during streaming

- **File:** `client/src/hooks/use-coda-orchestrator.ts:97-119`
- **Status:** Open
- **Description:** During streaming, React re-renders on every chunk. The `usageByModel` computation iterates all messages to aggregate token usage per model — unnecessary work during active streaming when usage data hasn't changed.
- **Current state:** A `useRef` cache (`usageByModelRef`) exists at line 99, but the aggregation logic still runs when messages change during streaming.
- **Proposed fix:** Skip recomputation while `isStreaming` is true. Only recalculate on stream completion.
- **Effort:** 1 hour

#### P1-4: TOCTOU race in `updateConversationMeta`

- **File:** `server/src/cache/conversation-cache.ts:254-273`
- **Status:** Open
- **Description:** The method does `hget` (read existing) -> compare `ifUpdatedAt` -> `hset` (write update) as three separate Redis operations. A concurrent update between the read and write would be silently overwritten. The method's JSDoc acknowledges this: "narrowing the TOCTOU window by keeping the read + compare + write in a single method call" — but the operations are still non-atomic at the Redis level.
- **Proposed fix:** Use a Redis Lua script to perform the read-compare-write atomically:
  ```lua
  local existing = redis.call('HGET', KEYS[1], ARGV[1])
  if not existing then return nil end
  local parsed = cjson.decode(existing)
  if ARGV[3] ~= '' and parsed.updatedAt ~= ARGV[3] then return 'conflict' end
  redis.call('HSET', KEYS[1], ARGV[1], ARGV[2])
  redis.call('EXPIRE', KEYS[1], ARGV[4])
  return 'ok'
  ```
- **Effort:** 2-3 hours (Lua script + tests + migration of existing callers)

#### P1-5: Stale closure in `deleteConversationFn`

- **File:** `client/src/providers/chat-provider.tsx:298-321` (may now be in `use-conversation-mutations.ts`)
- **Status:** Open
- **Description:** The delete handler captures the `conversations` array from its closure. If the user deletes a conversation while another operation is updating the list, the stale closure could cause the UI to revert to an old state.
- **Proposed fix:** Use a functional state updater (`setConversations(prev => prev.filter(...))`) instead of reading from the closure.
- **Effort:** 0.5 hours

#### P1-6: Unbounded `messagesMapRef` growth

- **File:** `client/src/providers/chat-provider.tsx:83` (now referenced in `use-conversation-mutations.ts`)
- **Status:** Open
- **Description:** `messagesMapRef` is a `Map<string, Message[]>` that grows without bound as users open conversations. Each entry stores the full message array for a conversation. In a long session, this can consume significant memory.
- **Proposed fix:** Implement a max-size LRU eviction policy. Keep the most recent N conversations (e.g., 50) in the map. When a new entry is added and the map exceeds the limit, evict the least recently accessed entry.
- **Effort:** 1-2 hours

#### P1-7: Bedrock warmup uses wrong API type

- **File:** `server/src/ai/providers/bedrock/provider.ts:363-367`
- **Status:** Open
- **Description:** The warmup method uses `ConverseCommand` (non-streaming), but production requests use `ConverseStreamCommand` (streaming). Bedrock prompt cache entries are per-API-type, so warmup populates the wrong cache. The first real streaming request still pays a cold-start penalty.
- **Proposed fix:** Switch warmup to use `ConverseStreamCommand` and consume/discard the response stream. The code comment at line 363-366 already documents this limitation — it was a deliberate trade-off, but the cost of consuming a short warmup stream is negligible.
- **Effort:** 1-2 hours (change command + consume stream + tests)

#### P1-8: `resolveModelId` null silently falls back

- **File:** `server/src/routes/stream-handler.ts:116`
- **Status:** Open
- **Description:** When `resolveModelId()` returns `null` (unrecognized model slug), the code silently falls back to the default model with no logging. Users believe they are using a specific model but are actually getting the default.
- **Proposed fix:** Add a `logger.warn()` when fallback occurs, including the rejected slug and the fallback model ID.
- **Effort:** 0.5 hours

#### P1-9: Auto-naming has no rate-limit budget

- **File:** `server/src/routes/stream-handler.ts:294-326`
- **Status:** Open
- **Description:** After the first exchange, the handler fires a second LLM call (`generateConversationTitle`) to auto-name the conversation. This call has no rate-limit budget or token budget cap, and runs against the same Bedrock endpoint. Under load, auto-naming calls compete with user-facing requests.
- **Proposed fix:** Use a lightweight model or lower-priority queue for title generation. Alternatively, add a concurrency semaphore (e.g., max 5 concurrent auto-name calls) and a short max-token limit.
- **Effort:** 2 hours

#### P1-10: `handleStreamSelection` doesn't clean up session

- **File:** `client/src/lib/stream-session.ts:177-193`
- **Status:** Open
- **Description:** When `handleStreamSelection` aborts, it doesn't call `cleanup()`. The session object lingers, potentially holding references to stale state.
- **Proposed fix:** Call `cleanup()` in the abort path.
- **Effort:** 0.5 hours

#### P1-11: RAF callback fires after cleanup

- **File:** `client/src/lib/stream-session.ts:141-151`
- **Status:** Open
- **Description:** A `requestAnimationFrame` callback can fire after `cleanup()` has been called, invoking `setMsgs()` on unmounted or stale state. This can cause React warnings or subtle UI bugs.
- **Proposed fix:** Track the RAF handle and cancel it in `cleanup()` via `cancelAnimationFrame()`. Add a `disposed` guard flag.
- **Effort:** 0.5 hours

#### P1-12: `auth0Middleware` built at module load time

- **File:** `server/src/middleware/auth.ts:78-81`
- **Status:** Likely resolved
- **Description:** Originally, `auth0Middleware` was built at module load time. If configuration wasn't loaded yet, the middleware would be permanently null. The current code uses lazy initialization (`auth0Middleware ??= buildAuth0Middleware()`), which defers construction until first use.
- **Proposed fix:** Verify current lazy initialization is correct and mark as resolved.
- **Effort:** 0.5 hours (verification only)

### P2 — Medium

#### P2-1: Doc says "max 10 rounds" but constant is 15

- **File:** `server/src/ai/orchestrator.ts:19, 74`
- **Status:** Open
- **Description:** The module JSDoc (line 19) says "max 10 rounds" but `MAX_TOOL_ROUNDS` is `15`. Misleading for anyone reading the docs.
- **Proposed fix:** Update the JSDoc to say "max 15 rounds".
- **Effort:** 5 minutes

#### P2-2: `Math.random()` ID in auto-batch

- **File:** `server/src/ai/auto-batch.ts:16`
- **Status:** Open
- **Description:** Uses `Math.random()` to generate IDs, inconsistent with Bedrock's UUID format used elsewhere.
- **Proposed fix:** Replace with `crypto.randomUUID()`.
- **Effort:** 10 minutes

#### P2-3: Synthetic `"(continued)"` placeholder could confuse model

- **File:** `server/src/ai/sanitize.ts:75-79`
- **Status:** Open
- **Description:** When turns are malformed, a synthetic `"(continued)"` text placeholder is inserted. This could confuse the LLM.
- **Proposed fix:** Evaluate dropping malformed turns entirely instead of patching them. Requires analysis of how often this path is hit (add logging first).
- **Effort:** 1-2 hours (analysis + fix)

#### P2-4: Double-sort in `listChatsPaginated`

- **File:** `server/src/cache/conversation-cache.ts:289-292`
- **Status:** Open
- **Description:** Conversations are sorted twice — once isn't needed.
- **Proposed fix:** Remove the redundant sort.
- **Effort:** 15 minutes

#### P2-5: Redundant sort in `computeETag`

- **File:** `server/src/cache/conversation-cache.ts:374-383`
- **Status:** Open
- **Description:** Data is sorted before hashing, but the sort is unnecessary if the input order is deterministic.
- **Proposed fix:** Remove the sort or document why it's needed.
- **Effort:** 15 minutes

#### P2-6: `StreamWriter.error()` sends identical fields

- **File:** `api/src/stream.ts:242`
- **Status:** Open
- **Description:** The error event sends both `error` and `message` fields with the same value, which is redundant.
- **Proposed fix:** Send only `message` (or only `error`), update consumers.
- **Effort:** 30 minutes

#### P2-7: TypeError check uses fragile `.message.includes("fetch")`

- **File:** `api/src/client.ts:264`
- **Status:** Open
- **Description:** Network error detection relies on `error.message.includes("fetch")`, which is brittle across browser/runtime implementations.
- **Proposed fix:** Check `error instanceof TypeError` and/or use `error.name === "TypeError"` combined with absence of a response.
- **Effort:** 30 minutes

#### P2-8: Snake_case naming in `getMessages_api()`

- **File:** `client/src/data-client.ts:309`
- **Status:** Open
- **Description:** Inconsistent naming convention — the rest of the codebase uses camelCase.
- **Proposed fix:** Rename to `getMessagesApi()` or `fetchMessages()`.
- **Effort:** 15 minutes

#### P2-9: `toMessageRecord()` uses `new Date()` not server timestamp

- **File:** `client/src/data-client.ts:357`
- **Status:** Open
- **Description:** Client-side timestamp may differ from server time, causing ordering inconsistencies.
- **Proposed fix:** Use the server-provided timestamp from the API response.
- **Effort:** 30 minutes

#### P2-10: `evictStale()` unnecessary cursor traversal

- **File:** `client/src/offline/coda-store.ts:454-457`
- **Status:** Open
- **Description:** Iterates all entries with a cursor to find stale ones when a direct key lookup would suffice.
- **Proposed fix:** Use `IDBKeyRange` or batch delete by known stale keys.
- **Effort:** 1 hour

#### P2-11: `trimMessages()` not atomic with `putMessages()`

- **File:** `client/src/offline/coda-store.ts:293-319`
- **Status:** Open
- **Description:** The trim and put operations are not in the same IndexedDB transaction, creating a window where data can be inconsistent.
- **Proposed fix:** Combine into a single read-write transaction.
- **Effort:** 1 hour

### File Splits (from audit)

| File                                          | Lines | Recommendation                                              | Status                                               |
| --------------------------------------------- | ----- | ----------------------------------------------------------- | ---------------------------------------------------- |
| `api/src/client.ts`                           | 720   | Split into `ApiClient`, `StreamQueryRunner`, `RetryFetcher` | **Deferred** — well-organized, low benefit           |
| `client/src/hooks/use-coda-orchestrator.ts`   | 602   | Extract `useStreamCallbacks`, `useMessageHandlers`          | **Deferred** — tight coupling makes extraction risky |
| `server/src/ai/tools/graphql/schema-index.ts` | 507   | Split into `introspect.ts`, `search.ts`, facade             | **Done**                                             |
| `client/src/offline/coda-store.ts`            | 469   | Split into `MessageStore`, `ChatStore`, `BlobStore`         | **Deferred** — IndexedDB transactions span stores    |
| `client/src/providers/chat-provider.tsx`      | 427   | Extract `useConversationMutations`                          | **Done** (file exists)                               |

## Architecture Impact

### Architectural changes required

- **P0-1 (abort on disconnect):** Requires threading `AbortSignal` through the orchestrator, provider, and tool execution pipeline. This touches the core request lifecycle. The orchestrator's `converseWithTools()` function signature changes (new optional parameter), and the Bedrock provider must pass the signal to `client.send()`. Not a breaking change — the signal is optional — but it touches the critical path.
- **P1-4 (TOCTOU race):** Introduces Redis Lua scripting to the conversation cache layer. First use of Lua in the codebase. Requires establishing a pattern for script management (inline vs. `EVALSHA` + preload).
- **P1-6 (unbounded map):** Introduces LRU eviction in the client state layer. Changes the contract of `messagesMapRef` from "all loaded conversations" to "most recent N conversations." Any code that assumes all conversations are in the map will need to handle cache misses.

### Point fixes (no architectural impact)

All other issues are localized fixes: regex changes, logging additions, sort removals, naming corrections, and guard clauses. These can be merged independently without coordination.

## Alternatives Explored

### Fix in place vs. rewrite

**Decision: Fix in place.** The codebase is fundamentally sound. The audit found no design-level flaws — issues are implementation details (missing abort support, non-atomic Redis ops, brittle regex). A rewrite would introduce regression risk with no structural benefit.

### Incremental vs. big-bang

**Decision: Incremental.** Each fix is independently testable and deployable. P0 fixes ship as hotfixes. P1 fixes land in feature branches. P2 fixes are bundled with nearby work. This minimizes blast radius and allows rollback per fix.

### Regex fix vs. DB-driven model capabilities

**Decision: Both, phased.** The immediate P0-3 fix broadens the regex and adds logging. The longer-term solution (moving `supportsThinking` to the `models` DB table) is tracked separately in todos as "Model capabilities to DB" and should be implemented as a follow-up. The regex fix buys time; the DB solution is the correct long-term answer.

## Cost Analysis

### Engineering effort

| Priority  | Issue count (remaining)              | Estimated effort |
| --------- | ------------------------------------ | ---------------- |
| P0        | 2 (1 resolved, 1 needs verification) | 5-6 hours        |
| P1        | 10 (3 resolved)                      | 12-16 hours      |
| P2        | 11 (4 resolved)                      | 6-8 hours        |
| **Total** | **23 remaining**                     | **23-30 hours**  |

### Cost of NOT fixing P0s

- **P0-1 (no abort):** Each abandoned stream wastes $0.15-2.00 in Bedrock tokens depending on conversation length and thinking budget. With 50+ concurrent users, even a 5% abandon rate translates to $50-200/month in wasted API spend. Under load spikes, abandoned requests also consume Bedrock concurrency quota, potentially causing throttling for active users.
- **P0-3 (fragile regex):** The next Anthropic model family release silently disables extended thinking for all users on that model. Support tickets follow. The fix takes 5 minutes once identified, but detection could take days if no one notices the quality degradation.

## Performance Analysis

### Measurable improvements from P1 fixes

- **P1-1 (Map lookup):** Replaces O(n^2) `find()` with O(1) `Map.get()`. For a 15-round conversation with 8 tools per round (120 tool results), this eliminates ~14,000 comparisons. Measurable in orchestrator traces as reduced per-round overhead.
- **P1-2 (search cache):** LRU cache on `searchSchema()` eliminates redundant iterations over 11,500 entries. Within a single conversation, the model frequently re-searches the same terms. Expected 50-80% cache hit rate, saving 5-10ms per cached call.
- **P1-3 (usage memo):** Eliminates per-chunk recomputation of `usageByModel` during streaming. For a response with 500 chunks, this prevents 500 unnecessary iterations over the full message array. Reduces React render time during streaming.

## Scaling Characteristics

### Issues that worsen at scale

| Issue                    | Scaling behavior                                                                                 | Threshold                               |
| ------------------------ | ------------------------------------------------------------------------------------------------ | --------------------------------------- |
| P0-1 (no abort)          | Linear with concurrent users. More users = more abandoned streams = more wasted spend.           | Already problematic at current scale    |
| P1-1 (O(n^2) find)       | Quadratic with tools-per-round. Adding more tools (current roadmap) makes this worse.            | Noticeable above 10 tools/round         |
| P1-2 (no search cache)   | Linear with schema size. As Snowflake schemas grow, each search gets slower.                     | Approaches 50ms at 50k entries          |
| P1-6 (unbounded map)     | Linear with session duration. Power users with 100+ conversations accumulate significant memory. | Noticeable after 2+ hours of active use |
| P2-10 (cursor traversal) | Linear with IndexedDB store size. Grows with offline message history.                            | Noticeable above 10k messages           |

## Breakdown Points & Mitigations

### What happens if P0s are not fixed

**P0-1 — Bedrock cost waste escalates:**
At current usage, wasted spend from abandoned streams is tolerable but growing. When the product launches to a broader user base (planned Q2 2026), the waste scales linearly. A 10x increase in users with the same 5% abandon rate means $500-2,000/month in wasted Bedrock tokens. Beyond cost, abandoned requests hold Bedrock concurrency slots, increasing p99 latency for active users during peak hours.

**P0-3 — Silent thinking degradation on model updates:**
Anthropic's model release cadence is approximately quarterly. Each release that introduces a new family name (not just a version bump) will silently disable thinking. The failure mode is insidious: responses get shorter and less thoughtful, but no errors are logged. Users may not report it because they attribute quality changes to "the AI being inconsistent." The team discovers the issue days or weeks later via quality metrics, if at all.

### Mitigation if fixes are delayed

- **P0-1:** Add Bedrock cost alerting (CloudWatch) to detect spend anomalies. This doesn't fix the waste but makes it visible.
- **P0-3:** Add a startup check that logs the thinking-support status of all configured models. Review after each model addition.

## Decision Log

| Decision                                                                       | Rationale                                                                                                                                                                                                 |
| ------------------------------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| P0 classification for abort-on-disconnect                                      | Direct financial impact (wasted Bedrock spend) and resource contention under load. No workaround exists short of the fix.                                                                                 |
| P0 classification for error masking                                            | Masked errors in the persistence layer can hide database outages from monitoring. **Update:** Code review indicates this may already be fixed — needs verification.                                       |
| P0 classification for fragile regex                                            | Silent quality degradation with no error signal. The failure mode (thinking silently disabled) is harder to detect than an explicit error.                                                                |
| P1 for TOCTOU race                                                             | The race window is narrow (read-compare-write in one method call) and the consequence is a lost title update, not data corruption. Low frequency in practice but wrong in principle.                      |
| Defer file splits for `client.ts`, `use-coda-orchestrator.ts`, `coda-store.ts` | Per the [file-splits-dedup plan](../superpowers/plans/2026-03-20-file-splits-dedup.md), analysis showed these splits would add indirection without meaningful benefit. Revisit if the files grow further. |
| Incremental rollout over big-bang                                              | Each fix is independently testable. Bundling risks delays and increases rollback blast radius.                                                                                                            |

## Dependencies

### Internal dependencies between fixes

- **P1-10 and P1-11** (stream-session cleanup + RAF guard) should be fixed together — both are in `stream-session.ts` and relate to lifecycle management.
- **P1-5 and P1-6** (stale closure + unbounded map) are both in the chat-provider / conversation-mutations area and can be bundled.

### Dependencies on planned work

- **P0-1 (abort)** partially overlaps with the [stream module refactor](../superpowers/plans/2026-03-20-stream-module.md), which restructures the SSE pipeline. However, the abort fix should **not** wait for the stream module — it's a hotfix. The stream module can adopt the `AbortSignal` pattern when it lands.
- **P0-3 (regex)** is a stopgap until "Model capabilities to DB" (tracked in [todos](../superpowers/todos.md)) moves `supportsThinking` into the database. The regex fix and the DB migration are independent — the regex fix provides immediate safety, and the DB migration provides the long-term solution.
- **P1-4 (Lua script)** is self-contained but establishes a pattern. If the [collections consistency plan](../superpowers/plans/2026-03-21-collections-consistency.md) also needs atomic Redis operations, align on the Lua scripting approach first.

## Testing Strategy

### Per-fix testing

Every fix requires:

1. **Unit tests** covering the fixed behavior and regression against the original bug.
2. **Existing test suite** must pass (CI gate).

### Specific test requirements

| Fix               | Test approach                                                                                                                      |
| ----------------- | ---------------------------------------------------------------------------------------------------------------------------------- |
| P0-1 (abort)      | Mock `res.on('close')` to fire mid-stream. Assert Bedrock `send()` is not called after abort. Assert `persister.fail()` is called. |
| P0-3 (regex)      | Parameterized tests with current models, hypothetical new family names, and edge cases (`parseInt` parsing).                       |
| P1-1 (Map lookup) | Existing orchestrator tests cover correctness. Add a benchmark assertion if needed (optional).                                     |
| P1-4 (Lua script) | Integration test against Redis (already using ioredis-mock in tests). Test concurrent updates to verify atomicity.                 |
| P1-6 (LRU map)    | Unit test that adding entry N+1 evicts the least-recently-used entry.                                                              |

### Regression suite

After all P0 fixes land, run the full functional test suite (`server/src/__tests__/functional/`) which exercises the complete stream-handler -> orchestrator -> provider pipeline.

## Rollout Plan

### Phase 1: P0 hotfixes (this sprint)

1. **P0-2 verification** — Confirm the `fail()` fix is correct and tested. Update tracking. (Day 1)
2. **P0-3 regex fix** — Broaden regex, add warning log, add parameterized tests for new model families. (Day 1-2)
3. **P0-1 abort on disconnect** — Thread `AbortSignal` through orchestrator and provider. Wire to `res.on('close')`. (Day 2-4)
4. **Deploy** P0 fixes together after full test suite passes.

### Phase 2: P1 fixes (sprints 2-3)

Bundle P1s by area to minimize context switching:

- **Server batch:** P1-1 (Map lookup), P1-2 (search cache), P1-7 (warmup API), P1-8 (model fallback logging), P1-9 (auto-name budget)
- **Client batch:** P1-3 (usage memo), P1-5 (stale closure), P1-6 (LRU map), P1-10 (session cleanup), P1-11 (RAF guard)
- **Redis batch:** P1-4 (Lua script for TOCTOU)
- **Verification:** P1-12 (auth0 lazy init — confirm and close)

### Phase 3: P2 opportunistic (ongoing)

P2 fixes are picked up when:

- A developer is already modifying the affected file (e.g., P2-1 doc fix when touching orchestrator)
- A P2 fix is trivial and can be included in a nearby PR (e.g., P2-2 `randomUUID()`)
- A P2 issue causes a user-reported bug (e.g., P2-9 timestamp mismatch)

## Open Questions

1. **P0-2 status discrepancy:** The code appears to already have the fix (`if (!this.begun) return` at line 304), but the todos tracker says "(Still open.)". Is there a subtlety being missed, or is this simply a tracking oversight? Needs verification by the author of the fix.

2. **P1-4 Lua script approach:** Is `EVAL` acceptable for the conversation cache, or should we use `EVALSHA` with script preloading for performance? This is the first Lua usage in the codebase — establishing the pattern matters.

3. **P1-7 warmup trade-off:** The current `ConverseCommand` warmup was a deliberate choice (documented in code comments). Switching to `ConverseStreamCommand` means consuming a full response stream during warmup. Is the cache-hit benefit worth the added complexity? Need to measure the actual cold-start penalty.

4. **P1-9 auto-naming model:** Should auto-naming use a cheaper/faster model (e.g., Haiku) instead of the conversation model? This would reduce both cost and contention, but requires testing title quality.

5. **P0-1 abort granularity:** Should abort cancel only the Bedrock call, or also in-flight tool executions (Snowflake queries, GraphQL calls)? Canceling tool executions is more complex but saves more resources.