boocode/docs/plans/post-review-backlog/artifacts/implementation-decision-log.md

Implementation Decision Log: Post-Review Backlog (F1–F9)

Source of truth: ../scope-brief.md (ground-truth spec stand-in) and synthesis-input.md (the consolidated Round-1 specialist aggregation; its file:line evidence is treated as verbatim). The D-N counter is shared across the Trivial and Full sections below.

Trivial decisions

• D-2: F3 logger threading shape — pass an optional log?: { debug } param to extractToolCallBlocks from its single call site in stream-phase.ts executeStreamPhase; no interface (one site, one impl). — Referenced in plan: Implementation Approach (F2+F3), Decomposition and Sequencing.
• D-6: F7 query shape — read messages_with_parts WHERE role != 'system' (strips sentinels), params session_id + optional chat_id + limit (default 50, max 200), ORDER BY created_at ASC, returns {role,content,...}[]. — Referenced in plan: Implementation Approach (F7), External Interfaces.
• D-9: Patch-tag-per-unit — each ready item ships as its own sequential patch tag (one batch per coherent unit), not a minor bump; Sam declined v2.8.0 twice. — Referenced in plan: Decomposition and Sequencing, Operational Readiness.
• D-10: F1 Stop-button terminal label — render a muted "Stopped" label (not red, not a toast) for status='cancelled'. — Referenced in plan: Implementation Approach (F1), Testing Strategy.

Full decisions

D-1: F1 cancel registry shape and finalization-fix scope

• Question: How does a Stop on an external agent task actually abort the running child, and what message-state corruption does wiring that abort newly expose?
• Decision: Add taskControllers = new Map<string, AbortController>() inside createDispatcher; taskControllers.set(taskId, ac) at each of the four run-functions (dispatcher.ts ~316/655/991/1248) and .delete in the existing .finally() (dispatcher.ts:117); export cancelExternalTask(taskId): boolean (idempotent — ac.abort() no-ops when already aborted, so double-Stop and cancel-after-exit are safe). Pass a narrow ExternalCancelFn (not the whole dispatcher) into registerTaskRoutes, wired in index.ts:254. In the same batch, fix the two pre-existing finalization bugs this newly makes reachable: (1) the four catch blocks update only tasks state and leave the assistant messages row status='streaming' (the BooChat 5-min sweep is a different process and cannot recover it); (2) the warm-backend success path writes messages.status='complete' unconditionally before checking abort (dispatcher.ts ~853/1122/1377). Fix via a shared cancelAndFinalize helper across all four paths: after await backend.prompt(...), if (ac.signal.aborted) → write status='cancelled', publish the terminal message_complete frame, emit idle, return; in each catch finalize the message with WHERE status='streaming' (idempotent), mapping AbortError → cancelled vs error → failed.
• Rationale: 4-way specialist consensus (on-call B1, behavioral B1, architect A1, junior). The frontend and all four backends already honor the abort signal correctly (PTY child.kill pty-dispatch.ts:159; warm-ACP session/cancel warm-acp.ts:318; opencode session.abort opencode-server.ts:775; claude-sdk interrupt claude-sdk.ts:209); the only missing link is the registry + export + route wiring. Shipping the abort wiring without the finalization fixes would convert a silent no-op into a new bug (cancelled turns recorded complete, or messages stuck streaming), so the two are inseparable in one batch (on-call OCE-001/OCE-002, behavioral B2/B3).
• Evidence: routes/tasks.ts:110-148 (cancel route calls cancelPendingPermission + inference.cancel native-only + DB cancelled; never reaches dispatcher); dispatcher.ts:316/655/991/1248 (four private ac unreachable); cancelExternalTask absent anywhere (synthesis-input C1); finalization bugs at dispatcher.ts catch blocks + ~853/1122/1377 (synthesis-input C2); backend signal honoring cited above (scope-brief F1).
• Rejected alternatives:
  • Abort wiring only, defer the finalization fixes — rejected because wiring abort makes the streaming/complete corruption reachable from the UI for the first time; deferring ships a new bug (synthesis-input C2, on-call/behavioral).
  • Recover stuck messages via the BooChat 5-min sweep — rejected because the sweep runs in a different process (BooChat) and does not see BooCoder's agent_sessions/tasks finalization (on-call OCE-001).
  • Pass the whole dispatcher into registerTaskRoutes — rejected for over-coupling; a narrow ExternalCancelFn is sufficient (architect A1).
