feat(acp): session/resume, set_mode, set_config_option - #86
Conversation
PR #63 shipped 8 client->agent methods. ACP spec defines three more client->agent methods that fall to METHOD_NOT_FOUND today; this commit implements all of them. session/resume — like session/load but skips history replay. Per ACP spec: 'useful for agents that can resume sessions but don't implement full session loading.' Refreshes cwd + mcpServers on the session record and claims ownership so subsequent agent::events route to this subprocess. session/set_mode — persists modeId on the session record. Brain workers can read it on the next session/prompt turn (e.g. via a system prompt suffix or per-mode tool gating). Validation of the mode against any agent-specific catalog is left to the brain. session/set_config_option — persists configId/value pairs in config_options on the session record. Same rationale as set_mode: persistence here, semantics in the brain. Implementation notes: - SessionRecord gains mode: Option<String> (skip_serializing_if = is_none) and config_options: serde_json::Map<String, Value> (defaults empty). Backward-compatible with sessions written before this change since both fields use serde defaults. - update_session_record helper: read-modify-write of one record guarded by the per-session history mutex (we reuse the lock that already exists for append_history rather than adding a parallel lock map for the same key). - agentCapabilities.sessionCapabilities now advertises { list: {}, close: {}, resume: {} } — matches the ACP schema slots that exist in this version. set_mode and set_config_option ship without an explicit capability flag because the in-repo schema doesn't have those slots yet; clients that try them succeed, clients that don't try are unaffected. README method table is now exhaustive: every ACP method is listed with its current implementation status. The previously-misleading 'session/request_permission deferred to v0.2' line is replaced with explicit per-method rows for fs/* and terminal/* so the deferred surface is fully visible. Reverse-RPC paths (session/request_permission, fs/*, terminal/*) remain deferred — they need a JSON-RPC framer that can originate requests from the agent side, and our internal iii brains use iii primitives directly for filesystem and terminal access. Those can land later without breaking this PR. Validation: - 17 lib tests + 10 protocol envelope tests pass (3 new round-trip tests for the new param shapes). - cargo clippy --all-targets -- -D warnings clean. - Live smoke against the 7-worker brain stack: - sessionCapabilities advertises {close, list, resume} on initialize. - session/set_mode persists modeId='code' (verified via state::get). - session/set_config_option persists configId='thinking' value='high'. - session/resume refreshes cwd to '/new'. - All three return {} on success, INVALID_PARAMS (-32602) on missing sessionId. - Full canonical brain flow (session/prompt streaming real Claude through agent::events) still passes.
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis PR extends the ACP session management with three new capabilities: session resume (restoring environment state without history replay), session mode configuration, and per-session configuration options. Supporting types, handler methods, session record fields, and tests are added across the codebase. ChangesACP Session Lifecycle Extension
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR #86 implemented session/resume, session/set_mode, and session/set_config_option, but the README header still said reverse-RPC paths are 'deferred to v0.2'. That line conflated the three new client->agent methods (now done) with the reverse-RPC surface (still deferred), and v0.2 isn't accurate framing for the latter either since those methods only land when external ACP agents (future acp-client) need them. Replaces the line with an explicit per-method enumeration in the status block, splitting client->agent (all eleven implemented) from reverse-RPC (deferred). Points at the Methods table for full status.
Summary
PR #63 shipped 8 client→agent methods. ACP defines three more that fell to
METHOD_NOT_FOUNDuntil now. This PR implements all of them and updates the method table to be exhaustive so the deferred surface is fully visible.Methods added
session/resumesession/loadbut skips history replay. Refreshescwd+mcpServerson the session record, claims ownership so subsequentagent::eventsroute here. Per ACP: "useful for agents that can resume sessions but don't implement full session loading."session/set_modemodeIdon the session record. Brain workers can read it on the next prompt turn (system-prompt suffix, per-mode tool gating, etc.). Catalog validation is the brain's concern.session/set_config_optionconfigId/valuepairs inconfig_optionson the session record. Same split: persistence here, semantics in the brain.Implementation notes
SessionRecordgains:mode: Option<String>(skip_serializing_if = "Option::is_none"— backward-compatible with pre-existing records)config_options: serde_json::Map<String, Value>(default empty)update_session_recordhelper does a read-modify-write of one record under the per-session history mutex that already serializesappend_history. No parallel lock map.agentCapabilities.sessionCapabilitiesnow advertises{ list: {}, close: {}, resume: {} }— matches the ACP schema slots in this version.set_mode/set_config_optionship without an explicit capability flag because the in-repo schema doesn't include those slots yet; clients that try them succeed, clients that don't try are unaffected.README
Method table is now exhaustive. The misleading "deferred to v0.2" line is replaced with explicit rows for every method:
initialize,authenticate,session/{new,load,resume,list,prompt,cancel,close,set_mode,set_config_option},session/updatesession/request_permission,fs/{read,write}_text_file,terminal/{create,kill,output,release,wait_for_exit}Validation
cargo clippy --all-targets -- -D warningscleansessionCapabilitiesadvertises{close, list, resume}oninitializesession/set_modepersistsmodeId="code"(verified viastate::get)session/set_config_optionpersistsconfigId="thinking"value"high"session/resumerefreshes cwd to"/new"{}on success,INVALID_PARAMS (-32602)on missingsessionIdagent::events) still passesTest plan
cargo test— 17 lib + 10 protocol envelope tests passcargo clippy --all-targets -- -D warningscleanFollow-ups
Reverse-RPC paths remain deferred — they need a JSON-RPC framer that can originate requests from the agent side. Internal iii brains today use iii primitives directly for filesystem and terminal access. Lands as a separate PR when an external ACP agent (consumed via future
acp-client) needs the editor to act on its behalf.Summary by CodeRabbit
New Features
Documentation