fix(mcp): Resolve nori-client MCP review issues (#502)

## Summary
- keep replacement nori-client MCP servers pending until the ACP
replacement session is accepted
- preserve eager nori-client initialization state in initial and
post-replacement capability snapshots
- add mock-agent regression coverage for failed replacement sessions and
eager MCP initialization

## Test Plan
- cargo test -p nori-acp
- cargo build --bin nori
- cargo test -p tui-pty-e2e
- cargo test -p mock-acp-agent
- just fix -p nori-acp
- just fix -p mock-acp-agent
- just fmt
- TUI tmux verification with elizacp: launch, assert prompt, send hello,
assert prompt returns

🤖 Generated with [Nori](https://noriagentic.com/)

Share Nori with your team: https://www.npmjs.com/package/nori-skillsets

Author: CSRessel

nori-rs/acp/docs.md

@@ -119,7 +119,7 @@ The ACP backend owns the `/goal` feature as per-session state instead of delegat
 
 `nori_client_mcp.rs` is a bridge, not a second store. The goal tools are typed rmcp `#[tool]` handlers on `NoriClientService`; they lock the same `ThreadGoalState` used by TUI `/goal` operations, return JSON snapshots shaped for model consumption, and emit the same `ThreadGoalUpdated` client event after mutations. The bridge records those emitted events through `@/nori-rs/acp/src/backend/transcript.rs` when a transcript recorder is available; `NoriClientShared` stores the recorder behind a shared cell because the service is built before the session id is known.
 
-The local `nori-client` server is only advertised when `register_for_session` in `@/nori-rs/acp/src/backend/nori_client_mcp.rs` sees HTTP MCP support from `@/nori-rs/acp/src/connection/mod.rs`. Nori advertises a real `http://127.0.0.1:<port>/mcp` endpoint rather than an ACP pseudo-URL, because Codex ACP and Claude ACP both forward ACP `mcpServers` to their underlying clients as ordinary HTTP MCP server config. Each eligible session registration gets a fresh loopback server with a generated bearer token advertised as an `Authorization` header; an `axum` middleware rejects unauthenticated requests before they reach rmcp's `StreamableHttpService` (stateless mode). The server is owned by the ACP backend (abort-on-drop) and talks directly to the same in-memory goal state as `/goal`. The server is named `nori-client` rather than `nori-goal` because it is Nori's general harness-side channel to the external ACP agent -- the single point of contact for harness-specific tooling the ACP protocol does not yet provide -- and goal tools are only its live-state surface. The backend emits a `SessionCapabilitiesChanged` projection after session setup so clients can derive built-in command availability from the same capability state. That projection separates raw agent capabilities (`agent.http_mcp`, `agent.load_session`), `nori_client.advertised`, `nori_client.initialized`, and derived `builtin_commands` availability.
+The local `nori-client` server is only advertised when `register_for_session` in `@/nori-rs/acp/src/backend/nori_client_mcp.rs` sees HTTP MCP support from `@/nori-rs/acp/src/connection/mod.rs`. Nori advertises a real `http://127.0.0.1:<port>/mcp` endpoint rather than an ACP pseudo-URL, because Codex ACP and Claude ACP both forward ACP `mcpServers` to their underlying clients as ordinary HTTP MCP server config. Each eligible session registration gets a fresh loopback server with a generated bearer token advertised as an `Authorization` header; an `axum` middleware rejects unauthenticated requests before they reach rmcp's `StreamableHttpService` (stateless mode). The server is owned by the ACP backend (abort-on-drop) and talks directly to the same in-memory goal state as `/goal`. The server is named `nori-client` rather than `nori-goal` because it is Nori's general harness-side channel to the external ACP agent -- the single point of contact for harness-specific tooling the ACP protocol does not yet provide -- and goal tools are only its live-state surface. The backend emits a `SessionCapabilitiesChanged` projection after session setup so clients can derive built-in command availability from the same capability state. That projection separates raw agent capabilities (`agent.http_mcp`, `agent.load_session`), `nori_client.advertised`, `nori_client.initialized`, and derived `builtin_commands` availability. Initial and post-replacement snapshots read the same connected flag flipped by MCP `initialize`, so agents that eagerly initialize the advertised server during session setup do not observe initialized state regress from true back to false.
 
 `@/nori-rs/acp/src/backend/nori_client_context.rs` owns the fixed read-only catalog exposed through the same server. `nori_client_mcp.rs` advertises tools, resources, and prompts in `ServerInfo`, but delegates list/read/get operations to that sibling module so transport, initialization, connected-gate behavior, and capability registration stay separate from Nori's curated operating context. The catalog intentionally serves Nori-owned harness facts, minimal ACP debugging and custom-agent help, and a compact source map for answering Nori CLI questions; it is not an arbitrary filesystem, repo-read, or skill-workflow API. Unknown resource URIs and prompt names are rejected as MCP errors, keeping the context surface closed and predictable.
 
@@ -697,7 +697,7 @@ This means `to_sacp_mcp_servers()` has side effects (reads from keyring/file sys
 
 The server uses a normal `http://127.0.0.1:<port>/mcp` URL because current Codex and Claude ACP adapters forward ACP MCP server entries into their underlying clients as ordinary HTTP MCP config. Avoiding `acp:` keeps startup compatible with those adapters while still keeping the tool implementation in process.
 
-ACP session setup paths build the MCP server list in two phases: first convert configured MCP servers with `to_sacp_mcp_servers()`, then let backend-owned features append local MCP servers when their own eligibility checks pass. The `nori-client` server requires HTTP MCP support. This setup applies to resumed, fresh, fallback, and compaction-created sessions. Compaction-created replacement sessions re-emit `SessionCapabilitiesChanged` after `create_session` succeeds, deriving `nori_client.advertised` from the actual MCP server list passed to that replacement session. Hook-only ACP sessions pass an empty list because hooks do not need user-configured or backend-owned MCP servers.
+ACP session setup paths build the MCP server list in two phases: first convert configured MCP servers with `to_sacp_mcp_servers()`, then let backend-owned features append local MCP servers when their own eligibility checks pass. The `nori-client` server requires HTTP MCP support. This setup applies to resumed, fresh, fallback, and compaction-created sessions. `register_for_session` returns a pending server registration; callers commit that server to `AcpBackend` only after `session/new` or `session/load` succeeds, so a failed replacement session drops the new listener and preserves the still-active session's previously advertised endpoint. Compaction-created replacement sessions re-emit `SessionCapabilitiesChanged` after `create_session` succeeds, deriving `nori_client.advertised` from the actual MCP server list passed to that replacement session. Hook-only ACP sessions pass an empty list because hooks do not need user-configured or backend-owned MCP servers.
 
 The local `nori-client` MCP server is intentionally additive. User-configured MCP servers are still forwarded normally, and ineligible agents simply do not receive the loopback endpoint. `SessionCapabilitiesChanged` exposes both endpoint advertisement and initialization so clients do not need to infer either state from raw HTTP MCP support. Continuation chaining depends on the local MCP server actually being initialized for the current advertised endpoint, not just on HTTP MCP support or endpoint advertisement. Durable follow-up work for this surface lives in `@/docs/followups/nori-client-mcp.md`.
 

nori-rs/acp/src/backend/nori_client_mcp.rs

@@ -395,6 +395,29 @@ impl Drop for NoriClientServer {
     }
 }
 
+pub(super) struct PendingNoriClientServer {
+    server: Option<NoriClientServer>,
+    connected: Arc<AtomicBool>,
+    previous_connected: bool,
+    committed: bool,
+}
+
+impl PendingNoriClientServer {
+    pub(super) async fn commit(mut self, http_server: &Arc<Mutex<Option<NoriClientServer>>>) {
+        *http_server.lock().await = self.server.take();
+        self.committed = true;
+    }
+}
+
+impl Drop for PendingNoriClientServer {
+    fn drop(&mut self) {
+        if !self.committed {
+            self.connected
+                .store(self.previous_connected, Ordering::Relaxed);
+        }
+    }
+}
+
 async fn require_bearer_token(
     State(expected): State<Arc<String>>,
     request: Request<Body>,
@@ -413,11 +436,12 @@ async fn require_bearer_token(
 
 pub(super) fn capabilities_update_for_session(
     connection: &SacpConnection,
+    connected: &AtomicBool,
 ) -> nori_protocol::SessionCapabilitiesView {
     capabilities_update_for_nori_client(
         connection,
         connection.capabilities().mcp_capabilities.http,
-        false,
+        connected.load(Ordering::Relaxed),
     )
 }
 
@@ -464,15 +488,11 @@ pub(super) async fn register_for_session(
     backend_event_tx: mpsc::Sender<BackendEvent>,
     transcript_recorder: Arc<Mutex<Option<Arc<TranscriptRecorder>>>>,
     connected: Arc<AtomicBool>,
-    http_server: Arc<Mutex<Option<NoriClientServer>>>,
-) -> Result<()> {
-    connected.store(false, Ordering::Relaxed);
-
+) -> Result<Option<PendingNoriClientServer>> {
     if !connection.capabilities().mcp_capabilities.http {
-        return Ok(());
+        return Ok(None);
     }
 
-    let mut server = http_server.lock().await;
     let next_server = NoriClientServer::spawn(NoriClientShared::new(
         thread_goal_state,
         backend_event_tx,
@@ -489,10 +509,15 @@ pub(super) async fn register_for_session(
         acp::McpServerHttp::new("nori-client", next_server.url().to_string())
             .headers(vec![next_server.authorization_header()]),
     );
-    *server = Some(next_server);
+    let previous_connected = connected.swap(false, Ordering::Relaxed);
     mcp_servers.push(advertised_server);
 
-    Ok(())
+    Ok(Some(PendingNoriClientServer {
+        server: Some(next_server),
+        connected,
+        previous_connected,
+        committed: false,
+    }))
 }
 
 #[cfg(test)]

nori-rs/acp/src/backend/session.rs

@@ -138,19 +138,21 @@ impl AcpBackend {
                 &config.mcp_servers,
                 config.mcp_oauth_credentials_store_mode,
             );
-            nori_client_mcp::register_for_session(
+            let pending_nori_client_server = nori_client_mcp::register_for_session(
                 &connection,
                 &mut mcp_servers,
                 Arc::clone(&thread_goal_state),
                 backend_event_tx.clone(),
                 Arc::clone(&transcript_recorder_cell),
                 Arc::clone(&goal_mcp_connected),
-                Arc::clone(&goal_mcp_http_server),
             )
             .await?;
 
             match connection.load_session(sid, &cwd, mcp_servers).await {
                 Ok(session_id) => {
+                    if let Some(server) = pending_nori_client_server {
+                        server.commit(&goal_mcp_http_server).await;
+                    }
                     // Signal the collector that load is done, then collect results.
                     let _ = load_done_tx.send(());
                     let (session_driver, recovered_rx, buffered_client_events) =
@@ -187,14 +189,13 @@ impl AcpBackend {
                         &config.mcp_servers,
                         config.mcp_oauth_credentials_store_mode,
                     );
-                    nori_client_mcp::register_for_session(
+                    let pending_nori_client_server = nori_client_mcp::register_for_session(
                         &connection,
                         &mut mcp_servers,
                         Arc::clone(&thread_goal_state),
                         backend_event_tx.clone(),
                         Arc::clone(&transcript_recorder_cell),
                         Arc::clone(&goal_mcp_connected),
-                        Arc::clone(&goal_mcp_http_server),
                     )
                     .await?;
                     let session_id =
@@ -214,6 +215,9 @@ impl AcpBackend {
                                     &agent_config.install_hint,
                                 ))
                             })?;
+                    if let Some(server) = pending_nori_client_server {
+                        server.commit(&goal_mcp_http_server).await;
+                    }
 
                     let (replay_events, summary) = if let Some(t) = transcript {
                         let client_events = transcript_to_replay_client_events(t);
@@ -246,14 +250,13 @@ impl AcpBackend {
                 &config.mcp_servers,
                 config.mcp_oauth_credentials_store_mode,
             );
-            nori_client_mcp::register_for_session(
+            let pending_nori_client_server = nori_client_mcp::register_for_session(
                 &connection,
                 &mut mcp_servers,
                 Arc::clone(&thread_goal_state),
                 backend_event_tx.clone(),
                 Arc::clone(&transcript_recorder_cell),
                 Arc::clone(&goal_mcp_connected),
-                Arc::clone(&goal_mcp_http_server),
             )
             .await?;
             let session_id = connection
@@ -272,6 +275,9 @@ impl AcpBackend {
                         &agent_config.install_hint,
                     ))
                 })?;
+            if let Some(server) = pending_nori_client_server {
+                server.commit(&goal_mcp_http_server).await;
+            }
 
             let (replay_events, summary) = if let Some(t) = transcript {
                 let client_events = transcript_to_replay_client_events(t);
@@ -305,7 +311,8 @@ impl AcpBackend {
                 thread_goal::ThreadGoalState::from_replay_events(&replay_events_for_goal_state);
         }
 
-        let capabilities_update = nori_client_mcp::capabilities_update_for_session(&connection);
+        let capabilities_update =
+            nori_client_mcp::capabilities_update_for_session(&connection, &goal_mcp_connected);
         let connection = Arc::new(connection);
         let pending_approvals = Arc::new(Mutex::new(Vec::new()));
         let session_driver = Arc::new(Mutex::new(session_driver_state));

nori-rs/acp/src/backend/session_runtime_driver.rs

@@ -491,19 +491,22 @@ impl AcpBackend {
                     &self.mcp_servers,
                     self.mcp_oauth_credentials_store_mode,
                 );
-                if let Err(err) = nori_client_mcp::register_for_session(
+                let pending_nori_client_server = match nori_client_mcp::register_for_session(
                     &self.connection,
                     &mut mcp_servers,
                     Arc::clone(&self.thread_goal_state),
                     self.backend_event_tx.clone(),
                     Arc::clone(&self.transcript_recorder_cell),
                     Arc::clone(&self.goal_mcp_connected),
-                    Arc::clone(&self.goal_mcp_http_server),
                 )
                 .await
                 {
-                    warn!("Failed to register goal MCP server after compact: {err}");
-                }
+                    Ok(server) => server,
+                    Err(err) => {
+                        warn!("Failed to register goal MCP server after compact: {err}");
+                        None
+                    }
+                };
                 let nori_client_advertised = mcp_servers.iter().any(|server| {
                     matches!(
                         server,
@@ -512,14 +515,18 @@ impl AcpBackend {
                 });
                 match self.connection.create_session(&cwd, mcp_servers).await {
                     Ok(new_session_id) => {
+                        if let Some(server) = pending_nori_client_server {
+                            server.commit(&self.goal_mcp_http_server).await;
+                        }
                         debug!("Created new session after compact: {:?}", new_session_id);
                         *self.session_id.write().await = new_session_id;
                         self.forward_and_record_client_event(
                             ClientEvent::SessionCapabilitiesChanged(
                                 nori_client_mcp::capabilities_update_for_nori_client(
                                     &self.connection,
                                     nori_client_advertised,
-                                    false,
+                                    self.goal_mcp_connected
+                                        .load(std::sync::atomic::Ordering::Relaxed),
                                 ),
                             ),
                         )

nori-rs/acp/src/backend/spawn_and_relay.rs

@@ -64,19 +64,23 @@ impl AcpBackend {
             &config.mcp_servers,
             config.mcp_oauth_credentials_store_mode,
         );
-        nori_client_mcp::register_for_session(
+        let pending_nori_client_server = nori_client_mcp::register_for_session(
             &connection,
             &mut mcp_servers,
             Arc::clone(&thread_goal_state),
             backend_event_tx.clone(),
             Arc::clone(&transcript_recorder_cell),
             Arc::clone(&goal_mcp_connected),
-            Arc::clone(&goal_mcp_http_server),
         )
         .await?;
         let session_result = connection.create_session(&cwd, mcp_servers).await;
         let session_id = match session_result {
-            Ok(id) => id,
+            Ok(id) => {
+                if let Some(server) = pending_nori_client_server {
+                    server.commit(&goal_mcp_http_server).await;
+                }
+                id
+            }
             Err(e) => {
                 // Get the full error chain to check for nested auth errors
                 let error_string = format!("{e:?}");
@@ -122,7 +126,8 @@ impl AcpBackend {
             }
         }
 
-        let capabilities_update = nori_client_mcp::capabilities_update_for_session(&connection);
+        let capabilities_update =
+            nori_client_mcp::capabilities_update_for_session(&connection, &goal_mcp_connected);
         let event_rx = connection.take_event_receiver();
 
         let connection = Arc::new(connection);