• Specialist owner: on-call-engineer (resilience) with software-architect (registry shape).
• Revisit criterion: a fifth external backend is added whose abort contract differs from signal-based cancellation, or the warm-vs-one-shot worktree-cleanup split changes.
• Dissent (if any): none.
• Driven by rounds: R1.
• Dependent decisions: D-7 (F1 terminal state), D-8 (F1 frame-extension over new frame type), D-10.
• Referenced in plan: Implementation Approach (F1), Runtime Behavior, On-Call Resilience Posture, Decomposition and Sequencing, RAID Log (R1).

D-3: F2 prune scope — option A (prune-now-minimal), keep the load-bearing guard

• Question: How far does the tool-call-parser prune go — unexport the dead symbols only, or the full flag-gated retirement of the text-scrape fallback?
• Decision: Option A only. KEEP extractToolCallBlocks + stripToolMarkup and their types (ToolCallExtraction, ParsedCall) — the load-bearing <invoke>-as-text guard. REMOVE only the export keyword (not the implementations) from the 8 zero-external-caller symbols: isPlaceholderArgValue, parseXmlToolCall, parseInvokeToolCall, partialXmlOpenerStart, and the 4 consts XML_TOOL_OPEN/CLOSE, INVOKE_TOOL_OPEN/CLOSE. Zero runtime effect; public export surface goes 11 → 4.
• Rationale: The TS parser is dormant defense-in-depth but the <invoke>-as-text path is the only guard for "tool call emitted as plain text" — experimental_repairToolCall does not cover that case, and the sidecar --jinja state is confirmed only by a live probe, not pinned in-repo, so keeping the guard is correct (architect A2, confirms junior OQ-F2c). Unexporting is pure simplification with no behavior change; the relicense already removed the AGPL-dead exports, so there is no license pressure forcing the larger move.
• Evidence: live consumers stream-phase.ts:263-284 (extractToolCallBlocks text-delta fallback), tool-phase.ts:122 + error-handler.ts:25,106 (stripToolMarkup); structured path stream-phase.ts:285 does all real work today; llama-swap --jinja confirmed ON by live probe of :8401 only (scope-brief F2, synthesis-input C3).
• Rejected alternatives:
  • Option B — flag-gate the text-scrape fallback, validate native parsing on live qwen3.6 for one release, then delete — rejected (deferred) because there is no evidence qwen3.6 stopped emitting <invoke> text on live and the sidecar --jinja state is unconfirmed in-repo; deleting the only plain-text-tool-call guard on that basis is unsafe (architect/test-engineer R1). See Deferred (YAGNI).
  • Delete the 8 symbols' implementations outright — rejected because three of them (parseXmlToolCall, parseInvokeToolCall, isPlaceholderArgValue) are called internally by the retained extractToolCallBlocks; only the export keyword is dead.
• Specialist owner: software-architect.
• Revisit criterion: a documented multi-session live probe shows zero text-delta tool calls from qwen3.6 (then Option B reopens).
• Dissent (if any): none.
• Driven by rounds: R1.
• Dependent decisions: D-2 (F3 logger — safe because F2 keeps extractToolCallBlocks), D-4 (F2 gate test).
• Referenced in plan: Implementation Approach (F2+F3), Decomposition and Sequencing, Testing Strategy, RAID Log (R2), Deferred (YAGNI).

D-4: F2 fallback gate test — pin the untested guard before pruning

• Question: The <invoke>-as-text fallback is currently exercised by no test; how do we prevent the prune from silently removing it?
• Decision: Add a gate test before the prune: stub streamText to emit a text-delta containing a complete <invoke> block; assert the call lands in result.toolCalls and the markup is NOT present in result.content. The test must stay green through the prune and fail if extractToolCallBlocks is ever removed from the text-delta path.
• Rationale: Pruning around an untested load-bearing path risks a silent regression; the gate test converts D-3's "keep the guard" commitment into an enforced invariant (test-engineer T6).
• Evidence: untested fallback at stream-phase.ts:263-284 (synthesis-input C4, test-engineer T6).
• Rejected alternatives:
  • Prune without the gate test, relying on review — rejected because the fallback has no current coverage, so a future removal would pass CI silently (test-engineer T6).
• Specialist owner: test-engineer.
• Revisit criterion: the fallback path is intentionally retired under Option B (then this test is rewritten, not deleted).
• Dissent (if any): none.
• Driven by rounds: R1.
• Dependent decisions: none.
• Referenced in plan: Testing Strategy, Decomposition and Sequencing.

D-5: F6 stall-timeout via AbortSignal.any; no retry

