From 770c928d734434cd52b2df203a473e448d11f736 Mon Sep 17 00:00:00 2001 From: Clifford Ressel Date: Mon, 6 Jul 2026 10:04:22 -0400 Subject: [PATCH 1/2] =?UTF-8?q?feat(acp):=20adopt=20the=20agent=20session?= =?UTF-8?q?=20lifecycle=20=E2=80=94=20session/resume,=20/close,=20structur?= =?UTF-8?q?ed=20errors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cloud agent (nori-handroll acp --type cloud) advertises sessionCapabilities.{list,resume,close} with loadSession:false, but the CLI never called session/resume or session/close, gated the agent-sourced /resume picker on load_session (hiding every cloud session), and mapped ACP errors by substring only. - Project session_resume/session_close into AgentCapabilitiesView - Reattach via session/resume when load is unsupported; never fall back to session/new on a resume failure (it would claim a fresh cloud VM) - /close: capability-gated release of the live session, new chat only on success; session-switching commands blocked while the close is in flight - Structured error mapping by JSON-RPC code (-32000 auth_required with error.data.detail surfaced, -32002/-32010/-32012/-32014/-32015), code wins over message heuristics; spawn/resume/close and prompt failures all use it - /resume picker gate is now session_list && (load_session || session_resume); the cloud sentinel cwd "/" is hidden from picker rows - Mock agent gains env-gated resume/close support and failure switches πŸ€– Generated with Nori πŸ€– Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori --- nori-rs/acp-host/docs.md | 4 +- .../acp-host/src/connection/acp_connection.rs | 41 +++ .../src/connection/acp_connection_tests.rs | 145 ++++++++++ nori-rs/acp-host/src/connection/docs.md | 3 +- nori-rs/acp-host/src/error_category.rs | 152 +++++++++- nori-rs/acp-host/src/lib.rs | 2 + nori-rs/cli/docs.md | 1 + nori-rs/docs.md | 2 +- nori-rs/harness/docs.md | 47 +++- nori-rs/harness/src/backend/mod.rs | 41 ++- .../harness/src/backend/nori_client_mcp.rs | 39 +-- nori-rs/harness/src/backend/session.rs | 153 ++++++++--- .../src/backend/session_runtime_driver.rs | 17 +- nori-rs/harness/src/backend/tests/mod.rs | 1 + nori-rs/harness/src/backend/tests/part8.rs | 192 +++++++++++++ nori-rs/harness/src/runtime.rs | 19 ++ nori-rs/mock-acp-agent/docs.md | 8 +- nori-rs/mock-acp-agent/src/main.rs | 71 ++++- nori-rs/nori-protocol/docs.md | 1 + nori-rs/nori-protocol/src/lib.rs | 9 + nori-rs/tui/docs.md | 9 +- nori-rs/tui/src/app/event_handling.rs | 3 + nori-rs/tui/src/app_event.rs | 5 + .../bottom_pane/chat_composer/tests/part2.rs | 31 +++ ...omposer__tests__part2__slash_popup_cl.snap | 9 + nori-rs/tui/src/chatwidget/constructors.rs | 2 + nori-rs/tui/src/chatwidget/helpers.rs | 7 + nori-rs/tui/src/chatwidget/key_handling.rs | 52 ++++ nori-rs/tui/src/chatwidget/mod.rs | 4 + nori-rs/tui/src/chatwidget/pickers.rs | 11 +- nori-rs/tui/src/chatwidget/tests/mod.rs | 1 + nori-rs/tui/src/chatwidget/tests/part2.rs | 2 + nori-rs/tui/src/chatwidget/tests/part9.rs | 259 ++++++++++++++++++ nori-rs/tui/src/nori/resume_session_picker.rs | 51 +++- nori-rs/tui/src/slash_command.rs | 3 + 35 files changed, 1294 insertions(+), 103 deletions(-) create mode 100644 nori-rs/harness/src/backend/tests/part8.rs create mode 100644 nori-rs/tui/src/bottom_pane/chat_composer/tests/snapshots/nori_tui__bottom_pane__chat_composer__tests__part2__slash_popup_cl.snap diff --git a/nori-rs/acp-host/docs.md b/nori-rs/acp-host/docs.md index 51c3b05de..9202d30e4 100644 --- a/nori-rs/acp-host/docs.md +++ b/nori-rs/acp-host/docs.md @@ -23,7 +23,7 @@ nori-acp-host <---> ACP Agent subprocess (JSON-RPC over stdio) - `nori-harness` (`@/nori-rs/harness/`) is the primary consumer and re-exports every module (`pub use nori_acp_host::connection;` and friends in `@/nori-rs/harness/src/lib.rs`), so downstream consumers such as `@/nori-rs/tui/` import through `nori_harness` paths. - Wire/schema types come from the official `agent-client-protocol` SDK; the schema's own `unstable` feature is enabled unconditionally for the Model-category config option. - Depends on `codex-protocol` (`@/nori-rs/protocol/`) for the internal event vocabulary, `nori-config` (`@/nori-rs/nori-config/`) for agent/MCP/wire-proxy configuration types, and `codex-rmcp-client` (`@/nori-rs/rmcp-client/`) for OAuth token loading in `connection/mcp.rs`. -- Error classification lives here (`AcpErrorCategory`, `categorize_acp_error`); the harness-side user-facing message composition (`enhanced_error_message`) stays in `@/nori-rs/harness/src/backend/`. +- Error classification lives here (`AcpErrorCategory`, `AcpErrorDetails`, `categorize_acp_error`, `categorize_acp_error_chain`); the harness-side user-facing message composition (`enhanced_error_message`) stays in `@/nori-rs/harness/src/backend/`. ### Core Implementation @@ -31,7 +31,7 @@ nori-acp-host <---> ACP Agent subprocess (JSON-RPC over stdio) - `registry.rs` β€” data-driven agent registry merging built-in agents (Claude Code, Codex, Gemini) with custom `[[agents]]` config entries; resolves an agent slug to a spawnable `AcpAgentConfig` across npx/bunx/pipx/uvx/local distributions. The registry is process-global state (`AGENT_REGISTRY`, a `RwLock`) initialized once via `initialize_registry()` at startup, with a built-in-defaults fallback when uninitialized. - `translator.rs` β€” converts user input into ACP `ContentBlock`s (text plus base64 image blocks) and provides local parsing/display helpers. - `patch.rs` β€” diff/patch construction (`create_patch_with_context`) used to normalize file mutations for rendering and transcripts. -- `error_category.rs` β€” priority-chained substring matching (Auth > Quota > ExecutableNotFound > Initialization > PromptTooLong > ApiServerError > Unknown) over the Debug-formatted error chain; `is_retryable()` marks only server errors and quota limits as transient. +- `error_category.rs` β€” two-tier categorization. `categorize_acp_error_chain()` is the preferred entry point: it walks the anyhow chain for a structured `acp::Error`, maps it by JSON-RPC code (`-32000` auth, `-32002` session not found, `-32010` agent backing service unreachable, `-32012` not resumable, `-32014` no active session, `-32015` session already active β€” the code table mirrors the `nori-handroll acp` agent side), and extracts the agent-supplied `error.data.detail` into `AcpErrorDetails`. Structured codes always win over message text. Without a structured error it falls back to the legacy `categorize_acp_error()` priority-chained substring matching (Auth > Quota > ExecutableNotFound > Initialization > PromptTooLong > ApiServerError > Unknown) over the Debug-formatted error chain. `is_retryable()` marks server errors, quota limits, and `AgentUnreachable` as transient; the session-lifecycle states are persistent and never retryable. ### Things to Know diff --git a/nori-rs/acp-host/src/connection/acp_connection.rs b/nori-rs/acp-host/src/connection/acp_connection.rs index 814a9f217..07d256bdb 100644 --- a/nori-rs/acp-host/src/connection/acp_connection.rs +++ b/nori-rs/acp-host/src/connection/acp_connection.rs @@ -699,6 +699,47 @@ impl AcpConnection { Ok(acp::SessionId::from(session_id.to_string())) } + /// Resume (reattach to) an existing session via ACP `session/resume`. + /// + /// Unlike `session/load`, resume is a live reattach with no history + /// replay β€” the path for agents that advertise + /// `sessionCapabilities.resume` with `loadSession: false` (nori cloud). + /// The returned `SessionId` is the input id (the response carries none). + pub async fn resume_session( + &self, + session_id: &str, + cwd: &Path, + mcp_servers: Vec, + ) -> Result { + let response = self + .cx + .send_request( + acp::ResumeSessionRequest::new(session_id.to_string(), cwd) + .mcp_servers(mcp_servers), + ) + .block_task() + .await + .context("Failed to resume ACP session")?; + + if let Some(config_options) = response.config_options + && let Ok(mut state) = self.session_config_state.write() + { + state.config_options = config_options; + } + + Ok(acp::SessionId::from(session_id.to_string())) + } + + /// Close (release) a session via ACP `session/close`. + pub async fn close_session(&self, session_id: &str) -> Result<()> { + self.cx + .send_request(acp::CloseSessionRequest::new(session_id.to_string())) + .block_task() + .await + .context("Failed to close ACP session")?; + Ok(()) + } + /// List the agent's known sessions via ACP `session/list`. /// /// `cwd` is forwarded as the spec's working-directory filter. Cursor-based diff --git a/nori-rs/acp-host/src/connection/acp_connection_tests.rs b/nori-rs/acp-host/src/connection/acp_connection_tests.rs index 9012b3905..50fe1f4e1 100644 --- a/nori-rs/acp-host/src/connection/acp_connection_tests.rs +++ b/nori-rs/acp-host/src/connection/acp_connection_tests.rs @@ -1212,3 +1212,148 @@ async fn test_list_sessions_maps_agent_session_info() { ]; assert_eq!(summaries, expected); } + +/// `resume_session` reattaches to an agent-side session over ACP +/// `session/resume` β€” the reattach path for agents (like nori cloud) that +/// advertise `sessionCapabilities.resume` with `loadSession: false`. +/// `MOCK_AGENT_FAIL_NEW_SESSION_FROM=0` makes any silent `session/new` +/// fallback fail loudly. +#[tokio::test] +async fn test_resume_session_reattaches_over_session_resume() { + let Some(mut config) = mock_agent_config() else { + return; + }; + config.env.insert( + "MOCK_AGENT_SUPPORT_SESSION_RESUME".to_string(), + "1".to_string(), + ); + config.env.insert( + "MOCK_AGENT_FAIL_NEW_SESSION_FROM".to_string(), + "0".to_string(), + ); + + let temp_dir = tempdir().expect("temp dir"); + let conn = AcpConnection::spawn( + &config, + temp_dir.path(), + nori_config::AcpProxyConfig::disabled(), + ) + .await + .expect("Failed to spawn AcpConnection"); + + assert!( + conn.capabilities().session_capabilities.resume.is_some(), + "mock should advertise session/resume when env-gated on" + ); + + let session_id = conn + .resume_session("mock-session-1", temp_dir.path(), Vec::new()) + .await + .expect("resume_session should succeed against a live agent"); + assert_eq!(session_id.to_string(), "mock-session-1"); +} + +/// A structured `session/resume` failure must stay categorizable: the agent's +/// error code (-32002 resource_not_found) and `data.detail` survive the real +/// stdio transport and the anyhow chain so callers can show a precise message. +#[tokio::test] +async fn test_resume_session_failure_categorizes_as_session_not_found() { + let Some(mut config) = mock_agent_config() else { + return; + }; + config.env.insert( + "MOCK_AGENT_SUPPORT_SESSION_RESUME".to_string(), + "1".to_string(), + ); + config.env.insert( + "MOCK_AGENT_RESUME_SESSION_FAIL".to_string(), + "1".to_string(), + ); + + let temp_dir = tempdir().expect("temp dir"); + let conn = AcpConnection::spawn( + &config, + temp_dir.path(), + nori_config::AcpProxyConfig::disabled(), + ) + .await + .expect("Failed to spawn AcpConnection"); + + let err = conn + .resume_session("gone-session", temp_dir.path(), Vec::new()) + .await + .expect_err("resume_session should fail when the agent rejects the id"); + let details = crate::categorize_acp_error_chain(&err); + assert_eq!( + (details.category, details.detail.as_deref()), + ( + crate::AcpErrorCategory::SessionNotFound, + Some("the session is no longer claimed") + ) + ); +} + +/// `close_session` releases an agent-side session over ACP `session/close`. +#[tokio::test] +async fn test_close_session_round_trips() { + let Some(mut config) = mock_agent_config() else { + return; + }; + config.env.insert( + "MOCK_AGENT_SUPPORT_SESSION_CLOSE".to_string(), + "1".to_string(), + ); + + let temp_dir = tempdir().expect("temp dir"); + let conn = AcpConnection::spawn( + &config, + temp_dir.path(), + nori_config::AcpProxyConfig::disabled(), + ) + .await + .expect("Failed to spawn AcpConnection"); + + assert!( + conn.capabilities().session_capabilities.close.is_some(), + "mock should advertise session/close when env-gated on" + ); + + conn.close_session("mock-session-1") + .await + .expect("close_session should succeed against a live agent"); +} + +/// A structured `session/close` failure propagates β€” proving the request +/// actually crosses the process boundary (a no-op `close_session` could not +/// observe the agent-side error). +#[tokio::test] +async fn test_close_session_failure_categorizes_as_session_not_found() { + let Some(mut config) = mock_agent_config() else { + return; + }; + config.env.insert( + "MOCK_AGENT_SUPPORT_SESSION_CLOSE".to_string(), + "1".to_string(), + ); + config + .env + .insert("MOCK_AGENT_CLOSE_SESSION_FAIL".to_string(), "1".to_string()); + + let temp_dir = tempdir().expect("temp dir"); + let conn = AcpConnection::spawn( + &config, + temp_dir.path(), + nori_config::AcpProxyConfig::disabled(), + ) + .await + .expect("Failed to spawn AcpConnection"); + + let err = conn + .close_session("gone-session") + .await + .expect_err("close_session should fail when the agent rejects the id"); + assert_eq!( + crate::categorize_acp_error_chain(&err).category, + crate::AcpErrorCategory::SessionNotFound + ); +} diff --git a/nori-rs/acp-host/src/connection/docs.md b/nori-rs/acp-host/src/connection/docs.md index 2190dca1a..e4b3699cc 100644 --- a/nori-rs/acp-host/src/connection/docs.md +++ b/nori-rs/acp-host/src/connection/docs.md @@ -32,7 +32,7 @@ Local ACP Agent (subprocess, own process group) ### Core Implementation -- **`AcpConnection`** (`acp_connection.rs`): The central `Send + Sync` type that owns the SDK connection task, the child's teardown handles, and the event receiver. Key methods: `create_session()`, `load_session()`, `list_sessions()`, `prompt()`, `cancel()`, `set_config_option()` +- **`AcpConnection`** (`acp_connection.rs`): The central `Send + Sync` type that owns the SDK connection task, the child's teardown handles, and the event receiver. Key methods: `create_session()`, `load_session()`, `resume_session()`, `close_session()`, `list_sessions()`, `prompt()`, `cancel()`, `set_config_option()` - **`establish_connection()`**: A free function that registers all SDK handlers (notification, permission request, file read/write) and performs the initialization handshake. The caller (`spawn()`) supplies the `ConnectionEvent` channel so extra producers -- notably the child exit watcher -- can report through the same ordered stream - **Child exit watcher**: A background task OWNS the `Child`: it `wait()`s and reaps the process, publishes the exit status on a `watch` channel, and emits `ConnectionEvent::ChildExited { status, stderr_tail }` (defined in `mod.rs`) when the child dies. The connection holds only a `ChildHandle` (pid, exit-status receiver, kill `Notify`) -- never the `Child` itself - **stderr capture**: A background task logs each stderr line via tracing and keeps a bounded tail of recent lines. The tail is attached to startup failures and `ChildExited` events so the real cause (e.g. an auth hint) is user-visible @@ -50,6 +50,7 @@ Local ACP Agent (subprocess, own process group) - Without the `ChildExited` event, a dead child is a silent EOF that the ACP SDK layer treats as non-terminal -- pending requests would hang forever. The backend relay turns it into a visible error and fails any in-flight prompt - File write handlers restrict writes to the workspace directory or `/tmp` (canonicalized path check). File read handlers are unrestricted. Both emit synthetic `ToolCall` updates for TUI rendering - `AcpConnection::prompt()` absorbs stale cancel-tail responses (empty `end_turn` responses left over from a previous cancellation) by retrying until streamed updates arrive, keeping the reducer contract unchanged +- `resume_session()` sends ACP `session/resume` β€” a live reattach with **no** history replay, unlike `session/load`. It is the reattach path for agents that advertise `sessionCapabilities.resume` with `loadSession: false` (the nori cloud contract). The response carries no session id (the input id is returned) but may carry `config_options`, which replace the live session-config snapshot. `close_session()` sends ACP `session/close` to release the agent-side session. Failures on both preserve the structured `acp::Error` (JSON-RPC code plus `data.detail`) in the anyhow chain so `categorize_acp_error_chain()` in `@/nori-rs/acp-host/src/error_category.rs` can classify them precisely - `list_sessions()` sends the ACP `session/list` request and drains cursor pagination internally (following `next_cursor` until exhausted, bounded by a page cap so a misbehaving agent that keeps returning a cursor cannot loop unbounded), concatenating every page in agent order. Each ACP `SessionInfo` is mapped into `AcpSessionSummary` (defined in `mod.rs`, re-exported from `nori_harness`), an owned boundary type carrying `session_id`, `cwd`, optional `title`, and optional `updated_at`. The boundary type decouples consumers from the raw ACP schema, letting `@/nori-rs/tui` -- which has no ACP-schema dependency -- render the agent-sourced `/resume` picker. It mirrors `load_session()`'s structure; the agent only honors `session/list` when it advertises the capability (projected as `agent.session_list`, see `@/nori-rs/harness/docs.md`) Created and maintained by Nori. diff --git a/nori-rs/acp-host/src/error_category.rs b/nori-rs/acp-host/src/error_category.rs index 0f6c460e6..bca5fe3a1 100644 --- a/nori-rs/acp-host/src/error_category.rs +++ b/nori-rs/acp-host/src/error_category.rs @@ -1,3 +1,5 @@ +use agent_client_protocol as acp; + /// Categories of ACP spawn errors for providing actionable user messages. #[derive(Debug, Clone, PartialEq, Eq)] pub enum AcpErrorCategory { @@ -13,6 +15,16 @@ pub enum AcpErrorCategory { PromptTooLong, /// API returned a server error (5xx) ApiServerError, + /// The requested session no longer exists on the agent (ACP `-32002`) + SessionNotFound, + /// The agent's backing service is unreachable (ACP `-32010`) + AgentUnreachable, + /// The session exists but cannot be reattached (ACP `-32012`) + SessionNotResumable, + /// No session is active on the agent connection (ACP `-32014`) + NoActiveSession, + /// A session is already active on the agent connection (ACP `-32015`) + SessionAlreadyActive, /// Unknown error (fallback) Unknown, } @@ -20,21 +32,70 @@ pub enum AcpErrorCategory { impl AcpErrorCategory { /// Whether this error is transient and worth retrying (e.g. for an /// unattended loop that should survive a momentary API blip). Server - /// errors are momentary and rate/quota limits ease over time (seconds for - /// rate limits, longer for usage windows); everything else reflects a - /// persistent problem that a retry cannot fix. + /// errors are momentary, rate/quota limits ease over time (seconds for + /// rate limits, longer for usage windows), and an unreachable backing + /// service is a network condition; everything else reflects a persistent + /// problem that a retry cannot fix. pub fn is_retryable(&self) -> bool { match self { - AcpErrorCategory::ApiServerError | AcpErrorCategory::QuotaExceeded => true, + AcpErrorCategory::ApiServerError + | AcpErrorCategory::QuotaExceeded + | AcpErrorCategory::AgentUnreachable => true, AcpErrorCategory::Authentication | AcpErrorCategory::ExecutableNotFound | AcpErrorCategory::Initialization | AcpErrorCategory::PromptTooLong + | AcpErrorCategory::SessionNotFound + | AcpErrorCategory::SessionNotResumable + | AcpErrorCategory::NoActiveSession + | AcpErrorCategory::SessionAlreadyActive | AcpErrorCategory::Unknown => false, } } } +/// A categorized ACP error, carrying the agent-supplied `error.data.detail` +/// (e.g. a login hint) when the structured error included one. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AcpErrorDetails { + pub category: AcpErrorCategory, + pub detail: Option, +} + +/// Categorize an error, preferring the structured ACP error (JSON-RPC `code` +/// + `data`) when one is present in the anyhow chain over string heuristics. +/// +/// The code table mirrors the agent side (`nori-handroll acp`): `-32000` +/// auth_required, `-32002` resource/session not found, `-32010` backing +/// service unreachable, `-32012` session not resumable, `-32014` no active +/// session, `-32015` a session is already active. +pub fn categorize_acp_error_chain(error: &anyhow::Error) -> AcpErrorDetails { + for cause in error.chain() { + let Some(acp_error) = cause.downcast_ref::() else { + continue; + }; + let detail = acp_error.data.as_ref().and_then(|data| { + data.get("detail") + .and_then(serde_json::Value::as_str) + .map(str::to_string) + }); + let category = match i32::from(acp_error.code) { + -32000 => AcpErrorCategory::Authentication, + -32002 => AcpErrorCategory::SessionNotFound, + -32010 => AcpErrorCategory::AgentUnreachable, + -32012 => AcpErrorCategory::SessionNotResumable, + -32014 => AcpErrorCategory::NoActiveSession, + -32015 => AcpErrorCategory::SessionAlreadyActive, + _ => categorize_acp_error(&format!("{acp_error:?}")), + }; + return AcpErrorDetails { category, detail }; + } + AcpErrorDetails { + category: categorize_acp_error(&format!("{error:?}")), + detail: None, + } +} + /// Categorize an ACP error based on error string patterns. /// /// This function analyzes error messages and categorizes them to enable @@ -87,3 +148,86 @@ pub fn categorize_acp_error(error: &str) -> AcpErrorCategory { AcpErrorCategory::Unknown } } + +#[cfg(test)] +mod tests { + use super::*; + use agent_client_protocol as acp; + use pretty_assertions::assert_eq; + + /// Wrap a structured ACP error the way connection methods surface it: as + /// the source of an anyhow chain with a human context message on top. + fn chain_with(acp_error: acp::Error) -> anyhow::Error { + anyhow::Error::new(acp_error).context("Failed to resume ACP session") + } + + #[test] + fn auth_required_categorizes_as_authentication_with_login_detail() { + let error = chain_with( + acp::Error::auth_required() + .data(serde_json::json!({ "detail": "run: nori-handroll login" })), + ); + + let details = categorize_acp_error_chain(&error); + + assert_eq!( + (details.category, details.detail.as_deref()), + ( + AcpErrorCategory::Authentication, + Some("run: nori-handroll login") + ) + ); + } + + #[test] + fn session_lifecycle_codes_map_to_dedicated_categories() { + let cases = [ + (-32002, AcpErrorCategory::SessionNotFound), + (-32010, AcpErrorCategory::AgentUnreachable), + (-32012, AcpErrorCategory::SessionNotResumable), + (-32014, AcpErrorCategory::NoActiveSession), + (-32015, AcpErrorCategory::SessionAlreadyActive), + ]; + for (code, expected) in cases { + let error = chain_with(acp::Error::new(code, "structured error")); + assert_eq!( + categorize_acp_error_chain(&error).category, + expected, + "code {code}" + ); + } + } + + #[test] + fn structured_code_takes_precedence_over_message_heuristics() { + // The message alone would substring-match the auth heuristics; the + // structured code must win. + let error = chain_with(acp::Error::new(-32002, "unauthorized session")); + + assert_eq!( + categorize_acp_error_chain(&error).category, + AcpErrorCategory::SessionNotFound + ); + } + + #[test] + fn falls_back_to_string_heuristics_without_structured_error() { + let error = anyhow::anyhow!("429 too many requests"); + + let details = categorize_acp_error_chain(&error); + + assert_eq!( + (details.category, details.detail), + (AcpErrorCategory::QuotaExceeded, None) + ); + } + + #[test] + fn broker_unreachable_is_retryable_but_session_states_are_not() { + assert!(AcpErrorCategory::AgentUnreachable.is_retryable()); + assert!(!AcpErrorCategory::SessionNotFound.is_retryable()); + assert!(!AcpErrorCategory::SessionNotResumable.is_retryable()); + assert!(!AcpErrorCategory::SessionAlreadyActive.is_retryable()); + assert!(!AcpErrorCategory::NoActiveSession.is_retryable()); + } +} diff --git a/nori-rs/acp-host/src/lib.rs b/nori-rs/acp-host/src/lib.rs index 8a1b95b85..8aaee39cf 100644 --- a/nori-rs/acp-host/src/lib.rs +++ b/nori-rs/acp-host/src/lib.rs @@ -12,4 +12,6 @@ pub mod registry; pub mod translator; pub use error_category::AcpErrorCategory; +pub use error_category::AcpErrorDetails; pub use error_category::categorize_acp_error; +pub use error_category::categorize_acp_error_chain; diff --git a/nori-rs/cli/docs.md b/nori-rs/cli/docs.md index 78e20f05b..eb99a6dcc 100644 --- a/nori-rs/cli/docs.md +++ b/nori-rs/cli/docs.md @@ -53,6 +53,7 @@ match subcommand { - The dispatch in `main.rs` forces `interactive.agent = "nori-cloud"` AFTER flag merging, so `--agent` cannot bypass Sessions, and passes the entry via the clap-skipped `TuiCli.extra_agents` field (see `@/nori-rs/tui/src/cli.rs`) - From there the handroll child rides the ordinary local-agent path end to end: registry lookup, `AcpConnection::spawn()`, and unconditional local transcript recording (duplicating the broker's server-side recording is intentional) - Auth, broker REST, session acquisition/release, and tunnel transport all live inside `nori-handroll cloud-acp`. Clean release relies on the graceful stdin-EOF shutdown contract in `@/nori-rs/acp-host/src/connection/acp_connection.rs` +- Cloud session lifecycle rides ordinary ACP capabilities, not cloud-specific plumbing: the handroll agent advertises `sessionCapabilities.{list,resume,close}` with `loadSession: false`, so the in-TUI `/resume` picker lists broker sessions and reattaches live via `session/resume` (no history replay), and `/close` releases the session via `session/close`. The one-active-session contract (close before claiming another) is enforced agent-side. See `@/nori-rs/harness/docs.md` (resume strategy selection) and `@/nori-rs/tui/docs.md` (`/resume`, `/close`) - TUI flags such as `--agent`, `--profile`, `--sandbox` can still be passed after `cloud` (only `--agent` is overridden) **Debug Sandbox** (`debug_sandbox.rs`): Implementation of the sandbox testing commands. diff --git a/nori-rs/docs.md b/nori-rs/docs.md index caa15936c..dc129b652 100644 --- a/nori-rs/docs.md +++ b/nori-rs/docs.md @@ -23,7 +23,7 @@ The workspace is converging on a three-layer structure -- publishable ACP-host l ### Core Implementation -The TUI drives user interaction through a Ratatui-based interface. When using ACP mode (the primary mode for Nori), user prompts flow through `nori-harness` which communicates with ACP agents over JSON-RPC 2.0 via stdin/stdout of a local subprocess. Cloud sessions (`nori cloud`) use the same path: the CLI pins the agent to an external `nori-handroll cloud-acp` child, and all broker/auth/transport concerns live in that binary (nori-sessions repo). Configuration is loaded unconditionally from `~/.nori/cli/config.toml` via the `nori-config` crate, which the frontends depend on directly. +The TUI drives user interaction through a Ratatui-based interface. When using ACP mode (the primary mode for Nori), user prompts flow through `nori-harness` which communicates with ACP agents over JSON-RPC 2.0 via stdin/stdout of a local subprocess. Cloud sessions (`nori cloud`) use the same path: the CLI pins the agent to an external `nori-handroll cloud-acp` child, and all broker/auth/transport concerns live in that binary (nori-sessions repo). Cloud session lifecycle is expressed through standard ACP capabilities -- the agent advertises `sessionCapabilities.{list,resume,close}` (with `loadSession: false`), so `/resume` lists and live-reattaches broker sessions via `session/resume` and `/close` releases them via `session/close`; the one-active-session contract lives agent-side (see `@/nori-rs/cli/docs.md` and `@/nori-rs/harness/docs.md`). Configuration is loaded unconditionally from `~/.nori/cli/config.toml` via the `nori-config` crate, which the frontends depend on directly. Architecture: - nori-tui (TUI) -> Terminal User Interface diff --git a/nori-rs/harness/docs.md b/nori-rs/harness/docs.md index a751ea3a4..90f67edf4 100644 --- a/nori-rs/harness/docs.md +++ b/nori-rs/harness/docs.md @@ -122,7 +122,7 @@ The ACP backend owns the `/goal` feature as per-session state instead of delegat `nori_client_mcp.rs` is a bridge, not a second store. The goal tools are typed rmcp `#[tool]` handlers on `NoriClientService`; they lock the same `ThreadGoalState` used by TUI `/goal` operations, return JSON snapshots shaped for model consumption, and emit the same `ThreadGoalUpdated` client event after mutations. The bridge records those emitted events through `@/nori-rs/harness/src/backend/transcript.rs` when a transcript recorder is available; `NoriClientShared` stores the recorder behind a shared cell because the service is built before the session id is known. -The local `nori-client` server is only advertised when `register_for_session` in `@/nori-rs/harness/src/backend/nori_client_mcp.rs` sees HTTP MCP support from `@/nori-rs/acp-host/src/connection/mod.rs`. Nori advertises a real `http://127.0.0.1:/mcp` endpoint rather than an ACP pseudo-URL, because Codex ACP and Claude ACP both forward ACP `mcpServers` to their underlying clients as ordinary HTTP MCP server config. Each eligible session registration gets a fresh loopback server with a generated bearer token advertised as an `Authorization` header; an `axum` middleware rejects unauthenticated requests before they reach rmcp's `StreamableHttpService` (stateless mode). The server is owned by the ACP backend (abort-on-drop) and talks directly to the same in-memory goal state as `/goal`. The server is named `nori-client` rather than `nori-goal` because it is Nori's general harness-side channel to the external ACP agent -- the single point of contact for harness-specific tooling the ACP protocol does not yet provide -- and goal tools are only its live-state surface. The backend emits a `SessionCapabilitiesChanged` projection after session setup so clients can derive built-in command availability from the same capability state. That projection separates raw agent capabilities (`agent.http_mcp`, `agent.load_session`, `agent.session_list`), `nori_client.advertised`, `nori_client.initialized`, and derived `builtin_commands` availability. `agent.session_list` is the raw projection of the agent's ACP `session/list` capability (`session_capabilities.list.is_some()`), set at both the `capabilities_update_for_nori_client` and `register_for_session` build sites in `@/nori-rs/harness/src/backend/nori_client_mcp.rs`; the TUI consumes it to decide whether the in-session `/resume` picker is sourced from the live agent (see `@/nori-rs/tui/docs.md`). Initial and post-replacement snapshots read the same connected flag flipped by MCP `initialize`, so agents that eagerly initialize the advertised server during session setup do not observe initialized state regress from true back to false. +The local `nori-client` server is only advertised when `register_for_session` in `@/nori-rs/harness/src/backend/nori_client_mcp.rs` sees HTTP MCP support from `@/nori-rs/acp-host/src/connection/mod.rs`. Nori advertises a real `http://127.0.0.1:/mcp` endpoint rather than an ACP pseudo-URL, because Codex ACP and Claude ACP both forward ACP `mcpServers` to their underlying clients as ordinary HTTP MCP server config. Each eligible session registration gets a fresh loopback server with a generated bearer token advertised as an `Authorization` header; an `axum` middleware rejects unauthenticated requests before they reach rmcp's `StreamableHttpService` (stateless mode). The server is owned by the ACP backend (abort-on-drop) and talks directly to the same in-memory goal state as `/goal`. The server is named `nori-client` rather than `nori-goal` because it is Nori's general harness-side channel to the external ACP agent -- the single point of contact for harness-specific tooling the ACP protocol does not yet provide -- and goal tools are only its live-state surface. The backend emits a `SessionCapabilitiesChanged` projection after session setup so clients can derive built-in command availability from the same capability state. That projection separates raw agent capabilities (`agent.http_mcp`, `agent.load_session`, `agent.session_list`, `agent.session_resume`, `agent.session_close`), `nori_client.advertised`, `nori_client.initialized`, and derived `builtin_commands` availability. The session-lifecycle fields are raw projections of the agent's ACP `sessionCapabilities` (e.g. `session_capabilities.list.is_some()`), built by the shared `agent_capabilities_view()` helper in `@/nori-rs/harness/src/backend/nori_client_mcp.rs` so the initial and update snapshots cannot drift; the TUI consumes them to gate the agent-sourced `/resume` picker and the `/close` command (see `@/nori-rs/tui/docs.md`). Initial and post-replacement snapshots read the same connected flag flipped by MCP `initialize`, so agents that eagerly initialize the advertised server during session setup do not observe initialized state regress from true back to false. `@/nori-rs/harness/src/backend/nori_client_context.rs` owns the fixed read-only catalog exposed through the same server. `nori_client_mcp.rs` advertises tools, resources, and prompts in `ServerInfo`, but delegates list/read/get operations to that sibling module so transport, initialization, connected-gate behavior, and capability registration stay separate from Nori's curated operating context. The catalog intentionally serves Nori-owned harness facts, minimal ACP debugging and custom-agent help, and a compact source map for answering Nori CLI questions; it is not an arbitrary filesystem, repo-read, or skill-workflow API. Unknown resource URIs and prompt names are rejected as MCP errors, keeping the context surface closed and predictable. @@ -1006,11 +1006,11 @@ The `ContextCompactedEvent.summary` field is the coupling point between the ACP **Session Resume** (`backend/mod.rs`, `connection.rs`): -`AcpBackend::resume_session()` allows reconnecting to a previous ACP session. It takes `acp_session_id: Option<&str>`, `transcript: Option<&Transcript>`, and a single `backend_event_tx`, then selects between two resume strategies based on agent capabilities. The resulting `BackendEvent` stream carries both normalized ACP session events and shared control-plane events. +`AcpBackend::resume_session()` allows reconnecting to a previous ACP session. It takes `acp_session_id: Option<&str>`, `transcript: Option<&Transcript>`, and a single `backend_event_tx`, then selects between three resume strategies based on agent capabilities. The resulting `BackendEvent` stream carries both normalized ACP session events and shared control-plane events. `resume_session()` always spawns the agent via `AcpConnection::spawn()` -- like every other code path in this crate, it only knows about local subprocess agents. -The session *list* that feeds the in-session `/resume` picker can be sourced two ways. When the live agent advertises `session/list` (surfaced as `agent.session_list`), `@/nori-rs/tui` calls `AcpConnection::list_sessions()` on the already-running connection to enumerate the agent's own resumable sessions, then resumes the selected one over ACP via `session/load` against that same session (no local transcript involved). Otherwise the TUI falls back to enumerating the local on-disk transcript store. Either way, the chosen session is resumed through the `resume_session()` strategy selection above; the agent-sourced path passes `transcript: None` and relies on server-side `session/load` replay rather than transcript-derived replay. The standalone `nori resume` startup picker is unaffected by `session/list` and remains transcript-backed. See `@/nori-rs/acp-host/src/connection/docs.md` for `list_sessions()` and the `AcpSessionSummary` boundary type. +The session *list* that feeds the in-session `/resume` picker can be sourced two ways. When the live agent advertises `session/list` (surfaced as `agent.session_list`), `@/nori-rs/tui` calls `AcpConnection::list_sessions()` on the already-running connection to enumerate the agent's own resumable sessions, then resumes the selected one over ACP -- via `session/load` (history replay) or `session/resume` (live reattach), whichever the agent advertises -- with no local transcript involved. Otherwise the TUI falls back to enumerating the local on-disk transcript store. Either way, the chosen session is resumed through the `resume_session()` strategy selection above; the agent-sourced path passes `transcript: None` and relies on the agent-side session rather than transcript-derived replay. The standalone `nori resume` startup picker is unaffected by `session/list` and remains transcript-backed. See `@/nori-rs/acp-host/src/connection/docs.md` for `list_sessions()` and the `AcpSessionSummary` boundary type. ``` AcpBackend::resume_session(config, acp_session_id, transcript, backend_event_tx) @@ -1019,7 +1019,7 @@ AcpBackend::resume_session(config, acp_session_id, transcript, backend_event_tx) AcpConnection::spawn() | v -check capabilities().load_session +check capabilities (branch order: load_session > session_resume > client-side replay) | β”œβ”€β”€ Agent supports session/load AND acp_session_id is Some: β”‚ | @@ -1036,6 +1036,16 @@ check capabilities().load_session β”‚ Falls through to client-side replay (see below) β”‚ WarningEvent emitted to TUI about the fallback β”‚ + β”œβ”€β”€ Agent supports session/resume (not session/load) AND acp_session_id is Some: + β”‚ | + β”‚ v + β”‚ AcpConnection::resume_session(session_id, cwd, mcp_servers) + β”‚ | + β”‚ β”œβ”€β”€ Success: + β”‚ β”‚ Live reattach β€” no history replay, no deferred events + β”‚ β”‚ + β”‚ └── Failure: propagates as an enhanced error (NO session/new fallback) + β”‚ └── Otherwise (client-side replay fallback): | v @@ -1056,7 +1066,11 @@ Deferred replay relay spawned (sends buffered events, then optional goal resume **Server-side path:** A collect task runs concurrently during `load_session()`, taking ownership of the ordered `ConnectionEvent` receiver and buffering the normalized `ClientEvent` stream into a `Vec`. `AcpConnection::load_session()` reuses that same ordered inbox for the agent's replay notifications, so the collector can observe session updates in source order without a special side channel. The buffered events are returned as `deferred_replay_events` and a relay task is spawned only _after_ all setup events (`SessionConfigured`, `Warning`, etc.) have been sent to the outbound backend-event channel. This deferred-relay pattern prevents a deadlock: the outbound channel is bounded, and the TUI consumer only starts after `resume_session()` returns, so sending replay events before setup events would fill the channel and block `resume_session()` from making progress. If `load_session()` fails at runtime (e.g., the agent advertises the capability but the call itself errors), the collect task is aborted and the method falls back to a fresh session. A `WarningEvent` is emitted to inform the user that the restored session will not have server-side replay. -**Client-side path:** When the agent does not support `session/load` (e.g., Claude Code's ACP adapter returns `method_not_found`), or when the server-side `load_session()` call fails at runtime, a fresh session is created via `session/new`. The previous conversation is replayed through normalized `ClientEvent::ReplayEntry` items derived from the transcript rather than through `SessionConfigured.initial_messages`. The transcript summary path remains available for context management and `/compact`-style behavior. A `TRANSCRIPT_SUMMARY_WARN_CHARS` threshold (200K chars) logs a warning when summaries are very large; the actual safety net is the agent-side "prompt too long" rejection, which the caller handles gracefully. +**Live-reattach path (`session/resume`):** When the agent advertises `sessionCapabilities.resume` but not `session/load` -- the nori cloud contract (`nori-handroll acp --type cloud` advertises `list`/`resume`/`close` with `loadSession: false`) -- the backend reattaches to the still-live agent-side session via `AcpConnection::resume_session()`. There is no history replay and no deferred-relay machinery; the session simply continues. Unlike the load path, a failure here does **not** fall back to `session/new`: on a cloud agent that fallback would silently claim a brand-new VM the user never asked for, so failures propagate as enhanced errors (categorized via `categorize_acp_error_chain`, e.g. `SessionNotFound` when the broker session is gone). The `nori-client` MCP server is registered before the resume call, mirroring the other paths. + +**Session close:** `AcpBackend::close_active_session()` releases the current session via ACP `session/close`. It is reached through `AcpAgentHandle::close_session()` and the `AcpAgentCommand::CloseSession` command in `@/nori-rs/harness/src/runtime.rs`; the TUI's `/close` command is the caller and gates on the agent advertising `sessionCapabilities.close` (see `@/nori-rs/tui/docs.md`). The one-active-session contract (close before resuming or creating another) is enforced agent-side, not here. + +**Client-side path:** When the agent does not support `session/load` or `session/resume` (e.g., Claude Code's ACP adapter returns `method_not_found`), or when the server-side `load_session()` call fails at runtime, a fresh session is created via `session/new`. The previous conversation is replayed through normalized `ClientEvent::ReplayEntry` items derived from the transcript rather than through `SessionConfigured.initial_messages`. The transcript summary path remains available for context management and `/compact`-style behavior. A `TRANSCRIPT_SUMMARY_WARN_CHARS` threshold (200K chars) logs a warning when summaries are very large; the actual safety net is the agent-side "prompt too long" rejection, which the caller handles gracefully. A new `TranscriptRecorder` is created for the resumed session in all paths, persisting the `acp_session_id` so the session can be resumed again in the future. @@ -1112,7 +1126,20 @@ The `handle_undo_to()` completion message includes a warning: "the agent is not - Ghost commits are unreferenced git objects (not on any branch) created by the `codex-git` crate - `GhostSnapshotStack` is deliberately a standalone type (not embedded inside `AcpBackend`) so it can be tested independently without requiring an ACP agent connection -**ACP Error Categorization** (`@/nori-rs/acp-host/src/error_category.rs`, re-exported as `nori_acp_host::{AcpErrorCategory, categorize_acp_error}`; user-facing message composition via `enhanced_error_message()` stays in `backend/mod.rs`): +**ACP Error Categorization** (`@/nori-rs/acp-host/src/error_category.rs`, re-exported as `nori_acp_host::{AcpErrorCategory, AcpErrorDetails, categorize_acp_error, categorize_acp_error_chain}`; user-facing message composition via `enhanced_error_message()` stays in `backend/mod.rs`): + +Categorization is two-tier. `categorize_acp_error_chain()` first walks the anyhow chain for a structured `acp::Error` and maps it by JSON-RPC code -- the code always wins over message text -- returning `AcpErrorDetails` with the category plus any agent-supplied `error.data.detail`. Structured code mapping (the table mirrors the `nori-handroll acp` agent side): + +| Code | Category | Retryable | User Message (spawn path) | +| -------- | ---------------------- | --------- | -------------------------------------------------------------------------------- | +| `-32000` | `Authentication` | no | "Authentication required for {provider}. {auth_hint}" | +| `-32002` | `SessionNotFound` | no | Session no longer exists -- pick another from /resume or start a new one | +| `-32010` | `AgentUnreachable` | yes | Backing service unreachable -- check your network and try again | +| `-32012` | `SessionNotResumable` | no | Session cannot be reattached -- start a new session | +| `-32014` | `NoActiveSession` | no | No session is active -- start a new chat with /new | +| `-32015` | `SessionAlreadyActive` | no | A session is already active -- close it with /close first | + +Without a structured error, the legacy string heuristics (`categorize_acp_error()`) apply: | Category | Detection Patterns | Retryable | User Message | | -------------------- | ----------------------------------------------------------------------------- | --------- | ---------------------------------------------------------------------------------------- | @@ -1123,11 +1150,13 @@ The `handle_undo_to()` completion message includes a warning: "the agent is not | `PromptTooLong` | "prompt is too long" | no | "Prompt is too long. Try using /compact to reduce context size, or start a new session." | | `ApiServerError` | "500", "502", "503", "504", "529", "server error", "api_error", "overloaded" | yes | "The API returned a server error. This is usually temporary -- please try again." | -The priority chain is: Auth > Quota > ExecutableNotFound > Initialization > PromptTooLong > ApiServerError > Unknown. Earlier categories take precedence when an error message matches multiple patterns (e.g., "500 authentication service unavailable" categorizes as `Authentication`, not `ApiServerError`). The bare `529` (Anthropic `overloaded_error`) and the underscore form `rate_limit` (Anthropic `rate_limit_error`) are matched so real overloaded/rate-limit strings land in the retryable buckets instead of falling through to `Unknown`. +The heuristic priority chain is: Auth > Quota > ExecutableNotFound > Initialization > PromptTooLong > ApiServerError > Unknown. Earlier categories take precedence when an error message matches multiple patterns (e.g., "500 authentication service unavailable" categorizes as `Authentication`, not `ApiServerError`). The bare `529` (Anthropic `overloaded_error`) and the underscore form `rate_limit` (Anthropic `rate_limit_error`) are matched so real overloaded/rate-limit strings land in the retryable buckets instead of falling through to `Unknown`. + +The heuristic fallback operates on the `Debug`-formatted (`{e:?}`) anyhow error to inspect the full error chain, while the user-facing `display_error` string uses `{e:#}` (alternate Display) in the prompt error handler to show the complete chain rather than just the outermost `.context()` wrapper. Both error message paths (spawn/resume/close errors via `enhance_agent_error()` in `backend/mod.rs` and prompt errors via the match block in `send_prompt_error` in `backend/session_runtime_driver.rs`) use `categorize_acp_error_chain()` and handle all categories. -Error categorization operates on the `Debug`-formatted (`{e:?}`) anyhow error to inspect the full error chain, while the user-facing `display_error` string uses `{e:#}` (alternate Display) in the prompt error handler to show the complete chain rather than just the outermost `.context()` wrapper. Both error message paths (spawn errors via `enhanced_error_message()` in `backend/mod.rs` and prompt errors via the match block in `send_prompt_error` in `backend/session_runtime_driver.rs`) handle all categories. +**Agent-supplied detail:** `enhance_agent_error()` treats the structured `data.detail` as more precise than static registry hints. For `Authentication` errors the detail (e.g. the deployment's exact login command from `nori-handroll`) replaces the registry's static `auth_hint`; for every other category the detail is appended verbatim in parentheses (e.g. "the session is no longer claimed"). -**Retryable classification and the failure disposition:** `AcpErrorCategory::is_retryable()` returns `true` only for `ApiServerError` and `QuotaExceeded` -- transient server errors and rate/quota limits that clear on their own -- and `false` for the persistent categories a retry cannot fix. The retryable bool no longer rides on `EventMsg::Error`; `ErrorEvent` in `@/nori-rs/protocol/src/protocol/mod.rs` now carries only `message` and is display-only. Instead the disposition is threaded onto the prompt **completion** as a `nori_protocol::TurnFailure` (`Retryable`/`Fatal`) so a single ordered event decides both how the failure is surfaced and whether an unattended loop should keep going (the TUI consumes this in `@/nori-rs/tui/docs.md`, Loop Mode section). +**Retryable classification and the failure disposition:** `AcpErrorCategory::is_retryable()` returns `true` only for `ApiServerError`, `QuotaExceeded`, and `AgentUnreachable` -- transient server errors, rate/quota limits, and network conditions that clear on their own -- and `false` for the persistent categories a retry cannot fix (including all session-lifecycle states). The retryable bool no longer rides on `EventMsg::Error`; `ErrorEvent` in `@/nori-rs/protocol/src/protocol/mod.rs` now carries only `message` and is display-only. Instead the disposition is threaded onto the prompt **completion** as a `nori_protocol::TurnFailure` (`Retryable`/`Fatal`) so a single ordered event decides both how the failure is surfaced and whether an unattended loop should keep going (the TUI consumes this in `@/nori-rs/tui/docs.md`, Loop Mode section). The path: `send_prompt_error` in `backend/session_runtime_driver.rs` emits the `EventMsg::Error` for display and *returns* the retryable bool (computed as `category.is_retryable()` for user prompts; `Compact` and `GoalContinuation` failures are always non-retryable). The prompt-task error handler maps that bool to `Some(TurnFailure::Retryable | Fatal)` and sends it as `InboundEvent::PromptFailed { failure }` to the reducer (`backend/session_reducer.rs`). `reduce_prompt_failed` threads `failure` straight onto the `PromptCompleted` it pushes, while clean completions (`reduce_prompt_response`) carry `failure: None`. The two non-prompt-task producers of `PromptFailed` use fixed dispositions: the cancel watchdog's force-cancel/timeout sends `failure: None` (a clean forced cancel, not a failure), and unexpected agent process exit (`ConnectionEvent::ChildExited`) in `backend/spawn_and_relay.rs` sends `Some(TurnFailure::Fatal)`. The generic `send_error` op path in `backend/submit_and_ops.rs` only emits a display `EventMsg::Error` and is not part of the prompt-completion path. The bare `529` and underscore `rate_limit` matches above still route real overloaded/rate-limit strings into the retryable buckets; `send_prompt_error` logs the category, the resolved retryable bool, and the raw error for observability. diff --git a/nori-rs/harness/src/backend/mod.rs b/nori-rs/harness/src/backend/mod.rs index fc77d32d3..6ceb00c79 100644 --- a/nori-rs/harness/src/backend/mod.rs +++ b/nori-rs/harness/src/backend/mod.rs @@ -54,7 +54,9 @@ use crate::undo::GhostSnapshotStack; // ============================================================================= pub use nori_acp_host::AcpErrorCategory; +pub use nori_acp_host::AcpErrorDetails; pub use nori_acp_host::categorize_acp_error; +pub use nori_acp_host::categorize_acp_error_chain; /// Generate an enhanced error message with actionable instructions. /// @@ -91,6 +93,24 @@ pub fn enhanced_error_message( "The API returned a server error. This is usually temporary β€” please try again." .to_string() } + AcpErrorCategory::SessionNotFound => format!( + "That session no longer exists on {provider_name} β€” it may have expired or been \ + closed elsewhere. Pick another session from /resume or start a new one." + ), + AcpErrorCategory::SessionNotResumable => format!( + "That session can't be reattached on {provider_name}. Start a new session instead." + ), + AcpErrorCategory::SessionAlreadyActive => format!( + "A session is already active on the {provider_name} connection. Close it with /close \ + before resuming or creating another." + ), + AcpErrorCategory::NoActiveSession => format!( + "No session is active on the {provider_name} connection. Start a new chat with /new." + ), + AcpErrorCategory::AgentUnreachable => format!( + "Could not reach the {provider_name} backing service. Check your network and try \ + again. Original error: {original_error}" + ), AcpErrorCategory::Unknown => original_error.to_string(), } } @@ -102,15 +122,26 @@ pub fn enhance_agent_error( error: anyhow::Error, config: &crate::registry::AcpAgentConfig, ) -> anyhow::Error { - let category = categorize_acp_error(&format!("{error:?}")); - anyhow::anyhow!(enhanced_error_message( - category, + let details = categorize_acp_error_chain(&error); + // An agent-supplied `error.data.detail` (e.g. the exact login command for + // this deployment) is more precise than the registry's static auth hint. + let auth_hint = details.detail.as_deref().unwrap_or(&config.auth_hint); + let mut message = enhanced_error_message( + details.category.clone(), &format!("{error}"), &config.provider_info.name, - &config.auth_hint, + auth_hint, &config.display_name, &config.install_hint, - )) + ); + // For non-auth categories the detail is agent-supplied context (e.g. + // "the session is no longer claimed") β€” keep it verbatim. + if details.category != AcpErrorCategory::Authentication + && let Some(detail) = details.detail + { + message = format!("{message} ({detail})"); + } + anyhow::anyhow!(message) } /// Configuration for spawning an ACP backend. diff --git a/nori-rs/harness/src/backend/nori_client_mcp.rs b/nori-rs/harness/src/backend/nori_client_mcp.rs index 37c707597..c578d151a 100644 --- a/nori-rs/harness/src/backend/nori_client_mcp.rs +++ b/nori-rs/harness/src/backend/nori_client_mcp.rs @@ -463,16 +463,23 @@ pub(super) fn capabilities_update_for_nori_client( nori_client_advertised: bool, nori_client_initialized: bool, ) -> nori_protocol::SessionCapabilitiesView { - let agent = nori_protocol::AgentCapabilitiesView { - http_mcp: connection.capabilities().mcp_capabilities.http, - load_session: connection.capabilities().load_session, - session_list: connection - .capabilities() - .session_capabilities - .list - .is_some(), - }; - capabilities_update(agent, nori_client_advertised, nori_client_initialized) + capabilities_update( + agent_capabilities_view(connection), + nori_client_advertised, + nori_client_initialized, + ) +} + +/// Project the agent's ACP capabilities into the client-facing view. +fn agent_capabilities_view(connection: &AcpConnection) -> nori_protocol::AgentCapabilitiesView { + let capabilities = connection.capabilities(); + nori_protocol::AgentCapabilitiesView { + http_mcp: capabilities.mcp_capabilities.http, + load_session: capabilities.load_session, + session_list: capabilities.session_capabilities.list.is_some(), + session_resume: capabilities.session_capabilities.resume.is_some(), + session_close: capabilities.session_capabilities.close.is_some(), + } } fn capabilities_update( @@ -516,15 +523,7 @@ pub(super) async fn register_for_session( backend_event_tx, transcript_recorder, Arc::clone(&connected), - nori_protocol::AgentCapabilitiesView { - http_mcp: connection.capabilities().mcp_capabilities.http, - load_session: connection.capabilities().load_session, - session_list: connection - .capabilities() - .session_capabilities - .list - .is_some(), - }, + agent_capabilities_view(connection), true, )) .await?; @@ -552,6 +551,8 @@ mod tests { http_mcp: true, load_session: false, session_list: false, + session_resume: false, + session_close: false, } } diff --git a/nori-rs/harness/src/backend/session.rs b/nori-rs/harness/src/backend/session.rs index 62da8f7bd..a00efc91c 100644 --- a/nori-rs/harness/src/backend/session.rs +++ b/nori-rs/harness/src/backend/session.rs @@ -1,5 +1,36 @@ use super::*; +/// Build the session's MCP server list (CLI-configured servers plus the +/// nori-client goal server when the agent advertises HTTP MCP) and commit +/// the registered server. Shared by every branch of `resume_session`. +async fn session_mcp_servers( + config: &AcpBackendConfig, + connection: &AcpConnection, + thread_goal_state: &Arc>, + backend_event_tx: &mpsc::Sender, + transcript_recorder_cell: &Arc>>>, + goal_mcp_connected: &Arc, + goal_mcp_http_server: &Arc>>, +) -> Result> { + let mut mcp_servers = crate::connection::mcp::to_acp_mcp_servers( + &config.mcp_servers, + config.mcp_oauth_credentials_store_mode, + ); + let nori_client_server = nori_client_mcp::register_for_session( + connection, + &mut mcp_servers, + Arc::clone(thread_goal_state), + backend_event_tx.clone(), + Arc::clone(transcript_recorder_cell), + Arc::clone(goal_mcp_connected), + ) + .await?; + if let Some(server) = nori_client_server { + server.commit(goal_mcp_http_server).await; + } + Ok(mcp_servers) +} + impl AcpBackend { /// Resume a previous ACP session. /// @@ -30,6 +61,11 @@ impl AcpBackend { .map_err(|e| enhance_agent_error(e, &agent_config))?; let supports_load_session = connection.capabilities().load_session; + let supports_session_resume = connection + .capabilities() + .session_capabilities + .resume + .is_some(); let initial_goal_replay_events = transcript .map(transcript_to_replay_client_events) .unwrap_or_default(); @@ -126,22 +162,16 @@ impl AcpBackend { (session_driver, event_rx, buffered_events) }); - let mut mcp_servers = crate::connection::mcp::to_acp_mcp_servers( - &config.mcp_servers, - config.mcp_oauth_credentials_store_mode, - ); - let nori_client_server = nori_client_mcp::register_for_session( + let mcp_servers = session_mcp_servers( + config, &connection, - &mut mcp_servers, - Arc::clone(&thread_goal_state), - backend_event_tx.clone(), - Arc::clone(&transcript_recorder_cell), - Arc::clone(&goal_mcp_connected), + &thread_goal_state, + &backend_event_tx, + &transcript_recorder_cell, + &goal_mcp_connected, + &goal_mcp_http_server, ) .await?; - if let Some(server) = nori_client_server { - server.commit(&goal_mcp_http_server).await; - } match connection.load_session(sid, &cwd, mcp_servers).await { Ok(session_id) => { @@ -177,22 +207,16 @@ impl AcpBackend { anyhow::anyhow!("load session collector task panicked: {err}") })?; - let mut mcp_servers = crate::connection::mcp::to_acp_mcp_servers( - &config.mcp_servers, - config.mcp_oauth_credentials_store_mode, - ); - let nori_client_server = nori_client_mcp::register_for_session( + let mcp_servers = session_mcp_servers( + config, &connection, - &mut mcp_servers, - Arc::clone(&thread_goal_state), - backend_event_tx.clone(), - Arc::clone(&transcript_recorder_cell), - Arc::clone(&goal_mcp_connected), + &thread_goal_state, + &backend_event_tx, + &transcript_recorder_cell, + &goal_mcp_connected, + &goal_mcp_http_server, ) .await?; - if let Some(server) = nori_client_server { - server.commit(&goal_mcp_http_server).await; - } let session_id = connection .create_session(&cwd, mcp_servers) .await @@ -231,25 +255,57 @@ impl AcpBackend { ) } } - } else { - debug!("Agent does not support session/load β€” using client-side replay"); + } else if let Some(sid) = acp_session_id.filter(|_| supports_session_resume) { + debug!("Agent supports session/resume β€” using live reattach"); + + // Live reattach with no history replay (`session/resume`), for + // agents that advertise `sessionCapabilities.resume` with + // `loadSession: false` β€” the nori cloud contract. Failures + // propagate instead of falling back to `session/new`: on a cloud + // agent that fallback would silently claim a brand-new VM the + // user never asked for. + let mcp_servers = session_mcp_servers( + config, + &connection, + &thread_goal_state, + &backend_event_tx, + &transcript_recorder_cell, + &goal_mcp_connected, + &goal_mcp_http_server, + ) + .await?; + + let session_id = connection + .resume_session(sid, &cwd, mcp_servers) + .await + .map_err(|e| enhance_agent_error(e, &agent_config))?; + debug!("ACP session resumed via session/resume: {sid}"); - let mut mcp_servers = crate::connection::mcp::to_acp_mcp_servers( - &config.mcp_servers, - config.mcp_oauth_credentials_store_mode, + let event_rx = connection.take_event_receiver(); + ( + session_id, + None, + false, + None, + Vec::new(), + event_rx, + session_runtime_driver::SessionDriver::new(), + ) + } else { + debug!( + "Agent supports neither session/load nor session/resume β€” using client-side replay" ); - let nori_client_server = nori_client_mcp::register_for_session( + + let mcp_servers = session_mcp_servers( + config, &connection, - &mut mcp_servers, - Arc::clone(&thread_goal_state), - backend_event_tx.clone(), - Arc::clone(&transcript_recorder_cell), - Arc::clone(&goal_mcp_connected), + &thread_goal_state, + &backend_event_tx, + &transcript_recorder_cell, + &goal_mcp_connected, + &goal_mcp_http_server, ) .await?; - if let Some(server) = nori_client_server { - server.commit(&goal_mcp_http_server).await; - } let session_id = connection .create_session(&cwd, mcp_servers) .await @@ -488,4 +544,21 @@ impl AcpBackend { Ok(backend) } + + /// Close (release) the active session via ACP `session/close`. + /// + /// Used by the explicit close action; callers gate on the agent + /// advertising `sessionCapabilities.close`. Failures carry the same + /// enhanced structured-code-aware message as spawn/resume errors, so the + /// agent's error code and `data.detail` reach the user. + pub async fn close_active_session(&self) -> Result<()> { + let session_id = self.session_id.read().await.clone(); + self.connection + .close_session(&session_id.to_string()) + .await + .map_err(|e| match get_agent_config(&self.agent_name) { + Ok(agent_config) => enhance_agent_error(e, &agent_config), + Err(_) => e, + }) + } } diff --git a/nori-rs/harness/src/backend/session_runtime_driver.rs b/nori-rs/harness/src/backend/session_runtime_driver.rs index 214e2d87f..c975fb88b 100644 --- a/nori-rs/harness/src/backend/session_runtime_driver.rs +++ b/nori-rs/harness/src/backend/session_runtime_driver.rs @@ -769,7 +769,7 @@ impl AcpBackend { } QueuedPromptKind::User => { let error_string = format!("{err:?}"); - let category = categorize_acp_error(&error_string); + let category = categorize_acp_error_chain(err).category; let display_error = format!("{err:#}"); let retryable = category.is_retryable(); warn!( @@ -799,6 +799,21 @@ impl AcpBackend { "The API returned a server error. This is usually temporary β€” please try again." .to_string() } + AcpErrorCategory::SessionNotFound => format!( + "The session no longer exists on the agent β€” it may have expired or been closed elsewhere: {display_error}" + ), + AcpErrorCategory::SessionNotResumable => format!( + "The session can't be reattached β€” start a new session instead: {display_error}" + ), + AcpErrorCategory::SessionAlreadyActive => format!( + "A session is already active on the agent connection β€” close it before switching: {display_error}" + ), + AcpErrorCategory::NoActiveSession => format!( + "No session is active on the agent connection β€” start a new chat with /new: {display_error}" + ), + AcpErrorCategory::AgentUnreachable => format!( + "The agent's backing service is unreachable β€” check your network and try again: {display_error}" + ), AcpErrorCategory::Unknown => format!("ACP prompt failed: {display_error}"), }; (message, retryable) diff --git a/nori-rs/harness/src/backend/tests/mod.rs b/nori-rs/harness/src/backend/tests/mod.rs index 9e6e4d591..fe02b4e82 100644 --- a/nori-rs/harness/src/backend/tests/mod.rs +++ b/nori-rs/harness/src/backend/tests/mod.rs @@ -381,3 +381,4 @@ mod part3; mod part4; mod part5; mod part7; +mod part8; diff --git a/nori-rs/harness/src/backend/tests/part8.rs b/nori-rs/harness/src/backend/tests/part8.rs new file mode 100644 index 000000000..3b61f54cf --- /dev/null +++ b/nori-rs/harness/src/backend/tests/part8.rs @@ -0,0 +1,192 @@ +//! Session-lifecycle tests for agents that advertise the ACP +//! `sessionCapabilities.{resume,close}` methods with `loadSession: false` β€” +//! the nori cloud contract (`nori-handroll acp --type cloud`). Reattach must +//! go over `session/resume` (never `session/load`, never a fresh +//! `session/new`), and explicit cleanup goes over `session/close`. + +use super::*; + +/// Whether the mock agent binary is available; tests skip quietly otherwise +/// (same convention as the other backend test parts). +fn mock_agent_available() -> bool { + let mock_config = + crate::registry::get_agent_config("mock-model").expect("mock-model should be registered"); + if std::path::Path::new(&mock_config.command).exists() { + return true; + } + eprintln!( + "Skipping test: mock_acp_agent not found at {}", + mock_config.command + ); + false +} + +/// When the agent advertises `session/resume` but not `session/load`, +/// resume must reattach via `session/resume` β€” not silently create a fresh +/// session. `MOCK_AGENT_FAIL_NEW_SESSION_FROM=0` makes any `session/new` +/// fallback fail loudly, so success proves the resume path was taken. +#[tokio::test] +#[serial] +async fn resume_uses_session_resume_when_agent_cannot_load() { + use std::time::Duration; + + if !mock_agent_available() { + return; + } + + let _resume_guard = EnvGuard::set("MOCK_AGENT_SUPPORT_SESSION_RESUME", "1"); + let _fail_new_guard = EnvGuard::set("MOCK_AGENT_FAIL_NEW_SESSION_FROM", "0"); + + let temp_dir = tempfile::tempdir().expect("Failed to create temp dir"); + let (backend_event_tx, mut backend_event_rx) = mpsc::channel(64); + let config = build_test_config(temp_dir.path()); + + let result = + AcpBackend::resume_session(&config, Some("acp-session-42"), None, backend_event_tx).await; + + let _backend = result.expect( + "resume_session should reattach via session/resume instead of creating a new session", + ); + + let event = recv_backend_control(&mut backend_event_rx, Duration::from_secs(5)) + .await + .expect("Should receive SessionConfigured within timeout"); + match event.msg { + EventMsg::SessionConfigured(_) => {} + other => panic!( + "Expected SessionConfigured event, got: {:?}", + std::mem::discriminant(&other) + ), + } +} + +/// A failed `session/resume` must propagate a clear user-facing error β€” not +/// silently fall back to `session/new`, which on a cloud agent would claim a +/// brand-new VM the user never asked for. +#[tokio::test] +#[serial] +async fn resume_failure_propagates_a_clear_error_without_new_session_fallback() { + if !mock_agent_available() { + return; + } + + let _resume_guard = EnvGuard::set("MOCK_AGENT_SUPPORT_SESSION_RESUME", "1"); + let _fail_guard = EnvGuard::set("MOCK_AGENT_RESUME_SESSION_FAIL", "1"); + let _fail_new_guard = EnvGuard::set("MOCK_AGENT_FAIL_NEW_SESSION_FROM", "0"); + + let temp_dir = tempfile::tempdir().expect("Failed to create temp dir"); + let (backend_event_tx, _backend_event_rx) = mpsc::channel(64); + let config = build_test_config(temp_dir.path()); + + let result = + AcpBackend::resume_session(&config, Some("gone-session"), None, backend_event_tx).await; + + let err = result + .err() + .expect("resume_session must propagate a session/resume failure"); + let message = format!("{err}"); + assert!( + message.contains("no longer exists"), + "the error must explain the session is gone in user terms, got: {message}" + ); + assert!( + message.contains("the session is no longer claimed"), + "the agent-supplied error.data.detail must reach the user verbatim, got: {message}" + ); + assert!( + !message.contains("new_session"), + "no session/new fallback may run on a resume failure, got: {message}" + ); +} + +/// A failed `session/close` propagates through `close_active_session` with +/// the enhanced structured-code-aware message β€” proving the request crosses +/// the process boundary (a no-op close could not observe the agent-side +/// error) and that close failures get the same message treatment as +/// spawn/resume failures. +#[tokio::test] +#[serial] +async fn close_active_session_failure_carries_the_enhanced_message() { + use std::time::Duration; + + if !mock_agent_available() { + return; + } + + let _close_guard = EnvGuard::set("MOCK_AGENT_SUPPORT_SESSION_CLOSE", "1"); + let _fail_guard = EnvGuard::set("MOCK_AGENT_CLOSE_SESSION_FAIL", "1"); + + let temp_dir = tempfile::tempdir().expect("Failed to create temp dir"); + let (backend_event_tx, mut backend_event_rx) = mpsc::channel(64); + let config = build_test_config(temp_dir.path()); + + let backend = AcpBackend::spawn(&config, backend_event_tx) + .await + .expect("Failed to spawn ACP backend"); + + // Wait for the session to be fully configured before closing it. + let event = recv_backend_control(&mut backend_event_rx, Duration::from_secs(5)) + .await + .expect("Should receive SessionConfigured within timeout"); + match event.msg { + EventMsg::SessionConfigured(_) => {} + other => panic!( + "Expected SessionConfigured event, got: {:?}", + std::mem::discriminant(&other) + ), + } + + let err = backend + .close_active_session() + .await + .expect_err("close_active_session must propagate the agent's failure"); + let message = format!("{err}"); + assert!( + message.contains("no longer exists"), + "close failures must carry the enhanced session-not-found message, got: {message}" + ); +} + +/// The capabilities view projected to frontends must expose the agent's +/// `sessionCapabilities.{resume,close}` so the TUI can gate the cloud resume +/// picker and the /close command. +#[tokio::test] +#[serial] +async fn capabilities_view_exposes_session_resume_and_close() { + use std::time::Duration; + + if !mock_agent_available() { + return; + } + + let _resume_guard = EnvGuard::set("MOCK_AGENT_SUPPORT_SESSION_RESUME", "1"); + let _close_guard = EnvGuard::set("MOCK_AGENT_SUPPORT_SESSION_CLOSE", "1"); + + let temp_dir = tempfile::tempdir().expect("Failed to create temp dir"); + let (backend_event_tx, mut backend_event_rx) = mpsc::channel(64); + let config = build_test_config(temp_dir.path()); + + let _backend = AcpBackend::spawn(&config, backend_event_tx) + .await + .expect("Failed to spawn ACP backend"); + + let update = loop { + match recv_backend_client(&mut backend_event_rx, Duration::from_secs(5)).await { + Some(nori_protocol::ClientEvent::SessionCapabilitiesChanged(update)) => break update, + Some(_) => {} + None => panic!("Timed out waiting for SessionCapabilitiesChanged"), + } + }; + + assert_eq!( + update.agent, + nori_protocol::AgentCapabilitiesView { + http_mcp: false, + load_session: false, + session_list: false, + session_resume: true, + session_close: true, + }, + "the view must mirror exactly what the agent advertised" + ); +} diff --git a/nori-rs/harness/src/runtime.rs b/nori-rs/harness/src/runtime.rs index c1bf7a574..e814fe2e3 100644 --- a/nori-rs/harness/src/runtime.rs +++ b/nori-rs/harness/src/runtime.rs @@ -84,6 +84,10 @@ pub enum AcpAgentCommand { cwd: PathBuf, response_tx: oneshot::Sender>>, }, + /// Close (release) the active session via ACP `session/close`. + CloseSession { + response_tx: oneshot::Sender>, + }, } /// Handle for communicating with an ACP agent. @@ -144,6 +148,17 @@ impl AcpAgentHandle { .await .map_err(|_| anyhow::anyhow!("ACP agent did not respond"))? } + + /// Close (release) the active session via ACP `session/close`. + pub async fn close_session(&self) -> anyhow::Result<()> { + let (response_tx, response_rx) = oneshot::channel(); + self.command_tx + .send(AcpAgentCommand::CloseSession { response_tx }) + .map_err(|_| anyhow::anyhow!("ACP agent command channel closed"))?; + response_rx + .await + .map_err(|_| anyhow::anyhow!("ACP agent did not respond"))? + } } /// Resume parameters for reattaching to a previous session. @@ -368,6 +383,10 @@ pub fn launch_session(spec: SessionLaunchSpec) -> LaunchedSession { let result = backend_for_agent.connection().list_sessions(&cwd).await; let _ = response_tx.send(result); } + AcpAgentCommand::CloseSession { response_tx } => { + let result = backend_for_agent.close_active_session().await; + let _ = response_tx.send(result); + } } } }); diff --git a/nori-rs/mock-acp-agent/docs.md b/nori-rs/mock-acp-agent/docs.md index 2062d3d78..efa8947d2 100644 --- a/nori-rs/mock-acp-agent/docs.md +++ b/nori-rs/mock-acp-agent/docs.md @@ -12,7 +12,7 @@ Used by `@/nori-rs/tui-pty-e2e/` for end-to-end integration testing. The mock ag ### Core Implementation -**Agent wiring**: The mock is assembled in `main()` from the SDK's `Agent.builder()` with typed `on_receive_request`/`on_receive_notification` handler closures -- one per method (`initialize`, `authenticate`, `session/new`, `session/load`, `session/list`, `session/prompt`, `session/set_mode`, `session/set_config_option`, plus the `session/cancel` notification) -- then connected over `ByteStreams` to the wrapped stdin/stdout. Each closure receives the typed request/notification, a `Responder`, and a `ConnectionTo`; the closures share state through an `Arc`. Handled concerns include: +**Agent wiring**: The mock is assembled in `main()` from the SDK's `Agent.builder()` with typed `on_receive_request`/`on_receive_notification` handler closures -- one per method (`initialize`, `authenticate`, `session/new`, `session/load`, `session/resume`, `session/close`, `session/list`, `session/prompt`, `session/set_mode`, `session/set_config_option`, plus the `session/cancel` notification) -- then connected over `ByteStreams` to the wrapped stdin/stdout. Each closure receives the typed request/notification, a `Responder`, and a `ConnectionTo`; the closures share state through an `Arc`. Handled concerns include: - Session creation, load (history replay), and config-option/mode mutation - Prompt processing with simulated responses, streaming, and tool calls - Permission request/response flow @@ -20,9 +20,13 @@ Used by `@/nori-rs/tui-pty-e2e/` for end-to-end integration testing. The mock ag **Mock Behaviors**: Controlled via environment variables that the E2E tests set on the mock agent process. Each env var activates a specific behavior scenario. Key scenarios include multi-turn conversations, tool call streaming, permission requests, file operations, race condition simulations, and session lifecycle behaviors. -**Session Lifecycle Testing**: Several env vars control `session/load` behavior for testing the resume path in `@/nori-rs/harness/src/backend/session.rs`: +**Session Lifecycle Testing**: Several env vars control `session/load`, `session/resume`, and `session/close` behavior for testing the resume/close paths in `@/nori-rs/harness/src/backend/session.rs`: - `MOCK_AGENT_SUPPORT_LOAD_SESSION` -- when set, the agent advertises `load_session: true` in its capabilities during `initialize()` - `MOCK_AGENT_SUPPORT_SESSION_LIST` -- when set, the agent advertises the ACP `session/list` capability during `initialize()` and its `session/list` handler returns two canned `SessionInfo` rows; exercises the agent-sourced `/resume` picker wire path (`AcpConnection::list_sessions()` in `@/nori-rs/acp-host/src/connection/`, surfaced as `agent.session_list`) +- `MOCK_AGENT_SUPPORT_SESSION_RESUME` -- when set, the agent advertises the ACP `session/resume` capability and its handler reattaches to the requested id, returning `config_options`; exercises the live-reattach resume path (`AcpConnection::resume_session()`), the branch the nori cloud agent uses (`resume` without `loadSession`) +- `MOCK_AGENT_SUPPORT_SESSION_CLOSE` -- when set, the agent advertises the ACP `session/close` capability and its handler acknowledges the close; exercises `AcpConnection::close_session()` and the `/close` command path +- `MOCK_AGENT_RESUME_SESSION_FAIL` -- when set, the `session/resume` handler returns a structured `-32002` error with `data.detail` "the session is no longer claimed", exercising `categorize_acp_error_chain()` in `@/nori-rs/acp-host/src/error_category.rs` (SessionNotFound plus detail extraction over the real stdio transport) +- `MOCK_AGENT_CLOSE_SESSION_FAIL` -- when set, the `session/close` handler returns a structured `-32002` error, proving close failures propagate across the process boundary - `MOCK_AGENT_MCP_HTTP` -- when set, the agent advertises HTTP MCP capability so the backend-owned `nori-client` MCP server in `@/nori-rs/harness/src/backend/nori_client_mcp.rs` can be tested through the normal `session/new` MCP server advertisement path - `MOCK_AGENT_INITIALIZE_NORI_CLIENT_DURING_NEW_SESSION` -- when set, `new_session()` eagerly sends an MCP `initialize` request to the advertised `nori-client` server before returning, mirroring agents that initialize advertised MCP servers during session setup - `MOCK_AGENT_FAIL_NEW_SESSION_FROM` -- when set to an integer N, `new_session()` returns an error once the generated session id is at least N, allowing tests to exercise replacement-session failures without breaking the initial backend startup diff --git a/nori-rs/mock-acp-agent/src/main.rs b/nori-rs/mock-acp-agent/src/main.rs index 880765f55..22484a79c 100644 --- a/nori-rs/mock-acp-agent/src/main.rs +++ b/nori-rs/mock-acp-agent/src/main.rs @@ -1341,11 +1341,32 @@ async fn main() -> acp::Result<()> { has_capabilities = true; } + let mut session_capabilities = acp::SessionCapabilities::new(); + let mut has_session_capabilities = false; + if std::env::var("MOCK_AGENT_SUPPORT_SESSION_LIST").is_ok() { eprintln!("Mock agent: advertising session/list capability"); - capabilities = capabilities.session_capabilities( - acp::SessionCapabilities::new().list(acp::SessionListCapabilities::new()), - ); + session_capabilities = + session_capabilities.list(acp::SessionListCapabilities::new()); + has_session_capabilities = true; + } + + if std::env::var("MOCK_AGENT_SUPPORT_SESSION_RESUME").is_ok() { + eprintln!("Mock agent: advertising session/resume capability"); + session_capabilities = + session_capabilities.resume(acp::SessionResumeCapabilities::new()); + has_session_capabilities = true; + } + + if std::env::var("MOCK_AGENT_SUPPORT_SESSION_CLOSE").is_ok() { + eprintln!("Mock agent: advertising session/close capability"); + session_capabilities = + session_capabilities.close(acp::SessionCloseCapabilities::new()); + has_session_capabilities = true; + } + + if has_session_capabilities { + capabilities = capabilities.session_capabilities(session_capabilities); has_capabilities = true; } @@ -1457,6 +1478,50 @@ async fn main() -> acp::Result<()> { }, agent_client_protocol::on_receive_request!(), ) + .on_receive_request( + { + let state = state.clone(); + async move |arguments: acp::ResumeSessionRequest, + responder: Responder, + _cx: ConnectionTo| { + if std::env::var("MOCK_AGENT_RESUME_SESSION_FAIL").is_ok() { + eprintln!("Mock agent: simulating resume_session failure"); + return responder.respond_with_error( + acp::Error::new(-32002, "session not found").data(serde_json::json!({ + "detail": "the session is no longer claimed" + })), + ); + } + eprintln!("Mock agent: resume_session id={}", arguments.session_id); + let session_config = state + .session_configs + .lock() + .unwrap() + .entry(arguments.session_id.to_string()) + .or_insert_with(default_session_config) + .clone(); + responder.respond( + acp::ResumeSessionResponse::new() + .config_options(config_options_for_state(&session_config)), + ) + } + }, + agent_client_protocol::on_receive_request!(), + ) + .on_receive_request( + async move |arguments: acp::CloseSessionRequest, + responder: Responder, + _cx: ConnectionTo| { + if std::env::var("MOCK_AGENT_CLOSE_SESSION_FAIL").is_ok() { + eprintln!("Mock agent: simulating close_session failure"); + return responder + .respond_with_error(acp::Error::new(-32002, "session not found")); + } + eprintln!("Mock agent: close_session id={}", arguments.session_id); + responder.respond(acp::CloseSessionResponse::new()) + }, + agent_client_protocol::on_receive_request!(), + ) .on_receive_request( async move |arguments: acp::ListSessionsRequest, responder: Responder, diff --git a/nori-rs/nori-protocol/docs.md b/nori-rs/nori-protocol/docs.md index fd73093ab..0783a2064 100644 --- a/nori-rs/nori-protocol/docs.md +++ b/nori-rs/nori-protocol/docs.md @@ -34,6 +34,7 @@ agent_client_protocol_schema::SessionUpdate - `SessionInfoUpdate` becomes a lightweight `SessionUpdateInfo` summary. - `UsageUpdate` also becomes `SessionUpdateInfo`, but the usage variant additionally carries `SessionUsageState` so the TUI can update footer context without reparsing the display string. - **Persisted session metadata** now includes `session_info` and `session_usage` alongside available commands, current mode, and config options. `nori-harness` owns persistence, but these structs live here so the reducer and replay pipeline share one runtime model. +- **Session capability projection**: `SessionCapabilitiesView` (with its nested `AgentCapabilitiesView`) is the client-facing snapshot behind `ClientEvent::SessionCapabilitiesChanged`. `AgentCapabilitiesView` carries the raw agent ACP capability projection -- `http_mcp`, `load_session`, and the session-lifecycle capabilities `session_list`, `session_resume`, and `session_close` -- built by `@/nori-rs/harness/src/backend/nori_client_mcp.rs` and consumed by `@/nori-rs/tui` to gate the agent-sourced `/resume` picker and the `/close` command. The three `session_*` fields are serde-defaulted so snapshots from older writers deserialize as `false` rather than failing. - **Thread-goal client events** carry the current goal snapshot (`objective`, lifecycle status, token usage, active time, and timestamps) or a clear notification. They are not derived from ACP provider messages; they are backend session-state projections emitted through the same `ClientEvent` stream as normalized ACP data. - **`is_generic_tool_call()`** gates initial `ToolCall` emission: tool calls with no `raw_input`, no `locations`, empty `content`, and no `/` in the title are suppressed (return empty `Vec`). The normalizer still records them internally so that later attributed `ToolCallUpdate` messages can refine the existing call without forcing the TUI to render a placeholder cell first. - **Invocation priority cascade** in `invocation_from_tool_call()` resolves what the tool is doing, in priority order: diff --git a/nori-rs/nori-protocol/src/lib.rs b/nori-rs/nori-protocol/src/lib.rs index 48d7600b2..afb15e418 100644 --- a/nori-rs/nori-protocol/src/lib.rs +++ b/nori-rs/nori-protocol/src/lib.rs @@ -111,6 +111,13 @@ pub struct AgentCapabilitiesView { /// Whether the agent advertises the ACP `session/list` capability. #[serde(default)] pub session_list: bool, + /// Whether the agent advertises the ACP `session/resume` capability + /// (live reattach without history replay β€” the cloud agent's resume path). + #[serde(default)] + pub session_resume: bool, + /// Whether the agent advertises the ACP `session/close` capability. + #[serde(default)] + pub session_close: bool, } #[derive(Debug, Clone, PartialEq, Eq, Default, Serialize, Deserialize)] @@ -1860,6 +1867,8 @@ mod tests { http_mcp: false, load_session: true, session_list: false, + session_resume: false, + session_close: false, }, nori_client: NoriClientCapabilitiesView { advertised: false, diff --git a/nori-rs/tui/docs.md b/nori-rs/tui/docs.md index 6a168a34a..06dae9e1b 100644 --- a/nori-rs/tui/docs.md +++ b/nori-rs/tui/docs.md @@ -321,6 +321,7 @@ During background system info collection on unix, `check_worktree_cleanup()` run | `/browse` | Open a terminal file manager to browse and edit files | | `/new` | Start a new chat during a conversation | | `/resume` | Resume a previous ACP session | +| `/close` | Close (release) the current agent-side session and start a fresh chat -- capability-gated on ACP `session/close` | | `/init` | Create an AGENTS.md file with instructions | | `/resume-viewonly` | View a previous session transcript (read-only) | | `/compact` | Summarize conversation to prevent context limit | @@ -860,9 +861,13 @@ Resume hints use the shared `RESUME_HINT_LEAD` and `resume_command_for_conversat **Session Resume (`/resume`):** -The `/resume` command allows reconnecting to a previous ACP session. It uses the ACP agent's `session/load` RPC when available, and otherwise falls back to a fresh ACP session plus normalized replay derived from the saved transcript (see `@/nori-rs/harness/docs.md`). +The `/resume` command allows reconnecting to a previous ACP session. It uses the ACP agent's `session/load` RPC (history replay) or `session/resume` RPC (live reattach) when the agent advertises one of them, and otherwise falls back to a fresh ACP session plus normalized replay derived from the saved transcript (see `@/nori-rs/harness/docs.md`). -The picker list itself comes from one of two sources. `ChatWidget::open_resume_session_picker()` has a capability-gated branch: when the agent advertises *both* `session/list` and `load_session` (`self.session_agent_capabilities.session_list && self.session_agent_capabilities.load_session`) and an `acp_handle` exists, it spawns an async task that calls `handle.list_sessions(cwd)` on the live agent (via the `ListSessions { cwd, response_tx }` `AcpAgentCommand`) and emits `AppEvent::ShowAcpResumeSessionPicker { sessions }` with the agent's own `AcpSessionSummary` rows. An empty result inserts a "no resumable sessions" error cell and a list failure inserts an error cell instead of opening a picker. Otherwise it falls back to the existing local-transcript picker (`AppEvent::ShowResumeSessionPicker`) described below. `load_session` is required in addition to `session_list` because resuming an agent-sourced row passes `transcript: None` and depends entirely on server-side `session/load` replay; without `load_session` an agent would silently start a blank session, so such agents fall through to the transcript-backed picker. The `session_list` capability is the raw agent-capability projection sourced from `@/nori-rs/harness`; this is generic to any agent that advertises ACP `session/list`, not Nori/Codex-specific. Selecting an agent-sourced row emits `AppEvent::ResumeAcpSession { acp_session_id }`, whose handler shuts down the current conversation and starts a new resumed ACP chat widget via `new_resumed_acp(init, Some(acp_session_id), None)` -- there is no local transcript, so server-side `session/load` replay rehydrates history. `show_acp_resume_session_picker()` builds the modal via `acp_resume_session_picker_params()` in `@/nori-rs/tui/src/nori/resume_session_picker.rs`, mapping each summary to a row (name = title falling back to session_id, description = relative time plus cwd, action = `ResumeAcpSession`). +The picker list itself comes from one of two sources. `ChatWidget::open_resume_session_picker()` has a capability-gated branch: when the agent advertises `session/list` plus at least one resume mechanism (`session_list && (load_session || session_resume)`) and an `acp_handle` exists, it spawns an async task that calls `handle.list_sessions(cwd)` on the live agent (via the `ListSessions { cwd, response_tx }` `AcpAgentCommand`) and emits `AppEvent::ShowAcpResumeSessionPicker { sessions }` with the agent's own `AcpSessionSummary` rows. An empty result inserts a "no resumable sessions" error cell and a list failure inserts an error cell instead of opening a picker. Otherwise it falls back to the existing local-transcript picker (`AppEvent::ShowResumeSessionPicker`) described below. A resume mechanism is required in addition to `session_list` because resuming an agent-sourced row passes `transcript: None` and depends entirely on the agent-side session -- either `session/load` replay or `session/resume` live reattach; with neither, an agent would silently start a blank session, so such agents fall through to the transcript-backed picker. The `session/resume` arm is what makes the picker work for the `nori cloud` agent, which advertises `list`/`resume`/`close` with `loadSession: false`. The capabilities are raw agent-capability projections sourced from `@/nori-rs/harness`; this is generic to any agent that advertises them, not Nori/Codex-specific. Selecting an agent-sourced row emits `AppEvent::ResumeAcpSession { acp_session_id }`, whose handler shuts down the current conversation and starts a new resumed ACP chat widget via `new_resumed_acp(init, Some(acp_session_id), None)` -- there is no local transcript, so the harness's capability-based strategy selection (`session/load` replay or `session/resume` reattach) rehydrates or reattaches the session. `show_acp_resume_session_picker()` builds the modal via `acp_resume_session_picker_params()` in `@/nori-rs/tui/src/nori/resume_session_picker.rs`, mapping each summary to a row (name = title falling back to session_id, description = relative time plus cwd, action = `ResumeAcpSession`). Cloud sessions carry the sentinel cwd `/` (the broker tracks no real working directory); the ACP picker treats that sentinel as "no cwd" -- it is neither displayed in the row description nor included in the search haystack. + +**Session Close (`/close`):** + +The `/close` command releases the current agent-side session over ACP `session/close` and, on success, starts a fresh chat. The handler in `@/nori-rs/tui/src/chatwidget/key_handling.rs` gates on `session_agent_capabilities.session_close` (an error cell explains the missing capability otherwise), then calls `AcpAgentHandle::close_session()` (the `CloseSession` `AcpAgentCommand`, handled by `AcpBackend::close_active_session()` in `@/nori-rs/harness`). Only after the close succeeds does it emit `AppEvent::NewSession`; a failure comes back as `AppEvent::SessionCloseFailed`, which renders an error cell and keeps the current session intact. While the close is in flight the widget sets `session_close_in_flight`, blocking session-switching commands (`/new`, `/resume`, `/resume-viewonly`, `/agent`, `/close`) so the deferred `NewSession` cannot clobber a conversation the user switched to mid-close. For the `nori cloud` agent this is the explicit "release my VM session" affordance -- the agent enforces a one-active-session contract, so closing is how a user frees the slot before resuming or creating another. `/close` is not available during a task (`available_during_task = false`). The transcript-backed flow involves three layers: diff --git a/nori-rs/tui/src/app/event_handling.rs b/nori-rs/tui/src/app/event_handling.rs index 6d1e8d354..f1fee9db8 100644 --- a/nori-rs/tui/src/app/event_handling.rs +++ b/nori-rs/tui/src/app/event_handling.rs @@ -106,6 +106,9 @@ impl App { } tui.frame_requester().schedule_frame(); } + AppEvent::SessionCloseFailed { message } => { + self.chat_widget.on_session_close_failed(message); + } AppEvent::InsertHistoryCell(cell) => { let cell: Arc = cell.into(); if let Some(Overlay::Transcript(t)) = &mut self.overlay { diff --git a/nori-rs/tui/src/app_event.rs b/nori-rs/tui/src/app_event.rs index 7afea0cfc..9a60d83f0 100644 --- a/nori-rs/tui/src/app_event.rs +++ b/nori-rs/tui/src/app_event.rs @@ -25,6 +25,11 @@ pub(crate) enum AppEvent { /// Start a new session. NewSession, + /// A `/close` failed: surface the error and unblock the widget. + SessionCloseFailed { + message: String, + }, + /// Request to exit the application gracefully. ExitRequest, diff --git a/nori-rs/tui/src/bottom_pane/chat_composer/tests/part2.rs b/nori-rs/tui/src/bottom_pane/chat_composer/tests/part2.rs index bccf68ecb..c7c791127 100644 --- a/nori-rs/tui/src/bottom_pane/chat_composer/tests/part2.rs +++ b/nori-rs/tui/src/bottom_pane/chat_composer/tests/part2.rs @@ -117,6 +117,37 @@ fn slash_popup_model_first_for_mo_ui() { insta::assert_snapshot!("slash_popup_mo", terminal.backend()); } +#[test] +fn slash_popup_close_visible_for_cl_ui() { + use ratatui::Terminal; + use ratatui::backend::TestBackend; + + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + + let mut composer = ChatComposer::new( + true, + sender, + false, + "Ask Nori to do anything".to_string(), + false, + ); + + // Type "/cl" humanlike so paste-burst doesn’t interfere. + type_chars_humanlike(&mut composer, &['/', 'c', 'l']); + + let mut terminal = match Terminal::new(TestBackend::new(60, 5)) { + Ok(t) => t, + Err(e) => panic!("Failed to create terminal: {e}"), + }; + terminal + .draw(|f| composer.render(f.area(), f.buffer_mut())) + .unwrap_or_else(|e| panic!("Failed to draw composer: {e}")); + + // Visual snapshot should show the slash popup offering /close. + insta::assert_snapshot!("slash_popup_cl", terminal.backend()); +} + #[test] fn slash_popup_model_first_for_mo_logic() { use crate::bottom_pane::command_popup::CommandItem; diff --git a/nori-rs/tui/src/bottom_pane/chat_composer/tests/snapshots/nori_tui__bottom_pane__chat_composer__tests__part2__slash_popup_cl.snap b/nori-rs/tui/src/bottom_pane/chat_composer/tests/snapshots/nori_tui__bottom_pane__chat_composer__tests__part2__slash_popup_cl.snap new file mode 100644 index 000000000..f149471de --- /dev/null +++ b/nori-rs/tui/src/bottom_pane/chat_composer/tests/snapshots/nori_tui__bottom_pane__chat_composer__tests__part2__slash_popup_cl.snap @@ -0,0 +1,9 @@ +--- +source: tui/src/bottom_pane/chat_composer/tests/part2.rs +expression: terminal.backend() +--- +" " +"/ cl " +" " +" /close close (release) the current session and " +" start a fresh chat " diff --git a/nori-rs/tui/src/chatwidget/constructors.rs b/nori-rs/tui/src/chatwidget/constructors.rs index 20d1f4fdc..e86807205 100644 --- a/nori-rs/tui/src/chatwidget/constructors.rs +++ b/nori-rs/tui/src/chatwidget/constructors.rs @@ -97,6 +97,7 @@ impl ChatWidget { expected_agent, session_configured_received: false, acp_handle: spawn_result.acp_handle, + session_close_in_flight: false, acp_config_option_snapshot: None, acp_mode_config: None, acp_mode_config_generation: super::session_config_mode::next_acp_mode_config_generation( @@ -224,6 +225,7 @@ impl ChatWidget { expected_agent, session_configured_received: false, acp_handle: spawn_result.acp_handle, + session_close_in_flight: false, acp_config_option_snapshot: None, acp_mode_config: None, acp_mode_config_generation: super::session_config_mode::next_acp_mode_config_generation( diff --git a/nori-rs/tui/src/chatwidget/helpers.rs b/nori-rs/tui/src/chatwidget/helpers.rs index 155efd57d..036b15e6a 100644 --- a/nori-rs/tui/src/chatwidget/helpers.rs +++ b/nori-rs/tui/src/chatwidget/helpers.rs @@ -48,6 +48,13 @@ impl ChatWidget { self.request_redraw(); } + /// A `/close` failed: surface the (already enhanced) error and unblock the + /// session-switching commands that were held while the close was in flight. + pub(crate) fn on_session_close_failed(&mut self, message: String) { + self.session_close_in_flight = false; + self.add_error_message(format!("Failed to close the session: {message}")); + } + pub(crate) fn handle_acp_session_config_update( &mut self, config_options: &[nori_harness::SessionConfigOption], diff --git a/nori-rs/tui/src/chatwidget/key_handling.rs b/nori-rs/tui/src/chatwidget/key_handling.rs index 4bd1bf56f..33bbe2d16 100644 --- a/nori-rs/tui/src/chatwidget/key_handling.rs +++ b/nori-rs/tui/src/chatwidget/key_handling.rs @@ -91,6 +91,24 @@ impl ChatWidget { self.request_redraw(); return; } + // While a /close awaits the agent, block session-switching commands: + // the deferred NewSession would otherwise clobber whatever session the + // user switched to in the meantime. + if self.session_close_in_flight + && matches!( + cmd, + SlashCommand::New + | SlashCommand::Resume + | SlashCommand::ResumeViewonly + | SlashCommand::Agent + | SlashCommand::Close + ) + { + let message = format!("'/{}' is disabled while the session closes.", cmd.command()); + self.add_to_history(history_cell::new_error_event(message)); + self.request_redraw(); + return; + } match cmd { SlashCommand::New => { self.app_event_tx.send(AppEvent::NewSession); @@ -101,6 +119,40 @@ impl ChatWidget { SlashCommand::ResumeViewonly => { self.open_viewonly_session_picker(); } + SlashCommand::Close => { + if !self.session_agent_capabilities.session_close { + self.add_error_message( + "The active agent does not support closing sessions \ + (/close needs the ACP session/close capability)." + .to_string(), + ); + return; + } + let Some(handle) = self.acp_handle.clone() else { + self.add_error_message("No live agent connection to close.".to_string()); + return; + }; + self.session_close_in_flight = true; + let tx = self.app_event_tx.clone(); + tokio::spawn(async move { + match handle.close_session().await { + Ok(()) => { + tx.send(AppEvent::InsertHistoryCell(Box::new( + history_cell::new_info_event( + "Session closed (released).".to_string(), + None, + ), + ))); + tx.send(AppEvent::NewSession); + } + Err(e) => { + tx.send(AppEvent::SessionCloseFailed { + message: format!("{e}"), + }); + } + } + }); + } SlashCommand::Init => { let init_target = self.config.cwd.join(DEFAULT_PROJECT_DOC_FILENAME); if init_target.exists() { diff --git a/nori-rs/tui/src/chatwidget/mod.rs b/nori-rs/tui/src/chatwidget/mod.rs index e3419ebc6..0bbbc7918 100644 --- a/nori-rs/tui/src/chatwidget/mod.rs +++ b/nori-rs/tui/src/chatwidget/mod.rs @@ -406,6 +406,10 @@ pub(crate) struct ChatWidget { session_configured_received: bool, // ACP agent handle for session config and model switching (only present in ACP mode) acp_handle: Option, + // True while /close awaits the agent's session/close response. Blocks + // session-switching commands so the deferred NewSession can't clobber a + // conversation the user switched to mid-close. + session_close_in_flight: bool, acp_config_option_snapshot: Option, acp_mode_config: Option, acp_mode_config_generation: i64, diff --git a/nori-rs/tui/src/chatwidget/pickers.rs b/nori-rs/tui/src/chatwidget/pickers.rs index 6fe5d7afc..90efe64a1 100644 --- a/nori-rs/tui/src/chatwidget/pickers.rs +++ b/nori-rs/tui/src/chatwidget/pickers.rs @@ -88,11 +88,14 @@ impl ChatWidget { pub(crate) fn open_resume_session_picker(&mut self) { // When the live agent advertises ACP `session/list`, source the picker // from the agent itself (and resume over ACP) instead of the local - // transcript store. Resuming a listed session loads it over ACP via - // `session/load`, so the agent must also advertise `load_session`; - // without it, fall back to the local-transcript picker. + // transcript store. Resuming a listed session goes over ACP via + // `session/load` (history replay) or `session/resume` (live reattach β€” + // the nori cloud contract, `loadSession: false`), so the agent must + // advertise at least one of the two; without either, fall back to the + // local-transcript picker. if self.session_agent_capabilities.session_list - && self.session_agent_capabilities.load_session + && (self.session_agent_capabilities.load_session + || self.session_agent_capabilities.session_resume) && let Some(handle) = self.acp_handle.clone() { let cwd = self.config.cwd.clone(); diff --git a/nori-rs/tui/src/chatwidget/tests/mod.rs b/nori-rs/tui/src/chatwidget/tests/mod.rs index fde5d044e..b5fce3e3b 100644 --- a/nori-rs/tui/src/chatwidget/tests/mod.rs +++ b/nori-rs/tui/src/chatwidget/tests/mod.rs @@ -306,6 +306,7 @@ pub(crate) fn make_chatwidget_manual() -> ( expected_agent: None, session_configured_received: false, acp_handle: None, + session_close_in_flight: false, acp_config_option_snapshot: None, acp_mode_config: None, acp_mode_config_generation: super::session_config_mode::next_acp_mode_config_generation(), diff --git a/nori-rs/tui/src/chatwidget/tests/part2.rs b/nori-rs/tui/src/chatwidget/tests/part2.rs index 491612a9e..92248d36d 100644 --- a/nori-rs/tui/src/chatwidget/tests/part2.rs +++ b/nori-rs/tui/src/chatwidget/tests/part2.rs @@ -93,6 +93,8 @@ fn goal_capabilities(goal_enabled: bool) -> nori_protocol::SessionCapabilitiesVi http_mcp: goal_enabled, load_session: true, session_list: false, + session_resume: false, + session_close: false, }, nori_client: nori_protocol::NoriClientCapabilitiesView { advertised: goal_enabled, diff --git a/nori-rs/tui/src/chatwidget/tests/part9.rs b/nori-rs/tui/src/chatwidget/tests/part9.rs index 5b260ddb0..0b06607ce 100644 --- a/nori-rs/tui/src/chatwidget/tests/part9.rs +++ b/nori-rs/tui/src/chatwidget/tests/part9.rs @@ -180,3 +180,262 @@ fn model_popup_pending_agent_snapshot() { let popup = render_bottom_popup(&chat, 80); assert_snapshot!("model_popup_pending_agent", popup); } + +/// Capabilities matching the nori cloud contract: `loadSession: false` with +/// `sessionCapabilities.{list,resume,close}` all advertised. +fn cloud_session_capabilities() -> nori_protocol::SessionCapabilitiesView { + nori_protocol::SessionCapabilitiesView { + agent: nori_protocol::AgentCapabilitiesView { + http_mcp: false, + load_session: false, + session_list: true, + session_resume: true, + session_close: true, + }, + nori_client: nori_protocol::NoriClientCapabilitiesView::default(), + builtin_commands: std::collections::HashMap::new(), + } +} + +/// The /resume picker must source rows from the agent's `session/list` when +/// the agent advertises list+resume WITHOUT `load_session` β€” the nori cloud +/// contract. Requiring `load_session` would silently fall back to the local +/// transcript picker, hiding every cloud session. +#[tokio::test] +async fn resume_picker_sources_from_agent_list_with_resume_capability() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(); + chat.handle_client_event(nori_protocol::ClientEvent::SessionCapabilitiesChanged( + cloud_session_capabilities(), + )); + + let (command_tx, mut command_rx) = + tokio::sync::mpsc::unbounded_channel::(); + tokio::spawn(async move { + while let Some(command) = command_rx.recv().await { + if let crate::chatwidget::agent::AcpAgentCommand::ListSessions { response_tx, .. } = + command + { + let _ = response_tx.send(Ok(vec![nori_harness::AcpSessionSummary { + session_id: "cloud-sess-1".to_string(), + cwd: std::path::PathBuf::from("/"), + title: Some("slack Β· claude".to_string()), + updated_at: None, + }])); + } + } + }); + chat.acp_handle = Some(crate::chatwidget::agent::AcpAgentHandle::from_command_tx( + command_tx, + )); + + chat.open_resume_session_picker(); + + let event = tokio::time::timeout(std::time::Duration::from_secs(2), rx.recv()) + .await + .expect("timed out waiting for the picker event") + .expect("channel closed"); + assert_matches!(event, AppEvent::ShowAcpResumeSessionPicker { sessions } => { + assert_eq!(sessions.len(), 1); + assert_eq!(sessions[0].session_id, "cloud-sess-1"); + }); +} + +/// Stub agent task that forwards each `CloseSession` response sender to the +/// test, so the test controls exactly when (and how) the close resolves. +fn stub_close_handle( + chat: &mut ChatWidget, +) -> tokio::sync::mpsc::UnboundedReceiver>> { + let (command_tx, mut command_rx) = + tokio::sync::mpsc::unbounded_channel::(); + let (pending_tx, pending_rx) = tokio::sync::mpsc::unbounded_channel(); + tokio::spawn(async move { + while let Some(command) = command_rx.recv().await { + if let crate::chatwidget::agent::AcpAgentCommand::CloseSession { response_tx } = command + { + let _ = pending_tx.send(response_tx); + } + } + }); + chat.acp_handle = Some(crate::chatwidget::agent::AcpAgentHandle::from_command_tx( + command_tx, + )); + pending_rx +} + +/// /close releases the live session over `session/close` and only then starts +/// a fresh chat β€” a fire-and-forget close (or one that ignores failure) must +/// not pass. +#[tokio::test] +async fn close_command_waits_for_close_before_starting_new_chat() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(); + chat.handle_client_event(nori_protocol::ClientEvent::SessionCapabilitiesChanged( + cloud_session_capabilities(), + )); + let mut pending_rx = stub_close_handle(&mut chat); + + chat.dispatch_command(SlashCommand::Close); + + let response_tx = tokio::time::timeout(std::time::Duration::from_secs(2), pending_rx.recv()) + .await + .expect("session/close must be requested on the agent handle") + .expect("stub agent task closed unexpectedly"); + + // While the close is still in flight, no new chat may start, and + // session-switching commands are blocked so the deferred NewSession + // can't clobber a conversation the user switches to. + while let Ok(event) = rx.try_recv() { + assert!( + !matches!(event, AppEvent::NewSession), + "NewSession must wait for the close response" + ); + } + chat.dispatch_command(SlashCommand::Resume); + let cells = drain_insert_history(&mut rx); + let rendered = lines_to_single_string(cells.last().expect("expected a blocked-command cell")); + assert!( + rendered.contains("disabled while the session closes"), + "session-switching must be blocked mid-close, got: {rendered}" + ); + + response_tx + .send(Ok(())) + .expect("widget dropped the close response receiver"); + + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(2); + loop { + let remaining = deadline.saturating_duration_since(std::time::Instant::now()); + let event = tokio::time::timeout(remaining, rx.recv()) + .await + .expect("timed out waiting for AppEvent::NewSession after /close") + .expect("channel closed"); + if matches!(event, AppEvent::NewSession) { + break; + } + } +} + +/// A failed /close reports the error and does NOT start a new chat β€” +/// otherwise /close is just /new with extra steps and the session leaks. +#[tokio::test] +async fn close_command_failure_reports_error_and_keeps_session() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(); + chat.handle_client_event(nori_protocol::ClientEvent::SessionCapabilitiesChanged( + cloud_session_capabilities(), + )); + let mut pending_rx = stub_close_handle(&mut chat); + + chat.dispatch_command(SlashCommand::Close); + + let response_tx = tokio::time::timeout(std::time::Duration::from_secs(2), pending_rx.recv()) + .await + .expect("session/close must be requested on the agent handle") + .expect("stub agent task closed unexpectedly"); + response_tx + .send(Err(anyhow::anyhow!("session not found"))) + .expect("widget dropped the close response receiver"); + + let event = tokio::time::timeout(std::time::Duration::from_secs(2), rx.recv()) + .await + .expect("timed out waiting for the close-failure event") + .expect("channel closed"); + let message = match event { + AppEvent::SessionCloseFailed { message } => message, + AppEvent::NewSession => panic!("a failed close must not start a new chat"), + other => panic!("unexpected event after failed close: {other:?}"), + }; + assert_matches!( + rx.try_recv(), + Err(tokio::sync::mpsc::error::TryRecvError::Empty) + ); + + // Route the failure back to the widget (as the app would): the error is + // surfaced and session-switching commands are unblocked again. + chat.on_session_close_failed(message); + let cells = drain_insert_history(&mut rx); + let rendered = lines_to_single_string(cells.last().expect("expected a close-failure cell")); + assert!( + rendered.contains("Failed to close the session"), + "expected a close-failure message, got: {rendered}" + ); + chat.dispatch_command(SlashCommand::Resume); + while let Ok(event) = rx.try_recv() { + if let AppEvent::InsertHistoryCell(cell) = event { + let rendered = lines_to_single_string(&cell.display_lines(80)); + assert!( + !rendered.contains("disabled while the session closes"), + "a failed close must unblock session-switching commands" + ); + } + } +} + +/// `session/list` alone (without `session/resume` or `load_session`) must NOT +/// route /resume to the agent picker β€” the rows would be unresumable. +#[tokio::test] +async fn resume_picker_ignores_agent_list_without_a_resume_path() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(); + let mut capabilities = cloud_session_capabilities(); + capabilities.agent.session_resume = false; + chat.handle_client_event(nori_protocol::ClientEvent::SessionCapabilitiesChanged( + capabilities, + )); + + let (command_tx, mut command_rx) = + tokio::sync::mpsc::unbounded_channel::(); + let (listed_tx, mut listed_rx) = tokio::sync::mpsc::unbounded_channel::<()>(); + tokio::spawn(async move { + while let Some(command) = command_rx.recv().await { + if let crate::chatwidget::agent::AcpAgentCommand::ListSessions { .. } = command { + let _ = listed_tx.send(()); + } + } + }); + chat.acp_handle = Some(crate::chatwidget::agent::AcpAgentHandle::from_command_tx( + command_tx, + )); + + chat.open_resume_session_picker(); + + // The local-transcript fallback path must run instead: it responds (via a + // history cell or the local picker event) without ever calling the agent. + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(2); + loop { + let remaining = deadline.saturating_duration_since(std::time::Instant::now()); + let event = tokio::time::timeout(remaining, rx.recv()) + .await + .expect("timed out waiting for the local fallback to respond") + .expect("channel closed"); + match event { + AppEvent::ShowAcpResumeSessionPicker { .. } => { + panic!("list without a resume path must not open the agent picker") + } + AppEvent::InsertHistoryCell(_) | AppEvent::ShowResumeSessionPicker { .. } => break, + _ => {} + } + } + assert_matches!( + listed_rx.try_recv(), + Err(tokio::sync::mpsc::error::TryRecvError::Empty), + "the agent's session/list must not be called without a resume path" + ); +} + +/// /close on an agent that does not advertise `session/close` must explain +/// itself instead of sending an unsupported request. +#[test] +fn close_command_is_gated_on_the_close_capability() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(); + + chat.dispatch_command(SlashCommand::Close); + + let cells = drain_insert_history(&mut rx); + assert!( + !cells.is_empty(), + "expected a user-visible message explaining /close is unavailable" + ); + let rendered = lines_to_single_string(cells.last().unwrap()); + assert!( + rendered.contains("does not support"), + "expected an unsupported-capability explanation, got: {rendered}" + ); +} diff --git a/nori-rs/tui/src/nori/resume_session_picker.rs b/nori-rs/tui/src/nori/resume_session_picker.rs index 7a0129e13..5ef468246 100644 --- a/nori-rs/tui/src/nori/resume_session_picker.rs +++ b/nori-rs/tui/src/nori/resume_session_picker.rs @@ -1,8 +1,10 @@ //! Resume session picker for /resume command. //! //! This module provides the UI for selecting a previous session to resume. -//! Selected sessions are resumed via the ACP `session/load` protocol method, -//! allowing the agent to restore its own context and stream conversation history. +//! Selected sessions are resumed over ACP via `session/load` (history replay) +//! or `session/resume` (live reattach, the nori cloud path), depending on +//! which capability the agent advertises; agents with neither fall back to a +//! fresh session plus client-side transcript replay. use std::path::Path; use std::path::PathBuf; @@ -113,20 +115,27 @@ pub fn acp_resume_session_picker_params(sessions: Vec) -> Sel let items: Vec = sessions .into_iter() .map(|session| { - let cwd_display = session.cwd.display().to_string(); + // Cloud sessions carry the sentinel cwd "/" β€” the broker tracks + // no real working directory β€” so treat it as "no cwd" rather + // than displaying or searching on it. + let cwd_display = Some(session.cwd.display().to_string()).filter(|cwd| cwd != "/"); let name = session .title .clone() .filter(|title| !title.is_empty()) .unwrap_or_else(|| session.session_id.clone()); - let description = match session.updated_at.as_deref() { - Some(updated_at) => Some(format!( - "{} Β· {cwd_display}", - format_relative_time(updated_at) - )), - None => Some(cwd_display.clone()), + let description = match (session.updated_at.as_deref(), cwd_display.as_deref()) { + (Some(updated_at), Some(cwd)) => { + Some(format!("{} Β· {cwd}", format_relative_time(updated_at))) + } + (Some(updated_at), None) => Some(format_relative_time(updated_at)), + (None, Some(cwd)) => Some(cwd.to_string()), + (None, None) => None, + }; + let search_value = match cwd_display.as_deref() { + Some(cwd) => format!("{} {name} {cwd}", session.session_id), + None => format!("{} {name}", session.session_id), }; - let search_value = format!("{} {name} {cwd_display}", session.session_id); let acp_session_id = session.session_id; let actions: Vec = vec![Box::new(move |tx: &AppEventSender| { @@ -369,6 +378,28 @@ mod tests { assert!(params.is_searchable); } + #[test] + fn acp_resume_picker_hides_cloud_cwd_sentinel() { + // Cloud sessions carry the sentinel cwd "/" β€” the broker tracks no + // real working directory β€” so the picker must neither display it nor + // include it in the search haystack. + let sessions = vec![AcpSessionSummary { + session_id: "cloud-sess-1".to_string(), + cwd: PathBuf::from("/"), + title: Some("slack Β· claude".to_string()), + updated_at: None, + }]; + + let params = acp_resume_session_picker_params(sessions); + + assert_eq!(params.items[0].name, "slack Β· claude"); + assert_eq!(params.items[0].description, None); + assert_eq!( + params.items[0].search_value.as_deref(), + Some("cloud-sess-1 slack Β· claude") + ); + } + #[test] fn acp_resume_picker_handles_empty_session_list() { let params = acp_resume_session_picker_params(vec![]); diff --git a/nori-rs/tui/src/slash_command.rs b/nori-rs/tui/src/slash_command.rs index 03017dbbe..06042ecc9 100644 --- a/nori-rs/tui/src/slash_command.rs +++ b/nori-rs/tui/src/slash_command.rs @@ -21,6 +21,7 @@ pub enum SlashCommand { New, Resume, ResumeViewonly, + Close, Init, Compact, Undo, @@ -48,6 +49,7 @@ impl SlashCommand { SlashCommand::New => "start a new chat during a conversation", SlashCommand::Resume => "resume a previous session", SlashCommand::ResumeViewonly => "view a previous session transcript (read-only)", + SlashCommand::Close => "close (release) the current session and start a fresh chat", SlashCommand::Init => "create an AGENTS.md file with instructions for Nori", SlashCommand::Compact => "summarize conversation to prevent hitting the context limit", SlashCommand::Undo => "ask Nori to undo a turn", @@ -97,6 +99,7 @@ impl SlashCommand { | SlashCommand::Logout | SlashCommand::SwitchSkillset | SlashCommand::Fork + | SlashCommand::Close | SlashCommand::Browser => false, SlashCommand::Browse | SlashCommand::Diff From 744c92a9e60b725730e9e472cb869d1a422db354 Mon Sep 17 00:00:00 2001 From: Clifford Ressel Date: Mon, 6 Jul 2026 19:12:55 -0400 Subject: [PATCH 2/2] chore(deps): cargo update -p crossbeam-epoch to 0.9.20 (RUSTSEC-2026-0204) New advisory published against crossbeam-epoch 0.9.18 (invalid pointer dereference in the fmt::Pointer/Display impls); lockfile-only patch bump per the advisory's recommended fix. cargo deny check advisories is green locally. --- nori-rs/Cargo.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nori-rs/Cargo.lock b/nori-rs/Cargo.lock index eecfb5b91..8ab223e91 100644 --- a/nori-rs/Cargo.lock +++ b/nori-rs/Cargo.lock @@ -1450,9 +1450,9 @@ dependencies = [ [[package]] name = "crossbeam-epoch" -version = "0.9.18" +version = "0.9.20" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5b82ac4a3c2ca9c3460964f020e1402edd5753411d7737aa39c3714ad1b5420e" +checksum = "2d6914041f254d6e9176c01941b21115dcfb7089e55135a35411081bd106ef3f" dependencies = [ "crossbeam-utils", ]