NOJIRA: Extend ACP support: list and resume sessions#205
Conversation
Advertise sessionCapabilities.list, dispatch listSessions through a SessionManager.list/listAll seam, and map pi SessionInfo to ACP. Pi's default lookup reads PI_CODING_AGENT_DIR, not KIMCHI_CODING_AGENT_DIR, so the cwd-scoped path threads kimchi's agentDir explicitly via an inlined copy of pi's getDefaultSessionDir encoding (not re-exported).
phase 3: replay walker emits tool_call + tool_call_update for historical calls (status from persisted result, failed for orphans), agent_thought_chunk for non-redacted thinking gated by hideThinkingBlock, no-ops for compaction/branch_summary/model_change/custom entries. fixes from review: - toAcpSessionInfo: empty `name` falls through to firstMessage (?? skipped "") - replay: coalesce contiguous text blocks into one chunk per natural segment - replay: strip ANSI escapes from text/thought content (ACP can't render) - shouldEmitThinking: read setting directly; wrapper probe broke on inner </think> - collectToolResults: capture details/toolName so rawOutput shape matches live - loadSession: reject + dispose on header-id vs requested-id mismatch
Pi writes `<isoTimestamp>_<sessionId>.jsonl`; loader was looking up bare `<sessionId>.jsonl` and never resolved a real session. Scan the cwd dir for the suffix (with bare-form forward-compat fallback). Distinguish ENOENT (→ "not found") from other errno (→ surface as invalidParams with the underlying message) so EACCES/EMFILE don't masquerade as missing sessions. Found via verify-acp-load.mjs.
Two scenarios driving real LLM via stdio JSON-RPC: - A) reconnect-and-resume: ACP creates session, kill, reconnect, session/load, follow-up recalls token from prior context. - B) ACP↔CLI round-trip: ACP turn 1, CLI `--session <id>` turn 2 via piped stdin, ACP reload+turn 3 recalls both tokens, fresh ACP reload asserts ≥3 user/agent chunks persisted on disk. Auto-rejects permission prompts so unexpected tool use fails loudly instead of hanging. Watchdog sized off PROMPT_TIMEOUT_MS so worst-case slow LLM doesn't trip before per-prompt timeouts can flag the slow call.
Kimchi Code Review
Summary📊 Review Score: 78/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Extensive unit tests added for listSessions, loadSession, replay logic, and helper functions in server.test.ts. A new integration test script verify-acp-load.mjs covers end-to-end reconnect-and-resume and cross-tool round-trip scenarios. Coverage is thorough for both happy paths and edge cases (tool calls, thinking blocks, ANSI stripping, concurrent cancel, error unwinding). 📝 Found 6 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 78/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 4/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Extensive unit tests added for listSessions, loadSession, replay logic, and helper functions in server.test.ts. A new integration test script verify-acp-load.mjs covers end-to-end reconnect-and-resume and cross-tool round-trip scenarios. Coverage is thorough for both happy paths and edge cases (tool calls, thinking blocks, ANSI stripping, concurrent cancel, error unwinding).
📝 Found 6 issue(s). See inline comments for details.
| await delay(200) | ||
| } finally { | ||
| await killProcess(a.proc) | ||
| } |
There was a problem hiding this comment.
The CLI leg does not handle 'error' events on cli or cli.stdin. If the child exits before reading stdin, the subsequent cli.stdin.end(cliPrompt) emits an EPIPE error that becomes an uncaught exception and aborts the test.
💡 Suggestion: Add cli.on("error", (e) => { process.stderr.write("cli error: " + e + "\n"); }) and cli.stdin.on("error", () => {}) immediately after spawn, before calling cli.stdin.end(cliPrompt).
There was a problem hiding this comment.
Added cli.on("error", ...) and cli.stdin.on("error", () => {}) before end(cliPrompt). Exit-code check remains the source of truth.
- loadSession: guard concurrent loads of same id via loadingSessions Set - replayTranscript: hoist isHideThinkingEnabled() out of per-block loop - replayTranscript: null-entry guard, mirroring collectToolResults - perf test: relax 1s -> 3s, retitle as regression guard - verify-acp-load.mjs: catch on conn.prompt before race; cli error/EPIPE handlers - comments: strip phase/PRD references throughout
1 participant
Why
ACP clients reconnect frequently and need to resume prior conversations with full context. Without
listSessions/loadSession, every reconnect is a blank slate.What
replays transcript.
notifications (bypasses session emitter so extensions don't double-count). ANSI stripped; thinking gated by live hide-thinking
setting.
Kimchi Summary
What changed
Adds
listSessionsandloadSessionsupport to the ACP server so clients can enumerate and resume persisted sessions with full transcript replay (text, thinking, and tool calls). Includes a new integration test that validates session continuity across ACP reconnects and ACP↔CLI round-trips.Why
ACP clients need to discover existing conversation threads and resume them with historical context intact, rather than starting a fresh session on every connection.
Key changes
src/modes/acp/server.ts: ImplementedlistSessions(sorts newest-first with title fallback),loadSession(validates request, replays transcript, registers session), andreplayTranscript/replayAssistantBlocks(emitsuser_message_chunk,agent_message_chunk,agent_thought_chunk,tool_call,tool_call_update). AddedtoAcpSessionInfo,stripAnsi,collectToolResults, and default disk-backedsessionLister/sessionLoader.src/modes/acp/server.ts:initializenow advertisesloadSession: trueandsessionCapabilities.list.src/modes/acp/server.test.ts: Added comprehensive tests for listing, loading, replay behaviors (text coalescing, ANSI stripping, hide-thinking redaction, tool-call kind matrix), and helper utilities (shouldEmitThinking,stripAnsi,userMessageText,assistantMessageText).src/extensions/hide-thinking.ts: ExportedisHideThinkingEnabled()so the ACP replay path shares the live TUI's hide-thinking setting without relying on fragile synthetic probes.scripts/verify-acp-load.mjs: New end-to-end integration test driving a real LLM via JSON-RPC to verify reconnect-and-resume and cross-client session persistence.Impact
loadSessionrejects non-emptymcpServers(invalidParams) and already-live session IDs (invalidRequest).sessionIdandcwdagainst the request; mismatches are rejected and the session is disposed to prevent accidental re-rooting or double-opens.Kimchi Summary
What changed
Adds ACP
listSessionsandloadSessionsupport so clients can enumerate and resume persisted disk sessions. Historical transcripts are replayed assession/updatenotifications covering user text, assistant text/thinking, and tool calls (paired with their results). A new integration test verifies reconnect-and-resume and ACP↔CLI round-trip behavior end-to-end.Why
ACP clients (e.g., Zed) need thread persistence and resume to avoid losing conversation context across reconnects or when switching between ACP and CLI modes.
Key changes
src/modes/acp/server.ts: ImplementslistSessionswith deduplication acrossadditionalDirectories, newest-first sorting byupdatedAt, and explicitnextCursor: null. ImplementsloadSessionwith atomic registration, headersessionIdvalidation, cwd verification, and full transcript replay viareplayTranscript/replayAssistantBlocks.src/modes/acp/server.ts: AddsdefaultSessionListeranddefaultSessionLoaderto bridge pi'sSessionManagerwith ACP requests, includingresolveSessionPathByIdto locate timestamp-prefixed JSONL files.src/modes/acp/server.ts: AddsstripAnsi,userMessageText,assistantMessageText,collectToolResults, andshouldEmitThinkinghelpers for sanitizing and structuring replayed content.src/extensions/hide-thinking.ts: ExportsisHideThinkingEnabled()so the ACP replay path respects the hide-thinking setting without probingfilterThinkingForDisplaywith a fragile synthetic wrapper.scripts/verify-acp-load.mjs: New integration test script that validates ACP reconnect-and-resume and CLI round-trip session sharing using real LLM prompts.src/modes/acp/server.test.ts: Extensive unit tests added forlistSessions,loadSession, replay semantics (text coalescing, ANSI stripping, thinking redaction, tool-call pairing), and helper functions.Impact
initializenow advertisesagentCapabilities.loadSession: trueandsessionCapabilities.list.loadSessionrejects requests with non-emptymcpServersor already-loaded session IDs to prevent JSONL interleaving and double-registration.failedand empty content instead of hanging asin_progress.agent_message_chunkupdates during replay.