Expose session tree inspection and navigation over RPC

[m-aebrer]

Closes #313

Adds get_tree and navigate_tree RPC commands so external frontends (the planned web dashboard) can inspect the session tree and navigate to another node — TUI /tree parity over RPC. Also adds a missing streaming guard to AgentSession.navigateTree in core.

Implementation plan posted as a comment below.

[m-aebrer]

Implementation Plan

Problem

The planned web dashboard (issue 307) needs session-tree visibility and navigation for parity with the TUI's /tree command. Core already implements both — SessionManager.getTree() returns SessionTreeNode[] and AgentSession.navigateTree(targetId, options) performs navigation with optional branch summarization — but neither is reachable as a first-class RPC command. navigateTree is wired in rpc-mode.ts only via commandContextActions for extensions; an external RPC client cannot call it.

Prior art

The Agent Client Protocol (ACP) has a session/list RFD but no standard for conversation-tree inspection or navigation — session trees are a dreb-specific concept. The right reference is therefore dreb's own established pattern: the list_all_sessions / delete_session work (PR for issue 312) introduced small exported handler/mapper functions in rpc-mode.ts that are unit-testable without a live RPC process, plus typed command/response variants, RpcClient methods, docs, and tests. This plan follows that pattern exactly.

Design decisions

1. Tree DTO — trimmed, stable, no raw entry dumps

New RpcTreeNode interface in rpc-types.ts. Per node:

• id — entry id
• parentId — parent entry id or null
• type — entry type (message, compaction, branch_summary, model_change, thinking_level_change, custom, custom_message, label, session_info)
• role — message role for type: "message" entries (user, assistant, toolResult, bashExecution); absent otherwise
• preview — short single-line content preview, whitespace-normalized, capped at 200 chars (mirrors the TUI tree selector's extraction logic, minus styling: user/assistant text, tool name for tool results, bash command, summary text for branch summaries/compactions, model id for model changes, etc.)
• timestamp — ISO timestamp (already a string on SessionEntry)
• label — resolved label if any
• children — child nodes, sorted oldest-first (as getTree() already returns them)

Response payload: { roots: RpcTreeNode[], leafId: string | null }. leafId (from sessionManager.getLeafId()) is included because a tree UI cannot render "you are here" without it — the TUI selector receives the same value.

Full message payloads are deliberately not serialized; get_messages covers content after navigation.

2. navigate_tree command

{"type": "navigate_tree", "targetId": "abc123", "summarize": true, "customInstructions": "...", "replaceInstructions": false, "label": "..."}

Options pass through verbatim to AgentSession.navigateTree — same signature the extension wiring uses. RPC does not replicate the TUI's summarize-prompt UX and does not consult getBranchSummarySkipPrompt; explicit options only (per the issue's technical note; reserveTokens from getBranchSummarySettings still applies inside core, which is a summarizer implementation detail, not prompt behavior).

Response: { cancelled: boolean, editorText?: string }.

editorText is a deliberate small superset of the issue's proposed { cancelled }: core already returns it (the text of the user message navigated to), and a dashboard needs it for TUI parity — after navigating to a user message the editor is pre-filled with that text. Omitting it would force a second command and awkward client-side reconstruction. Flagging explicitly since the issue spec only mentions { cancelled }.

3. Core fix — streaming guard in AgentSession.navigateTree

navigateTree currently has no isStreaming guard. Navigating mid-stream would call agent.replaceMessages() during an active run — a latent corruption bug reachable from the TUI and extensions too, not just RPC. The issue's acceptance criteria mention "explicit errors for … navigation during streaming (matching existing AgentSession guards)", but no such guard exists today; it must be added.

