Synced from monorepo

Synced from monorepo

Changes:
- Temporarily disable session share link creation in the TUI
- Do not approve plan on empty Enter from the revise prompt
- Expose chat product Skills via ACP available_commands_update
- Return immediately from a blocking wait on an already-completed ACP task
- Split headless pager module for clearer structure
- Stop git worktree prune from removing user registrations on resume
- Use compaction sampler tokenizer for item token counts
- Opt-in extra root CAs via GROK_EXTRA_CA_BUNDLE
- Cancel all session subagents when the user stops
- Let the session persistence actor exit when its session ends
- Make fullscreen terminal resize much cheaper on long sessions
- Report honestly from kill_task when an ACP task does not exist
- Hide /usage for external-auth deployments
- Forward the history-load trailer’s computer_reason to the client
- Remove ineffective no-op tool reminder
- Declare slash-command screen-mode support in one place
- Keep settings enum picker on the committed value until Enter
- Reap a PTY’s full process tree
- Stream tool calls from headless mode over ACP
- Bridge gateway task lifecycle to ACP for chat session background tasks
- Don’t warn about truncated history on a suppressed replay
- Fit full-replace summarizer input and recover on context-length errors
- Stop dropping agents over an unrecognized frontmatter color
- Add /undo as a slash alias for /rewind
- Harden sleep/wake token-refresh paths against forced re-login
- Add session/list ACP method
- Give each sampling backend its own conversion module
- Treat an unenrolled child process as a lint error
- Suppress the cancelled marker on send-now wake turns
- Stop tearing down Roslyn on every edit, and read C# diagnostics

Source-Revision: 2a28b4a86cfc4a4c133c35b7fc2a6a9964387c39

crates/codegen/xai-grok-mcp/Cargo.toml

@@ -14,6 +14,7 @@ rmcp = { version = "2.1", features = [
     "transport-streamable-http-client-reqwest",
     "reqwest",
 ] }
+xai-grok-extra-ca = { workspace = true }
 xai-grok-version = { workspace = true }
 # reqwest 0.13 feature set for MCP transports. Notably:
 # - `blocking` is for build-script style use (not actually used here today, but

crates/codegen/xai-grok-mcp/src/credentials.rs

@@ -121,22 +121,12 @@ impl McpCredentialStore {
         self.save_to(&path)
     }
 
