# Junior-Developer Review: PTY Exit Notifications Plan ## Scope **Artifacts reviewed:** - `openspec/changes/pty-exit-notifications/proposal.md` - `openspec/changes/pty-exit-notifications/design.md` - `openspec/changes/pty-exit-notifications/tasks.md` - `openspec/changes/pty-exit-notifications/specs/pty-exit-notification/spec.md` **Source files read for context:** - `apps/booterm/src/ws/attach.ts` (onExit handler, WS lifecycle) - `apps/booterm/src/pty/registry.ts` (SessionMeta, ring buffer, appendOutput) - `apps/booterm/src/pty/manager.ts` (sweepExpired, killSession) - `packages/contracts/src/ws-frames.ts` (WsFrameSchema, KNOWN_FRAME_TYPES, drift test) - `apps/web/src/lib/terminal-protocol.ts` (ServerControlFrame, parseServerFrame) - `apps/web/src/hooks/terminal/useTerminalSocket.ts` (WS connect, message handler, exit handling) - `apps/web/src/api/types.ts` (web-side strict WsFrame) - `apps/server/src/services/inference/types.ts` (InferenceFrame loose union) - `apps/server/src/services/broker.ts` (publishFrame validation flow) - `packages/contracts/src/__tests__/ws-frames.test.ts` (drift test) ## Plain-Language Restatement When a terminal process exits inside a booterm pane, instead of sending a bare `{type: 'exit', code: N}` frame and closing the socket, send a richer `pty_exited` frame that includes the exit code, the last few lines of output, session metadata (title, description, parent agent), and whether it was killed by a timeout. Add the frame type to the cross-app wire contract. Update the web frontend to parse and display it. Defer making the inference loop aware of it. ## Question Log ### Who and Why - **Q1 [Answered]:** Who is the primary consumer of this notification? — The browser user (via the styled notification). Inference-loop consumption is deferred (design.md:93-95). The spec (spec.md:56-64) contradicts this — see JD-001. - **Q2 [Assumed]:** Why now instead of deferring the whole thing? — The proposal says "The data is present but never surfaced on exit." The assumption is that surfacing existing data is low-effort and high-value. If the effort is actually higher than estimated (e.g. timeout integration, ring-buffer races), this assumption weakens. - **Q3 [Open]:** Has the person who originally asked for this (who?) seen the current design? — No citation. The proposal mentions "the inference loop in apps/server and apps/coder cannot react" as the motivating problem, but the design defers that exact use case. If the requester expected inference-loop integration, they may reject this scope. ### What and Scope - **Q4 [Assumed]:** Is the old `{type: 'exit', code: N}` frame removed or kept additive? — Tasks.md:16 says "the old bare exit frame is replaced (not additive)." Assumes no code anywhere depends on receiving `{type: 'exit'}` after upgrade. The web handler has backward compat for receiving the old type (spec.md:46-54), but the server sends only the new type. This only matters during mixed-version deployments (if booterm and web are upgraded independently). - **Q5 [Answered]:** What is the smallest valuable version? — The frame over the booterm WS + web handler + contract type. Timeout integration (task 4) and inference-loop integration are deferred. The spec includes an inference-loop requirement that isn't delivered — see JD-001. - **Q6 [Open]:** What is the rollback plan? — Not mentioned. If the new frame breaks the frontend, how do we revert? The old `{type: 'exit'}` handler still exists, so downgrading the web code is safe, but rolling back a deploy isn't described. ### Assumptions and Evidence - **Q7 [Assumed]:** The ring buffer contains useful "last lines" at exit time. — Depends on timing. `unregister(pid)` deletes the ring buffer. The socket `close` handler calls `unregister`. The ordering in the current code is: `handle.onExit` → `socket.close()` → socket `close` event → `unregister()`. So the buffer is alive during `onExit`. But if any future change reorders this, `getLastLines` returns `[]`. (See JD-004.) - **Q8 [Uncited claim]:** "The reference implementation (`/opt/forks/opencode-extras/opencode-pty`) solves this with `` structured notifications." — Mentioned in proposal.md:5 but no specific lines, tests, or code cited from that fork. What exactly does it do for ANSI stripping, line count, metadata shape? The design doesn't borrow specifics. - **Q9 [Assumed]:** `getLastLines` filtering empty lines is the right behavior. — Filtering `trim().length > 0` strips lines with only whitespace or carriage returns. But terminal output lines containing ANSI escape sequences (e.g. `\x1b[31m`) pass through, and a line containing only `\r` gets filtered. The spec says "at least one last_lines entry" in the normal-exit scenario, but if the last 5 lines were all `\r`-only (common in progress-bar output), the array would be empty. See JD-007. - **Q10 [Open]:** How does the inference loop subscribe to `pty_exited` in the future? — The design defers this but the frame already goes into `WsFrameSchema`, which would pass `publishFrame` validation. However, `session_id` in the booterm frame uses short IDs (`sanitizeId` pattern), while `WsFrameSchema` uses UUIDs. When someone wires up broker publish, the `session_id: z.string().min(1)` field will pass Zod... but to be useful to subscribers, the session ID needs to match the server's UUID-based session key. See JD-005. ### Prior Art, Specialist Domains, Done and Exit - **Q11 [Answered]:** Does this conflict with any existing standard? — The cross-app convention (root CLAUDE.md) says "Adding a new WS frame type (cross-app)" requires updating three files: contracts, server InferenceFrame, web WsFrame. The design updates contracts and the booterm/web terminal protocol, but the server's `InferenceFrame` and web's `WsFrame` are deliberately skipped because broker publish is deferred. This is explicitly called out (design.md:93-95), so the departure from convention is intentional but unmarked. - **Q12 [Open]:** What does "Done" look like concretely? — Tasks.md lists checkboxes. But the spec.md:56-64 requires inference-loop subscription which is NOT in the tasks. If someone marks all tasks done, the spec is still unsatisfied. See JD-001. - **Q13 [Open]:** Who owns the post-ship maintenance? — The tasks don't assign owners. When the inference-loop integration is eventually done, someone needs to know that `session_id` in the booterm world doesn't match the server's UUID schema. ## Assumptions This review assumes: 1. The current codebase state in the source files as read today is what the plan targets. 2. The booterm WS and the server broker WS are separate connections (booterm WS is per-terminal-pane; server broker WS is per-session). 3. The `pnpm -C packages/contracts build` step correctly produces `dist/` exports consumed by other packages. 4. No non-booterm code currently depends on `{type: 'exit'}` frames over the terminal WS. ## Open Questions **OQ1: Does the delivery spec contradict the design on inference-loop requirement?** - **Why it matters:** Spec.md:56-64 says "MUST be able to react" and "MUST receive the frame." Design.md:93-95 defers it entirely. If these are both meant to be true, there's a gap: the frame is in `WsFrameSchema` but nothing publishes it through the broker. The spec would be unsatisfied on delivery. - **Findings affected:** JD-001 - **How to resolve:** Either (a) remove the inference-loop requirement from the spec, (b) make it a future-scope note instead of a MUST, or (c) ship broker integration in this same batch. The artifacts must agree. **OQ2: Will the timeout path (`sweepExpired`) correctly populate `timed_out: true`?** - **Why it matters:** The design's proposed `onExit` handler (design.md:72) hardcodes `timed_out: false`. Task 4 says to set `meta.timedOut = true` in `sweepExpired`, but the `onExit` handler never reads it. The two changes are disconnected. Additionally, `sweepExpired` calls `unregister` after `killSession`, which destroys the registry entry before `onExit` can read `meta.timedOut`. - **Findings affected:** JD-002, JD-003, JD-004 - **How to resolve:** The `onExit` handler must read `meta?.timedOut ?? false` instead of hardcoding `false`. The `sweepExpired` path must not call `unregister` until after the `onExit` callback has had a chance to fire (or must send the frame itself). **OQ3: How is the "styled notification" rendered in the web frontend?** - **Why it matters:** Design.md:114 says "display a styled notification block." The current exit handler writes ANSI-dim text to the terminal: `\x1b[2m[process exited with code N]\x1b[0m`. The new handler should "display a styled exit notification with the exit code and last output line(s)." But: is this more ANSI terminal text? A React overlay? A toast? The plan doesn't include a mockup or describe the presentation, which makes task 6.1 ambiguous. - **Findings affected:** JD-008 - **How to resolve:** Specify whether the notification is (a) written to xterm as ANSI text (extending current pattern), (b) a React component above/below the terminal, or (c) something else. Provide an example of expected output. **OQ4: What is the future compatibility story for broker publish?** - **Why it matters:** The `PtyExitedFrame` schema uses `session_id: z.string().min(1)`, but the server broker uses UUID-based session IDs (`z.string().uuid()`). Booterm uses short non-UUID session IDs. When someone eventually wires up broker publish, the frame will need either (a) a UUID-converted session_id, (b) the booterm side to map to UUID, or (c) the schema relaxed. If (a)/(b) isn't planned for now, at minimum a comment on the schema field should warn about this. - **Findings affected:** JD-005 - **How to resolve:** Add a code comment or ADR note about the session_id mismatch. Consider using `z.string().min(1)` in the schema (as designed) but add a `// XXX: will need UUID conversion for broker publish` note near the field definition. ## Summary The plan is structurally sound but suffers from one hard contradiction (spec vs design on inference-loop), one implementation gap (timed_out flag never read), and several ragged edges that will bite the implementer mid-task. The timeout path in particular needs re-thinking before coding begins. | Severity | Count | |----------|-------| | Blocks decision | 0 | | Muddies artifact | 4 | | Worth clarifying | 4 | | Polish | 2 | Open Questions: 4 Specialist handoffs: 0 Full review written to: `/opt/boocode/junior-dev-review.md` ## Findings **JD-001: Spec requires inference-loop delivery; design defers it — contradiction in MUST** - **Protocol:** Clarifying-Question Sweep - **Location:** `specs/pty-exit-notification/spec.md:56-64` vs `design.md:93-95` - **Evidence:** - Spec: "The inference loop MUST be able to react to PTY exit events via the broker." (line 57) - Spec: "any subscriber on the per-session channel MUST receive the frame" (line 64) - Design: "Option B for this change. The notification reaches the browser. Inference-loop integration requires a callback mechanism ... which is a separate concern." (lines 93-95) - Deferred section: "Inference-loop broker publish: ... Reopen when: (a) the server needs to react to PTY exits" (lines 165-166) - **What the artifact claims / leaves unclear:** Two MUST requirements in the spec cannot be delivered by the design as scoped. The design is aware it's deferring this, but the spec wasn't updated to match. An implementer following the spec would expect broker publish to be in scope; following the tasks, it isn't. - **Why this matters:** If a QA or reviewer validates against the spec (which is the canonical requirements document), they'd flag the deliverable as incomplete. The inference-loop subscription has no task, no implementation, and no test. - **Related questions:** Q1, Q6, Q12 (OQ1) - **Standard or precedent:** N/A - **Specialist to consult:** N/A — generalist scoping issue - **Severity:** Muddies artifact - **Suggested next step:** Remove or downgrade inference-loop requirements from spec.md:56-64 to "future" or "non-goal" to match the design. Or add tasks for broker integration to this batch. **JD-002: `onExit` handler hardcodes `timed_out: false` — never reads registry flag** - **Protocol:** Hidden-Assumption Audit - **Location:** `design.md:72` (code block showing the new `onExit` handler) - **Evidence:** - Design code block lines 57-80: ```typescript handle.onExit(({ exitCode }) => { const meta = registry.get(pid); // ... const frame = { // ... timed_out: false, // <-- line 72: always false }; ``` - Design.md:120-121: "sweepExpired sets timedOut = true before calling killSession" - Task 2.2: Add `timedOut?: boolean` to `SessionMeta` - Task 4.1: Set `meta.timedOut = true` in `sweepExpired` - **What the artifact assumes / leaves unclear:** The design assumes someone will connect the `meta.timedOut` flag to the frame payload, but the code block shows a hardcoded `false`. The tasks don't mention reading the flag in `onExit`. The implementer following the code block literally will ship `timed_out: false` on every frame regardless of timeout status. - **Why this matters:** The timeout feature is one of the three named additions (exit code, last lines, timeout status). If it's always `false`, the feature is broken. This isn't caught by any task checkbox. - **Related questions:** Q3 (OQ2) - **Standard or precedent:** N/A - **Specialist to consult:** N/A - **Severity:** Muddies artifact - **Suggested next step:** Change the code block in design.md:72 to `timed_out: meta?.timedOut ?? false`. Add a task to read `meta.timedOut` in the `onExit` handler. **JD-003: `sweepExpired` calls `unregister` before `onExit` can read `timedOut`** - **Protocol:** Hidden-Assumption Audit - **Location:** `apps/booterm/src/pty/manager.ts:172-198` (sweepExpired) vs `design.md:119-123` - **Evidence:** - `sweepExpired` in `manager.ts:194` calls `registry.unregister(meta.paneId)` AFTER `killSession` but within the same async function - The `onExit` callback in `attach.ts` reads `registry.get(pid)` — but if `sweepExpired` already called `unregister`, this returns `undefined` - Design.md:121: "Add a `timedOut` flag to `SessionMeta`" — but `unregister` deletes the entire SessionMeta entry (`registry.ts:58-61`) - **What the artifact assumes / leaves unclear:** Assumes `onExit` fires before `unregister` runs, or that `registry.get(pid)` still works after `unregister`. Neither is guaranteed. `sweepExpired` kills the tmux session (which eventually causes the SSH/node-pty process to exit and fire `onExit`), but this is all async. The `await killSession(...)` completes, then `unregister` runs synchronously. There's no coordination with the `onExit` callback. - **Why this matters:** For timeout-killed sessions, the `onExit` handler may not find the registry entry at all, resulting in `meta ?? null` for all metadata fields (title, description, parentAgent) and `timed_out: undefined` becoming `false`. - **Related questions:** Q3 (OQ2), Q7 - **Standard or precedent:** N/A - **Specialist to consult:** N/A (but a concurrency-aware dev should look at this) - **Severity:** Muddies artifact - **Suggested next step:** Either (a) have `sweepExpired` NOT call `unregister` but instead set a flag that the `onExit` handler checks and cleans up, or (b) have `sweepExpired` capture WS socket references and send the frame itself before closing, or (c) accept that timeout notifications are best-effort and document the gap explicitly. **JD-004: Ring buffer may be deleted before `getLastLines` runs in timeout path** - **Protocol:** Evidence-and-Reasoning Check - **Location:** `apps/booterm/src/pty/registry.ts:58-61` (`unregister`), `manager.ts:194`, `attach.ts:57-61` (proposed onExit) - **Evidence:** - `unregister(pid)` in `registry.ts:58-61`: ```typescript sessions.delete(paneId); ringBuffers.delete(paneId); ``` - `sweepExpired` in `manager.ts` calls `unregister` after `killSession` - Proposed `onExit` handler calls `getLastLines(pid, 5)` which reads from `ringBuffers` - Normal exit path (socket close triggers unregister): safe — `onExit` fires before `socket.close()` triggers the close handler. - Timeout exit path: `sweepExpired` calls `unregister` directly — race with `onExit`. - **What the artifact assumes / leaves unclear:** The design's proposed code for `onExit` reads the ring buffer, assuming it's always available. The timeout integration section (design.md:117-123) doesn't mention the ring buffer deletion. An implementer following the timeout tasks alone would not know to protect against this. - **Why this matters:** `last_lines` will be empty `[]` for timeout-killed processes, even if the process produced output. The spec's "timed_out: true and the exit code" requirement doesn't mention last_lines for timeouts, but an empty array for a process that ran for 30 seconds is confusing. - **Related questions:** Q7 - **Standard or precedent:** N/A - **Specialist to consult:** N/A - **Severity:** Worth clarifying - **Suggested next step:** Add a note in the design that timeout-killed processes may produce empty `last_lines` because the registry/ring buffer is cleaned up by the sweep. Or refactor `sweepExpired` to not destroy data the `onExit` handler needs. **JD-005: `session_id` type mismatch between booterm world and server broker UUID convention** - **Protocol:** Standards & Conventions Conflict Check - **Location:** `design.md:32` (`session_id: z.string().min(1)`), `packages/contracts/src/ws-frames.ts:13` (`Uuid = z.string().uuid()`) - **Evidence:** - All existing frames in `WsFrameSchema` use `Uuid` for `session_id`, `chat_id`, `message_id`, etc. - The design proposes `session_id: z.string().min(1)` — which accepts anything non-empty, including booterm's short IDs like `"sess_abc123"`. - Booterm session IDs come from `sanitizeId()` which validates against `^[a-zA-Z0-9_-]{1,64}$` — these are NOT UUIDs. - The server broker's `publishFrame` validates against `WsFrameSchema` before publishing. If someone wires up broker publish using the booterm session_id, Zod validation passes (`z.string().min(1)` accepts it), but the subscriber expects a UUID-format session_id to match against. - **What the artifact assumes / leaves unclear:** The schema uses `z.string().min(1)` when it should either match the server's UUID convention OR have a clear migration path. The design adds the frame to `WsFrameSchema` (task 1), which is the cross-app contract, but booterm session IDs don't match the server's session ID format. This frame will sit in the contract with a schema that validates technically but carries the wrong-shaped data for broker consumers. - **Why this matters:** When inference-loop integration happens (the deferred requirement from JD-001), someone will try to publish `pty_exited` through `publishFrame`. The frame will validate against `WsFrameSchema` (good), but subscribers expecting UUID-based session_id matching will break silently. The `z.string().min(1)` field makes the schema technically correct but semantically wrong. - **Related questions:** Q11, Q13 (OQ4) - **Standard or precedent:** The rest of `WsFrameSchema` uses `Uuid` for every session/chat/message identifier. This is the only frame that uses `z.string().min(1)` for a session identifier. - **Specialist to consult:** N/A - **Severity:** Worth clarifying - **Suggested next step:** Add a comment on the `session_id` field: `// XXX: booterm short-ID format; broker publish will need UUID conversion`. Consider adding the UUID mapping to the deferred section so future integrators know about it. **JD-006: The web's strict `WsFrame` union in `api/types.ts` is NOT updated — but it's unclear if it needs to be** - **Protocol:** Standards & Conventions Conflict Check - **Location:** Root `CLAUDE.md` ("Adding a new WS frame type (cross-app)"), `apps/web/src/api/types.ts:599` (WsFrame union) - **Evidence:** - Root CLAUDE.md says: "The server's `InferenceFrame` loose union (`services/inference/turn.ts`) and the web's strict `WsFrame` discriminated union (`apps/web/src/api/types.ts`) still exist separately and also need updating." - The design only lists `apps/web/src/lib/terminal-protocol.ts` and `apps/web/src/hooks/terminal/useTerminalSocket.ts` as web-side changes. - The web's `WsFrame` is for the server broker WS (session stream + user events), NOT the booterm terminal WS. So it genuinely doesn't need `pty_exited` unless/until broker publish happens. - **What the artifact assumes / leaves unclear:** The design assumes the existing convention doesn't apply here because `pty_exited` flows through a different WS (booterm vs server broker). This is a correct assumption, but it's not documented. A future reader might see that contracts/ `WsFrameSchema` has `pty_exited` but the web's `WsFrame` doesn't, assume drift, and add it — creating dead code. - **Why this matters:** Low today, but risks a "fix" from someone who doesn't understand the two-WS architecture. - **Related questions:** Q11 - **Standard or precedent:** Root CLAUDE.md adding-new-WS-frame-type convention. - **Specialist to consult:** N/A - **Severity:** Polish - **Suggested next step:** Add a `// NOT in web WsFrame — booterm WS only` comment near the `PtyExitedFrame` definition in `ws-frames.ts`, or add a paragraph to design.md explaining why only the contract is updated, not the web/server union types. **JD-007: `getLastLines` filters "non-empty" lines — unclear what this means for terminal content** - **Protocol:** Evidence-and-Reasoning Check - **Location:** `design.md:130-136` - **Evidence:** ```typescript export function getLastLines(paneId: string, n: number): string[] { const buf = ringBuffers.get(paneId); if (!buf || buf.length === 0) return []; const nonEmpty = buf.filter(l => l.trim().length > 0); return nonEmpty.slice(-n); } ``` - The ring buffer stores raw PTY data split on newlines. Lines can contain ANSI escape sequences, carriage returns (common in terminal output), or be genuinely empty. - `trim()` removes whitespace but NOT ANSI escape characters. A line like `"\x1b[31m"` passes through (length 5 after trim). A line like `"\r"` is removed (length 0 after trim). A line containing only `\r` after a progress-bar overwrite is common. - Scenario from spec.md:15-16: "a process exits immediately without producing output" — this naturally produces empty `last_lines`. Fine. - But scenario spec.md:10-12: "normal process exit with output" requires "at least one `last_lines` entry." If the only output lines in the last N were carriage-return-only (progress bar/spinner output), the array could be empty, failing the spec assertion. - **What the artifact assumes / leaves unclear:** Assumes that filtering "non-empty" lines is straightforward for terminal content. Terminal output with progress bars, spinners, and ANSI art makes "empty" a spectrum, not a binary. - **Why this matters:** The spec asserts "at least one `last_lines` entry" for a normal exit with output. If the filtering is aggressive, this could fail. More importantly, the inference loop (future) or user seeing `last_lines` might be confused if a line like `"\x1b[2K\x1b[1G"` (erase-line + cursor-home) appears. - **Related questions:** Q9 - **Standard or precedent:** N/A - **Specialist to consult:** N/A - **Severity:** Worth clarifying - **Suggested next step:** Define "non-empty" explicitly in the design. Consider whether ANSI stripping is needed. Add a `// NOTE: filters whitespace-only lines` comment. Accept that some ANSI-only lines may appear. **JD-008: "Styled notification" for the web handler is underspecified** - **Protocol:** Scope & Definition-of-Done Check - **Location:** `design.md:112-115`, `spec.md:43-44` - **Evidence:** - Design.md:112-115: "Handle `pty_exited` in the message handler: Display a styled notification block (similar to existing `exit` handling at line 233-236) with exit code and last output lines." - Current exit handling (useTerminalSocket.ts:233-236): ```typescript if (frame?.type === 'exit') { t.write(`\r\n\x1b[2m[process exited with code ${frame.code}]\x1b[0m\r\n`); } ``` - This writes ANSI-dim text into the xterm terminal. It's "styled" in the sense of ANSI escape codes. - The new notification should also show `last_lines`. But how? Concatenated into the same terminal write? Each line on its own row? As a separate React element outside xterm? - The existing `{type: 'exit'}` handler doesn't display last lines because it doesn't have them. The new handler does. The presentation of "last output lines" is completely unspecified. - **What the artifact assumes / leaves unclear:** Assumes "styled notification" is unambiguous. It isn't. The two plausible implementations (ANSI text in terminal vs React component outside terminal) have very different scopes and testing implications. The tasks don't describe expected output in any testable way. - **Why this matters:** Two implementers could interpret this differently and produce different shipped behavior. If the answer is "write to terminal as ANSI text," the task is small. If the answer is "build a React notification component," it's significantly larger. - **Related questions:** Q5 (OQ3) - **Standard or precedent:** Current code uses terminal text for exit notifications. Terminal text is the convention for this code path. - **Specialist to consult:** `user-experience-designer` — terminal notification UX - **Severity:** Muddies artifact - **Suggested next step:** Clarify in design.md: "write last_lines as ANSI-dim text to xterm, each on its own line, followed by the exit code line" or "render a React notification banner above the terminal." Show an example of expected output. **JD-009: `parseServerFrame` return type needs update but `as` cast in current implementation may mask issues** - **Protocol:** Evidence-and-Reasoning Check - **Location:** `apps/web/src/lib/terminal-protocol.ts:37-46`, `useTerminalSocket.ts:233-236` - **Evidence:** - Current `parseServerFrame`: ```typescript export function parseServerFrame(data: string): ServerControlFrame | null { try { const parsed = JSON.parse(data) as { type?: string; code?: number }; if (parsed.type === 'init') return { type: 'init' }; if (parsed.type === 'exit') return { type: 'exit', code: parsed.code ?? 0 }; } catch { /* ... */ } return null; } ``` - The `as { type?: string; code?: number }` cast will NOT contain `last_lines`, `session_title`, etc. The `parseServerFrame` implementation needs to extract these fields for the new `pty_exited` object. - Task 5.2 says "Update `parseServerFrame` to recognize `type: 'pty_exited'` and return the structured frame." - **What the artifact assumes / leaves unclear:** The task exists but the design doesn't show the updated `parseServerFrame` implementation. The return type is `ServerControlFrame | null` — the new return variant needs `last_lines`, `exit_code`, `session_title`, etc. This is straightforward but the type cast pattern (`as { type?: string; code?: number }`) will need widening. - **Why this matters:** Low — the task covers it. But the design doesn't provide the updated code for review, so an implementer has to derive it from scratch. Minor polish issue. - **Related questions:** N/A - **Standard or precedent:** N/A - **Specialist to consult:** N/A - **Severity:** Polish - **Suggested next step:** Add the updated `parseServerFrame` code to `design.md` for review, showing how the widened type cast and extraction work. **JD-010: Task 7 verification doesn't include a unit test for `getLastLines`** - **Protocol:** YAGNI Evidence Sweep - **Location:** `tasks.md:32-37` - **Evidence:** - Task 7 (Verify) checks: contracts build, booterm build, web typecheck, grep for `pty_exited` - No mention of running `pnpm -C apps/booterm test` or adding a test for `getLastLines` - `getLastLines` is a new pure function with edge cases (empty buffer, non-empty lines filter, partial lines, ANSI-only lines) - **What the artifact assumes / leaves unclear:** Assumes type-checking is sufficient verification for `getLastLines`. The function has filtering logic (`trim().length > 0`) and slice behavior that type-checking won't catch. A unit test costs ~5 minutes to write but would catch e.g. the `[]`-on-empty-buffer case. - **Why this matters:** Not blocking, and YAGNI says don't add tests speculatively. But `getLastLines` is a pure function with clear inputs/outputs — exactly the kind of code where a missing test means the first bug ships silently. - **Related questions:** Q9 - **Standard or precedent:** Root CLAUDE.md mentions "Extract pure helpers to unit-test" — `backends/turn-guard.ts` and `lifecycle-decisions.ts` are given as the pattern. - **Specialist to consult:** N/A - **Severity:** Worth clarifying - **Suggested next step:** Add a task 7.5: `pnpm -C apps/booterm test` passes. Or add a unit test for `getLastLines` alongside its implementation (task 2) and note it in verification. > **Protocol 1 - Clarifying-Question Sweep:** Complete. 13 questions generated, 5 answered, 5 assumed, 3 open. See Question Log and OQ1-OQ4. > **Protocol 2 - Hidden-Assumption Audit:** Complete. Four assumptions surfaced in JD-002 (hardcoded timed_out), JD-003 (unregister timing), JD-004 (ring buffer race), and JD-007 (empty-line definition). > **Protocol 3 - Evidence-and-Reasoning Check:** Complete. One uncited claim found (reference implementation, Q8). Number claims are absent (no "10x faster" etc.). The reasoning chain from "current data model exists" to "just surface it" is sound in principle but misses the timed-out coordination gap. > **Protocol 4 - Standards & Conventions Conflict Check:** Complete. One convention departure found (JD-006: web `WsFrame` not updated per root CLAUDE.md) — deliberately justified but undocumented. One schema-landmine found (JD-005: session_id type mismatch with UUID convention). Drift test in `ws-frames.test.ts` would pass because `KNOWN_FRAME_TYPES` and the Zod union would both be updated. > **Protocol 5 - Specialist-Domain Boundary Check:** No specialist domains deeply touched. The timeout coordination logic and ring-buffer data race are concurrency-ish but fixable by a generalist. The web notification presentation (JD-008) borders on UX — flagged for `user-experience-designer` but manageable with a clearer spec. No security, compliance, or production-readiness changes. > **Protocol 6 - Scope & Definition-of-Done Check:** Complete. Done is stated (tasks.md checkboxes). One undone requirement found (inference-loop spec, JD-001). Rollback is not documented. Post-ship ownership is unassigned (OQ4's session_id mismatch will need future knowledge). > **Protocol 7 - YAGNI Evidence Sweep:** Complete. Most items pass the evidence test (existing data pattern, explicit user ask). One YAGNI candidate surfaced (JD-010: no unit test for getLastLines — borderline, recommended to add). The `PtyExitedFrame` schema entry in contracts could be argued as YAGNI (nothing publishes it through the broker yet), but adding the contract shape early is standard practice for the project and prevents future drift — keep it. > **Protocol 8 - Plain-Language Reframing:** Complete (see Plain-Language Restatement at top). The restatement exposes two holes: (a) the inference-loop contradiction ("tell the inference loop about PTY exits" vs "defer that") and (b) the timeout integration gap ("set a flag... that the handler never reads"). ## Junior-Developer Review Summary ### What I Don't Understand Yet 1. **Spec vs Design contradiction on inference-loop delivery (OQ1).** The spec says MUST. The design says definitely not now. Which one wins? 2. **How the timeout path actually works (OQ2).** The design sets a `timedOut` flag on the registry entry, but the `onExit` handler never reads it — it hardcodes `false`. And `sweepExpired` deletes the registry entry before `onExit` can even look at it. 3. **What "styled notification" means for the web frontend (OQ3).** Is it more ANSI text written to xterm, or a React component? The two options have very different build and test scopes. 4. **How the session_id mismatch will be resolved for future broker publish (OQ4).** Booterm uses short IDs; the server broker uses UUIDs. The contract schema accepts both. When someone wires up publish, it'll pass validation but carry the wrong-shaped key. ### What the Artifact Seems to Assume - **The `timed_out` flag propagates automatically.** It doesn't — the `onExit` handler code explicitly sets `timed_out: false`. (JD-002) - **Ring buffer data survives the `sweepExpired` cleanup path.** It doesn't — `unregister` deletes it before `onExit` runs. (JD-004) - **The reference implementation is straightforward to copy.** The fork at `/opt/forks/opencode-extras/opencode-pty` is cited but no specifics (ANSI stripping, line count, metadata shape) are extracted. (Q8) - **The existing unit test for `getLastLines` isn't needed.** Pure functions with filtering logic are exactly the kind of code the project patterns recommend testing. (JD-010) - **"Non-empty lines" is unambiguous for terminal content.** Progress bars, spinners, and ANSI art make this ambiguous. (JD-007) ### Where the Artifact Conflicts with How We Already Work - **Root CLAUDE.md convention** for adding WS frame types says to update the web `WsFrame` and server `InferenceFrame`. The design skips both (correctly, because booterm WS ≠ server broker WS), but doesn't explain why. (JD-006) - **UUID convention** in `WsFrameSchema` — all session/chat/message identifiers are UUIDs. The `PtyExitedFrame` uses `z.string().min(1)` for `session_id`, which will become a problem when broker publish is added. (JD-005) ### Where a Specialist Should Take Over - **`user-experience-designer`** — The "styled notification" rendering for the web frontend needs a clearer spec than "display a styled notification block." (JD-008) ### What "Done" Looks Like — and What It Doesn't Done is the 7 task checkboxes. But: - Spec requirement 4 (inference-loop subscription) has no checkbox. If the spec is the requirements source, done won't be done. (JD-001) - The timeout path tasks (4.1, 4.2) look done on paper but produce the wrong behavior (always `timed_out: false`). (JD-002) - No verification step runs booterm tests. (JD-010) - No rollback plan described. ### What the Artifact Includes That Has No Evidence of Being Needed - **Unit test for `getLastLines`** — currently absent from tasks. This is a borderline YAGNI candidate *against* the tasks (the tasks don't include it, but the project patterns recommend it for pure helpers). Recommend adding a test rather than skipping it, as the filtering logic (trim/length) has edge cases that type-checking won't catch. ### The Artifact in Plain Terms We want to send a richer message when a terminal process dies. Instead of "process exited with code 0", send "process exited with code 0, and here are the last 5 lines of output, and the session was called 'build' spun up by 'claude', and it was/wasn't killed by a timeout." Add the message type to the shared contract. Update the web UI to show it. Don't wire up the server-side inference loop yet. The two things that don't work in this summary are: (1) the spec says the inference loop MUST be wired up, contradicting "don't wire it up yet", and (2) the timeout path says "timed_out: true" for timeout-killed processes, but the code always sends "timed_out: false".