boocode/junior-dev-review.md

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 <pty_exited> 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:

    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:

    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:

  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):

    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:

    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".