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). diff --git a/CURRENT-PROGRESS.md b/CURRENT-PROGRESS.md new file mode 100644 index 000000000..e99fbe976 --- /dev/null +++ b/CURRENT-PROGRESS.md @@ -0,0 +1,57 @@ +# Current Progress + +## Status: All CLI-side items complete + +### 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 + +### Completed: Cloud Mode Feature Gating (Spec Item 2) + +**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-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 + +### Completed: Slack/Discord Session Resumption (Spec Item 3) + +**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 — 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 new file mode 100644 index 000000000..ada5a9c16 --- /dev/null +++ b/RESEARCH-NOTES.md @@ -0,0 +1,80 @@ +# 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 + +## 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` — 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 +3. **Worktree warning** in App: suppress in cloud mode 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 e2f953446..38386c1ce 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, prints the broker's `/api/auth/cli?redirect_uri=...` URL to stderr, opens that 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()`. Printing the URL before opening it keeps `nori cloud` usable from SSH or other headless shells. 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 562fc381d..e2f087954 100644 --- a/nori-rs/acp/src/broker/mod.rs +++ b/nori-rs/acp/src/broker/mod.rs @@ -45,6 +45,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")] @@ -59,11 +69,34 @@ 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("invalid session ID: {0}")] + InvalidSessionId(String), + #[error("network error: {0}")] NetworkError(#[from] reqwest::Error), #[error("invalid token: {0}")] InvalidToken(String), + + #[error("invalid response: {0}")] + 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 { @@ -129,11 +162,86 @@ 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) + } + + 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::InvalidResponse(e.to_string()))?; + Ok(sessions) + } + + pub async fn resume_session( + &self, + session_id: &str, + ) -> std::result::Result { + validate_session_id(session_id)?; + 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::InvalidResponse(e.to_string()))?; Ok(info) } 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 662838591..5141a7ac8 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(); } @@ -470,7 +475,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(); } @@ -540,3 +545,452 @@ 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, + 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(), + }, + ] + ); + + 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_eq!(request.url(), "/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, + SessionInfo { + session_id: "sess-abc123".to_string(), + ws_url: "wss://broker.test/ws/sess-abc123".to_string(), + } + ); + + 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)); +} + +#[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(); +} + +// ── 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/cli/docs.md b/nori-rs/cli/docs.md index 42275e0cd..0eb9e7edd 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()`. Browser OAuth prints the exact URL being opened so headless SSH sessions can copy it into another browser. 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. Browser OAuth prints the exact URL being opened so headless SSH sessions can copy it into another browser. 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` -- the retry handles the edge case where `has_valid_token()` passes at startup but the broker rejects the token with HTTP 401 +- 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..d88a3076a --- /dev/null +++ b/nori-rs/cli/src/cloud.rs @@ -0,0 +1,166 @@ +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)"); + let active = format_timestamp(&session.last_active_at); + out.push_str(&format!( + " [{num}] ({source}) \"{preview}\" — last active {active}\n", + num = i + 1, + source = session.source, + )); + } + out.push_str(" [n] Start new session\n"); + 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() { + 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_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" + ); + } + + #[test] + fn format_session_list_handles_empty_list() { + let output = format_cloud_session_list(&[]); + + assert_eq!( + output, + "Cloud Sessions:\n\ + \x20 [n] Start new session\n" + ); + } + + #[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_eq!( + output, + "Cloud Sessions:\n\ + \x20 [1] (discord) \"(no preview)\" — last active 2025-01-27 14:30\n\ + \x20 [n] Start new session\n" + ); + } + + #[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_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)); + 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 06dccd269..f527f3d53 100644 --- a/nori-rs/cli/src/main.rs +++ b/nori-rs/cli/src/main.rs @@ -557,20 +557,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); diff --git a/nori-rs/tui/docs.md b/nori-rs/tui/docs.md index ca597815b..cb68597d4 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 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. + **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");