docs: archive shipped openspec batches; add feature/plan/research notes
Move 13 shipped openspec change docs under openspec/changes/archived/. Add docs/features/git-diff-panel, docs/plans/post-review-backlog, and docs/research/cross-app-contract-ssot.md (the research behind the @boocode/contracts SSOT work). Update BOOCHAT.md, BOOCODER.md, and boocode_roadmap.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,147 @@
|
||||
# Implementation Decision Log: Post-Review Backlog (F1–F9)
|
||||
|
||||
<!--
|
||||
This file records every implementation decision committed while planning the post-review backlog.
|
||||
Behavioral and implementation statements live in [../feature-implementation-plan.md](../feature-implementation-plan.md) —
|
||||
this file captures the question, rationale, evidence, and rejected alternatives for each decision.
|
||||
Round-by-round history lives in [implementation-iteration-history.md](implementation-iteration-history.md).
|
||||
-->
|
||||
|
||||
Source of truth: [../scope-brief.md](../scope-brief.md) (ground-truth spec stand-in) and
|
||||
[synthesis-input.md](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).
|
||||
Reference in New Issue
Block a user