From 9d500365ed6b382be6081a7c2ec367536f59eb60 Mon Sep 17 00:00:00 2001 From: Amol Kapoor Date: Mon, 8 Jun 2026 12:35:14 -0400 Subject: [PATCH 01/10] =?UTF-8?q?wip:=20add=20APPLICATION=5FSPEC.md=20?= =?UTF-8?q?=F0=9F=A4=96=20Generated=20with=20[Nori](https://noriagentic.co?= =?UTF-8?q?m)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Nori --- APPLICATION_SPEC.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 APPLICATION_SPEC.md diff --git a/APPLICATION_SPEC.md b/APPLICATION_SPEC.md new file mode 100644 index 000000000..ddbbc92c3 --- /dev/null +++ b/APPLICATION_SPEC.md @@ -0,0 +1,15 @@ +PR https://github.com/tilework-tech/nori-sessions/pull/830 and PR https://github.com/tilework-tech/nori-cli/pull/486 together add support for the `nori cloud` command, a cli-local command that allows a user to connect to and spin up a nori sessions (~/code/nori/nori-sessions/**) session. + +We now want to improve the behavior of nori sessions out so that the product experience is more seamless. + +These changes will require a PR on the nori-sessions repo AND the nori-cli repo. + +1. When the user runs `nori cloud`, instead of automatically starting a new session, it should show a list of previous sessions and have an option to select a new one. If the user selects one of the previous sessions it should just resume that one on the remote machine. + +2. Many of the nori-cli options are exclusively for client side behavior. That includes: +- most of the things under /settings +- many of the slash commands +- many of the other options like the worktree stuff and the skillset switching stuff. +These should all be default disabled when in cloud mode. + +3. The user should be able to restart conversations that were originally started on slack or discord through the cli (nori cloud). From 4beb8a8237c12a60d29e97cdcc64f8fe8b0620a8 Mon Sep 17 00:00:00 2001 From: Amol Kapoor Date: Mon, 8 Jun 2026 13:08:34 -0400 Subject: [PATCH 02/10] feat(cli): add session selection to `nori cloud` command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When running `nori cloud` in an interactive terminal, the CLI now lists existing cloud sessions and lets the user pick one to resume or start a new one, instead of always creating a new session. Changes: - Add CloudSessionSummary type and list_sessions/resume_session methods to BrokerClient (calls GET /api/sessions and POST /api/sessions/{id}/resume) - Add new BrokerError variants: ListFailed, ResumeFailed - Create cli/src/cloud.rs with session picker UI (format, parse, prompt) - Modify cloud subcommand handler to show picker before TUI launch - Graceful fallback: 404 or errors from list_sessions → new session only - Non-interactive terminals skip picker (preserves scriptability) 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori --- CURRENT-PROGRESS.md | 29 ++++ RESEARCH-NOTES.md | 36 +++++ nori-rs/acp/docs.md | 2 +- nori-rs/acp/src/broker/docs.md | 20 ++- nori-rs/acp/src/broker/mod.rs | 89 ++++++++++ nori-rs/acp/src/broker/tests.rs | 278 ++++++++++++++++++++++++++++++++ nori-rs/cli/docs.md | 6 +- nori-rs/cli/src/cloud.rs | 155 ++++++++++++++++++ nori-rs/cli/src/lib.rs | 1 + nori-rs/cli/src/main.rs | 98 +++++++++-- 10 files changed, 694 insertions(+), 20 deletions(-) create mode 100644 CURRENT-PROGRESS.md create mode 100644 RESEARCH-NOTES.md create mode 100644 nori-rs/cli/src/cloud.rs diff --git a/CURRENT-PROGRESS.md b/CURRENT-PROGRESS.md new file mode 100644 index 000000000..b763ee842 --- /dev/null +++ b/CURRENT-PROGRESS.md @@ -0,0 +1,29 @@ +# Current Progress + +## Status: Item 1 implemented (CLI side) + +### Completed: Cloud Session Selection (Spec Item 1) + +**What was done:** +- Added `CloudSessionSummary` type and `BrokerError::ListFailed`/`ResumeFailed` variants to `acp/src/broker/mod.rs` +- Added `BrokerClient::list_sessions()` (GET /api/sessions) and `BrokerClient::resume_session()` (POST /api/sessions/{id}/resume) methods +- Created `cli/src/cloud.rs` with session selection UI: `format_cloud_session_list()`, `parse_session_choice()`, `prompt_session_selection()` +- Modified `cli/src/main.rs` cloud handler to list sessions, show picker, and route to resume or acquire +- Added 9 integration tests for broker methods and 8 unit tests for CLI selection logic +- Updated broker, CLI, and ACP docs + +**Design decisions:** +- Pre-TUI session picker (stdin/stderr) — TUI needs WebSocket URL before it can launch +- Graceful degradation on 404 (broker doesn't support listing yet) — falls back to new session +- Non-interactive terminals skip picker and use old behavior (auto-acquire) +- No new crate dependencies + +### Remaining: Items 2 and 3 + +**Item 2**: Disable client-side features in cloud mode (settings, slash commands, worktree, skillset switching). No feature gating currently exists in the TUI for cloud mode. Needs a new mechanism to conditionally hide/disable features. + +**Item 3**: Resume Slack/Discord sessions via CLI. This is naturally supported by item 1 — if the broker returns all sessions regardless of source, the `source` field on `CloudSessionSummary` shows where each session originated. The broker-side implementation needs to return these sessions. + +### Broker-side work needed (nori-sessions repo) +- `GET /api/sessions` endpoint returning `Vec` with sessions from all sources +- `POST /api/sessions/{id}/resume` endpoint returning `SessionInfo { session_id, ws_url }` diff --git a/RESEARCH-NOTES.md b/RESEARCH-NOTES.md new file mode 100644 index 000000000..ef55754eb --- /dev/null +++ b/RESEARCH-NOTES.md @@ -0,0 +1,36 @@ +# Research Notes + +## Cloud Session Architecture + +### Current State +- `nori cloud` authenticates with broker, acquires a NEW session every time, connects via WebSocket, launches TUI +- `BrokerClient` has only `acquire_session()` and `release_session()` — no list/resume +- Cloud sessions skip local transcript recording (broker records server-side) +- No feature gating in cloud mode — all TUI features available identically + +### Key Files +- `nori-rs/acp/src/broker/mod.rs` — BrokerClient, CloudConnectionInfo, SessionInfo, BrokerError +- `nori-rs/cli/src/main.rs:522-608` — Cloud subcommand handler +- `nori-rs/tui/src/nori/resume_session_picker.rs` — Local resume picker (reference for patterns) +- `nori-rs/acp/src/broker/tests.rs` — Broker client tests (integration tests with tiny_http mock servers) + +### API Flow +1. CLI resolves broker URL (flag > config.toml > interactive prompt) +2. CLI authenticates (OAuth browser flow → JWT persisted to cloud-auth.json) +3. CLI acquires session (POST /api/sessions/acquire → SessionInfo { session_id, ws_url }) +4. CLI launches TUI with CloudConnectionInfo { ws_url, auth_token } +5. On exit, CLI releases session (POST /api/sessions/{id}/release, best-effort 5s timeout) + +### Design Decision: Pre-TUI Session Picker +The session selection must happen BEFORE the TUI launches because the TUI needs a WebSocket URL. +Using simple stdin interaction (eprintln + read_line) — no new crate deps needed. +The local `/resume` picker uses in-TUI SelectionView, but that requires a running TUI. + +### New Broker API Endpoints Assumed +- `GET /api/sessions` → Vec (list user's sessions) +- `POST /api/sessions/{id}/resume` → SessionInfo { session_id, ws_url } (resume existing session) + +### New Types Needed +- `CloudSessionSummary` — session_id, source, created_at, last_active_at, first_message_preview, status +- `BrokerError::ListFailed` — for list endpoint failures +- `BrokerError::ResumeFailed` — for resume endpoint failures diff --git a/nori-rs/acp/docs.md b/nori-rs/acp/docs.md index 7a466fe59..03cd33bfd 100644 --- a/nori-rs/acp/docs.md +++ b/nori-rs/acp/docs.md @@ -34,7 +34,7 @@ Key files: - `registry.rs` - Agent configuration and npm package detection - `connection/` - SACP v11-based agent communication (local subprocess and remote WebSocket) -- `broker/` - Cloud session broker client: OAuth auth, JWT management, session acquisition/release, and `CloudConnectionInfo` (see `@/nori-rs/acp/src/broker/docs.md`) +- `broker/` - Cloud session broker client: OAuth auth, JWT management, session listing/acquisition/resume/release, and `CloudConnectionInfo` (see `@/nori-rs/acp/src/broker/docs.md`) - `translator.rs` - User input to ACP `ContentBlock` conversion and related parsing helpers - `backend/mod.rs` - Implements `ConversationClient` trait from codex-core and emits normalized ACP session events - `backend/thread_goal.rs` - Owns per-session `/goal` state, prompt goal-context formatting, transcript rehydration, and usage checkpoint updates diff --git a/nori-rs/acp/src/broker/docs.md b/nori-rs/acp/src/broker/docs.md index 285086ddf..911092ee6 100644 --- a/nori-rs/acp/src/broker/docs.md +++ b/nori-rs/acp/src/broker/docs.md @@ -5,7 +5,7 @@ Path: @/nori-rs/acp/src/broker ### Overview - Implements the client-side integration with the nori-sessions broker for cloud VM sessions -- Manages OAuth browser-based authentication, JWT credential persistence, session acquisition, and session release +- Manages OAuth browser-based authentication, JWT credential persistence, and the full session lifecycle: list, acquire, resume, and release - Defines `CloudConnectionInfo`, the value type whose presence signals cloud mode throughout the codebase ### How it fits into the larger codebase @@ -14,7 +14,13 @@ Path: @/nori-rs/acp/src/broker nori cloud (CLI) nori-sessions broker (external) | | v | -BrokerClient ----HTTP POST /api/sessions/acquire---->| +BrokerClient ----GET /api/sessions------------>| (list existing sessions) + | | + v | + |--- user picks session or new --------->| + | | + +----POST /api/sessions/acquire--------->| (new session) + +----POST /api/sessions/{id}/resume----->| (resume existing) | | v v CloudConnectionInfo { ws_url, auth_token } SessionInfo { session_id, ws_url } @@ -36,7 +42,8 @@ BrokerClient ----HTTP POST /api/sessions/{id}/release---->| (best-effort, 5s timeout, called from CLI after run_main) ``` -- The CLI (`@/nori-rs/cli/src/main.rs`) is the sole caller of `BrokerClient` -- it authenticates, acquires a session, constructs the `CloudConnectionInfo` that flows downstream, and releases the session after the TUI exits +- The CLI (`@/nori-rs/cli/src/main.rs`) is the sole caller of `BrokerClient` -- it authenticates, lists sessions, acquires or resumes a session, constructs the `CloudConnectionInfo` that flows downstream, and releases the session after the TUI exits +- The session selection UI (`@/nori-rs/cli/src/cloud.rs`) formats listed sessions and parses user input before the TUI launches; this is a pre-TUI step because the TUI needs a WebSocket URL to start - `CloudConnectionInfo` is threaded through the TUI layer (`Cli` -> `App` -> `ChatWidgetInit` -> `spawn_agent()` or `spawn_acp_agent_resume()`) without modification; the TUI does not interact with the broker directly - The ACP backend (`@/nori-rs/acp/src/backend/spawn_and_relay.rs`) branches on `config.cloud_connection` in `spawn()`: when present, it calls `SacpConnection::connect_remote()` instead of `SacpConnection::spawn()`. `resume_session()` does not have a cloud path -- cloud sessions are not recorded locally, so they never appear in the resume pickers - The WebSocket transport adapter (`@/nori-rs/acp/src/connection/ws_transport.rs`) is the component that actually opens the WebSocket connection using the `ws_url` and `auth_token` from `CloudConnectionInfo` @@ -45,10 +52,13 @@ BrokerClient ----HTTP POST /api/sessions/{id}/release---->| ### Core Implementation - `CloudConnectionInfo` is a plain struct with `ws_url: String` and `auth_token: String`. It is the branch condition throughout the system -- `Option` being `Some` means cloud mode, `None` means local subprocess mode +- `CloudSessionSummary` is the listing type returned by `list_sessions()`, carrying `session_id`, `source` (e.g. "cli", "slack", "discord"), `created_at`, `last_active_at`, optional `first_message_preview`, and `status`. The `source` field enables the CLI session picker to show where each session originated - `BrokerClient::new()` loads persisted credentials from `{nori_home}/cloud-auth.json` and filters them to match the current `broker_url`. If the stored credentials are for a different broker, they are ignored - `BrokerClient::has_valid_token()` checks whether the stored JWT is present and not expired, using `is_token_expired()` which decodes the base64url JWT payload and compares the `exp` claim against the current system time - `BrokerClient::authenticate()` runs an OAuth browser flow: it binds a local HTTP server on an ephemeral port, opens the broker's `/api/auth/cli?redirect_uri=...` URL in the default browser, waits up to 2 minutes for a callback with a `?token=` query parameter, and persists the credentials via `save_credentials()`. The 2-minute timeout prevents the CLI from hanging indefinitely if the user abandons the browser flow; on timeout, `server.unblock()` is called to shut down the callback listener thread cleanly +- `BrokerClient::list_sessions()` GETs `{broker_url}/api/sessions` with a Bearer token, returning `Vec`. This enables the CLI to show the user their existing sessions before deciding whether to create a new one or resume. HTTP 401 maps to `BrokerError::TokenExpired`; non-success responses map to `BrokerError::ListFailed` - `BrokerClient::acquire_session()` POSTs to `{broker_url}/api/sessions/acquire` with a Bearer token and a JSON body `{"source": "cli"}`, returning a `SessionInfo` containing `session_id` and `ws_url`. The `source` field identifies this as a CLI client so the broker uses the correct claim identity format. HTTP 401 responses map to `BrokerError::TokenExpired` +- `BrokerClient::resume_session()` POSTs to `{broker_url}/api/sessions/{session_id}/resume` with a Bearer token, returning a `SessionInfo` with the reconnection `ws_url`. This allows resuming sessions originally started from any source (CLI, Slack, Discord). HTTP 401 maps to `BrokerError::TokenExpired`; non-success responses map to `BrokerError::ResumeFailed` - `BrokerClient::release_session()` POSTs to `{broker_url}/api/sessions/{session_id}/release` with a Bearer token to explicitly release a cloud session. Called by the CLI as a best-effort cleanup after the TUI exits (wrapped in a 5-second timeout). HTTP 401 maps to `TokenExpired`; non-success responses map to `BrokerError::ReleaseFailed` - `CloudCredentials` is the serialized form persisted to `cloud-auth.json`; it pairs `broker_url` with `auth_token` so credentials for different brokers do not collide @@ -57,7 +67,9 @@ BrokerClient ----HTTP POST /api/sessions/{id}/release---->| - The authentication callback server runs in a separate `std::thread` (not a tokio task) because `tiny_http::Server` is synchronous. The server is wrapped in `Arc` so both the callback thread and the timeout handler can call `unblock()` to shut it down. Communication with the async caller uses a `tokio::sync::oneshot` channel, wrapped in a `tokio::time::timeout` for the 2-minute deadline - JWT expiry checking (`is_token_expired()`) is deliberately lenient: any token that cannot be decoded as a three-part base64url JWT with a valid `exp` claim is treated as expired. This avoids storing invalid tokens - The `auth_token()` accessor on `BrokerClient` is used by the CLI to extract the token for constructing `CloudConnectionInfo` after session acquisition -- the token flows to the WebSocket connection as a Bearer auth header -- `BrokerClient` now covers the full session lifecycle: authenticate -> acquire -> release. Release is the terminal step, called from the CLI layer (not the backend) since the broker client and session ID are scoped there +- `BrokerClient` covers the full session lifecycle: authenticate -> list -> acquire/resume -> release. The CLI drives this lifecycle; the TUI and backend never call the broker directly +- All broker API methods follow the same error pattern: check for token presence, check local JWT expiry, make the HTTP request, map 401 to `TokenExpired`, and map other non-success status codes to the method-specific `BrokerError` variant (`ListFailed`, `AcquireFailed`, `ResumeFailed`, `ReleaseFailed`) +- The CLI gracefully degrades when `list_sessions()` fails: a 404 (broker does not support listing) returns an empty list, other errors log a warning and fall back to creating a new session. Non-interactive terminals skip session listing entirely and go directly to `acquire_session()` - Cloud mode in `AcpBackend::spawn()` skips agent config lookup (`get_agent_config`) since the remote agent is already running on the cloud VM. Error messages for cloud connection failures use simple messages instead of the enhanced error categorization used for local subprocess failures. Cloud sessions also skip local transcript recording (`transcript_recorder` is left `None`) because the broker records transcripts server-side; as a result they are not persisted locally and do not surface in the local resume pickers - The CLI uses its local cwd for cloud sessions -- the broker's SACP proxy (in the nori-sessions repo) manages the sprite-side working directory independently via `AcpTunnelManager`. The `cwd` sent in the `session/new` RPC is discarded by the broker. This means client-side file handlers, transcript recording, and TUI display all reflect the user's local directory, not the sprite's workspace path diff --git a/nori-rs/acp/src/broker/mod.rs b/nori-rs/acp/src/broker/mod.rs index e5cf0d696..be06761e1 100644 --- a/nori-rs/acp/src/broker/mod.rs +++ b/nori-rs/acp/src/broker/mod.rs @@ -44,6 +44,16 @@ pub struct SessionInfo { pub ws_url: String, } +#[derive(Debug, Clone, Deserialize, PartialEq, Eq)] +pub struct CloudSessionSummary { + pub session_id: String, + pub source: String, + pub created_at: String, + pub last_active_at: String, + pub first_message_preview: Option, + pub status: String, +} + #[derive(Debug, thiserror::Error)] pub enum BrokerError { #[error("authentication required")] @@ -58,6 +68,12 @@ pub enum BrokerError { #[error("session release failed: HTTP {status}: {body}")] ReleaseFailed { status: u16, body: String }, + #[error("session list failed: HTTP {status}: {body}")] + ListFailed { status: u16, body: String }, + + #[error("session resume failed: HTTP {status}: {body}")] + ResumeFailed { status: u16, body: String }, + #[error("network error: {0}")] NetworkError(#[from] reqwest::Error), @@ -132,6 +148,79 @@ impl BrokerClient { Ok(info) } + pub async fn list_sessions( + &self, + ) -> std::result::Result, BrokerError> { + let token = self + .auth_token + .as_deref() + .ok_or(BrokerError::AuthRequired)?; + + if is_token_expired(token) { + return Err(BrokerError::TokenExpired); + } + + let url = format!("{}/api/sessions", self.broker_url); + let resp = self + .http + .get(&url) + .header("Authorization", format!("Bearer {token}")) + .send() + .await?; + + let status = resp.status().as_u16(); + if status == 401 { + return Err(BrokerError::TokenExpired); + } + if !resp.status().is_success() { + let body = resp.text().await.unwrap_or_default(); + return Err(BrokerError::ListFailed { status, body }); + } + + let sessions: Vec = resp + .json() + .await + .map_err(|e| BrokerError::InvalidToken(format!("invalid response: {e}")))?; + Ok(sessions) + } + + pub async fn resume_session( + &self, + session_id: &str, + ) -> std::result::Result { + let token = self + .auth_token + .as_deref() + .ok_or(BrokerError::AuthRequired)?; + + if is_token_expired(token) { + return Err(BrokerError::TokenExpired); + } + + let url = format!("{}/api/sessions/{session_id}/resume", self.broker_url); + let resp = self + .http + .post(&url) + .header("Authorization", format!("Bearer {token}")) + .send() + .await?; + + let status = resp.status().as_u16(); + if status == 401 { + return Err(BrokerError::TokenExpired); + } + if !resp.status().is_success() { + let body = resp.text().await.unwrap_or_default(); + return Err(BrokerError::ResumeFailed { status, body }); + } + + let info: SessionInfo = resp + .json() + .await + .map_err(|e| BrokerError::InvalidToken(format!("invalid response: {e}")))?; + Ok(info) + } + pub async fn release_session(&self, session_id: &str) -> std::result::Result<(), BrokerError> { let token = self .auth_token diff --git a/nori-rs/acp/src/broker/tests.rs b/nori-rs/acp/src/broker/tests.rs index 41876499f..56424cdd8 100644 --- a/nori-rs/acp/src/broker/tests.rs +++ b/nori-rs/acp/src/broker/tests.rs @@ -475,3 +475,281 @@ async fn release_session_errors_without_token() { let err = client.release_session("sess-abc123").await.unwrap_err(); assert!(matches!(err, BrokerError::AuthRequired)); } + +// ── list_sessions ───────────────────────────────────────────────── + +#[tokio::test] +async fn list_sessions_sends_get_with_auth_and_parses_response() { + let mock_server = tiny_http::Server::http("127.0.0.1:0").unwrap(); + let port = mock_server.server_addr().to_ip().unwrap().port(); + let broker_url = format!("http://127.0.0.1:{port}"); + let token = future_jwt(); + + let expected_token = token.clone(); + let server_handle = std::thread::spawn(move || { + let request = mock_server.recv().unwrap(); + assert_eq!(request.method(), &tiny_http::Method::Get); + assert_eq!(request.url(), "/api/sessions"); + + let auth_header = request + .headers() + .iter() + .find(|h| h.field.equiv("Authorization")) + .map(|h| h.value.to_string()); + assert_eq!(auth_header, Some(format!("Bearer {expected_token}"))); + + let response_body = serde_json::json!([ + { + "session_id": "sess-1", + "source": "cli", + "created_at": "2025-01-27T12:00:00Z", + "last_active_at": "2025-01-27T14:30:00Z", + "first_message_preview": "Fix the login bug", + "status": "idle" + }, + { + "session_id": "sess-2", + "source": "slack", + "created_at": "2025-01-26T10:00:00Z", + "last_active_at": "2025-01-26T11:00:00Z", + "first_message_preview": null, + "status": "idle" + } + ]); + let response = tiny_http::Response::from_string(response_body.to_string()).with_header( + "Content-Type: application/json" + .parse::() + .unwrap(), + ); + request.respond(response).unwrap(); + }); + + let dir = tempdir().unwrap(); + let creds = CloudCredentials { + broker_url: broker_url.clone(), + auth_token: token, + }; + save_credentials(dir.path(), &creds).unwrap(); + let client = BrokerClient::new(broker_url, dir.path().to_path_buf()); + + let sessions = client.list_sessions().await.unwrap(); + assert_eq!(sessions.len(), 2); + assert_eq!(sessions[0].session_id, "sess-1"); + assert_eq!(sessions[0].source, "cli"); + assert_eq!( + sessions[0].first_message_preview.as_deref(), + Some("Fix the login bug") + ); + assert_eq!(sessions[1].session_id, "sess-2"); + assert_eq!(sessions[1].source, "slack"); + assert!(sessions[1].first_message_preview.is_none()); + + server_handle.join().unwrap(); +} + +#[tokio::test] +async fn list_sessions_returns_token_expired_on_401() { + let mock_server = tiny_http::Server::http("127.0.0.1:0").unwrap(); + let port = mock_server.server_addr().to_ip().unwrap().port(); + let broker_url = format!("http://127.0.0.1:{port}"); + let token = future_jwt(); + + let server_handle = std::thread::spawn(move || { + let request = mock_server.recv().unwrap(); + let response = tiny_http::Response::from_string("Unauthorized").with_status_code(401); + request.respond(response).unwrap(); + }); + + let dir = tempdir().unwrap(); + let creds = CloudCredentials { + broker_url: broker_url.clone(), + auth_token: token, + }; + save_credentials(dir.path(), &creds).unwrap(); + let client = BrokerClient::new(broker_url, dir.path().to_path_buf()); + + let err = client.list_sessions().await.unwrap_err(); + assert!(matches!(err, BrokerError::TokenExpired)); + + server_handle.join().unwrap(); +} + +#[tokio::test] +async fn list_sessions_returns_error_on_server_error() { + let mock_server = tiny_http::Server::http("127.0.0.1:0").unwrap(); + let port = mock_server.server_addr().to_ip().unwrap().port(); + let broker_url = format!("http://127.0.0.1:{port}"); + let token = future_jwt(); + + let server_handle = std::thread::spawn(move || { + let request = mock_server.recv().unwrap(); + let response = + tiny_http::Response::from_string("Internal Server Error").with_status_code(500); + request.respond(response).unwrap(); + }); + + let dir = tempdir().unwrap(); + let creds = CloudCredentials { + broker_url: broker_url.clone(), + auth_token: token, + }; + save_credentials(dir.path(), &creds).unwrap(); + let client = BrokerClient::new(broker_url, dir.path().to_path_buf()); + + let err = client.list_sessions().await.unwrap_err(); + assert!(matches!(err, BrokerError::ListFailed { status: 500, .. })); + + server_handle.join().unwrap(); +} + +#[tokio::test] +async fn list_sessions_returns_empty_vec_when_no_sessions() { + let mock_server = tiny_http::Server::http("127.0.0.1:0").unwrap(); + let port = mock_server.server_addr().to_ip().unwrap().port(); + let broker_url = format!("http://127.0.0.1:{port}"); + let token = future_jwt(); + + let server_handle = std::thread::spawn(move || { + let request = mock_server.recv().unwrap(); + let response = tiny_http::Response::from_string("[]").with_header( + "Content-Type: application/json" + .parse::() + .unwrap(), + ); + request.respond(response).unwrap(); + }); + + let dir = tempdir().unwrap(); + let creds = CloudCredentials { + broker_url: broker_url.clone(), + auth_token: token, + }; + save_credentials(dir.path(), &creds).unwrap(); + let client = BrokerClient::new(broker_url, dir.path().to_path_buf()); + + let sessions = client.list_sessions().await.unwrap(); + assert!(sessions.is_empty()); + + server_handle.join().unwrap(); +} + +#[tokio::test] +async fn list_sessions_errors_without_token() { + let dir = tempdir().unwrap(); + let client = BrokerClient::new("http://unused.test".to_string(), dir.path().to_path_buf()); + + let err = client.list_sessions().await.unwrap_err(); + assert!(matches!(err, BrokerError::AuthRequired)); +} + +// ── resume_session ──────────────────────────────────────────────── + +#[tokio::test] +async fn resume_session_sends_post_with_auth_and_parses_response() { + let mock_server = tiny_http::Server::http("127.0.0.1:0").unwrap(); + let port = mock_server.server_addr().to_ip().unwrap().port(); + let broker_url = format!("http://127.0.0.1:{port}"); + let token = future_jwt(); + + let expected_token = token.clone(); + let server_handle = std::thread::spawn(move || { + let request = mock_server.recv().unwrap(); + assert_eq!(request.method(), &tiny_http::Method::Post); + assert!(request.url().contains("/api/sessions/sess-abc123/resume")); + + let auth_header = request + .headers() + .iter() + .find(|h| h.field.equiv("Authorization")) + .map(|h| h.value.to_string()); + assert_eq!(auth_header, Some(format!("Bearer {expected_token}"))); + + let response_body = serde_json::json!({ + "session_id": "sess-abc123", + "ws_url": "wss://broker.test/ws/sess-abc123" + }); + let response = tiny_http::Response::from_string(response_body.to_string()).with_header( + "Content-Type: application/json" + .parse::() + .unwrap(), + ); + request.respond(response).unwrap(); + }); + + let dir = tempdir().unwrap(); + let creds = CloudCredentials { + broker_url: broker_url.clone(), + auth_token: token, + }; + save_credentials(dir.path(), &creds).unwrap(); + let client = BrokerClient::new(broker_url, dir.path().to_path_buf()); + + let info = client.resume_session("sess-abc123").await.unwrap(); + assert_eq!(info.session_id, "sess-abc123"); + assert_eq!(info.ws_url, "wss://broker.test/ws/sess-abc123"); + + server_handle.join().unwrap(); +} + +#[tokio::test] +async fn resume_session_returns_token_expired_on_401() { + let mock_server = tiny_http::Server::http("127.0.0.1:0").unwrap(); + let port = mock_server.server_addr().to_ip().unwrap().port(); + let broker_url = format!("http://127.0.0.1:{port}"); + let token = future_jwt(); + + let server_handle = std::thread::spawn(move || { + let request = mock_server.recv().unwrap(); + let response = tiny_http::Response::from_string("Unauthorized").with_status_code(401); + request.respond(response).unwrap(); + }); + + let dir = tempdir().unwrap(); + let creds = CloudCredentials { + broker_url: broker_url.clone(), + auth_token: token, + }; + save_credentials(dir.path(), &creds).unwrap(); + let client = BrokerClient::new(broker_url, dir.path().to_path_buf()); + + let err = client.resume_session("sess-abc123").await.unwrap_err(); + assert!(matches!(err, BrokerError::TokenExpired)); + + server_handle.join().unwrap(); +} + +#[tokio::test] +async fn resume_session_returns_error_on_404() { + let mock_server = tiny_http::Server::http("127.0.0.1:0").unwrap(); + let port = mock_server.server_addr().to_ip().unwrap().port(); + let broker_url = format!("http://127.0.0.1:{port}"); + let token = future_jwt(); + + let server_handle = std::thread::spawn(move || { + let request = mock_server.recv().unwrap(); + let response = tiny_http::Response::from_string("Not Found").with_status_code(404); + request.respond(response).unwrap(); + }); + + let dir = tempdir().unwrap(); + let creds = CloudCredentials { + broker_url: broker_url.clone(), + auth_token: token, + }; + save_credentials(dir.path(), &creds).unwrap(); + let client = BrokerClient::new(broker_url, dir.path().to_path_buf()); + + let err = client.resume_session("sess-abc123").await.unwrap_err(); + assert!(matches!(err, BrokerError::ResumeFailed { status: 404, .. })); + + server_handle.join().unwrap(); +} + +#[tokio::test] +async fn resume_session_errors_without_token() { + let dir = tempdir().unwrap(); + let client = BrokerClient::new("http://unused.test".to_string(), dir.path().to_path_buf()); + + let err = client.resume_session("sess-abc123").await.unwrap_err(); + assert!(matches!(err, BrokerError::AuthRequired)); +} diff --git a/nori-rs/cli/docs.md b/nori-rs/cli/docs.md index 0e95f3931..3bd478e22 100644 --- a/nori-rs/cli/docs.md +++ b/nori-rs/cli/docs.md @@ -47,11 +47,13 @@ match subcommand { ``` **CloudCommand**: Runs a TUI session backed by a cloud VM via the nori-sessions broker: -- `nori cloud` - Connects to a cloud session using the broker URL from `config.toml`, or prompts interactively on first run +- `nori cloud` - Lists existing cloud sessions and lets the user pick one to resume, or start a new one - `nori cloud --broker-url https://broker.example.com` - Overrides the broker URL - TUI flags such as `--agent`, `--profile`, `--sandbox` can be passed after `cloud` - Broker URL resolution follows a three-step priority chain: `--broker-url` flag > `[cloud] broker_url` in `config.toml` > interactive stdin prompt. The interactive prompt only activates when stdin is a terminal (`std::io::IsTerminal`); non-interactive invocations without a configured URL receive an error with setup instructions. The prompt validates that the URL starts with `http://` or `https://` and persists the value to `config.toml` via `save_cloud_broker_url()` from `@/nori-rs/acp/src/config/loader.rs`, so subsequent runs use the saved URL automatically -- The dispatch flow: resolves broker URL (see above), creates `BrokerClient` (see `@/nori-rs/acp/src/broker/docs.md`), authenticates via browser OAuth if needed, acquires a session (with automatic re-authentication retry on `BrokerError::TokenExpired`), then sets `TuiCli.cloud_connection` and calls `nori_tui::run_main()`. The retry handles the edge case where `has_valid_token()` passes at startup but the broker rejects the token with HTTP 401 during `acquire_session()` -- the CLI re-triggers `broker.authenticate()` and retries once +- The dispatch flow: resolves broker URL, creates `BrokerClient` (see `@/nori-rs/acp/src/broker/docs.md`), authenticates via browser OAuth if needed, then enters session selection. In interactive terminals, the CLI lists existing sessions via `broker.list_sessions()`, formats them with `format_cloud_session_list()` from `@/nori-rs/cli/src/cloud.rs`, and prompts the user to select one or start new. The user enters a number to resume an existing session or "n" for new. Non-interactive terminals skip listing and go directly to `acquire_session()`. All broker calls include automatic re-authentication retry on `BrokerError::TokenExpired` +- Session selection is a pre-TUI step -- it happens before `nori_tui::run_main()` because the TUI needs a WebSocket URL to start. The session picker logic lives in `@/nori-rs/cli/src/cloud.rs`, which provides `format_cloud_session_list()`, `parse_session_choice()`, and `prompt_session_selection()` +- Graceful degradation: if `list_sessions()` returns 404 (broker does not support listing) or any other error, the CLI falls back to showing only the "new session" path. This ensures compatibility with older broker deployments - After `run_main()` returns, the CLI calls `broker.release_session()` with a 5-second timeout as best-effort cleanup. Release failures or timeouts are logged but do not affect the exit code - The `CloudConnectionInfo` flows through the TUI unchanged until it reaches `AcpBackend::spawn()`, which uses `SacpConnection::connect_remote()` instead of spawning a local subprocess diff --git a/nori-rs/cli/src/cloud.rs b/nori-rs/cli/src/cloud.rs new file mode 100644 index 000000000..0c0dc070d --- /dev/null +++ b/nori-rs/cli/src/cloud.rs @@ -0,0 +1,155 @@ +use nori_acp::broker::CloudSessionSummary; + +#[derive(Debug, PartialEq, Eq)] +pub enum SessionChoice { + New, + Resume(i32), +} + +pub fn format_cloud_session_list(sessions: &[CloudSessionSummary]) -> String { + let mut out = String::from("Cloud Sessions:\n"); + for (i, session) in sessions.iter().enumerate() { + let preview = session + .first_message_preview + .as_deref() + .unwrap_or("(no preview)"); + out.push_str(&format!( + " [{num}] ({source}) \"{preview}\"\n", + num = i + 1, + source = session.source, + )); + } + out.push_str(" [n] Start new session\n"); + out +} + +pub fn parse_session_choice(input: &str, session_count: i32) -> Result { + let trimmed = input.trim(); + if trimmed.is_empty() { + return Err("empty input".to_string()); + } + if trimmed.eq_ignore_ascii_case("n") { + return Ok(SessionChoice::New); + } + match trimmed.parse::() { + Ok(n) if n >= 1 && n <= session_count => Ok(SessionChoice::Resume(n - 1)), + Ok(n) => Err(format!("selection {n} out of range (1-{session_count})")), + Err(_) => Err(format!("invalid selection: {trimmed}")), + } +} + +pub fn prompt_session_selection(sessions: &[CloudSessionSummary]) -> anyhow::Result { + use std::io::BufRead; + use std::io::Write as IoWrite; + + eprint!("{}", format_cloud_session_list(sessions)); + eprint!("Select session (or 'n' for new): "); + std::io::stderr().flush()?; + + let mut line = String::new(); + std::io::stdin().lock().read_line(&mut line)?; + + parse_session_choice(&line, sessions.len() as i32) + .map_err(|e| anyhow::anyhow!("Invalid selection: {e}")) +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + fn make_session( + session_id: &str, + source: &str, + first_message_preview: Option<&str>, + status: &str, + ) -> CloudSessionSummary { + CloudSessionSummary { + session_id: session_id.to_string(), + source: source.to_string(), + created_at: "2025-01-27T12:00:00Z".to_string(), + last_active_at: "2025-01-27T14:30:00Z".to_string(), + first_message_preview: first_message_preview.map(String::from), + status: status.to_string(), + } + } + + #[test] + fn format_session_list_includes_numbered_entries_and_new_option() { + let sessions = vec![ + make_session("sess-1", "cli", Some("Fix the login bug"), "idle"), + make_session("sess-2", "slack", None, "idle"), + ]; + + let output = format_cloud_session_list(&sessions); + + assert!(output.contains("[1]"), "should have numbered entry [1]"); + assert!(output.contains("[2]"), "should have numbered entry [2]"); + assert!(output.contains("[n]"), "should have new session option"); + assert!( + output.contains("Fix the login bug"), + "should show first message preview" + ); + assert!(output.contains("cli"), "should show session source"); + assert!(output.contains("slack"), "should show session source"); + } + + #[test] + fn format_session_list_handles_empty_list() { + let output = format_cloud_session_list(&[]); + assert!( + output.contains("[n]"), + "should still show new session option" + ); + assert!( + !output.contains("[1]"), + "should not have numbered entries for empty list" + ); + } + + #[test] + fn format_session_list_handles_missing_preview() { + let sessions = vec![make_session("sess-1", "discord", None, "idle")]; + + let output = format_cloud_session_list(&sessions); + assert!(output.contains("[1]")); + assert!(output.contains("discord")); + assert!( + output.contains("(no preview)"), + "should show fallback text for missing preview" + ); + } + + #[test] + fn parse_choice_selects_valid_session() { + assert_eq!(parse_session_choice("1", 3), Ok(SessionChoice::Resume(0))); + assert_eq!(parse_session_choice("3", 3), Ok(SessionChoice::Resume(2))); + } + + #[test] + fn parse_choice_selects_new_session() { + assert_eq!(parse_session_choice("n", 3), Ok(SessionChoice::New)); + assert_eq!(parse_session_choice("N", 3), Ok(SessionChoice::New)); + assert_eq!(parse_session_choice(" n ", 3), Ok(SessionChoice::New)); + } + + #[test] + fn parse_choice_rejects_out_of_range() { + assert!(parse_session_choice("0", 3).is_err()); + assert!(parse_session_choice("4", 3).is_err()); + assert!(parse_session_choice("100", 3).is_err()); + } + + #[test] + fn parse_choice_rejects_invalid_input() { + assert!(parse_session_choice("abc", 3).is_err()); + assert!(parse_session_choice("", 3).is_err()); + assert!(parse_session_choice(" ", 3).is_err()); + } + + #[test] + fn parse_choice_works_with_zero_sessions() { + assert_eq!(parse_session_choice("n", 0), Ok(SessionChoice::New)); + assert!(parse_session_choice("1", 0).is_err()); + } +} diff --git a/nori-rs/cli/src/lib.rs b/nori-rs/cli/src/lib.rs index 01954a64f..0af0cb2c3 100644 --- a/nori-rs/cli/src/lib.rs +++ b/nori-rs/cli/src/lib.rs @@ -1,3 +1,4 @@ +pub mod cloud; pub mod debug_sandbox; mod exit_status; #[cfg(feature = "login")] diff --git a/nori-rs/cli/src/main.rs b/nori-rs/cli/src/main.rs index 6e5e802e6..4ca65f328 100644 --- a/nori-rs/cli/src/main.rs +++ b/nori-rs/cli/src/main.rs @@ -558,20 +558,92 @@ async fn cli_main(codex_linux_sandbox_exe: Option) -> anyhow::Result<() eprintln!("Authenticated."); } - eprintln!("Acquiring cloud session..."); - let session_info = match broker.acquire_session().await { - Ok(info) => info, - Err(nori_acp::broker::BrokerError::TokenExpired) => { - eprintln!("Token expired, re-authenticating..."); - broker.authenticate().await?; - broker.acquire_session().await.map_err(|e| { - anyhow::anyhow!( - "Failed to acquire cloud session after re-authentication: {e}" - ) - })? + let session_info = if std::io::IsTerminal::is_terminal(&std::io::stdin()) { + let sessions = match broker.list_sessions().await { + Ok(s) => s, + Err(nori_acp::broker::BrokerError::TokenExpired) => { + eprintln!("Token expired, re-authenticating..."); + broker.authenticate().await?; + match broker.list_sessions().await { + Ok(s) => s, + Err(e) => { + eprintln!( + "Could not load previous sessions: {e}. Starting new session." + ); + vec![] + } + } + } + Err(nori_acp::broker::BrokerError::ListFailed { status: 404, .. }) => { + vec![] + } + Err(e) => { + eprintln!("Could not load previous sessions: {e}. Starting new session."); + vec![] + } + }; + + let choice = if sessions.is_empty() { + nori_cli::cloud::SessionChoice::New + } else { + nori_cli::cloud::prompt_session_selection(&sessions)? + }; + + match choice { + nori_cli::cloud::SessionChoice::New => { + eprintln!("Acquiring new cloud session..."); + match broker.acquire_session().await { + Ok(info) => info, + Err(nori_acp::broker::BrokerError::TokenExpired) => { + eprintln!("Token expired, re-authenticating..."); + broker.authenticate().await?; + broker.acquire_session().await.map_err(|e| { + anyhow::anyhow!( + "Failed to acquire cloud session after re-authentication: {e}" + ) + })? + } + Err(e) => { + anyhow::bail!("Failed to acquire cloud session: {e}"); + } + } + } + nori_cli::cloud::SessionChoice::Resume(idx) => { + let session_id = &sessions[idx as usize].session_id; + eprintln!("Resuming session {session_id}..."); + match broker.resume_session(session_id).await { + Ok(info) => info, + Err(nori_acp::broker::BrokerError::TokenExpired) => { + eprintln!("Token expired, re-authenticating..."); + broker.authenticate().await?; + broker.resume_session(session_id).await.map_err(|e| { + anyhow::anyhow!( + "Failed to resume cloud session after re-authentication: {e}" + ) + })? + } + Err(e) => { + anyhow::bail!("Failed to resume cloud session: {e}"); + } + } + } } - Err(e) => { - anyhow::bail!("Failed to acquire cloud session: {e}"); + } else { + eprintln!("Acquiring cloud session..."); + match broker.acquire_session().await { + Ok(info) => info, + Err(nori_acp::broker::BrokerError::TokenExpired) => { + eprintln!("Token expired, re-authenticating..."); + broker.authenticate().await?; + broker.acquire_session().await.map_err(|e| { + anyhow::anyhow!( + "Failed to acquire cloud session after re-authentication: {e}" + ) + })? + } + Err(e) => { + anyhow::bail!("Failed to acquire cloud session: {e}"); + } } }; eprintln!("Connected to session {}", session_info.session_id); From 9a7925bfe01c7fcf80086f4bfe1ef8fa3e5a43d3 Mon Sep 17 00:00:00 2001 From: Amol Kapoor Date: Mon, 8 Jun 2026 15:12:56 -0400 Subject: [PATCH 03/10] feat(tui): disable client-only features in cloud mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the TUI runs in cloud mode (via `nori cloud`), client-side-only slash commands are now disabled. 11 commands that require local filesystem, config, or process access are greyed out in the command popup and blocked at dispatch: /settings, /init, /browse, /diff, /mention, /memory, /mcp, /browser, /switch-skillset, /resume, and /resume-viewonly. Uses the existing CommandAvailability/disabled_builtins infrastructure. Also skips auto-worktree setup and skillset-per-session deferred spawning in cloud mode. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori --- CURRENT-PROGRESS.md | 21 +++++- RESEARCH-NOTES.md | 41 +++++++++++ nori-rs/tui/docs.md | 16 +++- nori-rs/tui/src/app/mod.rs | 2 +- nori-rs/tui/src/chatwidget/cloud_mode.rs | 30 ++++++++ nori-rs/tui/src/chatwidget/constructors.rs | 4 + nori-rs/tui/src/chatwidget/goal.rs | 1 + nori-rs/tui/src/chatwidget/key_handling.rs | 8 ++ nori-rs/tui/src/chatwidget/mod.rs | 3 + nori-rs/tui/src/chatwidget/tests/mod.rs | 1 + nori-rs/tui/src/lib.rs | 77 ++++++++++--------- nori-rs/tui/src/slash_command.rs | 86 ++++++++++++++++++++++ 12 files changed, 250 insertions(+), 40 deletions(-) create mode 100644 nori-rs/tui/src/chatwidget/cloud_mode.rs diff --git a/CURRENT-PROGRESS.md b/CURRENT-PROGRESS.md index b763ee842..545009d99 100644 --- a/CURRENT-PROGRESS.md +++ b/CURRENT-PROGRESS.md @@ -18,9 +18,26 @@ - Non-interactive terminals skip picker and use old behavior (auto-acquire) - No new crate dependencies -### Remaining: Items 2 and 3 +### Completed: Cloud Mode Feature Gating (Spec Item 2) -**Item 2**: Disable client-side features in cloud mode (settings, slash commands, worktree, skillset switching). No feature gating currently exists in the TUI for cloud mode. Needs a new mechanism to conditionally hide/disable features. +**What was done:** +- Added `SlashCommand::available_in_cloud_mode()` method with exhaustive match (no wildcard) classifying each command +- 11 client-only commands disabled in cloud mode: `/settings`, `/init`, `/browse`, `/diff`, `/mention`, `/memory`, `/mcp`, `/browser`, `/switch-skillset`, `/resume`, `/resume-viewonly` +- Created `chatwidget/cloud_mode.rs` with `apply_cloud_mode_restrictions()` that uses existing `CommandAvailability`/`disabled_builtins` infrastructure +- Added `is_cloud_session: bool` field to `ChatWidget`, set from `cloud_connection.is_some()` at construction +- Belt-and-suspenders dispatch guard in `dispatch_command()` blocks cloud-disabled commands even via direct entry +- Auto-worktree setup skipped entirely when `cloud_connection.is_some()` in `lib.rs` +- Deferred skillset-per-session spawn disabled in cloud mode in `App::run()` +- Added 2 unit tests for command classification +- Updated TUI docs + +**Design decisions:** +- Reuses existing `CommandAvailability` infrastructure rather than a new mechanism +- Exhaustive match forces classification of new commands at compile time +- Cloud-only commands kept enabled: `/agent`, `/model`, `/config`, `/approvals`, `/goal`, `/new`, `/resume`, `/compact`, `/status`, `/quit`, etc. +- Backend capabilities are authoritative — if the remote backend sends `SessionCapabilitiesView`, it overwrites local cloud-mode settings + +### Remaining: Item 3 **Item 3**: Resume Slack/Discord sessions via CLI. This is naturally supported by item 1 — if the broker returns all sessions regardless of source, the `source` field on `CloudSessionSummary` shows where each session originated. The broker-side implementation needs to return these sessions. diff --git a/RESEARCH-NOTES.md b/RESEARCH-NOTES.md index ef55754eb..8e05f49e3 100644 --- a/RESEARCH-NOTES.md +++ b/RESEARCH-NOTES.md @@ -34,3 +34,44 @@ The local `/resume` picker uses in-TUI SelectionView, but that requires a runnin - `CloudSessionSummary` — session_id, source, created_at, last_active_at, first_message_preview, status - `BrokerError::ListFailed` — for list endpoint failures - `BrokerError::ResumeFailed` — for resume endpoint failures + +## Cloud Mode Feature Gating (Item 2) + +### Existing Infrastructure +The codebase already has a `CommandAvailability` / `disabled_builtins` system: +- `nori_protocol::CommandAvailability { enabled: bool, reason: Option }` — per-command gate with user-facing reason +- `ChatWidget::builtin_command_availability` (HashMap) — stores availability state +- `ChatWidget::ensure_builtin_command_enabled(cmd)` — checks map at dispatch time, shows error if disabled +- `BottomPane::set_builtin_command_disabled(cmd, reason)` — propagates to `ChatComposer` → `CommandPopup` for greyed-out rendering +- `CommandPopup::disabled_builtins` — prevents selection and dims disabled commands in the popup + +### Cloud Connection Propagation +- `CloudConnectionInfo` flows: CLI → `App::run` → `ChatWidgetInit` → `spawn_agent()` +- `App.cloud_connection: Option` stored on App struct +- `ChatWidget` does NOT currently store `cloud_connection` — it's consumed at construction time for agent spawning only +- Need to add `is_cloud_session: bool` field to `ChatWidget` for runtime checks + +### Slash Commands to Disable in Cloud Mode +Client-side only (need local filesystem/process/config access): +- `/settings` — local CLI config (theme, hotkeys, layout) +- `/init` — creates local AGENTS.md file +- `/browse` — opens local file manager +- `/diff` — runs local git diff +- `/mention` — references local files +- `/memory` — shows local instruction files +- `/mcp` — manages local MCP server connections +- `/browser` — launches local Chrome via CDP +- `/switch-skillset` — switches local skillsets + +Keep enabled in cloud mode (interact with backend or are UI-only): +- `/agent`, `/model`, `/config`, `/approvals` — remote session config +- `/goal`, `/compact`, `/status`, `/first-prompt` — session state +- `/new`, `/resume`, `/resume-viewonly` — session management +- `/undo`, `/fork` — conversation operations (sent to backend) +- `/quit`, `/exit` — exit TUI +- `/login`, `/logout` — authentication + +### Additional Gating Needed +1. **Auto-worktree setup** (`lib.rs:217-277`): skip when `cloud_connection.is_some()` +2. **Deferred spawn for skillset_per_session** (`App::run` lines 328-334): force `needs_deferred_spawn = false` in cloud mode +3. **Worktree warning** in App: suppress in cloud mode diff --git a/nori-rs/tui/docs.md b/nori-rs/tui/docs.md index ca597815b..586d85889 100644 --- a/nori-rs/tui/docs.md +++ b/nori-rs/tui/docs.md @@ -35,10 +35,11 @@ Entry point is `main.rs` which delegates to `run_app()` in `lib.rs`. The `run_ma `NoriConfig` is also the source of truth for ACP backend diagnostics. The chat widget passes the resolved ACP proxy configuration into `AcpBackendConfig` when spawning or resuming sessions, so enabling `[acp_proxy]` in config wraps every backend ACP subprocess in the wire logger without requiring the live backend to be reconfigured in place. -The auto-worktree startup flow first checks eligibility via `can_create_worktree()` (see `@/nori-rs/acp/docs.md`), then branches on the `AutoWorktree` enum: +The auto-worktree startup flow first checks eligibility via `can_create_worktree()` (see `@/nori-rs/acp/docs.md`), then branches on the `AutoWorktree` enum. In cloud mode (`cli.cloud_connection.is_some()`), the entire worktree flow is skipped -- `pending_worktree_ask` is set to `false` and `worktree_blocked_reason` is `None`. | State | Timing | Behavior | | ------------------------------------------------------ | -------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------- | +| Cloud mode | Before TUI init, in `run_main()` | Skips worktree setup entirely; cloud sessions operate against the remote sprite's filesystem | | Blocked (not a git repo, or already inside a worktree) | Before TUI init, in `run_main()` | Sets `worktree_blocked_reason`; a `WorktreeBlockedScreen` popup is shown after onboarding explaining why, then continues without a worktree | | `Automatic` (eligible) | Before TUI init, in `run_main()` | Calls `setup_auto_worktree()` immediately and overrides cwd | | `Ask` (eligible) | After TUI init, in `run_ratatui_app()` | Sets `pending_worktree_ask = true`, deferred to a TUI popup shown after onboarding but before `App::run()` | @@ -480,6 +481,17 @@ The fork context flows through `ChatWidgetInit.fork_context` -> `spawn_agent()` **Cloud connection threading:** `ChatWidgetInit.cloud_connection` carries an optional `CloudConnectionInfo` from the CLI's `nori cloud` subcommand. The value flows through `Cli` -> `App::run()` -> `ChatWidgetInit` -> `spawn_agent()` -> `spawn_acp_agent()` -> `AcpBackendConfig.cloud_connection`. When present, `spawn_agent()` bypasses the `get_agent_config()` check (the remote agent is already running) and routes directly to `spawn_acp_agent()`. The resume path is local-only: `new_resumed_acp()` ignores `cloud_connection`, because cloud sessions are not recorded locally and therefore never appear in the resume pickers. `spawn_deferred_agent()` passes cloud_connection from its caller. See `@/nori-rs/acp/src/broker/docs.md` for the broker client and `@/nori-rs/acp/docs.md` for the backend cloud-vs-local branching in `spawn()`. +**Cloud mode feature gating** (`chatwidget/cloud_mode.rs`, `slash_command.rs`, `chatwidget/key_handling.rs`): + +When running in cloud mode (`is_cloud_session = true` on `ChatWidget`, derived from `cloud_connection.is_some()` in `constructors.rs`), client-side-only slash commands are disabled because they rely on local filesystem, config, or process access that is meaningless in a remote session. The gating uses two layers: + +1. **Popup-level disabling** (`apply_cloud_mode_restrictions()` in `chatwidget/cloud_mode.rs`): Called during `ChatWidget::new()` construction. Iterates all `SlashCommand` variants, checks `available_in_cloud_mode()`, and for cloud-incompatible commands writes disabled entries into the `builtin_command_availability` HashMap and propagates the disabled state through `set_builtin_command_disabled` to the bottom pane. This reuses the same `CommandAvailability` infrastructure used by backend-driven capability gating (e.g., `/goal` availability from `SessionCapabilitiesChanged`). +2. **Dispatch-level guard** (`dispatch_command()` in `chatwidget/key_handling.rs`): A belt-and-suspenders check at the top of command dispatch. If `is_cloud_session && !cmd.available_in_cloud_mode()`, shows an error message and returns early. This catches direct typed command entry that bypasses the popup. + +The `available_in_cloud_mode()` method on `SlashCommand` uses an exhaustive match (no wildcard), so adding a new variant forces a compile-time decision about cloud compatibility. Commands like `/settings`, `/init`, `/browse`, `/diff`, `/mention`, `/memory`, `/mcp`, `/browser`, and `/switch-skillset` return `false`. Commands that interact with the remote session or are session-agnostic (e.g., `/agent`, `/config`, `/compact`, `/new`, `/status`, `/quit`) return `true`. + +Cloud mode also skips the auto-worktree setup entirely in `run_main()` (see auto-worktree section above) and disables skillset-per-session deferred spawning in `App::run()` (see skillset switching section below), since both features depend on local filesystem operations. + **Session context injection:** Both `spawn_acp_agent()` and `spawn_acp_agent_resume()` in `chatwidget/agent.rs` set `AcpBackendConfig.session_context` to the contents of `@/nori-rs/tui/session_context.md` (loaded at compile time via `include_str!`). The ACP backend only prepends that fallback `` block to the first user prompt when the active ACP connection lacks HTTP MCP support. MCP-capable agents instead receive the backend-owned `nori-client` server and discover Nori operating context through its resources and prompts (see `@/nori-rs/acp/docs.md` for the hook context injection mechanism). **Browser Session (`/browser`) (`chatwidget/key_handling.rs`, `app/event_handling.rs`, `app_event.rs`):** @@ -577,7 +589,7 @@ The `/switch-skillset` command integrates with the external `nori-skillsets` CLI The worktree context is detected by `handle_switch_skillset_command()`: if the cwd's parent directory is named `.worktrees`, the cwd is passed as `install_dir`. When `skillset_per_session` is enabled, the cwd is used as `install_dir` even when not in a worktree. This enables per-worktree or per-session skillset installation. -When `skillset_per_session` is enabled in `NoriConfig`, the skillset picker is automatically triggered at startup in `App::run()`, regardless of whether the session is in a worktree. The agent spawn is deferred (`ChatWidgetInit::deferred_spawn = true`) so that `nori-skillsets switch` can write `.claude/CLAUDE.md` to disk before the agent reads it. During the deferred period, a dummy channel is created in `constructors.rs` so the widget has a valid `op_tx`. The real agent spawns after the user picks a skillset (`SkillsetSwitchResult` triggers `spawn_deferred_agent()`). If the user dismisses the picker without selecting a skillset (Escape/Ctrl-C or choosing the "No Skillset" option), the `AppEvent::SkillsetPickerDismissed` event triggers `spawn_deferred_agent()` -- the agent starts without a skillset, behaving as if the feature were disabled. The `deferred_spawn` flag on `ChatWidgetInit` causes a dummy op channel to be created during construction; the real agent spawns after the user picks a skillset or dismisses the picker. +When `skillset_per_session` is enabled in `NoriConfig` and the session is not in cloud mode, the skillset picker is automatically triggered at startup in `App::run()`. The agent spawn is deferred (`ChatWidgetInit::deferred_spawn = true`) so that `nori-skillsets switch` can write `.claude/CLAUDE.md` to disk before the agent reads it. In cloud mode (`cloud_connection.is_some()`), deferred spawning is always disabled because skillset switching depends on local filesystem operations that are irrelevant for remote sessions. During the deferred period, a dummy channel is created in `constructors.rs` so the widget has a valid `op_tx`. The real agent spawns after the user picks a skillset (`SkillsetSwitchResult` triggers `spawn_deferred_agent()`). If the user dismisses the picker without selecting a skillset (Escape/Ctrl-C or choosing the "No Skillset" option), the `AppEvent::SkillsetPickerDismissed` event triggers `spawn_deferred_agent()` -- the agent starts without a skillset, behaving as if the feature were disabled. The `deferred_spawn` flag on `ChatWidgetInit` causes a dummy op channel to be created during construction; the real agent spawns after the user picks a skillset or dismisses the picker. When `skillset_per_session` is on and `auto_worktree` is `Off`, the picker subtitle changes from "Switching skillset in {dir}" to "Warning: skillset files will be added to {dir}" to warn that skillset files will be written directly to the current working directory (no worktree isolation). The `on_skillset_list_result()` method in `pickers.rs` loads `NoriConfig` to determine both the `show_no_skillset` flag (true when `skillset_per_session` is enabled) and the `auto_worktree_off` flag (true when per-session is on and `auto_worktree` is not enabled). diff --git a/nori-rs/tui/src/app/mod.rs b/nori-rs/tui/src/app/mod.rs index 7b6279a53..26114db7d 100644 --- a/nori-rs/tui/src/app/mod.rs +++ b/nori-rs/tui/src/app/mod.rs @@ -328,7 +328,7 @@ impl App { #[cfg(feature = "nori-config")] let needs_deferred_spawn = { let nori_cfg = nori_acp::config::NoriConfig::load().unwrap_or_default(); - nori_cfg.skillset_per_session + nori_cfg.skillset_per_session && cloud_connection.is_none() }; #[cfg(not(feature = "nori-config"))] let needs_deferred_spawn = false; diff --git a/nori-rs/tui/src/chatwidget/cloud_mode.rs b/nori-rs/tui/src/chatwidget/cloud_mode.rs new file mode 100644 index 000000000..74f091386 --- /dev/null +++ b/nori-rs/tui/src/chatwidget/cloud_mode.rs @@ -0,0 +1,30 @@ +use ratatui::text::Line; +use strum::IntoEnumIterator; + +use super::ChatWidget; +use crate::slash_command::SlashCommand; + +const CLOUD_MODE_DISABLED_REASON: &str = "Not available in cloud mode."; + +impl ChatWidget { + pub(super) fn apply_cloud_mode_restrictions(&mut self) { + if !self.is_cloud_session { + return; + } + for cmd in SlashCommand::iter() { + if !cmd.available_in_cloud_mode() { + self.builtin_command_availability.insert( + cmd.command().to_string(), + nori_protocol::CommandAvailability { + enabled: false, + reason: Some(CLOUD_MODE_DISABLED_REASON.to_string()), + }, + ); + self.bottom_pane.set_builtin_command_disabled( + cmd, + Some(Line::from(CLOUD_MODE_DISABLED_REASON)), + ); + } + } + } +} diff --git a/nori-rs/tui/src/chatwidget/constructors.rs b/nori-rs/tui/src/chatwidget/constructors.rs index 7246c382b..b89a3857f 100644 --- a/nori-rs/tui/src/chatwidget/constructors.rs +++ b/nori-rs/tui/src/chatwidget/constructors.rs @@ -19,6 +19,7 @@ impl ChatWidget { cloud_connection, fork_context, } = common; + let is_cloud_session = cloud_connection.is_some(); let mut rng = rand::rng(); let placeholder = PROMPT_MODE_PLACEHOLDERS [rng.random_range(0..PROMPT_MODE_PLACEHOLDERS.len())] @@ -126,8 +127,10 @@ impl ChatWidget { pinned_plan: None, terminal_title_animation_origin: std::time::Instant::now(), last_terminal_title: None, + is_cloud_session, }; + widget.apply_cloud_mode_restrictions(); widget.prefetch_rate_limits(); widget @@ -255,6 +258,7 @@ impl ChatWidget { pinned_plan: None, terminal_title_animation_origin: std::time::Instant::now(), last_terminal_title: None, + is_cloud_session: false, }; widget.prefetch_rate_limits(); diff --git a/nori-rs/tui/src/chatwidget/goal.rs b/nori-rs/tui/src/chatwidget/goal.rs index 64ca155c2..7587395fd 100644 --- a/nori-rs/tui/src/chatwidget/goal.rs +++ b/nori-rs/tui/src/chatwidget/goal.rs @@ -88,6 +88,7 @@ impl ChatWidget { self.bottom_pane .set_builtin_command_disabled(command, disabled_reason); } + self.apply_cloud_mode_restrictions(); self.request_redraw(); } diff --git a/nori-rs/tui/src/chatwidget/key_handling.rs b/nori-rs/tui/src/chatwidget/key_handling.rs index 2d89ec84c..fe70da2ee 100644 --- a/nori-rs/tui/src/chatwidget/key_handling.rs +++ b/nori-rs/tui/src/chatwidget/key_handling.rs @@ -82,6 +82,14 @@ impl ChatWidget { } pub(super) fn dispatch_command(&mut self, cmd: SlashCommand) { + if self.is_cloud_session && !cmd.available_in_cloud_mode() { + self.add_to_history(history_cell::new_error_event(format!( + "/{} is not available in cloud mode.", + cmd.command() + ))); + self.request_redraw(); + return; + } if !cmd.available_during_task() && self.bottom_pane.is_task_running() { let message = format!( "'/{}' is disabled while a task is in progress.", diff --git a/nori-rs/tui/src/chatwidget/mod.rs b/nori-rs/tui/src/chatwidget/mod.rs index cdbc01ee9..644744883 100644 --- a/nori-rs/tui/src/chatwidget/mod.rs +++ b/nori-rs/tui/src/chatwidget/mod.rs @@ -124,6 +124,7 @@ use self::agent::spawn_agent; mod session_header; mod approvals; +mod cloud_mode; mod constructors; mod event_handlers; mod goal; @@ -449,6 +450,8 @@ pub(crate) struct ChatWidget { terminal_title_animation_origin: Instant, // Terminal title state: cache to avoid redundant OSC writes. last_terminal_title: Option, + // Whether this session is connected to a remote cloud broker. + is_cloud_session: bool, } /// Information about a pending agent switch in ChatWidget. diff --git a/nori-rs/tui/src/chatwidget/tests/mod.rs b/nori-rs/tui/src/chatwidget/tests/mod.rs index d1d8f711d..056bedaa6 100644 --- a/nori-rs/tui/src/chatwidget/tests/mod.rs +++ b/nori-rs/tui/src/chatwidget/tests/mod.rs @@ -329,6 +329,7 @@ pub(crate) fn make_chatwidget_manual() -> ( pinned_plan: None, terminal_title_animation_origin: std::time::Instant::now(), last_terminal_title: None, + is_cloud_session: false, }; (widget, rx, op_rx) } diff --git a/nori-rs/tui/src/lib.rs b/nori-rs/tui/src/lib.rs index df010a299..389083c69 100644 --- a/nori-rs/tui/src/lib.rs +++ b/nori-rs/tui/src/lib.rs @@ -230,47 +230,54 @@ pub async fn run_main( #[cfg(feature = "nori-config")] let (pending_worktree_ask, worktree_blocked_reason) = { use nori_acp::config::AutoWorktree; - let auto_worktree = nori_config - .as_ref() - .map(|c| c.auto_worktree) - .unwrap_or_default(); - if !auto_worktree.is_enabled() { + if cli.cloud_connection.is_some() { (false, None) } else { - match cwd.clone().or_else(|| std::env::current_dir().ok()) { - Some(ref effective_cwd) => { - match nori_acp::auto_worktree::can_create_worktree(effective_cwd) { - Err(reason) => { - tracing::debug!("Worktree creation blocked: {reason}"); - (false, Some(reason.to_string())) - } - Ok(()) => match auto_worktree { - AutoWorktree::Automatic => { - match nori_acp::auto_worktree::setup_auto_worktree(effective_cwd) { - Ok(worktree_path) => { - tracing::info!( - "Auto-worktree created at {}", - worktree_path.display() - ); - cwd = Some(worktree_path); - } - Err(e) => { - tracing::warn!("Auto-worktree setup skipped: {e}"); + let auto_worktree = nori_config + .as_ref() + .map(|c| c.auto_worktree) + .unwrap_or_default(); + + if !auto_worktree.is_enabled() { + (false, None) + } else { + match cwd.clone().or_else(|| std::env::current_dir().ok()) { + Some(ref effective_cwd) => { + match nori_acp::auto_worktree::can_create_worktree(effective_cwd) { + Err(reason) => { + tracing::debug!("Worktree creation blocked: {reason}"); + (false, Some(reason.to_string())) + } + Ok(()) => match auto_worktree { + AutoWorktree::Automatic => { + match nori_acp::auto_worktree::setup_auto_worktree( + effective_cwd, + ) { + Ok(worktree_path) => { + tracing::info!( + "Auto-worktree created at {}", + worktree_path.display() + ); + cwd = Some(worktree_path); + } + Err(e) => { + tracing::warn!("Auto-worktree setup skipped: {e}"); + } } + (false, None) } - (false, None) - } - AutoWorktree::Ask => (true, None), - AutoWorktree::Off => (false, None), - }, + AutoWorktree::Ask => (true, None), + AutoWorktree::Off => (false, None), + }, + } + } + None => { + tracing::warn!( + "Auto-worktree setup skipped: could not determine working directory" + ); + (false, None) } - } - None => { - tracing::warn!( - "Auto-worktree setup skipped: could not determine working directory" - ); - (false, None) } } } diff --git a/nori-rs/tui/src/slash_command.rs b/nori-rs/tui/src/slash_command.rs index 03017dbbe..ce196daff 100644 --- a/nori-rs/tui/src/slash_command.rs +++ b/nori-rs/tui/src/slash_command.rs @@ -110,6 +110,40 @@ impl SlashCommand { } } + /// Whether this command is available in cloud mode (remote broker session). + /// Client-side-only commands that require local filesystem, config, or + /// process access return `false`. + pub fn available_in_cloud_mode(self) -> bool { + match self { + SlashCommand::Settings + | SlashCommand::Init + | SlashCommand::Browse + | SlashCommand::Diff + | SlashCommand::Mention + | SlashCommand::Memory + | SlashCommand::Mcp + | SlashCommand::Browser + | SlashCommand::SwitchSkillset + | SlashCommand::Resume + | SlashCommand::ResumeViewonly => false, + SlashCommand::Agent + | SlashCommand::Model + | SlashCommand::Config + | SlashCommand::Approvals + | SlashCommand::Goal + | SlashCommand::New + | SlashCommand::Compact + | SlashCommand::Undo + | SlashCommand::Status + | SlashCommand::FirstPrompt + | SlashCommand::Quit + | SlashCommand::Exit + | SlashCommand::Login + | SlashCommand::Logout + | SlashCommand::Fork => true, + } + } + fn is_visible(self) -> bool { match self { #[cfg(not(feature = "login"))] @@ -353,6 +387,58 @@ mod tests { ); } + #[test] + fn cloud_mode_disables_client_only_commands() { + let client_only = [ + SlashCommand::Settings, + SlashCommand::Init, + SlashCommand::Browse, + SlashCommand::Diff, + SlashCommand::Mention, + SlashCommand::Memory, + SlashCommand::Mcp, + SlashCommand::Browser, + SlashCommand::SwitchSkillset, + SlashCommand::Resume, + SlashCommand::ResumeViewonly, + ]; + for cmd in &client_only { + assert!( + !cmd.available_in_cloud_mode(), + "/{} should be disabled in cloud mode", + cmd.command() + ); + } + } + + #[test] + fn cloud_mode_keeps_session_commands_enabled() { + let cloud_enabled = [ + SlashCommand::Agent, + SlashCommand::Model, + SlashCommand::Config, + SlashCommand::Approvals, + SlashCommand::Goal, + SlashCommand::New, + SlashCommand::Compact, + SlashCommand::Undo, + SlashCommand::Status, + SlashCommand::FirstPrompt, + SlashCommand::Quit, + SlashCommand::Exit, + SlashCommand::Login, + SlashCommand::Logout, + SlashCommand::Fork, + ]; + for cmd in &cloud_enabled { + assert!( + cmd.available_in_cloud_mode(), + "/{} should remain enabled in cloud mode", + cmd.command() + ); + } + } + #[test] fn browse_parses_from_string() { let cmd: SlashCommand = "browse".parse().expect("/browse should parse from string"); From 8c184665234350f1e7411590c2d63e0a1f8a5446 Mon Sep 17 00:00:00 2001 From: Amol Kapoor Date: Mon, 8 Jun 2026 15:38:18 -0400 Subject: [PATCH 04/10] docs: fix cloud mode command list inconsistencies and update progress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix CURRENT-PROGRESS.md: remove /resume from cloud-enabled list (it's actually disabled), correct description of capability re-application - Fix RESEARCH-NOTES.md: move /resume and /resume-viewonly to disabled section with explanation (local transcripts don't exist in cloud) - Fix tui/docs.md: make cloud-disabled list less brittle, include /resume and /resume-viewonly - Mark all CLI-side spec items as complete 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori --- CURRENT-PROGRESS.md | 25 ++++++++++++++++++------- RESEARCH-NOTES.md | 5 ++++- nori-rs/tui/docs.md | 2 +- 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/CURRENT-PROGRESS.md b/CURRENT-PROGRESS.md index 545009d99..e99fbe976 100644 --- a/CURRENT-PROGRESS.md +++ b/CURRENT-PROGRESS.md @@ -1,6 +1,6 @@ # Current Progress -## Status: Item 1 implemented (CLI side) +## Status: All CLI-side items complete ### Completed: Cloud Session Selection (Spec Item 1) @@ -34,13 +34,24 @@ **Design decisions:** - Reuses existing `CommandAvailability` infrastructure rather than a new mechanism - Exhaustive match forces classification of new commands at compile time -- Cloud-only commands kept enabled: `/agent`, `/model`, `/config`, `/approvals`, `/goal`, `/new`, `/resume`, `/compact`, `/status`, `/quit`, etc. -- Backend capabilities are authoritative — if the remote backend sends `SessionCapabilitiesView`, it overwrites local cloud-mode settings +- Cloud-enabled commands: `/agent`, `/model`, `/config`, `/approvals`, `/goal`, `/new`, `/compact`, `/undo`, `/status`, `/first-prompt`, `/quit`, `/exit`, `/login`, `/logout`, `/fork` +- Cloud restrictions re-applied after `SessionCapabilitiesChanged` events to ensure local-only commands stay disabled even if the backend sends new capabilities -### Remaining: Item 3 +### Completed: Slack/Discord Session Resumption (Spec Item 3) -**Item 3**: Resume Slack/Discord sessions via CLI. This is naturally supported by item 1 — if the broker returns all sessions regardless of source, the `source` field on `CloudSessionSummary` shows where each session originated. The broker-side implementation needs to return these sessions. +**What was done:** +- Naturally supported by item 1 — the session picker displays all sessions from the broker, with a `source` field (cli, slack, discord, etc.) shown next to each entry +- `CloudSessionSummary.source` field differentiates session origins +- No additional CLI code needed; the `format_cloud_session_list()` function already renders `(source)` per session + +**Design decisions:** +- No CLI-side filtering by source — all sessions are shown regardless of origin +- The broker is responsible for returning sessions from all sources -### Broker-side work needed (nori-sessions repo) -- `GET /api/sessions` endpoint returning `Vec` with sessions from all sources +### Broker-side work needed (nori-sessions repo — separate PR) +- `GET /api/sessions` endpoint returning `Vec` with sessions from all sources (cli, slack, discord) - `POST /api/sessions/{id}/resume` endpoint returning `SessionInfo { session_id, ws_url }` + +## Status: All CLI-side items complete + +All three spec items are implemented on the CLI side. The broker-side work (nori-sessions repo) is tracked separately. diff --git a/RESEARCH-NOTES.md b/RESEARCH-NOTES.md index 8e05f49e3..ada5a9c16 100644 --- a/RESEARCH-NOTES.md +++ b/RESEARCH-NOTES.md @@ -66,11 +66,14 @@ Client-side only (need local filesystem/process/config access): Keep enabled in cloud mode (interact with backend or are UI-only): - `/agent`, `/model`, `/config`, `/approvals` — remote session config - `/goal`, `/compact`, `/status`, `/first-prompt` — session state -- `/new`, `/resume`, `/resume-viewonly` — session management +- `/new` — start new session - `/undo`, `/fork` — conversation operations (sent to backend) - `/quit`, `/exit` — exit TUI - `/login`, `/logout` — authentication +Disabled in cloud mode (added during implementation): +- `/resume`, `/resume-viewonly` — these pick from local transcripts which don't exist for cloud sessions + ### Additional Gating Needed 1. **Auto-worktree setup** (`lib.rs:217-277`): skip when `cloud_connection.is_some()` 2. **Deferred spawn for skillset_per_session** (`App::run` lines 328-334): force `needs_deferred_spawn = false` in cloud mode diff --git a/nori-rs/tui/docs.md b/nori-rs/tui/docs.md index 586d85889..cb68597d4 100644 --- a/nori-rs/tui/docs.md +++ b/nori-rs/tui/docs.md @@ -488,7 +488,7 @@ When running in cloud mode (`is_cloud_session = true` on `ChatWidget`, derived f 1. **Popup-level disabling** (`apply_cloud_mode_restrictions()` in `chatwidget/cloud_mode.rs`): Called during `ChatWidget::new()` construction. Iterates all `SlashCommand` variants, checks `available_in_cloud_mode()`, and for cloud-incompatible commands writes disabled entries into the `builtin_command_availability` HashMap and propagates the disabled state through `set_builtin_command_disabled` to the bottom pane. This reuses the same `CommandAvailability` infrastructure used by backend-driven capability gating (e.g., `/goal` availability from `SessionCapabilitiesChanged`). 2. **Dispatch-level guard** (`dispatch_command()` in `chatwidget/key_handling.rs`): A belt-and-suspenders check at the top of command dispatch. If `is_cloud_session && !cmd.available_in_cloud_mode()`, shows an error message and returns early. This catches direct typed command entry that bypasses the popup. -The `available_in_cloud_mode()` method on `SlashCommand` uses an exhaustive match (no wildcard), so adding a new variant forces a compile-time decision about cloud compatibility. Commands like `/settings`, `/init`, `/browse`, `/diff`, `/mention`, `/memory`, `/mcp`, `/browser`, and `/switch-skillset` return `false`. Commands that interact with the remote session or are session-agnostic (e.g., `/agent`, `/config`, `/compact`, `/new`, `/status`, `/quit`) return `true`. +The `available_in_cloud_mode()` method on `SlashCommand` uses an exhaustive match (no wildcard), so adding a new variant forces a compile-time decision about cloud compatibility. Commands that depend on the local filesystem or local process state (e.g., `/settings`, `/browse`, `/mcp`, `/resume`, `/resume-viewonly`) return `false`. Commands that interact with the remote session or are session-agnostic (e.g., `/agent`, `/config`, `/compact`, `/new`, `/status`, `/quit`) return `true`. Cloud mode also skips the auto-worktree setup entirely in `run_main()` (see auto-worktree section above) and disables skillset-per-session deferred spawning in `App::run()` (see skillset switching section below), since both features depend on local filesystem operations. From e471e46c3c741695dfd5b62a533dcd049a67f4ec Mon Sep 17 00:00:00 2001 From: Amol Kapoor Date: Mon, 8 Jun 2026 16:30:53 -0400 Subject: [PATCH 05/10] test: improve cloud feature test quality MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use whole-object assertions in broker happy-path tests per project convention, add completeness check for cloud mode classification, and add negative number edge case for session choice parsing. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori --- nori-rs/acp/src/broker/tests.rs | 36 +++++++++++++++++++++++--------- nori-rs/cli/src/cloud.rs | 6 ++++++ nori-rs/tui/src/slash_command.rs | 15 +++++++++++++ 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/nori-rs/acp/src/broker/tests.rs b/nori-rs/acp/src/broker/tests.rs index 56424cdd8..4adc33a64 100644 --- a/nori-rs/acp/src/broker/tests.rs +++ b/nori-rs/acp/src/broker/tests.rs @@ -533,16 +533,27 @@ async fn list_sessions_sends_get_with_auth_and_parses_response() { let client = BrokerClient::new(broker_url, dir.path().to_path_buf()); let sessions = client.list_sessions().await.unwrap(); - assert_eq!(sessions.len(), 2); - assert_eq!(sessions[0].session_id, "sess-1"); - assert_eq!(sessions[0].source, "cli"); assert_eq!( - sessions[0].first_message_preview.as_deref(), - Some("Fix the login bug") + sessions, + vec![ + CloudSessionSummary { + session_id: "sess-1".to_string(), + source: "cli".to_string(), + created_at: "2025-01-27T12:00:00Z".to_string(), + last_active_at: "2025-01-27T14:30:00Z".to_string(), + first_message_preview: Some("Fix the login bug".to_string()), + status: "idle".to_string(), + }, + CloudSessionSummary { + session_id: "sess-2".to_string(), + source: "slack".to_string(), + created_at: "2025-01-26T10:00:00Z".to_string(), + last_active_at: "2025-01-26T11:00:00Z".to_string(), + first_message_preview: None, + status: "idle".to_string(), + }, + ] ); - assert_eq!(sessions[1].session_id, "sess-2"); - assert_eq!(sessions[1].source, "slack"); - assert!(sessions[1].first_message_preview.is_none()); server_handle.join().unwrap(); } @@ -685,8 +696,13 @@ async fn resume_session_sends_post_with_auth_and_parses_response() { let client = BrokerClient::new(broker_url, dir.path().to_path_buf()); let info = client.resume_session("sess-abc123").await.unwrap(); - assert_eq!(info.session_id, "sess-abc123"); - assert_eq!(info.ws_url, "wss://broker.test/ws/sess-abc123"); + assert_eq!( + info, + SessionInfo { + session_id: "sess-abc123".to_string(), + ws_url: "wss://broker.test/ws/sess-abc123".to_string(), + } + ); server_handle.join().unwrap(); } diff --git a/nori-rs/cli/src/cloud.rs b/nori-rs/cli/src/cloud.rs index 0c0dc070d..e09d56710 100644 --- a/nori-rs/cli/src/cloud.rs +++ b/nori-rs/cli/src/cloud.rs @@ -147,6 +147,12 @@ mod tests { assert!(parse_session_choice(" ", 3).is_err()); } + #[test] + fn parse_choice_rejects_negative_numbers() { + assert!(parse_session_choice("-1", 3).is_err()); + assert!(parse_session_choice("-100", 3).is_err()); + } + #[test] fn parse_choice_works_with_zero_sessions() { assert_eq!(parse_session_choice("n", 0), Ok(SessionChoice::New)); diff --git a/nori-rs/tui/src/slash_command.rs b/nori-rs/tui/src/slash_command.rs index ce196daff..319f76548 100644 --- a/nori-rs/tui/src/slash_command.rs +++ b/nori-rs/tui/src/slash_command.rs @@ -439,6 +439,21 @@ mod tests { } } + #[test] + fn cloud_mode_tests_cover_all_variants() { + let client_only: Vec = SlashCommand::iter() + .filter(|cmd| !cmd.available_in_cloud_mode()) + .collect(); + let cloud_enabled: Vec = SlashCommand::iter() + .filter(|cmd| cmd.available_in_cloud_mode()) + .collect(); + assert_eq!( + client_only.len() + cloud_enabled.len(), + SlashCommand::iter().count(), + "every SlashCommand variant must be classified as either cloud-disabled or cloud-enabled" + ); + } + #[test] fn browse_parses_from_string() { let cmd: SlashCommand = "browse".parse().expect("/browse should parse from string"); From 25cd4af6662d29b76fa93e9bbb29625fd961852b Mon Sep 17 00:00:00 2001 From: Amol Kapoor Date: Mon, 8 Jun 2026 17:53:11 -0400 Subject: [PATCH 06/10] =?UTF-8?q?fix:=20resolve=20clippy=20needless=5Fcoll?= =?UTF-8?q?ect=20warning=20in=20cloud=20mode=20test=20=F0=9F=A4=96=20Gener?= =?UTF-8?q?ated=20with=20[Nori](https://noriagentic.com)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Nori --- nori-rs/tui/src/slash_command.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/nori-rs/tui/src/slash_command.rs b/nori-rs/tui/src/slash_command.rs index 319f76548..ee66bfae4 100644 --- a/nori-rs/tui/src/slash_command.rs +++ b/nori-rs/tui/src/slash_command.rs @@ -441,14 +441,14 @@ mod tests { #[test] fn cloud_mode_tests_cover_all_variants() { - let client_only: Vec = SlashCommand::iter() + let client_only = SlashCommand::iter() .filter(|cmd| !cmd.available_in_cloud_mode()) - .collect(); - let cloud_enabled: Vec = SlashCommand::iter() + .count(); + let cloud_enabled = SlashCommand::iter() .filter(|cmd| cmd.available_in_cloud_mode()) - .collect(); + .count(); assert_eq!( - client_only.len() + cloud_enabled.len(), + client_only + cloud_enabled, SlashCommand::iter().count(), "every SlashCommand variant must be classified as either cloud-disabled or cloud-enabled" ); From 32902ef2cbb9612f31aeced5ad5d7da13959422b Mon Sep 17 00:00:00 2001 From: Amol Kapoor Date: Mon, 8 Jun 2026 17:59:48 -0400 Subject: [PATCH 07/10] fix: use InvalidResponse variant for JSON parse errors, remove tautological test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add BrokerError::InvalidResponse for malformed broker response bodies (previously misused InvalidToken which confused error semantics) - Remove cloud_mode_tests_cover_all_variants which was a tautology that could never fail (exhaustive match already enforces coverage at compile time) 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori --- nori-rs/acp/src/broker/mod.rs | 9 ++++++--- nori-rs/acp/src/broker/tests.rs | 2 +- nori-rs/tui/src/slash_command.rs | 14 -------------- 3 files changed, 7 insertions(+), 18 deletions(-) diff --git a/nori-rs/acp/src/broker/mod.rs b/nori-rs/acp/src/broker/mod.rs index 846f1d93d..69541f8e9 100644 --- a/nori-rs/acp/src/broker/mod.rs +++ b/nori-rs/acp/src/broker/mod.rs @@ -80,6 +80,9 @@ pub enum BrokerError { #[error("invalid token: {0}")] InvalidToken(String), + + #[error("invalid response: {0}")] + InvalidResponse(String), } pub struct BrokerClient { @@ -145,7 +148,7 @@ impl BrokerClient { let info: SessionInfo = resp .json() .await - .map_err(|e| BrokerError::InvalidToken(format!("invalid response: {e}")))?; + .map_err(|e| BrokerError::InvalidResponse(e.to_string()))?; Ok(info) } @@ -181,7 +184,7 @@ impl BrokerClient { let sessions: Vec = resp .json() .await - .map_err(|e| BrokerError::InvalidToken(format!("invalid response: {e}")))?; + .map_err(|e| BrokerError::InvalidResponse(e.to_string()))?; Ok(sessions) } @@ -218,7 +221,7 @@ impl BrokerClient { let info: SessionInfo = resp .json() .await - .map_err(|e| BrokerError::InvalidToken(format!("invalid response: {e}")))?; + .map_err(|e| BrokerError::InvalidResponse(e.to_string()))?; Ok(info) } diff --git a/nori-rs/acp/src/broker/tests.rs b/nori-rs/acp/src/broker/tests.rs index e8177f900..88a485eb5 100644 --- a/nori-rs/acp/src/broker/tests.rs +++ b/nori-rs/acp/src/broker/tests.rs @@ -470,7 +470,7 @@ async fn acquire_session_returns_error_for_malformed_response() { let client = BrokerClient::new(broker_url, dir.path().to_path_buf()); let err = client.acquire_session().await.unwrap_err(); - assert!(matches!(err, BrokerError::InvalidToken(_))); + assert!(matches!(err, BrokerError::InvalidResponse(_))); server_handle.join().unwrap(); } diff --git a/nori-rs/tui/src/slash_command.rs b/nori-rs/tui/src/slash_command.rs index ee66bfae4..b3d152559 100644 --- a/nori-rs/tui/src/slash_command.rs +++ b/nori-rs/tui/src/slash_command.rs @@ -439,20 +439,6 @@ mod tests { } } - #[test] - fn cloud_mode_tests_cover_all_variants() { - let client_only = SlashCommand::iter() - .filter(|cmd| !cmd.available_in_cloud_mode()) - .count(); - let cloud_enabled = SlashCommand::iter() - .filter(|cmd| cmd.available_in_cloud_mode()) - .count(); - assert_eq!( - client_only + cloud_enabled, - SlashCommand::iter().count(), - "every SlashCommand variant must be classified as either cloud-disabled or cloud-enabled" - ); - } #[test] fn browse_parses_from_string() { From 1052a0dffeb47a53b6529641a32b3f88e366d429 Mon Sep 17 00:00:00 2001 From: Amol Kapoor Date: Mon, 8 Jun 2026 18:01:58 -0400 Subject: [PATCH 08/10] =?UTF-8?q?fix:=20remove=20extra=20blank=20line=20ca?= =?UTF-8?q?ught=20by=20nightly=20rustfmt=20=F0=9F=A4=96=20Generated=20with?= =?UTF-8?q?=20[Nori](https://noriagentic.com)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Nori --- nori-rs/tui/src/slash_command.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/nori-rs/tui/src/slash_command.rs b/nori-rs/tui/src/slash_command.rs index b3d152559..ce196daff 100644 --- a/nori-rs/tui/src/slash_command.rs +++ b/nori-rs/tui/src/slash_command.rs @@ -439,7 +439,6 @@ mod tests { } } - #[test] fn browse_parses_from_string() { let cmd: SlashCommand = "browse".parse().expect("/browse should parse from string"); From 8cdcdb348d07cd6695bb3e50894a4ba3b1e37de5 Mon Sep 17 00:00:00 2001 From: Amol Kapoor Date: Mon, 8 Jun 2026 18:25:23 -0400 Subject: [PATCH 09/10] test: improve cloud feature test coverage and session list UX MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add missing broker test cases (locally-expired JWT and malformed response for list_sessions/resume_session), improve assertion quality (full struct assert, exact URL matching), show last_active_at timestamp in session picker, and add variant count guard to cloud mode tests. 🤖 Generated with [Nori](https://noriagentic.com) --- nori-rs/acp/src/broker/tests.rs | 95 +++++++++++++++++++++++++++++++- nori-rs/cli/src/cloud.rs | 47 +++++++++------- nori-rs/tui/src/slash_command.rs | 15 +++++ 3 files changed, 133 insertions(+), 24 deletions(-) diff --git a/nori-rs/acp/src/broker/tests.rs b/nori-rs/acp/src/broker/tests.rs index 88a485eb5..214faeeea 100644 --- a/nori-rs/acp/src/broker/tests.rs +++ b/nori-rs/acp/src/broker/tests.rs @@ -307,8 +307,13 @@ async fn acquire_session_sends_auth_and_parses_response() { let client = BrokerClient::new(broker_url, dir.path().to_path_buf()); let info = client.acquire_session().await.unwrap(); - assert_eq!(info.session_id, "sess-abc123"); - assert_eq!(info.ws_url, "wss://broker.test/ws/sess-abc123"); + assert_eq!( + info, + SessionInfo { + session_id: "sess-abc123".to_string(), + ws_url: "wss://broker.test/ws/sess-abc123".to_string(), + } + ); server_handle.join().unwrap(); } @@ -731,7 +736,7 @@ async fn resume_session_sends_post_with_auth_and_parses_response() { let server_handle = std::thread::spawn(move || { let request = mock_server.recv().unwrap(); assert_eq!(request.method(), &tiny_http::Method::Post); - assert!(request.url().contains("/api/sessions/sess-abc123/resume")); + assert_eq!(request.url(), "/api/sessions/sess-abc123/resume"); let auth_header = request .headers() @@ -834,3 +839,87 @@ async fn resume_session_errors_without_token() { let err = client.resume_session("sess-abc123").await.unwrap_err(); assert!(matches!(err, BrokerError::AuthRequired)); } + +#[tokio::test] +async fn list_sessions_returns_token_expired_for_locally_expired_jwt() { + let dir = tempdir().unwrap(); + let broker_url = "http://unused.test".to_string(); + let creds = CloudCredentials { + broker_url: broker_url.clone(), + auth_token: expired_jwt(), + }; + save_credentials(dir.path(), &creds).unwrap(); + let client = BrokerClient::new(broker_url, dir.path().to_path_buf()); + + let err = client.list_sessions().await.unwrap_err(); + assert!(matches!(err, BrokerError::TokenExpired)); +} + +#[tokio::test] +async fn resume_session_returns_token_expired_for_locally_expired_jwt() { + let dir = tempdir().unwrap(); + let broker_url = "http://unused.test".to_string(); + let creds = CloudCredentials { + broker_url: broker_url.clone(), + auth_token: expired_jwt(), + }; + save_credentials(dir.path(), &creds).unwrap(); + let client = BrokerClient::new(broker_url, dir.path().to_path_buf()); + + let err = client.resume_session("sess-abc123").await.unwrap_err(); + assert!(matches!(err, BrokerError::TokenExpired)); +} + +#[tokio::test] +async fn list_sessions_returns_error_for_malformed_response() { + let mock_server = tiny_http::Server::http("127.0.0.1:0").unwrap(); + let port = mock_server.server_addr().to_ip().unwrap().port(); + let broker_url = format!("http://127.0.0.1:{port}"); + let token = future_jwt(); + + let server_handle = std::thread::spawn(move || { + let request = mock_server.recv().unwrap(); + let response = tiny_http::Response::from_string("not valid json").with_status_code(200); + request.respond(response).unwrap(); + }); + + let dir = tempdir().unwrap(); + let creds = CloudCredentials { + broker_url: broker_url.clone(), + auth_token: token, + }; + save_credentials(dir.path(), &creds).unwrap(); + let client = BrokerClient::new(broker_url, dir.path().to_path_buf()); + + let err = client.list_sessions().await.unwrap_err(); + assert!(matches!(err, BrokerError::InvalidResponse(_))); + + server_handle.join().unwrap(); +} + +#[tokio::test] +async fn resume_session_returns_error_for_malformed_response() { + let mock_server = tiny_http::Server::http("127.0.0.1:0").unwrap(); + let port = mock_server.server_addr().to_ip().unwrap().port(); + let broker_url = format!("http://127.0.0.1:{port}"); + let token = future_jwt(); + + let server_handle = std::thread::spawn(move || { + let request = mock_server.recv().unwrap(); + let response = tiny_http::Response::from_string("not valid json").with_status_code(200); + request.respond(response).unwrap(); + }); + + let dir = tempdir().unwrap(); + let creds = CloudCredentials { + broker_url: broker_url.clone(), + auth_token: token, + }; + save_credentials(dir.path(), &creds).unwrap(); + let client = BrokerClient::new(broker_url, dir.path().to_path_buf()); + + let err = client.resume_session("sess-abc123").await.unwrap_err(); + assert!(matches!(err, BrokerError::InvalidResponse(_))); + + server_handle.join().unwrap(); +} diff --git a/nori-rs/cli/src/cloud.rs b/nori-rs/cli/src/cloud.rs index e09d56710..d88a3076a 100644 --- a/nori-rs/cli/src/cloud.rs +++ b/nori-rs/cli/src/cloud.rs @@ -13,8 +13,9 @@ pub fn format_cloud_session_list(sessions: &[CloudSessionSummary]) -> String { .first_message_preview .as_deref() .unwrap_or("(no preview)"); + let active = format_timestamp(&session.last_active_at); out.push_str(&format!( - " [{num}] ({source}) \"{preview}\"\n", + " [{num}] ({source}) \"{preview}\" — last active {active}\n", num = i + 1, source = session.source, )); @@ -23,6 +24,13 @@ pub fn format_cloud_session_list(sessions: &[CloudSessionSummary]) -> String { out } +fn format_timestamp(iso_timestamp: &str) -> String { + iso_timestamp + .get(..16) + .unwrap_or(iso_timestamp) + .replace('T', " ") +} + pub fn parse_session_choice(input: &str, session_count: i32) -> Result { let trimmed = input.trim(); if trimmed.is_empty() { @@ -83,27 +91,23 @@ mod tests { let output = format_cloud_session_list(&sessions); - assert!(output.contains("[1]"), "should have numbered entry [1]"); - assert!(output.contains("[2]"), "should have numbered entry [2]"); - assert!(output.contains("[n]"), "should have new session option"); - assert!( - output.contains("Fix the login bug"), - "should show first message preview" + assert_eq!( + output, + "Cloud Sessions:\n\ + \x20 [1] (cli) \"Fix the login bug\" — last active 2025-01-27 14:30\n\ + \x20 [2] (slack) \"(no preview)\" — last active 2025-01-27 14:30\n\ + \x20 [n] Start new session\n" ); - assert!(output.contains("cli"), "should show session source"); - assert!(output.contains("slack"), "should show session source"); } #[test] fn format_session_list_handles_empty_list() { let output = format_cloud_session_list(&[]); - assert!( - output.contains("[n]"), - "should still show new session option" - ); - assert!( - !output.contains("[1]"), - "should not have numbered entries for empty list" + + assert_eq!( + output, + "Cloud Sessions:\n\ + \x20 [n] Start new session\n" ); } @@ -112,11 +116,12 @@ mod tests { let sessions = vec![make_session("sess-1", "discord", None, "idle")]; let output = format_cloud_session_list(&sessions); - assert!(output.contains("[1]")); - assert!(output.contains("discord")); - assert!( - output.contains("(no preview)"), - "should show fallback text for missing preview" + + assert_eq!( + output, + "Cloud Sessions:\n\ + \x20 [1] (discord) \"(no preview)\" — last active 2025-01-27 14:30\n\ + \x20 [n] Start new session\n" ); } diff --git a/nori-rs/tui/src/slash_command.rs b/nori-rs/tui/src/slash_command.rs index ce196daff..feac3f2cf 100644 --- a/nori-rs/tui/src/slash_command.rs +++ b/nori-rs/tui/src/slash_command.rs @@ -439,6 +439,21 @@ mod tests { } } + #[test] + fn cloud_mode_tests_cover_all_variants() { + let cloud_disabled = SlashCommand::iter() + .filter(|c| !c.available_in_cloud_mode()) + .count(); + let cloud_enabled = SlashCommand::iter() + .filter(|c| c.available_in_cloud_mode()) + .count(); + assert_eq!( + cloud_disabled + cloud_enabled, + SlashCommand::iter().count(), + "cloud mode classification must cover every SlashCommand variant" + ); + } + #[test] fn browse_parses_from_string() { let cmd: SlashCommand = "browse".parse().expect("/browse should parse from string"); From 1fb3d839004d5028a4fcf3352b8b4c497b45a139 Mon Sep 17 00:00:00 2001 From: Amol Kapoor Date: Mon, 8 Jun 2026 18:44:53 -0400 Subject: [PATCH 10/10] fix: validate session IDs before URL interpolation to prevent path traversal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Session IDs from the broker response are untrusted input. Without validation, a malformed ID containing '/' or '..' could construct a URL targeting an unintended endpoint. Add validate_session_id() that restricts IDs to alphanumeric, hyphen, and underscore characters. Also removes a tautological test (cloud_mode_tests_cover_all_variants) that could never fail — the exhaustive match already provides compile-time coverage. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori --- nori-rs/acp/src/broker/mod.rs | 16 +++++++ nori-rs/acp/src/broker/tests.rs | 71 ++++++++++++++++++++++++++++++++ nori-rs/tui/src/slash_command.rs | 15 ------- 3 files changed, 87 insertions(+), 15 deletions(-) diff --git a/nori-rs/acp/src/broker/mod.rs b/nori-rs/acp/src/broker/mod.rs index 69541f8e9..e2f087954 100644 --- a/nori-rs/acp/src/broker/mod.rs +++ b/nori-rs/acp/src/broker/mod.rs @@ -75,6 +75,9 @@ pub enum BrokerError { #[error("session resume failed: HTTP {status}: {body}")] ResumeFailed { status: u16, body: String }, + #[error("invalid session ID: {0}")] + InvalidSessionId(String), + #[error("network error: {0}")] NetworkError(#[from] reqwest::Error), @@ -85,6 +88,17 @@ pub enum BrokerError { InvalidResponse(String), } +fn validate_session_id(session_id: &str) -> std::result::Result<(), BrokerError> { + if session_id.is_empty() + || !session_id + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') + { + return Err(BrokerError::InvalidSessionId(session_id.to_string())); + } + Ok(()) +} + pub struct BrokerClient { broker_url: String, auth_token: Option, @@ -192,6 +206,7 @@ impl BrokerClient { &self, session_id: &str, ) -> std::result::Result { + validate_session_id(session_id)?; let token = self .auth_token .as_deref() @@ -226,6 +241,7 @@ impl BrokerClient { } pub async fn release_session(&self, session_id: &str) -> std::result::Result<(), BrokerError> { + validate_session_id(session_id)?; let token = self .auth_token .as_deref() diff --git a/nori-rs/acp/src/broker/tests.rs b/nori-rs/acp/src/broker/tests.rs index 214faeeea..5141a7ac8 100644 --- a/nori-rs/acp/src/broker/tests.rs +++ b/nori-rs/acp/src/broker/tests.rs @@ -923,3 +923,74 @@ async fn resume_session_returns_error_for_malformed_response() { server_handle.join().unwrap(); } + +// ── session_id validation ──────────────────────────────────────── + +#[test] +fn validate_session_id_accepts_valid_ids() { + assert!(validate_session_id("sess-abc123").is_ok()); + assert!(validate_session_id("my_session_42").is_ok()); + assert!(validate_session_id("ABC").is_ok()); +} + +#[test] +fn validate_session_id_rejects_path_traversal() { + assert!(matches!( + validate_session_id("../etc/passwd"), + Err(BrokerError::InvalidSessionId(_)) + )); + assert!(matches!( + validate_session_id("sess/../../admin"), + Err(BrokerError::InvalidSessionId(_)) + )); +} + +#[test] +fn validate_session_id_rejects_empty() { + assert!(matches!( + validate_session_id(""), + Err(BrokerError::InvalidSessionId(_)) + )); +} + +#[test] +fn validate_session_id_rejects_special_characters() { + assert!(matches!( + validate_session_id("sess id"), + Err(BrokerError::InvalidSessionId(_)) + )); + assert!(matches!( + validate_session_id("sess?query=1"), + Err(BrokerError::InvalidSessionId(_)) + )); +} + +#[tokio::test] +async fn resume_session_rejects_malformed_session_id() { + let dir = tempdir().unwrap(); + let broker_url = "http://unused.test".to_string(); + let creds = CloudCredentials { + broker_url: broker_url.clone(), + auth_token: future_jwt(), + }; + save_credentials(dir.path(), &creds).unwrap(); + let client = BrokerClient::new(broker_url, dir.path().to_path_buf()); + + let err = client.resume_session("../admin").await.unwrap_err(); + assert!(matches!(err, BrokerError::InvalidSessionId(_))); +} + +#[tokio::test] +async fn release_session_rejects_malformed_session_id() { + let dir = tempdir().unwrap(); + let broker_url = "http://unused.test".to_string(); + let creds = CloudCredentials { + broker_url: broker_url.clone(), + auth_token: future_jwt(), + }; + save_credentials(dir.path(), &creds).unwrap(); + let client = BrokerClient::new(broker_url, dir.path().to_path_buf()); + + let err = client.release_session("sess/../../x").await.unwrap_err(); + assert!(matches!(err, BrokerError::InvalidSessionId(_))); +} diff --git a/nori-rs/tui/src/slash_command.rs b/nori-rs/tui/src/slash_command.rs index feac3f2cf..ce196daff 100644 --- a/nori-rs/tui/src/slash_command.rs +++ b/nori-rs/tui/src/slash_command.rs @@ -439,21 +439,6 @@ mod tests { } } - #[test] - fn cloud_mode_tests_cover_all_variants() { - let cloud_disabled = SlashCommand::iter() - .filter(|c| !c.available_in_cloud_mode()) - .count(); - let cloud_enabled = SlashCommand::iter() - .filter(|c| c.available_in_cloud_mode()) - .count(); - assert_eq!( - cloud_disabled + cloud_enabled, - SlashCommand::iter().count(), - "cloud mode classification must cover every SlashCommand variant" - ); - } - #[test] fn browse_parses_from_string() { let cmd: SlashCommand = "browse".parse().expect("/browse should parse from string");