# 03 — response-parsing-storage (implementation plan) > **Design authority**: `05-clean-flowcharts.md` §3.7 (clean diagram + deletion list at lines 295–317), Part 1 bullshit items #20–#23 (lines 38–41), Part 2 decision **D5** (lines 77). This plan translates §3.7 into concrete edits. Where the audit disagrees with verified code, the live-file citations win and are called out. ## Dependencies - **Upstream** — `02-sqlite-persistence`. The sibling plan introduces a `UNIQUE(session_id, tool_use_id)` constraint on `pending_messages` and replaces the 30 s in-memory dedup window with `INSERT … ON CONFLICT DO NOTHING`. *This plan does not touch `pending_messages` schema, but the sibling's `markFailed` contract (`UPDATE … SET status='failed'`) must remain intact — parser-level failure marking continues to go through `PendingMessageStore.markFailed(messageId)` at `src/services/sqlite/PendingMessageStore.ts:349`.* Cite: 02-sqlite-persistence Phase 2 (UNIQUE-constraint phase). - **Downstream** — `07-session-lifecycle-management`. That plan owns `RestartGuard` evolution and the one-reaper timer. **Critical coupling**: today `RestartGuard` (`src/services/worker/RestartGuard.ts:12–70`) exposes only `recordRestart()`, `recordSuccess()`, and read-only counters — **there is no `recordFailure()` method**. The audit's D5 claim "RestartGuard already exists for repeated failures" is half-true: it covers process-restart loops, not per-message parse failures. Two legitimate options: 1. (preferred) Let parse-failure propagate via `PendingMessageStore.markFailed` only. Session exits through the existing idle path; on the next summarize or observation attempt the session is re-initialised. If parsing fails repeatedly enough to crash the SDK subprocess, `RestartGuard.recordRestart()` is the thing that trips — already wired via existing restart paths. No new RestartGuard surface area required. 2. (alt) Add `session.recordFailure(reason)` as a thin helper that logs + calls `markFailed` for each `processingMessageIds` entry. Still no RestartGuard API changes. **This plan adopts option (1)**: no new methods on RestartGuard. The flowchart box "session.recordFailure()" from §3.7 resolves to the block of code that marks all `processingMessageIds` as `'failed'` in `pending_messages` — identical shape to today's non-XML early-fail branch at `ResponseProcessor.ts:102–106`, but reached through the single `parseAgentXml` return path. See the `07-session-lifecycle-management` plan for any RestartGuard API additions; do not add them here. ## Verified facts (pinned to files) | # | Fact | Source | |---|---|---| | V7a | `coerceObservationToSummary` is a private fn used twice inside `parseSummary`. | `src/sdk/parser.ts:222` (def), `:152` + `:197` (call sites) | | V7b | Non-XML early-fail branch lives at lines 87–108. | `src/services/worker/agents/ResponseProcessor.ts:87–108` | | V7c | Consecutive-summary-failures circuit breaker lives at lines 176–200. | `src/services/worker/agents/ResponseProcessor.ts:176–200` | | V7d | `consecutiveSummaryFailures` field on `ActiveSession`. | `src/services/worker-types.ts:53` | | V7e | `consecutiveSummaryFailures` is also **read** by `SessionManager.queueSummarize` at line 340 to short-circuit. That site must be deleted too — the original Phase 3 draft in `06-implementation-plan.md` did not list it. | `src/services/worker/SessionManager.ts:340–346` | | V7f | `MAX_CONSECUTIVE_SUMMARY_FAILURES` constant in `src/sdk/prompts.ts:21` is imported by both `ResponseProcessor.ts:16` and `SessionManager.ts` (via prompts import). Delete the constant and both imports. | `src/sdk/prompts.ts:21` | | V7g | Pending-message FAILED state literal is **`'failed'`** (lowercase). CHECK constraint: `status IN ('pending','processing','processed','failed')`. `markFailed(messageId)` is the official API. | `src/services/sqlite/PendingMessageStore.ts:22`, `:349`, `:369`; `src/services/sqlite/migrations/runner.ts:533`; `src/services/sqlite/SessionStore.ts:565` | | V7h | RestartGuard has no `recordFailure()` method. Public surface: `recordRestart()`, `recordSuccess()`, `restartsInWindow`, `windowMs`, `maxRestarts`. | `src/services/worker/RestartGuard.ts:1–70` | | V7i | Prompts already mandate `` root tag for summary turns ("you MUST wrap your ENTIRE response in `...` tags", "The ONLY accepted root tag is ``"). `` is recognised by the parser (`parser.ts:124`) but is **not** documented in `buildSummaryPrompt` as a valid alternative. Prompt must be updated (Phase 1b) so the D5 contract is actually printed to the agent. | `src/sdk/prompts.ts:153–174`; `src/sdk/parser.ts:124` | | V7j | Atomic TX boundary is `sessionStore.storeObservations(...)` (single call, internal BEGIN/COMMIT). Do not split it. Today it wraps observations + optional summary in one transaction. | `src/services/worker/agents/ResponseProcessor.ts:149–164`, `src/services/sqlite/observations/store.ts` (module) | | V7k | `parseSummary` accepts `coerceFromObservation: boolean = false`. All coercion is gated on this flag — it is `true` only when `summaryExpected` (derived from `SUMMARY_MODE_MARKER` substring match) is true. | `src/sdk/parser.ts:122`, `ResponseProcessor.ts:75–81` | ## Concrete target signatures ```ts // src/sdk/parser.ts — replaces parseObservations + parseSummary + coerceObservationToSummary export type ParseFailureReason = 'no_xml' | 'missing_summary' | 'malformed'; export interface ParsedAgentOutput { observations: ParsedObservation[]; summary: ParsedSummary | null; skipSummary: boolean; } export type ParseResult = | { valid: true; data: ParsedAgentOutput } | { valid: false; reason: ParseFailureReason }; export function parseAgentXml( text: string, opts: { requireSummary: boolean; correlationId?: string; sessionId?: number } ): ParseResult; ``` Failure semantics (no coercion, per D5): - `text.trim()` is non-empty, no ``/``/`` and no `` → `{valid:false, reason:'missing_summary'}`. - Any regex match with empty sub-tag payload where `requireSummary` → `{valid:false, reason:'malformed'}`. - Otherwise → `{valid:true, data:{observations, summary|null, skipSummary}}`. ## Phases ### Phase 1 — Write `parseAgentXml` in `src/sdk/parser.ts` **(a) What to implement** 1. Copy `extractField` from `src/sdk/parser.ts:267–276` and `extractArrayElements` from `:282–305` verbatim into the new module layout. These remain private helpers. 2. Copy the observation-extraction loop body (field extraction, type validation, ghost-obs filter) from `src/sdk/parser.ts:40–108` into a private `extractObservations(text, correlationId)` that returns `ParsedObservation[]`. No behaviour change. 3. Copy the summary-extraction happy path (skip_summary check at `:124–133`, `` regex at `:136–137`, field extraction at `:164–169`, false-positive guard at `:191–214`) into a private `extractSummary(text, sessionId)` that returns `{ summary: ParsedSummary | null; skipSummary: boolean; malformed: boolean }`. **Delete the two `coerceFromObservation` branches at `:151–158` and `:196–203` — they do not survive.** 4. Delete `coerceObservationToSummary` (`src/sdk/parser.ts:222–259`, 38 lines) outright. 5. Write the public `parseAgentXml(text, opts)` that: - Computes `observations = extractObservations(text, opts.correlationId)`. - Computes `{ summary, skipSummary, malformed } = extractSummary(text, opts.sessionId)`. - Returns `{valid:false, reason:'no_xml'}` if `text.trim()` && `observations.length === 0` && `!summary` && `!skipSummary` && `!/||`; skip-summary token recognised at parser.ts:124). V7k (coerceFromObservation gating on `summaryExpected`). **(c) Verification** - `grep -n "coerceObservationToSummary" src/` → 0 hits. - `grep -n "parseObservations\|parseSummary\b" src/` → 0 hits outside `parser.ts` itself; inside `parser.ts` only the private helpers. - Unit test: `parseAgentXml('', {requireSummary:false})` → `{valid:true, data:{observations:[], summary:null, skipSummary:false}}` (empty string is not `no_xml`; trim is empty). - Unit test: `parseAgentXml('Error: auth token expired', {requireSummary:true})` → `{valid:false, reason:'no_xml'}`. - Unit test: agent returns `xt` with `requireSummary:true` → `{valid:false, reason:'missing_summary'}` (no coercion to summary). - Unit test: `` with `requireSummary:true` → `{valid:true, data:{observations:[], summary:null, skipSummary:true}}`. - Unit test: `r…` → `{valid:true, data:{…, summary:{…}, skipSummary:false}}`. **(d) Anti-pattern guards** - **Guard C (silent fallback)**: Coercion is *deleted*, not relocated. `grep -n "coerce" src/sdk/parser.ts` → 0 hits. - **Guard D (facades)**: `parseObservations` + `parseSummary` collapse to a single `parseAgentXml`. Two public fns → one. - **Guard A (invent APIs)**: No new classes. Pure function returning a discriminated union. No `ParserValidator`, no `SummaryCoercer`, no base class. --- ### Phase 1b — Update agent contract in `src/sdk/prompts.ts` **(a) What to implement** — Extend `buildSummaryPrompt()` at `src/sdk/prompts.ts:140–175` (the return-value template) so it explicitly permits `` as an alternative when there is literally nothing to summarise. Current text says "The ONLY accepted root tag is ``" (`:155`), which is incompatible with the parser's `` recognition (`parser.ts:124`) and incompatible with the D5 contract ("`` or ``"). Proposed insertion, directly after the existing line `:173`: ``` • If (and ONLY if) there is no work to summarise, you may return as the sole root tag instead of . Any other response is a protocol violation and the session will fail. ``` Also delete the export `MAX_CONSECUTIVE_SUMMARY_FAILURES` at `src/sdk/prompts.ts:21` and its JSDoc at `:17–20`. The constant is unused after Phase 2 + Phase 3. **(b) Docs** — §3.7 deletion list ("agent must return `` or ``", line 311). Part 2 D5 (line 77). V7i. **(c) Verification** - `grep -n "MAX_CONSECUTIVE_SUMMARY_FAILURES" src/` → 0 hits. - Manual diff of generated summary prompt shows the skip-summary clause. - Existing prompt-mandate text (`:153`, `:155`, `:173`) preserved so the normal-case contract stays strict. **(d) Anti-pattern guards** - **Guard C**: The contract is now self-describing — no silent downstream coercion needed because the agent is told the protocol explicitly. --- ### Phase 2 — Replace parse path in `ResponseProcessor.ts` **(a) What to implement** 1. Replace the import at `src/services/worker/agents/ResponseProcessor.ts:15` with `import { parseAgentXml, type ParsedObservation, type ParsedSummary } from '../../../sdk/parser.js';`. Delete `MAX_CONSECUTIVE_SUMMARY_FAILURES` from the `:16` import (keep `SUMMARY_MODE_MARKER`). 2. Replace `processAgentResponse` body at `:69–108`: - Keep `:62–67` (lastGeneratorActivity + conversationHistory append). - Compute `summaryExpected` exactly as today (`:75–79`). - Replace `:70` and `:81` (two separate parse calls) with a single call: ```ts const parsed = parseAgentXml(text, { requireSummary: summaryExpected, correlationId: session.contentSessionId, sessionId: session.sessionDbId, }); ``` - Replace the non-XML early-fail block `:83–108` (26 lines) with: ```ts if (!parsed.valid) { const preview = text.length > 200 ? `${text.slice(0, 200)}...` : text; logger.warn('PARSER', `${agentName} returned invalid response (${parsed.reason}); marking messages as failed`, { sessionId: session.sessionDbId, reason: parsed.reason, preview, }); const pendingStore = sessionManager.getPendingMessageStore(); for (const messageId of session.processingMessageIds) { pendingStore.markFailed(messageId); } session.processingMessageIds = []; return; } const { observations, summary } = parsed.data; ``` - Everything at `:110–174` stays unchanged (normalize, ensureMemorySessionIdRegistered, STORING log, labeledObservations, atomic TX, STORED log, lastSummaryStored) — the single-TX invariant is preserved. 3. **Delete the circuit-breaker block `:176–200`** (25 lines) entirely. After deleting, `:202` (claim-confirm) runs immediately after `:174` (lastSummaryStored). 4. No changes to `:202–241` (claim-confirm, restartGuard.recordSuccess, Chroma sync, SSE broadcast, cleanup). 5. **(Preflight edit 2026-04-22 — reconciliation C6)** Emit `summaryStoredEvent` when a summary row is committed. After setting `session.lastSummaryStored` (unchanged from today), if `session.summaryStoredEvent` exists (initialized by `SessionManager` when the session is created, see plan 07 Phase 7), call `session.summaryStoredEvent.emit('stored', summaryId)`. This unblocks the blocking `/api/session/end` handler in plan 07 Phase 7 without polling. Contract: emit exactly once per summary commit; `summaryId` is the newly inserted row id from the atomic TX. ```ts // inside the block that sets session.lastSummaryStored (around :170–174) session.lastSummaryStored = true; session.summaryStoredEvent?.emit('stored', summaryRowId); ``` **(b) Docs** — §3.7 clean diagram (B→C→D→{Fail | Store}→Confirm→…, lines 299–308). Part 1 #21 (line 39), #22 (line 40). Part 2 D5 (line 77). V7b (`:87–108`), V7c (`:176–200`), V7g (`'failed'` + `markFailed`). **(c) Verification** - `grep -n "parseObservations\|parseSummary\|coerceObservationToSummary\|consecutiveSummaryFailures" src/services/worker/agents/ResponseProcessor.ts` → 0 hits. - `grep -n "MAX_CONSECUTIVE_SUMMARY_FAILURES" src/services/worker/agents/ResponseProcessor.ts` → 0 hits. - Integration test A — malformed input: send `"Service temporarily unavailable"` as `text`, assert (i) no row inserted in `observations` table, (ii) no row in `session_summaries`, (iii) every id in `session.processingMessageIds` has `status='failed'` in `pending_messages` after the call returns, (iv) `session.processingMessageIds === []`. - Integration test B — observation-without-summary when summary expected: `summaryExpected=true`, text is `codex`, assert (i) no row in `session_summaries`, (ii) no row in `observations` (contract failure fails the whole batch — no partial write), (iii) pending messages marked `failed`. This is **the critical regression test** — today the coerce path would have written a coerced summary row. - Integration test C — valid obs + summary: single atomic TX still commits both rows together (pre-existing behaviour, no regression). **(d) Anti-pattern guards** - **Guard C**: No coercion, no "close-enough" branch. Every `parsed.valid === false` path leads to `markFailed` and `return`. - **Guard D**: One parse call (`parseAgentXml`) replaces two (`parseObservations` + `parseSummary`). No wrapper facade. - **Guard A**: No new method on `RestartGuard`, no new class, no new helper file. Direct calls to the existing `PendingMessageStore.markFailed`. --- ### Phase 3 — Remove `consecutiveSummaryFailures` from `ActiveSession` + its consumer **(a) What to implement** 1. Delete `src/services/worker-types.ts:51–53` (the three lines: JSDoc + `consecutiveSummaryFailures: number;` field). Field name must vanish from the type. 2. Delete `src/services/worker/SessionManager.ts:336–346` (the 11-line circuit-breaker check in `queueSummarize`). The method body goes straight from the auto-initialize check (`:331–334`) to the `// CRITICAL: Persist to database FIRST` comment (`:348`). **This deletion was omitted from the original Phase 3 draft at `06-implementation-plan.md:155–204` — V7e is the new citation.** 3. Delete the initialiser `consecutiveSummaryFailures: 0,` at `SessionManager.ts:232` (inside `initializeSession`). 4. Delete the `MAX_CONSECUTIVE_SUMMARY_FAILURES` import in `SessionManager.ts` (if present). Use `grep -n "MAX_CONSECUTIVE_SUMMARY_FAILURES" src/services/worker/SessionManager.ts` first; remove the line. 5. No schema changes. No new `RestartGuard` API (see Dependencies above — option (1)). **(b) Docs** — §3.7 deletion bullet "consecutiveSummaryFailures counter + circuit-breaker logic (RestartGuard covers this already)" (line 314). Part 1 #22 (line 40). Part 2 D5 (line 77). V7d, V7e, V7f. **(c) Verification** - `grep -rn "consecutiveSummaryFailures" src/` → 0 hits. - `grep -rn "MAX_CONSECUTIVE_SUMMARY_FAILURES" src/` → 0 hits (constant, its JSDoc, all imports gone). - TypeScript compile succeeds (removing a field and all references is mechanical; no union fallout expected). - Behavioural test: call `sessionManager.queueSummarize(sessionDbId)` five times in rapid succession with intentionally failing agent output; assert every call enqueues to `pending_messages` (no silent drop) and each failed attempt marks that message `'failed'`. The old circuit breaker would have swallowed calls 4–5; the new contract doesn't. - Behavioural test: existing `RestartGuard` still trips after the configured restart count (`MAX_WINDOWED_RESTARTS = 10`, `RESTART_WINDOW_MS = 60_000`) — prove that repeated parse failures + subsequent subprocess restarts still converge to guard-tripped within the window. Covered by `07-session-lifecycle-management` tests; no duplication here. **(d) Anti-pattern guards** - **Guard A**: No new `RestartGuard.recordFailure()` invented. The class stays at 70 lines, public API unchanged. Dependency coupling to `07-session-lifecycle-management` is documentation-only. - **Guard C**: Removing the circuit breaker means failures flow to queue-level `'failed'` state — a single, visible, DB-backed failure signal. No silent swallow. --- ### Phase 4 — Verification sweep **(a) What to implement** — Grep audit + targeted regression tests. No new code. **(b) Docs** — §3.7 full deletion list (lines 310–315), Phase 3 verification block in `06-implementation-plan.md:189–195`. **(c) Verification — must all return 0 matches** - `grep -rn "coerceObservationToSummary" src/` → 0. - `grep -rn "consecutiveSummaryFailures" src/` → 0. - `grep -rn "MAX_CONSECUTIVE_SUMMARY_FAILURES" src/` → 0. - `grep -rn "parseObservations\|parseSummary" src/ | grep -v "src/sdk/parser.ts"` → 0 (the only survivors are private helpers inside `parser.ts` itself; if you named them without the `parse` prefix this grep is also 0). - `grep -rn "coerceFromObservation" src/` → 0. **(c-cont) Regression tests — must all pass** - Parser fuzz: feed 1 000 synthetic agent outputs mixing valid/invalid XML + present/absent ``; assert `valid:false` paths never write to `observations` or `session_summaries`. Must be 0 coerced summary rows. - Atomic-TX sanity: inject a DB error on `INSERT INTO session_summaries`; assert `storeObservations` rolls back so `observations` for that batch also revert. (Pre-existing invariant; we didn't touch it, but prove it.) - Idempotency of failure: double-delivery of the same malformed response (e.g., via worker crash + retry) results in the same `pending_messages` row in `'failed'` status; second attempt does not create a duplicate observation. Relies on upstream `02-sqlite-persistence` `UNIQUE(session_id, tool_use_id)` — cross-check with that plan. - End-to-end: Stop-hook summarize path exercises `parseAgentXml({requireSummary:true})`. With a mocked agent returning garbage, assert the hook receives the 110 s timeout path (no silent summary write), the pending message is `'failed'`, and SessionManager does NOT short-circuit subsequent summarize enqueues (circuit breaker is gone). **(d) Anti-pattern guards** — All four grep checks enforce Guards A/C/D structurally. --- ## Blast radius **Files modified**: - `src/sdk/parser.ts` — full rewrite of public surface; private helpers preserved. - `src/sdk/prompts.ts` — two-edit surgical change (skip-summary clause, constant delete). - `src/services/worker/agents/ResponseProcessor.ts` — replace lines 15–16 imports, 69–108 parse block, delete 176–200 circuit breaker. - `src/services/worker-types.ts` — delete 3 lines. - `src/services/worker/SessionManager.ts` — delete 11 lines (queueSummarize guard) + 1 line initialiser + maybe 1 import. **Files not touched**: `src/services/sqlite/observations/store.ts` (atomic TX lives here and is preserved). `src/services/worker/RestartGuard.ts` (API unchanged — see Dependencies option 1). `src/services/worker/agents/SessionCleanupHelper.ts`. `ObservationBroadcaster.ts`. Any Chroma sync module. **Schema changes**: none. **Estimated lines deleted**: - `coerceObservationToSummary` body + JSDoc: ~43 lines - `coerceFromObservation` branches in `parseSummary`: ~16 lines - `parseSummary` / `parseObservations` wrapper deduplication: ~15 lines (after collapse into `parseAgentXml`) - Non-XML early-fail block in `ResponseProcessor.ts:83–108`: ~26 lines (replaced by ~12 lines → net –14) - Circuit breaker in `ResponseProcessor.ts:176–200`: ~25 lines - `consecutiveSummaryFailures` field + initialiser + SessionManager guard: ~15 lines - `MAX_CONSECUTIVE_SUMMARY_FAILURES` constant + JSDoc + imports: ~8 lines **Net**: ~135 lines deleted, ~35 lines added → **~100 LoC net reduction**. ## Confidence + gaps **High confidence**: - Parser rewrite is mechanical (extract three private fns, compose them, add the discriminated-union return). - `'failed'` status string + `markFailed` API are verified. - Circuit-breaker + field removals are pure deletion once call sites are enumerated (V7e catches the missed site). **Gaps**: 1. **RestartGuard contract claim in D5 is overstated.** D5 says "RestartGuard already exists for repeated failures — delete the separate counter". RestartGuard today only handles **process-restart** loops, not per-message parse failures. This plan adopts the narrower interpretation (parse failure → `markFailed`; existing RestartGuard handles the subprocess-restart side effects unchanged). If the `07-session-lifecycle-management` plan decides to add `RestartGuard.recordFailure()`, callers here can start using it in a follow-up — no churn to this plan. **Flag for `07-session-lifecycle-management` author**: confirm the RestartGuard surface they want. 2. **Prompt updates assumed in-scope.** The audit implies the agent contract "already states `` or ``". Verified: prompts enforce `` strictly but never mention ``. Phase 1b adds the missing clause. If the team prefers to keep `` as a *recognised-but-undocumented* escape hatch, Phase 1b can be dropped — but then the parser should be stricter too (reason `missing_summary` when only skip-summary is emitted without prompt permission). Flag for product owner.