• Question: How does BooChat detect and recover a hung llama-swap stream server-side, and does it retry?
• Decision: Wrap the stream-phase.ts:261 fullStream loop with a per-chunk stall deadline. Create a local stallAc = new AbortController(), pass effectiveSignal = AbortSignal.any([signal, stallAc.signal]) to streamText, bump a setTimeout(STALL_TIMEOUT_MS = 90_000) on each chunk, clear it in the existing finally. At the post-loop check (stream-phase.ts:337) test signal?.aborted || stallAc.signal.aborted and throw AbortError (→ handleAbortOrError writes cancelled). No retry at executeStreamPhase/streamCompletion.
• Rationale: Today a hung stream relies entirely on the frontend 60s discard_stale watchdog with zero server-side guard; the 90s server stall-timeout closes that gap and reuses the existing abort/finalize path. Retry is deferred (YAGNI): a retry after a partial stream re-emits already-streamed deltas (state.accumulated + live delta frames are non-idempotent), which is worse than the current behavior; at single-local-instance scale the user re-sending is the correct recovery (on-call, strong).
• Evidence: stream-phase.ts:261 fullStream loop, :333 abort check, :343 usage; frontend 60s discard_stale is the only stall guard today (scope-brief F6, synthesis-input C6).
• Rejected alternatives:
  • Promise.race of the loop against a timeout — rejected in favor of AbortSignal.any, which threads cancellation through streamText and the existing finalize path cleanly (OQ-F6a, on-call).
  • Retry/backoff classifier (transient-5xx / stall) — rejected (deferred) because partial-stream re-emit is non-idempotent and llama-swap is a single local instance (synthesis-input YAGNI ledger). See Deferred (YAGNI).
• Specialist owner: on-call-engineer.
• Revisit criterion: llama-swap gains restart-in-place-with-clear-partial, or a second instance is added for failover (then retry reopens).
• Dissent (if any): none.
• Driven by rounds: R1.
• Dependent decisions: none.
• Referenced in plan: Implementation Approach (F6), On-Call Resilience Posture, Testing Strategy, RAID Log (R3), Deferred (YAGNI).

D-7: F1 terminal state for user Stop — cancelled, not failed

• Question: When a user hits Stop, what terminal messages.status does the finalized assistant message land in?
• Decision: cancelled for a user-initiated Stop (AbortError); failed only for a genuine thrown error in the catch path.
• Rationale: A user Stop is a deliberate, non-error outcome; MessageStatus already includes 'cancelled' and the web reducer can map it without a new enum value. Distinguishing AbortError → cancelled vs error → failed keeps the human-inbox / failure surfaces honest (resolved OQ, on-call/behavioral).
• Evidence: MessageStatus includes 'cancelled'; reducer map point CoderPane.tsx:299 (synthesis-input F1 UX + resolved OQs).
• Rejected alternatives:
  • Record user Stop as failed — rejected because it pollutes failure surfaces with deliberate user actions (resolved OQ F1 terminal state).
• Specialist owner: behavioral-analyst.
• Revisit criterion: product decides a user Stop should count against a failure/alerting budget.
• Dissent (if any): none.
• Driven by rounds: R1.
• Dependent decisions: D-8, D-10.
• Referenced in plan: Implementation Approach (F1), Runtime Behavior.

D-8: F1 status surfacing — extend the existing frame, no new frame type

• Question: How does the cancelled terminal state reach the web reducer — a new WS frame type, or an extension of the existing one?
• Decision: Extend the coder message_complete frame with an optional status field (Option A — minimal); map it in the reducer (CoderPane.tsx:299). No new frame type, so the cross-app WsFrame parity rule does not force a paired strict-union arm beyond the optional field.
• Rationale: Adding a whole new frame type triggers the full cross-app parity dance (server InferenceFrame/ws-frames.ts + web WsFrame) for a single optional value already carried on a terminal frame; an optional field on the existing frame is the smaller change with the same observable result (UX agent).
• Evidence: reducer map point CoderPane.tsx:299; cross-app frame parity rule (CLAUDE.md Conventions; scope-brief cross-cutting constraints).
• Rejected alternatives:
  • New agent_cancelled frame type — rejected because it forces a paired strict-union arm in two files for a single optional status value (UX agent, Option A vs B).
• Specialist owner: user-experience-designer.
• Revisit criterion: a second distinct terminal sub-state needs carrying that does not fit message_complete.status.
• Dissent (if any): none.
• Driven by rounds: R1.
• Dependent decisions: D-10.
• Referenced in plan: Implementation Approach (F1), External Interfaces.