-    /// Atomically insert a credential and save — safe for concurrent use.
-    ///
-    /// Instead of the caller doing `insert_rmcp` + `save_default` (which races
-    /// with other processes), this method:
-    /// 1. Acquires a file lock on `mcp_credentials.json.lock`
-    /// 2. Reloads the store from disk (picks up other processes' writes)
-    /// 3. Inserts the new entry
-    /// 4. Saves atomically (temp + rename)
-    /// 5. Updates `self` with the merged result
-    /// 6. Releases the lock
-    pub fn insert_and_save(
-        &mut self,
-        server_name: &str,
-        server_url: &url::Url,
-        creds: rmcp::transport::auth::StoredCredentials,
-    ) -> Result<()> {
+    /// Read-modify-write the **default** store under the cross-process
+    /// `mcp_credentials.json.lock` flock: reload from disk (merging concurrent
+    /// writers), apply `mutate`, save atomically, and update `self` with the
+    /// merged result. On flock failure (non-EINTR error, or non-Unix), falls
+    /// back to mutating `self` and saving best-effort — the pre-lock behavior.
+    fn locked_mutate_and_save(&mut self, mutate: &dyn Fn(&mut Self)) -> Result<()> {
         let path = Self::default_path().ok_or_else(|| {
             McpCredentialError::Other("no user grok home (set $GROK_HOME or $HOME)".into())
         })?;
@@ -165,14 +155,14 @@ impl McpCredentialStore {
                 if err.kind() == std::io::ErrorKind::Interrupted {
                     continue; // Retry on EINTR.
                 }
-                // Lock failed for another reason — fall back to non-atomic insert.
-                self.insert_rmcp(server_name, server_url, creds);
+                // Lock failed for another reason — fall back to non-atomic write.
+                mutate(self);
                 return self.save_to(&path);
             }
 
             // Reload from disk under lock to merge with concurrent writes.
             let mut fresh = Self::load_from(&path).unwrap_or_default();
-            fresh.insert_rmcp(server_name, server_url, creds);
+            mutate(&mut fresh);
             fresh.save_to(&path)?;
             *self = fresh;
 
@@ -182,13 +172,38 @@ impl McpCredentialStore {
         #[cfg(not(unix))]
         {
             // No flock on non-unix — best-effort.
-            self.insert_rmcp(server_name, server_url, creds);
+            mutate(self);
             self.save_to(&path)?;
         }
 
         Ok(())
     }
 
+    /// Locked insert ([`Self::locked_mutate_and_save`]) with a freshness
+    /// guard: skipped when the disk entry is strictly newer by
+    /// `token_received_at` (see [`disk_entry_is_newer`]) — otherwise a slow
+    /// writer (canonically a refresh suspended across system sleep that
+    /// completes after wake) rolls the stored refresh token back to a
+    /// rotated-out value (`invalid_grant` on its next use).
+    pub fn insert_and_save(
+        &mut self,
+        server_name: &str,
+        server_url: &url::Url,
+        creds: rmcp::transport::auth::StoredCredentials,
+    ) -> Result<()> {
+        let key = Self::key(server_name, server_url);
+        self.locked_mutate_and_save(&move |store: &mut Self| {
+            if disk_entry_is_newer(store.entries.get(&key), &creds) {
+                tracing::info!(
+                    key = key.as_str(),
+                    "mcp credentials: skipping stale save (disk entry is newer)"
+                );
+                return;
+            }
+            store.entries.insert(key.clone(), creds.clone());
+        })
+    }
+
     /// Save to a specific path.
     ///
     /// Writes atomically via temp file + rename to prevent credential loss on
@@ -273,6 +288,18 @@ impl McpCredentialStore {
         self.entries.remove(&Self::key(server_name, server_url));
     }
 
+    /// Remove a server's credentials and persist, under the cross-process
+    /// file lock (reload-merge → remove → atomic save). The locked
+    /// counterpart of [`Self::remove`] + [`Self::save_default`] for callers
+    /// that persist the removal — an unlocked whole-file rewrite can drop
+    /// other processes' concurrent writes for unrelated servers.
+    pub fn remove_and_save(&mut self, server_name: &str, server_url: &Url) -> Result<()> {
+        let key = Self::key(server_name, server_url);
+        self.locked_mutate_and_save(&move |store: &mut Self| {
+            store.entries.remove(&key);
+        })
+    }
+
     /// Remove all credentials for a server by name (any URL).
     pub fn remove_by_server_name(&mut self, server_name: &str) -> usize {
         let prefix = format!("{server_name}:");
@@ -292,6 +319,23 @@ impl McpCredentialStore {
     }
 }
 
+/// `true` when the on-disk `existing` entry is strictly newer than the
+/// `incoming` credentials by `token_received_at` — the [`Self::insert_and_save`]
+/// freshness guard. Missing timestamps on either side compare as "not newer"
+/// (the write proceeds), preserving pre-guard behavior for expiry-less tokens.
+fn disk_entry_is_newer(
+    existing: Option<&rmcp::transport::auth::StoredCredentials>,
+    incoming: &rmcp::transport::auth::StoredCredentials,
+) -> bool {
+    match (
+        existing.and_then(|e| e.token_received_at),
+        incoming.token_received_at,
+    ) {
+        (Some(existing), Some(incoming)) => existing > incoming,
+        _ => false,
+    }
+}
+
 /// Adapter implementing rmcp's `CredentialStore` trait backed by the on-disk
 /// `McpCredentialStore`. Each adapter instance is scoped to a single MCP server
 /// (keyed by name + URL); rmcp's `AuthorizationManager` calls load/save/clear
@@ -349,10 +393,13 @@ impl rmcp::transport::auth::CredentialStore for McpCredentialStoreAdapter {
         let name = self.server_name.clone();
         let url = self.server_url.clone();
         tokio::task::spawn_blocking(move || {
+            // Under the same flock as `insert_and_save`: this is a whole-file
+            // read-modify-write, and an unlocked snapshot here could silently
+            // drop *other servers'* entries written concurrently by another
+            // process (their just-rotated refresh tokens with them).
             let mut store = McpCredentialStore::load_default().unwrap_or_default();
-            store.remove(&name, &url);
             store
-                .save_default()
+                .remove_and_save(&name, &url)
                 .map_err(|e| rmcp::transport::auth::AuthError::InternalError(e.to_string()))
         })
         .await
@@ -515,4 +562,76 @@ mod tests {
         let mode = std::fs::metadata(&path).unwrap().permissions().mode();
         assert_eq!(mode & 0o777, 0o600);
     }
+
+    /// The `insert_and_save` freshness guard: a save older (by
+    /// `token_received_at`) than the on-disk entry must be skipped.
+    #[test]
+    fn stale_save_does_not_clobber_newer_disk_entry() {
+        // `StoredCredentials` is #[non_exhaustive]; construct via `new` and
+        // set the (public) timestamp field afterwards.
+        let mut older = test_stored_creds("c");
+        older.token_received_at = Some(1_000);
+        let mut newer = test_stored_creds("c");
+        newer.token_received_at = Some(2_000);
+        let no_ts = test_stored_creds("c");
+
+        assert!(
+            disk_entry_is_newer(Some(&newer), &older),
+            "older incoming vs newer disk → skip the write"
+        );
+        assert!(
+            !disk_entry_is_newer(Some(&older), &newer),
+            "newer incoming vs older disk → write proceeds"
+        );
+        assert!(
+            !disk_entry_is_newer(Some(&older), &older),
+            "equal timestamps → write proceeds (idempotent re-save)"
+        );
+        assert!(
+            !disk_entry_is_newer(None, &older),
+            "no disk entry → write proceeds"
+        );
+        assert!(
+            !disk_entry_is_newer(Some(&newer), &no_ts),
+            "timestamp-less incoming keeps pre-guard behavior (writes)"
+        );
+        assert!(
+            !disk_entry_is_newer(Some(&no_ts), &older),
+            "timestamp-less disk entry keeps pre-guard behavior (writes)"
+        );
+    }
+
+    /// The refresh-failure classifier that gates browser escalation
+    /// (`force_reauth`): network-level failures — the `oauth2` crate's
+    /// `Display` for request/parse errors — are transient; IdP rejections and
+    /// missing-credential states stay terminal (escalate, as before).
+    #[test]
+    fn refresh_failure_transient_classification() {
+        use crate::servers::mcp_refresh_failure_is_transient;
+        use rmcp::transport::auth::AuthError;
+
+        // oauth2 RequestTokenError::Request renders exactly "Request failed".
+        assert!(mcp_refresh_failure_is_transient(
+            &AuthError::TokenRefreshFailed("Request failed".into())
+        ));
+        // 5xx/proxy bodies that aren't OAuth JSON parse-fail.
+        assert!(mcp_refresh_failure_is_transient(
+            &AuthError::TokenRefreshFailed("Failed to parse server response".into())
+        ));
+
+        // IdP rejections carry the RFC 6749 code → terminal.
+        assert!(!mcp_refresh_failure_is_transient(
+            &AuthError::TokenRefreshFailed(
+                "Server returned error response: invalid_grant: token revoked".into()
+            )
+        ));
+        // No refresh token at all → only the browser flow can help.
+        assert!(!mcp_refresh_failure_is_transient(
+            &AuthError::TokenRefreshFailed("No refresh token available".into())
+        ));
+        // Empty credential store → interactive auth required.
+        assert!(!mcp_refresh_failure_is_transient(
+            &AuthError::AuthorizationRequired
+        ));
+    }
 }

crates/codegen/xai-grok-mcp/src/oauth.rs

@@ -28,6 +28,22 @@ const MCP_OAUTH_CLIENT_NAME: &str = "Grok";
 /// a login completed in another window or process.
 const CREDENTIAL_POLL_INTERVAL: std::time::Duration = std::time::Duration::from_secs(2);
 
+/// Overall budget for one interactive browser consent flow (waiting for the
+/// loopback callback / disk poll after opening the browser). Mirrors the main
+/// grok.com login's 10-minute callback budget. Without a bound, an abandoned
+/// browser tab left the leader parked in its `select!` forever — holding both
+/// the in-process watch channel and the cross-process `mcp_auth_*.lock`, so
+/// every other session blocked indefinitely on the same server's auth.
+const BROWSER_AUTH_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(600);
+
+/// How long a follower waits for the cross-process auth lock before giving up
+/// on dedup and proceeding with its own flow. Slightly above
+/// [`BROWSER_AUTH_TIMEOUT`] so a legitimately-slow leader (user reading the
+/// consent screen) finishes first and the follower reuses its token.
+#[cfg(unix)]
+const AUTH_LOCK_WAIT: std::time::Duration =
+    BROWSER_AUTH_TIMEOUT.saturating_add(std::time::Duration::from_secs(60));
+
 // ---------------------------------------------------------------------------
 // Two-layer dedup: prevents duplicate browser tabs both within one process
 // (multiple async tasks / sessions) and across separate processes (leader
@@ -185,31 +201,49 @@ async fn authenticate_with_fs_lock(
         }
     };
 
+    // Bounded, non-blocking poll instead of an unbounded `flock(LOCK_EX)`:
+    // the leader can legitimately hold this lock for minutes (user consent),
+    // but an abandoned/wedged leader must not park followers forever. On
+    // timeout we fall back to running our own flow (same as lock-acquisition
+    // failure), which the token-changed re-check below keeps from producing a
+    // duplicate consent when the leader did finish.
     let lock_file = tokio::task::spawn_blocking(move || {
         use std::os::unix::io::AsRawFd;
         let fd = lock_file.as_raw_fd();
+        let deadline = std::time::Instant::now() + AUTH_LOCK_WAIT;
         loop {
-            if unsafe { libc::flock(fd, libc::LOCK_EX) } == 0 {
+            if unsafe { libc::flock(fd, libc::LOCK_EX | libc::LOCK_NB) } == 0 {
                 return Some(lock_file);
             }
             let err = std::io::Error::last_os_error();
-            if err.kind() == std::io::ErrorKind::Interrupted {
-                continue;
+            match err.kind() {
+                std::io::ErrorKind::Interrupted => continue,
+                std::io::ErrorKind::WouldBlock => {
+                    if std::time::Instant::now() >= deadline {
+                        return None;
+                    }
+                    std::thread::sleep(std::time::Duration::from_millis(250));
+                }
+                _ => return None,
             }
-            return None;
         }
     })
     .await
     .ok()
     .flatten();
 
-    let Some(_lock_guard) = lock_file else {
-        tracing::warn!("Failed to acquire auth lock; proceeding without cross-process dedup");
-        return run_browser_auth_flow(server_name, server_url, auth_manager, byo_config).await;
-    };
+    if lock_file.is_none() {
+        tracing::warn!("Timed out waiting for auth lock; re-checking the store before a new flow");
+    }
+    // On timeout: proceed unlocked; the token-changed re-check below
+    // dedups a leader that finished just past our deadline.
+    let _lock_guard = lock_file;
 
-    // We hold the lock. Reload from disk and check if another process
-    // wrote a DIFFERENT token while we waited (not just any token).
+    // Reload from disk and check whether another process wrote a DIFFERENT
+    // token while we waited (not just any token). This runs on the timeout
+    // path too: a leader whose token exchange finished just past our deadline
+    // has already written fresh tokens, and opening a second consent browser
+    // would be strictly worse than this unlocked best-effort read.
     {
         let mut mgr = auth_manager.lock().await;
         if let Ok(true) = mgr.initialize_from_store().await {
@@ -436,6 +470,22 @@ async fn run_browser_auth_flow(
                 "Fresh tokens detected on disk from another auth flow; skipping callback wait"
             );
         }
+        // Abandoned consent: bound the wait so this leader releases the
+        // in-process watch and the cross-process `mcp_auth_*.lock` instead of
+        // wedging every future auth attempt for this server (see
+        // `BROWSER_AUTH_TIMEOUT`).
+        _ = tokio::time::sleep(BROWSER_AUTH_TIMEOUT) => {
+            callback_server.abort();
+            tracing::warn!(
+                server = server_name,
+                timeout_secs = BROWSER_AUTH_TIMEOUT.as_secs(),
+                "OAuth consent timed out (browser flow abandoned?)"
+            );
+            return Err(format!(
+                "OAuth consent timed out after {}s; re-run authentication to try again",
+                BROWSER_AUTH_TIMEOUT.as_secs()
+            ));
+        }
     }
 
     Ok(())

crates/codegen/xai-grok-mcp/src/servers.rs

@@ -44,6 +44,20 @@ use xai_grok_tools::util::{ProcessGroup, ProcessScope};
 /// for callers that historically imported it from this module.
 pub use xai_grok_workspace_types::MCP_TOOL_NAME_DELIMITER;
 
+/// Reqwest 0.13 adapter over `xai_grok_extra_ca::extra_root_ders` (DER is version-neutral).
+fn with_extra_root_certificates(mut builder: reqwest::ClientBuilder) -> reqwest::ClientBuilder {
+    for der in xai_grok_extra_ca::extra_root_ders() {
+        match reqwest::Certificate::from_der(der) {
+            Ok(cert) => builder = builder.add_root_certificate(cert),
+            Err(e) => tracing::warn!(
+                error = %e,
+                "GROK_EXTRA_CA_BUNDLE: validated DER rejected by reqwest 0.13; skipping cert"
+            ),
+        }
+    }
+    builder
+}
+
 /// Normalize an MCP server URL for comparison: strip trailing slashes.
 /// Must match the normalization the host's managed-config layer uses
 /// (e.g. shell's `session::managed_mcp::normalize_url`) so refresh
@@ -1148,6 +1162,24 @@ impl McpError {
     }
 }
 
+/// True when a failed refresh-token grant was a **network-level** failure
+/// that never reached the IdP (RT validity unknown, presumed good); IdP
+/// rejections and missing credentials stay terminal (escalate to browser).
+/// rmcp 2.1 collapses the error into `TokenRefreshFailed(String)`, so this
+/// anchors on the `oauth2` crate's stable `Display` texts via
+/// `starts_with` (an IdP error description can't spoof a match):
+/// `"Request failed"` = network, `"Failed to parse server response"` =
+/// non-OAuth 5xx/proxy bodies; `"Server returned error response: …"` does
+/// NOT match.
+pub(crate) fn mcp_refresh_failure_is_transient(err: &rmcp::transport::auth::AuthError) -> bool {
+    match err {
+        rmcp::transport::auth::AuthError::TokenRefreshFailed(msg) => {
+            msg.starts_with("Request failed") || msg.starts_with("Failed to parse server response")
+        }
+        _ => false,
+    }
+}
+
 /// True if an MCP error *message* indicates an auth rejection (vs. a transport
 /// drop, timeout, or protocol error), so host recovery can decide whether a
 /// credential re-fetch would help.
@@ -2037,6 +2069,7 @@ impl SafeTokioChildProcess {
             .stdout(std::process::Stdio::piped())
             .stderr(std::process::Stdio::piped());
 
+        #[allow(clippy::disallowed_methods)] // enrolled in the session scope below
         let mut child = cmd.spawn()?;
         let stdin = child
             .stdin
@@ -2810,7 +2843,12 @@ impl McpClient {
     /// Tries in order:
     /// 1. Reload from disk (picks up tokens from background auth task)
     /// 2. Refresh via refresh_token grant
-    /// 3. Full browser-based OAuth flow
+    /// 3. Full browser-based OAuth flow — unless the refresh failure was a
+    ///    pure network failure ([`mcp_refresh_failure_is_transient`]): the
+    ///    stored refresh token is then still presumed valid, and opening a
+    ///    browser tab / re-running DCR for a Wi-Fi blip right after
+    ///    wake-from-sleep is both useless (the IdP is unreachable for the
+    ///    browser too) and destructive (it discards a working credential).
     pub async fn force_reauth(&self, force: bool) -> bool {
         let (Some(auth_mgr), Some(config)) = (&self.auth_manager, &self.http_config) else {
             return false;
@@ -2861,22 +2899,43 @@ impl McpClient {
         }
 
         // Try token refresh.
-        let refresh_ok = {
+        let refresh_result = {
             let mgr = auth_mgr.lock().await;
-            mgr.refresh_token().await.is_ok()
+            mgr.refresh_token().await
         };
 
-        if refresh_ok {
-            tracing::info!(
-                server = self.server_name.as_str(),
-                "Token refreshed successfully (no browser)"
-            );
-            self.replace_state(ClientState::Pending(PendingTransport::HttpAuth {
-                config: config.clone(),
-                auth_manager: auth_mgr.clone(),
-            }))
-            .await;
-            return true;
+        match refresh_result {
+            Ok(_) => {
+                tracing::info!(
+                    server = self.server_name.as_str(),
+                    "Token refreshed successfully (no browser)"
+                );
+                self.replace_state(ClientState::Pending(PendingTransport::HttpAuth {
+                    config: config.clone(),
+                    auth_manager: auth_mgr.clone(),
+                }))
+                .await;
+                return true;
+            }
+            // Transient (network never reached the IdP): fail the attempt
+            // instead of discarding a presumed-good credential — the retry
+            // paths re-run the refresh once the network is back. An explicit
+            // user trigger (`force`) still opens the browser.
+            Err(ref e) if !force && mcp_refresh_failure_is_transient(e) => {
+                tracing::warn!(
+                    server = self.server_name.as_str(),
+                    error = %e,
+                    "Token refresh failed transiently (network); skipping browser escalation"
+                );
+                return false;
+            }
+            Err(e) => {
+                tracing::info!(
+                    server = self.server_name.as_str(),
+                    error = %e,
+                    "Token refresh failed terminally; falling back to browser auth"
+                );
+            }
         }
 
         // Full browser-based OAuth flow.
@@ -3416,12 +3475,11 @@ impl McpClient {
                     }
                 }
                 ensure_figma_user_agent(&mut headers, name, &config.url);
-                let http_client = reqwest::Client::builder()
-                    .default_headers(headers)
-                    .build()
-                    .map_err(|e| {
-                        McpError::ClientError(format!("Failed to build HTTP client: {e}"))
-                    })?;
+                let http_client = with_extra_root_certificates(
+                    reqwest::Client::builder().default_headers(headers),
+                )
+                .build()
+                .map_err(|e| McpError::ClientError(format!("Failed to build HTTP client: {e}")))?;
                 // `AuthClient::new` wants an owned manager, but ours is shared
                 // (`Arc`) with the OAuth flow; the struct is non_exhaustive, so
                 // build with a throwaway manager and swap in the shared one.
@@ -3626,10 +3684,10 @@ impl McpClient {
             }
         }
         ensure_figma_user_agent(&mut headers, server_name, &config.url);
-        let client = reqwest::Client::builder()
-            .default_headers(headers)
-            .build()
-            .map_err(|e| McpError::ClientError(format!("Failed to build HTTP client: {e}")))?;
+        let client =
+            with_extra_root_certificates(reqwest::Client::builder().default_headers(headers))
+                .build()
+                .map_err(|e| McpError::ClientError(format!("Failed to build HTTP client: {e}")))?;
         let mcp_http_client =
             crate::mcp_http_client::McpHttpClient::new(client, server_name, warn_budget);
         let transport_config = StreamableHttpClientTransportConfig::with_uri(config.url.as_str());