Fix in core, not in the RPC handler: add a loud throw at the top of navigateTree when this.isStreaming (same style as prompt()'s guard). The RPC layer maps thrown errors to success: false responses via its existing try/catch; the TUI already catches and displays thrown errors from navigateTree, so its behavior changes from silent corruption risk to a visible error message.

4. Error paths

All surface as explicit success: false RPC responses (never silent):

• Unknown targetId — existing core throw (Entry <id> not found)
• Navigation during streaming — new core guard
• summarize: true with no model/API key — existing core throws

5. RpcClient timeout

navigate_tree with summarize: true is LLM-bound and can exceed the client's fixed 30s response timeout. RpcClient.navigateTree will pass a longer/overridable timeout — implemented as an optional per-request timeout parameter on the private send() (defaulting to the current 30s), so other methods are unaffected.

Deliverables

1. get_tree and navigate_tree command + response variants in rpc-types.ts, including the documented RpcTreeNode DTO
2. Handlers in rpc-mode.ts with exported, unit-testable helpers (toRpcTreeNode mapper, getTreeForRpc) following the toRpcSessionInfo / deleteSessionForRpc pattern
3. Streaming guard in AgentSession.navigateTree (core)
4. RpcClient.getTree() and RpcClient.navigateTree(targetId, options?) methods
5. Tests (new test/rpc-tree-commands.test.ts + a guard test in the tree-navigation suite)
6. docs/rpc.md — new "Session Tree" section documenting both commands, the DTO shape, error cases, and the summarize-timeout caveat
7. CHANGELOG.md entry

Files to create or modify

File	Change
packages/coding-agent/src/modes/rpc/rpc-types.ts	Add get_tree / navigate_tree command variants, RpcTreeNode interface, response variants
packages/coding-agent/src/modes/rpc/rpc-mode.ts	Add exported toRpcTreeNode + getTreeForRpc helpers and the two handleCommand cases
packages/coding-agent/src/core/agent-session.ts	Add isStreaming guard at top of navigateTree
packages/coding-agent/src/modes/rpc/rpc-client.ts	Add getTree() / navigateTree() methods; optional per-request timeout on send()
packages/coding-agent/test/rpc-tree-commands.test.ts	New test file (see below)
packages/coding-agent/test/agent-session-tree-navigation.test.ts	Add streaming-guard test if expressible without API key; otherwise guard test lives in the new file with a stubbed session
packages/coding-agent/docs/rpc.md	New "Session Tree" section after "Session Listing"
packages/coding-agent/CHANGELOG.md	Unreleased → Added entries

Testing approach

All new tests run without an API key, using SessionManager.inMemory() and the existing buildTestTree helper from test/utilities.ts:

• DTO mapping (toRpcTreeNode / getTreeForRpc): branched tree maps with correct parent/child structure and ordering; labels resolved; previews single-line and truncated at 200 chars; per-entry-type previews (user/assistant/tool/bash/branch_summary/compaction/model_change); no message/full-content field leaks into the DTO; leafId reflects current leaf including null for a fresh session
• Handler wiring (navigate): options object forwarded verbatim to session.navigateTree (spy); unknown target id → success: false with core's error text; streaming guard → success: false (session stub with isStreaming: true); successful navigation without summarize → { cancelled: false, editorText } mapped correctly
• Post-navigation state: after navigate_tree to an earlier node, sessionManager.getLeafId() and rebuilt messages reflect the target branch — satisfying the acceptance criterion that get_state / get_messages reflect post-navigation state (both read from the same session/agent state)
• RpcClient: getTree() sends {type: "get_tree"} and unwraps data; navigateTree() sends command with options and unwraps { cancelled, editorText }; error responses reject with the RPC error message (mocked send, same style as rpc-session-commands.test.ts)
• Summarization end-to-end stays covered by the existing API-key-gated agent-session-tree-navigation.test.ts suite; the RPC layer's responsibility is verified as option pass-through, not summarizer behavior

Risks and open questions

• editorText supersets the issue spec — flagged above with rationale; trivially removable if unwanted
• Streaming guard changes TUI behavior — from silent corruption risk to a visible error. Strictly an improvement, but it is a core behavior change shipped in an RPC-titled PR; called out so reviewers see it
• No scriptable abort for branch summarization — abortBranchSummary() exists in core but is not exposed over RPC; a summarizing navigate_tree cannot be cancelled mid-flight by an RPC client (the general abort command targets the agent loop, not the summarizer). Out of scope here; noted as a consideration for the dashboard issue 307

---

Plan created by mach6

[github-actions]

Vitest coverage

Metric	Covered	Total	Coverage
Statements	18425	34532	53.35%
Branches	10030	21418	46.82%
Functions	2899	5423	53.45%
Lines	16284	30713	53.01%

View full coverage run

[m-aebrer]

Progress Update

Full implementation of the plan — all deliverables complete:

• rpc-types.ts — get_tree / navigate_tree command variants, documented RpcTreeNode DTO (id, parentId, type, role, 200-char single-line preview, timestamp, label, children), response variants
• rpc-mode.ts — exported unit-testable helpers following the established pattern: toRpcTreeNodes (iterative with explicit stack — deep linear sessions cannot overflow the call stack), getTreeForRpc (returns { roots, leafId }), navigateTreeForRpc (verbatim option pass-through, returns only { cancelled, editorText? }). Handlers wired into the command switch; navigate_tree errors surface with exact core error text
• agent-session.ts — new streaming guard at the top of navigateTree: throws loudly instead of silently replacing agent messages during an active run (latent bug previously reachable from TUI/extensions too)
• rpc-client.ts — getTree() and navigateTree() methods; private send() gained a per-request timeout, navigateTree defaults to 5 minutes (summarize is LLM-bound) via a client-side-only timeoutMs option that never goes over the wire
• rpc/index.ts — RpcTreeNode re-exported for consumers
• Tests — new rpc-tree-commands.test.ts (18 tests, no API key needed): DTO mapping incl. branch ordering/labels/whitespace-collapse/truncation/no-payload-leakage, 5000-entry deep-tree safety, option forwarding, real-session navigation (editor text, leaf + messages reflect target branch), unknown-id error, streaming-guard rejection, RpcClient wire format (timeoutMs stripped)
• Docs — new "Session Tree" section in docs/rpc.md (DTO shape, options, editorText semantics, error cases, timeout caveat); CHANGELOG entries

Verification: full workspace build, biome clean, entire test suite green (4014 passed / 0 failed via pre-commit hook). Additionally QA'd against the compiled binary in live RPC mode: tree DTO shape, payload-leakage check, navigation, post-navigation get_state/leafId consistency, and explicit error responses all confirmed end-to-end.

Commit: 89c64cb

---

Progress tracked by mach6

[m-aebrer]

Code Review

Important

Finding 1 — Streaming guard is a TOCTOU check; concurrent prompt during a summarizing navigation can still corrupt state (error-auditor, confidence 82)
packages/coding-agent/src/core/agent-session.ts (guard at ~3472, mutation at ~3639). The new guard checks isStreaming once at entry, but with summarize: true there are long await points (LLM branch summary, session_before_tree extension emit) between the check and agent.replaceMessages(...). RPC command dispatch is not serialized (void handleInputLine(line) per line) and prompt is fire-and-forget, so a client can send navigate_tree {summarize:true} then prompt: the prompt starts streaming while the summary is awaited, and when navigation resumes it replaces messages mid-stream — exactly what the guard's comment says it prevents. The prompt path (~783-787) already documents and re-checks this same race class after its await. Suggested fix: re-check isStreaming immediately before the replaceMessages mutation (and after each await), or hold a flag for the duration of navigateTree that prompt/steer respect. The no-summarize, no-async-extension path has no await between guard and mutation and is safe.

Suggestions

Finding 2 — navigate_tree client timeout silently diverges client state from server state; the late response is swallowed (error-auditor, confidence 85, severity medium)
packages/coding-agent/src/modes/rpc/rpc-client.ts (send timeout handling, handleLine). When a summarizing navigate_tree exceeds timeoutMs, the client rejects with a generic timeout and deletes the pending request — but the server keeps running and completes the navigation, moving the session leaf. The late success:true response no longer matches a pending request and falls through to the event-listener path, where it is silently ignored. A caller that retries or assumes the leaf is unchanged now operates against a moved tree. Docs mention the 5-minute timeout and the lack of a scriptable abort, but not that a timed-out navigation may still have completed server-side. Suggested fix: state in the timeout error that server state may have changed (and recommend a get_tree/get_state resync); at minimum document the divergence.

Finding 3 — getRpcEntryPreview returns undefined for unknown entry types, violating the preview: string contract (code-reviewer, confidence 85, severity medium)
packages/coding-agent/src/modes/rpc/rpc-mode.ts (~137-185). The switch (entry.type) has no default and no trailing return. Compile-time exhaustiveness doesn't hold at runtime: session files are parsed with JSON.parse(line) as FileEntry (no validation), so a forward-compat or corrupted entry with an unknown type flows through and produces preview: undefined in a DTO typed preview: string. The TUI equivalent (getEntryDisplayText) has a defensive default. Given the fail-loudly preference, suggested fix: exhaustiveness assertion that throws on unknown type (surfaces as an explicit success:false via the outer dispatch catch), or at minimum a placeholder string like [<type>].

Finding 4 — Most getRpcEntryPreview entry-type/role branches are untested (test-reviewer, confidence 88, severity medium)
Tests assert only 4 of ~13 preview branches (user, thinking_level_change, branch_summary, label). Untested but reachable: assistant fallbacks ((aborted), errorMessage, (no content)), toolResult → [<toolName>], bashExecution → [bash]: <command>, custom_message, compaction ([compaction: Nk tokens]), model_change, custom, session_info. These are the only human-readable representation of those nodes over RPC; a broken template ships silently. All constructible without an API key via SessionManager append methods. Suggested: parametrized preview-string assertions per entry type/role.

Finding 5 — RpcClient.navigateTree default 300000 ms timeout is untested (test-reviewer, confidence 85, severity medium)
The test passes timeoutMs: 123 explicitly; the default path (300000 when omitted) is never exercised. A regression dropping the default to send's 30000 would cause spurious timeouts on long summarizations and no test would catch it. Suggested: client.navigateTree("u1") with no options, assert send called with (..., 300000); optionally assert getTree() uses the single-arg default.

Finding 6 — RpcTreeNode.parentId doc comment ("null for a root") is inaccurate for orphaned roots (code-reviewer, confidence 82, severity low)
packages/coding-agent/src/modes/rpc/rpc-types.ts (~324-326). getTree() promotes orphaned entries (parent id pointing at a missing entry) to roots; such a root carries a non-null parentId referencing an entry not present in the tree. A client reconstructing hierarchy from parentId instead of children would misattach orphans. Suggested: clarify the comment (roots include orphans with non-null parentId; prefer children for hierarchy) — mapping behavior itself is correct.

Strengths

• toRpcTreeNodes is a clean, correct iterative translation with right sibling/DFS ordering; the "no raw payloads" DTO boundary is well-executed and pinned by an excellent leakage test (exact Object.keys + JSON.stringify negative assertions).
• The 5000-entry deep-linear-tree test is a real, targeted regression guard for the explicit-stack design.
• send(command, timeoutMs = 30000) is backward-safe — default matches the old hardcoded value; timeoutMs correctly stripped from the wire command and verified by test.
• Placing the streaming guard in core rather than the RPC handler was the right call — it closes the latent bug for TUI/extension callers too, and existing TUI call sites are idle-gated and try/catch-wrapped.
• Preview logic faithfully tracks the TUI's getEntryDisplayText (aborted/error/no-content fallbacks, bash/tool/label/compaction forms) — genuine /tree parity.
• Completeness: all issue acceptance criteria verified met, including the plan's fallback path for the streaming-guard test location. Simplifier found no unnecessary complexity: the TUI/RPC preview duplication is justified (styled vs plain output), and the unknown-based content extraction is doing different work than the typed helpers.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

---

Reviewed by mach6

[m-aebrer]

Review Assessment

Assessing the review findings above (#316 (comment)). Each finding was independently verified against the actual source.

Classifications

Finding	Classification	Reasoning
1 — Streaming guard is TOCTOU; concurrent prompt during summarizing navigation	Deferred	Genuine race, verified: generateBranchSummary does not set agent.state.isStreaming, RPC dispatch is unserialized, and prompt is fire-and-forget — so replaceMessages can land mid-stream. But it is not a regression from this PR (the same class already exists via fork), requires atypical concurrent client behavior, and the cheap re-check only narrows the window — the real fix is serializing RPC command dispatch, an architectural change out of scope. The guard this PR adds does what it claims and is a net improvement. Needs a tracking issue (none exists).
2 — navigate_tree client timeout diverges client/server state; late response swallowed	Nitpick (docs)	Divergence claim verified accurate, but it is inherent to every timed RPC request (compact etc.), and the PR already mitigates with a 5-minute default and documents no-scriptable-abort. Worth one doc sentence recommending a get_tree/get_state resync after timeout; not blocking.
3 — getRpcEntryPreview returns undefined for unknown entry types	Genuine (low)	Verified: no default, no trailing return; parseSessionEntries does JSON.parse with no validation and getTree builds nodes for every entry, so a forward-compat/corrupt entry yields preview: undefined in a preview: string DTO. TUI equivalent has a defensive default; fail-loudly norm supports fixing. Trivial fix.
4 — Most getRpcEntryPreview branches untested	Genuine	Verified: only 4 of ~13 branches asserted. Untested branches (assistant aborted/error/no-content, toolResult, bashExecution, custom_message, compaction, model_change, custom, session_info) are all constructible without an API key via existing SessionManager.append* methods. New code in this PR — test gaps must not be deferred.
5 — RpcClient.navigateTree default 300000 ms timeout untested	Genuine (low)	Verified: only explicit timeoutMs: 123 tested. A regression to the 30 s default would cause spurious timeouts on long summarizations with no test catching it. Trivial one-liner.
6 — parentId JSDoc "null for a root" inaccurate for orphaned roots	Nitpick	Verified orphan roots keep a dangling non-null parentId, but docs/rpc.md already documents orphans-as-roots; only the terse JSDoc wording is loose. Low materiality.

Counts: 3 genuine, 2 nitpicks, 0 false positives, 1 deferred.

Action Plan

1. Finding 3 — Add a default case to getRpcEntryPreview with a compile-time exhaustiveness guard (const _exhaustive: never = entry) returning a safe placeholder ([<type>]) so one unknown entry can't blank the tree or violate the DTO contract.
2. Finding 4 — Add preview-branch tests in test/rpc-tree-commands.test.ts for all untested branches, constructing entries via SessionManager.append*; include an assertion for the new unknown-type placeholder from item 1.
3. Finding 5 — Add a default-timeout test: navigateTree(id) with no options asserts send(..., 300000).
4. Finding 6 (polish) — Tighten the RpcTreeNode.parentId JSDoc (orphaned roots may carry a non-null parentId referencing a missing entry; prefer children for hierarchy).
5. Finding 2 (polish) — One sentence in docs/rpc.md: a timed-out navigate_tree may still complete server-side; reconcile via get_tree/get_state.

Follow-up to track separately (finding 1): file an issue for RPC command-dispatch serialization / navigation-vs-prompt atomicity (unserialized void handleInputLine + fire-and-forget prompt allows replaceMessages mid-stream; also affects fork).

---

Assessment by mach6

[m-aebrer]

Review Assessment — Amendment: finding 1 reclassified

Amending the assessment above: finding 1 (streaming-guard TOCTOU) is reclassified from deferred to genuine — fix in this PR.

Why the reclassification

Two corrections to the original assessment, based on a closer read of navigateTree (agent-session.ts):

1. The "re-check only narrows the window" claim was wrong. The mutation section (from the "Switch leaf" block through agent.replaceMessages) is fully synchronous — branchWithSummary/resetLeaf/branch, appendLabelChange, buildSessionContext, replaceMessages, with zero await points. JS is single-threaded, so an isStreaming re-check placed after the last await (extension emit / generateBranchSummary) fully closes the corruption race: isStreaming cannot change between a synchronous check and a synchronous mutation. And since branchWithSummary is the first session write, throwing at the re-check leaves the session completely unmutated — a clean, loud success: false. This is a ~6-line core fix plus one test (spy the isStreaming getter to flip after first access; exercisable without an API key), well within PR scope.

2. The dashboard (issue 307) makes the race normal usage, not atypical. Multi-device access (desktop + mobile via Tailscale) is a headline MVP feature; two browsers funneling into one RPC session make "navigate-with-summarize on one device, prompt from another" expected behavior, with a race window spanning the entire LLM-bound summary (minutes). Deferring a state-corruption bug that a planned first-party feature will reliably expose is the wrong call.

What stays out of scope

Full RPC command-dispatch serialization remains a follow-up: navigate-vs-navigate interleavings, the semantic oddity of a prompt completing entirely within the summary window (append-only tree, no corruption — the new entries are simply abandoned by the navigation, arguably as requested), and fork's milder pre-existing exposure. With the re-check landing here, that follow-up drops from "corruption bug" to "multi-client robustness hardening" — still worth a tracking issue for the dashboard's benefit, but no longer urgent.

Updated classifications

Counts: 4 genuine (findings 1, 3, 4, 5), 2 nitpicks (findings 2, 6), 0 false positives, 0 deferred.

Updated Action Plan

1. Finding 1 — Re-check isStreaming in navigateTree after the last await (immediately before the leaf-position/mutation block), throwing the same loud error as the entry guard. Add a test: navigation rejects when streaming starts during the summary/extension await, and session state (leaf, entries) is unchanged afterward.
2. Finding 3 — default case in getRpcEntryPreview with compile-time never exhaustiveness guard returning a [<type>] placeholder.
3. Finding 4 — Preview-branch tests for all untested entry types/roles (via SessionManager.append*), including the new unknown-type placeholder.
4. Finding 5 — Default-timeout test: navigateTree(id) with no options asserts send(..., 300000).
5. Finding 6 (polish) — Tighten RpcTreeNode.parentId JSDoc re orphaned roots.
6. Finding 2 (polish) — Doc sentence: a timed-out navigate_tree may still complete server-side; reconcile via get_tree/get_state.

Optional follow-up issue (downgraded, no longer tracks a corruption bug): serialize RPC command dispatch for multi-client robustness (navigate-vs-navigate, fork exposure) ahead of the dashboard work.

---

Assessment by mach6

[m-aebrer]

Progress Update

All six review findings addressed (per the amended assessment: findings 1, 3, 4, 5 genuine; 2, 6 nitpick polish):

• Finding 1 (TOCTOU race) — AgentSession.navigateTree now re-checks isStreaming after the extension/summarization awaits, immediately before the fully-synchronous mutation block. Since no await separates the re-check from replaceMessages, the corruption race is closed, not narrowed; a throw there leaves the session unmutated. New test forces the yield with an async session_before_tree extension handler, flips the isStreaming getter false→true, and asserts both the rejection and unchanged leaf/entries.
• Finding 3 (unknown entry types) — getRpcEntryPreview gained a default case with a compile-time never exhaustiveness guard and a runtime [<type>] placeholder, so forward-compat/corrupt session entries can no longer produce preview: undefined.
• Finding 4 (preview coverage) — 4 new tests assert exact preview strings for every branch: assistant text / (aborted) / error-message-normalized / (no content), [read], [tool] fallback, [bash]: npm test, unknown message role, [note]: hello there, [compaction: 50k tokens], [model: ...], [custom: artifact], [title: ...], and the new unknown-type placeholder.
• Finding 5 (default timeout) — new test pins navigateTree with no options to send(..., 300000); the getTree test now also asserts the single-arg (30s default) path.
• Finding 6 (parentId docs) — RpcTreeNode.parentId JSDoc and docs/rpc.md now note orphaned roots keep a dangling non-null parentId; prefer children for hierarchy.
• Finding 2 (timeout divergence docs) — docs/rpc.md now warns a client-side timeout does not stop the server: a timed-out navigate_tree may still complete and move the leaf; resync via get_tree/get_state.

Test suite for the file: 24 passed (up from 18). Pre-commit full run: 4036 passed / 0 failed. Biome clean; full workspace build green.

Commit: 6925e7e

---

Progress tracked by mach6

[m-aebrer]

Code Review (round 2)

This round verified the fixes from the previous review cycle and hunted for new issues. All six prior findings are confirmed genuinely resolved — both code-reviewer and error-auditor independently read the full navigateTree method and confirmed the TOCTOU re-check is correctly placed: no await exists between the re-check and agent.replaceMessages (the next await, the session_tree emit, occurs after the mutation), so the corruption race is fully closed, not narrowed. The exhaustiveness guard covers all 9 SessionEntry union members; the timeout-divergence doc mitigation is present and thorough.

Important

Finding 1 — navigateTree re-check throw wedges isCompacting true (abort-controller leak) (error-auditor, confidence 85, severity medium)
packages/coding-agent/src/core/agent-session.ts (controller set at ~3521, resets only at ~3572 and ~3663, getter at ~1325). this._branchSummaryAbortController = new AbortController() is assigned unconditionally on every navigateTree call, but reset to undefined on only two paths: inside the generateBranchSummary branch and at the success return. The new streaming re-check throws between the set and both resets, so tripping it leaves the controller defined — and isCompacting (computed as _branchSummaryAbortController !== undefined) reports true indefinitely, until a later navigateTree happens to overwrite the field. Consequences of the wedged state: interactive mode queues all subsequent user input as "during compaction" and blocks session reload ("Wait for compaction to finish before reloading"); RPC get_state reports isCompacting: true forever, misleading external frontends. The loud throw is correct — the lingering state is the silent failure that outlives it. The same leak pre-exists on two other early-exit paths (the session_before_tree cancel return and the "No API key" throw), so a patch targeting only the new throw would leave those latent. Suggested fix: wrap the body from the controller allocation onward in try { ... } finally { this._branchSummaryAbortController = undefined; } and drop the two manual resets — eliminating the wedged-state class on every exit path.

Suggestions

Finding 2 — get_tree / navigate_tree command dispatch never exercised end-to-end (test-reviewer, confidence 84, severity medium)
All 24 tests drive the exported helpers (getTreeForRpc, navigateTreeForRpc, toRpcTreeNodes) or RpcClient with send mocked; nothing exercises the actual handleCommand switch cases. Two pieces of new dispatch logic have zero coverage: (a) the option field-mapping in the navigate_tree case (summarize: command.summarize, ... label: command.label) — a dropped or mistyped field would compile fine and pass every existing test while silently disabling summarization or labels over RPC; (b) the case-level try/catch error contract (removal would surface errors as the generic top-level Command failed: ... instead of the documented clean error strings). The repo already has the idiom: rpc.test.ts spawns a real RPC child and drives commands through the live switch, and both new commands work without an API key when not summarizing. Suggested: add a spawned-client test — getTree() round-trip, navigateTree(id, { label }) asserting the label took effect via a subsequent getTree, and unknown-target rejection carrying the server-side error text.

Finding 3 — summarize: true with no model error path untested (completeness-checker, confidence 85, severity low)
AgentSession.navigateTree throws No model available for summarization (agent-session.ts ~3489) and both CHANGELOG and docs/rpc.md list it as one of the three explicit error cases — but unlike unknown-targetId, streaming-guard, and TOCTOU (which all have dedicated tests), this path has no test. Straightforward to add: session without a model, navigateTreeForRpc with summarize: true, assert the rejection message.

Finding 4 — session_info preview diverges from TUI for empty-string titles (code-reviewer, confidence 88, severity low)
packages/coding-agent/src/modes/rpc/rpc-mode.ts (~172). RPC uses entry.name ?? "empty" (nullish check) while the TUI's tree-selector uses a truthy check (entry.name ? ... : "[title: empty]"). SessionInfoEntry.name is name?: string and can be the empty string (e.g. set_session_name ""): the TUI renders [title: empty], RPC renders [title: ]. Given the PR's stated goal of TUI preview parity, suggested fix: entry.name || "empty". Edge case; not a functional defect.

Strengths

• The TOCTOU fix is verified sound by two independent agents: the post-re-check mutation block (_extractUserMessageText, branchWithSummary/resetLeaf/branch, appendLabelChange, buildSessionContext, replaceMessages) is fully synchronous — single-threaded JS guarantees atomicity.
• The TOCTOU regression test is non-vacuous: the call-counting mock (isStreamingCalls++ > 0) produces exactly false-then-true, matching the real two-read sequence, so it exercises the re-check path rather than the entry guard — and asserts unchanged leaf/entries afterward.
• Error propagation is clean throughout: get_tree throws surface loudly via the outer dispatch catch; navigateTreeForRpc propagation is correct; no unhandled rejections in void handleInputLine; RpcClient pending-request cleanup on timeout is correct.
• The exhaustiveness pattern (const _exhaustive: never), conditional DTO spreads, whitespace-collapse regex, and inline error strings all match existing repo conventions exactly — the simplifier found zero avoidable complexity in the fix commit and specifically endorsed keeping the duplicated guard string (repo convention is inline error literals).
• Completeness: all six issue acceptance criteria verified against actual code, including the technical note (RPC honors explicit options only; skipPrompt is never consulted — it's TUI-only).

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

---

Reviewed by mach6

[m-aebrer]

Review Assessment (round 2)

Assessing the round-2 review findings above (#316 (comment)). Each finding was independently verified against the actual source, with full control-flow tracing of navigateTree.

Classifications

Finding	Classification	Reasoning
1 — navigateTree re-check throw wedges isCompacting true (abort-controller leak)	Genuine (fix first)	Verified by tracing every exit path of navigateTree. The controller is set unconditionally (~3521); the new re-check throw (~3591) is reached with it still set on the non-summarize path (the summarize branch's reset never ran), permanently wedging isCompacting = true. Confirmed real consumers misbehave: interactive mode queues all input as "during compaction" and blocks reload; RPC get_state reports isCompacting: true forever. A genuine regression introduced by the fix commit, plus two pre-existing leaks on the cancel return and no-API-key throw. One correction to the review: on a real summarize path the reset already fired before the re-check, so the throw is safe there — the leak is specific to non-summarize / empty-branch / extension-summary paths. The suggested try/finally fix is verified safe: abortBranchSummary only reads the controller during in-flight await windows; a finally clears it only after the method settles.
2 — get_tree/navigate_tree dispatch never exercised end-to-end	Genuine (minor)	Verified: all 24 tests drive exported helpers or a mocked client; the handleCommand case glue (option field-mapping, inner try/catch clean-error contract) is new untested code. Correction: handleCommand is a non-exported closure, and the whole rpc.test.ts spawned-child suite is API-key-gated (describe.skipIf) — so the added test will be key-gated too, consistent with how all existing dispatch is tested. New-code coverage ships with the PR per repo norms.
3 — summarize:true with no model error path untested	Genuine (minor)	Verified: the throw exists (~3488, before the controller set), is one of three documented navigate_tree error cases, and is the only one without a test. Correction: createTestSession always sets a model, so the test mocks the model getter — same pattern as the existing isStreaming spy. Trivial.
4 — session_info preview diverges from TUI for empty-string titles	Genuine (trivial)	Verified both sites: RPC uses ?? "empty" (nullish), TUI uses truthy. Empty name IS reachable — the extension setSessionName("") path appends name.trim() = "" with no empty-guard (only the RPC set_session_name command rejects empty). One-character fix (?? → `

Counts: 4 genuine (1 correctness, 1 trivial correctness, 2 test coverage), 0 nitpicks, 0 false positives, 0 deferred.

Also confirmed this round: all six round-1 findings are genuinely resolved — the TOCTOU re-check placement was independently re-verified (no await between re-check and mutation block).

Action Plan

1. Finding 1 — Wrap navigateTree from the controller allocation onward in try { ... } finally { this._branchSummaryAbortController = undefined; }; drop the two manual resets (summarize branch, success return). This closes the new re-check leak AND the two pre-existing leaks (cancel return, no-API-key throw) in one move. Extend the existing TOCTOU test with expect(session.isCompacting).toBe(false) after the rejection.
2. Finding 4 — rpc-mode.ts preview: entry.name ?? "empty" → entry.name || "empty"; add an empty-name assertion to the existing metadata-previews test ([title: empty]).
3. Finding 3 — Add test: spy model getter → undefined, navigateTreeForRpc(session, id, { summarize: true }) rejects with No model available for summarization.
4. Finding 2 — Add get_tree/navigate_tree round-trip tests to the API-gated rpc.test.ts spawned-child suite: build a tree via prompt, getTree() asserts structure/leafId, navigateTree(userMsgId) asserts editorText and post-navigation state, unknown-target rejects with the clean server-side error text.

---

Assessment by mach6

[m-aebrer]

Progress Update

All four round-2 review findings addressed (all classified genuine by the assessment):

• Finding 1 (isCompacting wedge) — AgentSession.navigateTree now wraps everything from the abort-controller allocation onward in try/finally, clearing _branchSummaryAbortController on every exit path. This closes the leak on the new streaming re-check throw and the two pre-existing leaky paths (extension-cancel return, no-API-key throw) per the completeness rule. The two manual resets are gone — one mechanism, no wedged isCompacting state class. The TOCTOU test now also asserts isCompacting === false after the rejection.
• Finding 4 (empty-title parity) — preview uses entry.name || "empty" (truthy, matching the TUI) instead of ??; empty titles now render [title: empty] over RPC. Covered by a new assertion using appendSessionInfo(" ") (trims to empty — the reachable extension setSessionName path).
• Finding 3 (no-model error path) — new test spies the model getter to undefined and asserts navigate_tree with summarize: true rejects with No model available for summarization. All three documented error cases now have tests.
• Finding 2 (e2e dispatch coverage) — new spawned-child test in the API-gated rpc.test.ts suite drives get_tree and navigate_tree through the real handleCommand dispatch: tree round-trip, option field-mapping verified end-to-end (the label lands on the target entry, observed via a subsequent get_tree), post-navigation get_state consistency, and unknown-target rejection carrying the handler's clean error text. Verified live with a real API call — passes against the compiled binary.

Verification: test.sh green twice (4020/0 standalone, 3986/0 in pre-commit hook), biome clean, tsgo --noEmit clean, full workspace build green.

Commit: 00ca869

---

Progress tracked by mach6

[m-aebrer]

Code Review (round 3)

This round verified the round-2 fixes (commit 00ca869) and hunted for new issues. All four round-2 findings are confirmed correctly resolved:

• try/finally controller cleanup — verified correct by two independent agents. The controller allocation sits safely outside the try (new AbortController() cannot throw); the finally fires on every exit path (extension-cancel return, aborted-summary return, summary-error throw, TOCTOU re-check throw, success return). The ~140-line re-indent is a clean move — branching, leaf-position logic, label attachment, and emit ordering are byte-identical modulo indentation. Bonus: abortBranchSummary() now works better than before — the controller stays live through both await windows instead of being nulled immediately after the summarizer returns.
• Empty-title parity (entry.name || "empty") — correct, matches the TUI truthy check; the appendSessionInfo(" ") test genuinely exercises the fallback (trim happens in the appender).
• No-model error test — exercises the real throw site (before controller allocation, so it can't wedge).
• E2e dispatch test — present in the API-gated suite; round-trips get_tree → navigate_tree(label) → get_state and asserts the clean unwrapped error text.

The test-reviewer additionally verified the round-2 test additions are non-tautological: the isCompacting === false assertion fails without the fix, and the no-model mock hits the real guard.

Important

Finding 1 — Concurrent navigate_tree calls interleave on shared session state with no guard (error-auditor, confidence 88, severity high)
packages/coding-agent/src/core/agent-session.ts (~3474, 3523, 3594) + packages/coding-agent/src/modes/rpc/rpc-mode.ts (dispatch ~954). RPC dispatch is unserialized (void handleInputLine per line) and navigateTree never sets isStreaming — it only reads it. So neither the entry guard nor the TOCTOU re-check defends against navigate-vs-navigate: two overlapping summarizing navigations pass both guards, the second overwrites _branchSummaryAbortController (first becomes unabortable), whichever finishes first clears the field in its finally while the other is in flight (isCompacting reads false mid-summary), and both eventually run their mutation blocks against the same SessionManager — interleaved branchWithSummary/branch/replaceMessages producing a leaf that may not match the returned editorText. Not a round-2 regression — the old manual resets had the identical clobber — but the exposure is new in this PR (navigate_tree newly reachable over RPC), and the round-2 assessment's planned follow-up issue for dispatch serialization was never filed (no matching issue exists). The code-reviewer independently noted the same race but scored it ~50 ("well-behaved client awaits each response"). Suggested minimal fix: an in-flight navigation guard that throws loudly ("navigation already in progress"), consistent with the existing guard style — or file the serialization follow-up issue for real.

Suggestions

Finding 2 — session_tree extension-emit failure reports success:false after state already mutated (error-auditor, confidence 82, severity low)
agent-session.ts (~3648-3670). The session_tree emit runs after replaceMessages; if an extension handler throws there, navigate_tree maps to success:false while the leaf has already moved — an RPC client can't distinguish "nothing happened" from "everything happened except the notify hook." Pre-existing semantics (the emit threw identically pre-PR) and the finally now correctly clears the controller on this path; it's newly observable because navigate_tree is newly reachable over RPC. Suggested: wrap just the post-commit emit in a catch-and-log, or document that success:false doesn't guarantee no-op.

Finding 3 — Summarization branch of navigateTree is entirely untested (test-reviewer, confidence 85, severity medium)
No test reaches generateBranchSummary: RPC unit tests mock navigateTree, integration tests never pass summarize:true with entries to summarize, and the no-model test throws before the block. Uncovered: signal/customInstructions/replaceInstructions/reserveTokens forwarding, aborted return ({cancelled:true, aborted:true} + controller-cleared invariant on the round-2-restructured path), result.error throw, No API key throw, successful branchWithSummary + label attachment, and the extensionSummary branch. Not an infra limit — the harness already installs a faux runtime API key, or generateBranchSummary can be mocked. Suggested: one test mocking the summarizer to {aborted:true} asserting result + isCompacting === false; one returning a summary asserting a branch_summary entry + label.

Finding 4 — get_tree/navigate_tree dispatch only exercised by CI-skipped API-gated test (test-reviewer, confidence 90, severity medium)
The e2e test added in round 2 lives inside describe.skipIf(!ANTHROPIC_API_KEY && !ANTHROPIC_OAUTH_TOKEN) — skipped in normal CI. No non-gated test touches the handleCommand option field-mapping or the case-level try/catch error envelope; a dropped field or broken error mapping would pass CI. Partly a pre-existing infra pattern (the whole spawned-child suite is key-gated), which is why round 2 accepted the gated placement — flagged again because CI-blindness on new dispatch logic remains real.

Finding 5 — Extension-cancel path has no test (test-reviewer, confidence 82, severity low)
session_before_tree returning {cancel:true} → {cancelled: true} return with no mutation and controller cleared — unexercised. This early return was one of the two pre-existing leak paths the round-2 finally fixed, and only the throw path has a regression test. createHarnessWithExtensions (already used one test above) makes this trivial.

Strengths

• The round-2 fix strictly improves every early-exit path and introduces no new silent failure mode (error-auditor's explicit verdict after tracing all exits).
• Aborted-summary semantics verified behavior-identical pre/post fix; the timeoutMs wire-strip is correct; [undefined] placeholder for truly corrupt entries is loud-enough and non-throwing (and a corrupt message entry surfaces as an explicit error response, not silence).
• Simplifier round 3: clean bill. The try/finally is the simplest correct structure (it removed two scattered resets); inline error strings match the file's universal convention (zero extracted constants, including the other isStreaming guard); test helpers already well-factored; no dead code or unused imports.
• Completeness re-verified: all six issue acceptance criteria met, all three documented error strings match code exactly, timeout-divergence and orphan-parentId notes present, READMEs don't enumerate RPC commands (no update needed), editorText superset documented and reasoned.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

---

Reviewed by mach6

[m-aebrer]

Review Assessment (round 3)

Assessing the round-3 review findings above (#316 (comment)). Each finding was independently verified against the actual source, including cross-checking sibling methods (fork, compact, switchSession) and the API-gated tree-navigation suite that the review overlooked.

Classifications

Finding	Classification	Reasoning
1 — Concurrent navigate_tree calls interleave on shared session state	Genuine (small fix)	Verified real: dispatch is unserialized (void handleInputLine), navigateTree only reads isStreaming and never sets it, so both existing guards pass for a second navigation; the calls clobber _branchSummaryAbortController (first becomes unabortable, first finisher's finally clears the other's mid-flight) and their mutation blocks interleave on one SessionManager. Round 2 fixed navigate-vs-prompt in-PR using the issue-307 multi-client argument — the identical argument applies to navigate-vs-navigate from two dashboard devices. The fix is small and self-contained: a loud isCompacting entry guard before the controller is allocated (symmetric with the isStreaming guard; also closes navigate-vs-compaction), plus a non-gated regression test. Full dispatch serialization remains an optional follow-up, no longer blocking once navigation self-guards. No tracking issue exists yet for serialization (the round-2 "optional follow-up" was never filed).
2 — session_tree emit failure reports success:false after state mutated	Nitpick	Verified: every sibling post-commit emit (session_fork ~3438, session_compact ~2326, session_switch ~3333) is awaited after replaceMessages with no catch — mutate-then-emit-uncaught is the codebase-wide pattern. The suggested catch-and-log would make navigateTree the lone inconsistent method and would swallow extension errors, violating the fail-loudly norm. Consistent by design; no action.
3 — Summarization branch of navigateTree entirely untested	False positive	The premise is wrong: the API-gated agent-session-tree-navigation.test.ts suite covers the summarize path end-to-end (generation, aborted return + session-unchanged invariant, summary position, custom-instruction forwarding), exactly as the round-1 plan stated. The NEW cleanup behavior (throw-path controller clear) is covered non-gated in rpc-tree-commands.test.ts. The reviewer missed the existing suite.
4 — Dispatch only exercised by CI-skipped API-gated test	Nitpick	handleCommand is a non-exported closure inside runRpcMode; the entire dispatch layer is uniformly key-gated for every RPC command — this PR followed the established pattern and already extracted the real logic into non-gated-tested helpers (getTreeForRpc, navigateTreeForRpc). Only trivial pass-through wiring is gated. Round 2 assessed this exact question and accepted the placement; round 3 adds nothing new. A non-gated dispatch-harness refactor would be a repo-wide infra change, not this PR's job.
5 — Extension-cancel path untested	Genuine (tiny)	Verified: no test anywhere exercises session_before_tree → {cancel: true} (the only existing handler test returns a plain await). The cancel return is one of the two pre-existing leak paths the round-2 finally fixed — new-code cleanup behavior, so the test ships with the PR per repo norms. ~15 non-gated lines mirroring the adjacent TOCTOU test: assert {cancelled: true}, unchanged leaf/entries, isCompacting === false.

Counts: 2 genuine (findings 1, 5), 2 nitpicks (findings 2, 4), 1 false positive (finding 3), 0 deferred.

Also confirmed this round: all four round-2 fixes are correctly implemented — the try/finally fires on every exit path, the ~140-line re-indent dropped nothing, and the new tests are non-tautological (the isCompacting assertion fails without the fix).

Action Plan

The PR is merge-ready except for two cheap, in-scope items:

1. Finding 1 — Add a loud isCompacting guard at the top of navigateTree, before the abort-controller allocation (symmetric with the existing isStreaming guard): throw "Cannot navigate the session tree while summarization or compaction is in progress. Wait for idle first." The second concurrent navigation sees the first's controller still set and fails loudly; navigate-vs-compaction closes for free; interactive mode already blocks navigation on isCompacting, so no behavior regression. Add a non-gated regression test: park a first navigation at a controllable session_before_tree await, fire a second, assert it throws while the first completes cleanly.
2. Finding 5 — Add the extension-cancel regression test: session_before_tree returns {cancel: true} → navigation resolves {cancelled: true}, leaf/entries unchanged, isCompacting === false.

Optional (non-blocking) follow-up: file a dispatch-serialization hardening issue against the issue-307 dashboard work — read-only commands are interleave-safe and prompt/navigate will both self-guard after item 1, so it drops to robustness polish.

---

Assessment by mach6

[m-aebrer]

Progress Update

Both round-3 genuine findings addressed (per the round-3 assessment: findings 1 and 5 genuine; 2 and 4 nitpicks; 3 false positive):

• Finding 1 (navigate-vs-navigate race) — AgentSession.navigateTree gained a loud isCompacting guard at entry, immediately after the isStreaming guard and before the abort-controller allocation. A second concurrent navigation (or an active compaction) now throws Cannot navigate the session tree while summarization or compaction is in progress. Wait for idle first. instead of clobbering the shared branch-summary abort controller and interleaving tree mutations. Since only synchronous code sits between method entry and the controller allocation, the second call reliably sees the first's controller — the race is closed at the entry point. Interactive mode already blocks navigation on isCompacting, so no TUI behavior change. New regression test parks a first navigation at a deferred session_before_tree await, fires a second (asserts the rejection and that the guard tripped before the second call's extension emit), then releases the first and asserts it completes cleanly with isCompacting false afterward. Docs (rpc.md error list) and CHANGELOG updated for the fourth explicit error case.
• Finding 5 (extension-cancel path) — new regression test: session_before_tree returning { cancel: true } resolves exactly { cancelled: true }, leaves leaf/entries unchanged, and clears isCompacting — pinning the try/finally cleanup on the cancel-return path (one of the two pre-existing leak paths the round-2 fix closed, previously only the throw path had a test).

Also fixed in passing (no-pre-existing-failures rule): two lock-contention tests (dream.test.ts "prevents concurrent acquisition", performance-tracker.test.ts "prune() warns when lock cannot be acquired") exceeded the default 10s timeout under full-suite load — bumped to 20s.

Nitpick findings 2 (post-commit session_tree emit semantics — consistent with fork/compact/switch codebase-wide) and 4 (dispatch coverage uniformly API-key-gated — established infra pattern) require no action per the assessment.

Verification: rpc-tree-commands suite 27 passed; full coding-agent suite 2533 passed / 0 failed; pre-commit hook full run 3999 passed / 0 failed; biome clean; full workspace build green.

Commit: 5f83b35

---

Progress tracked by mach6