D-11: F9 retire :9502 SPA — delete the serve block, keep all API/WS routes

• Question: What exactly is removed when retiring the :9502 fallback SPA, and what must stay?
• Decision: Delete the if (existsSync(webRoot)) block in index.ts (~269-289) which already no-ops when the dist is absent; keep the inline 404 handler ({error:'not found'}). Remove apps/coder/web from pnpm-workspace.yaml, the coder build step, and the Dockerfile copy; remove the now-unused fastifyStatic import (verify it is only used there). KEEP all /api/coder/* REST + WS + /api/health + --mcp routes — CoderPane depends on them. Optionally add a 2-line GET / redirect-to-BooChat (no fastifyStatic).
• Rationale: Sam confirmed "I don't use 9502"; primary UI is CoderPane inside BooChat. OQ-F9a resolved: nothing probes GET / on :9502 (health is /api/health; the compose healthcheck targets the boocode container, not the host-systemd coder), so 404-or-redirect at / is safe (architect A5, verified).
• Evidence: serve block index.ts ~269-289; GET / unprobed (synthesis-input C8, OQ-F9a resolved); scope-brief F9 / DEFERRED #5 removal checklist.
• Rejected alternatives:
  • Keep the SPA — rejected; Sam greenlit removal and the build step is dead weight (scope-brief F9).
  • Remove the REST/WS routes too — rejected because CoderPane inside BooChat depends on every /api/coder/* route (architect A5).
• Specialist owner: software-architect.
• Revisit criterion: a standalone :9502 UI is ever wanted again (would be a fresh feature, not a revert).
• Dissent (if any): none.
• Driven by rounds: R1.
• Dependent decisions: none.
• Referenced in plan: Implementation Approach (F9), Operational Readiness, Decomposition and Sequencing.

D-12: F4/F5/F8 disposition under the standing override — document as Blocked, do not halt

• Question: The spec-maturity gate tripped on F4/F5/F8; the skill says recommend routing them out. Sam issued a standing override to plan everything. How are they recorded?
• Decision: Proceed with the plan but record F4, F5, F8 in a structurally separate BLOCKED tier with their exact blocking open question(s) and recommended resolution path, rather than halting synthesis. F4 → route to plan-a-feature (hook-firing-in-unattended-mode premise UNVERIFIED + goose hook mechanism unknown). F5 → SDK capability check (pinned @opencode-ai/sdk exposes no compaction event arm); UI treatment (sentinel-row vs ephemeral-frame) stays disputed until the event is confirmed to exist. F8 → route to plan-a-feature (no line-selection infra exists, diff source ambiguous). These three do NOT block the ready cluster (F1/F2/F3/F6/F7/F9).
• Rationale: The spec-maturity gate tripped with ≥5 spec-level findings (C9, C11, C12, C13, OQ-F4b, OQ-F8a) concentrated in F4/F5/F8 across junior, behavioral, and UX. Sam pre-acknowledged the WANT items would be planned more shallowly when choosing scope "everything we discussed," so the honest synthesis records them as Blocked with explicit reopen paths rather than fabricating plan-level resolutions or stalling the ready work.
• Evidence: spec-maturity gate TRIPPED (synthesis-input "Spec-maturity gate"); USER OVERRIDE STANDING (same section); blocking OQs OQ-F4a/F4b, OQ-F5a/F5b, OQ-F8a/b/c (synthesis-input Open Questions); claims C9, C11, C12, C13.
• Rejected alternatives:
  • Halt synthesis and route F4/F5/F8 out before any planning — rejected because Sam's standing override directs the plan to proceed and document (synthesis-input gate section).
  • Plan F4/F5/F8 at plan-level alongside the ready cluster — rejected because their core premises are unverified (F4) / capability-blocked (F5) / infra-absent (F8); plan-level decisions would rest on unproven assumptions (C9/C11/C13).
• Specialist owner: project-manager.
• Revisit criterion: the named blocking OQ for an item resolves (F4: hooks confirmed to fire unattended + goose format known; F5: SDK compaction event confirmed; F8: diff source chosen + line-selection approach specified) — then that item graduates to its own plan.
• Dissent (if any): none; the gate-trip is acknowledged rather than overridden silently.
• Driven by rounds: R1.
• Dependent decisions: none.
• Referenced in plan: Implementation Approach (Blocked tier), RAID Log (R4, R5, assumptions), Open Items, Deferred (YAGNI).