Synced from monorepo

Changes:
- Gate session-lifecycle heap steady state with a dhat soak
- Unbreak merge lifecycle e2e after default model → grok-4.5
- Scan home-scope rules dirs at <root>/rules
- Complete text-input paste and terminal parity
- Gate project roles and personas
- Use canonical editing in dialogs
- Use canonical editing in search bars
- Reject ambiguous MCP tool IDs
- Harden Git operands for plugins
- Simplify queue drain API
- Pass RFC 9207 iss through MCP OAuth token exchange
- Show leader roster when local agents map is empty
- Use canonical editing in Persona views
- Remove marketplace default-skills auto-install and purge old installs
- Use canonical editing in extension forms
- Add canonical dashboard text editing
- Use canonical editing in settings
- Add /summarize as a /recap alias
- Restore previous agent when exiting dashboard
- Use tool_choice auto for compaction
- Settings toggle for snap-prompt-to-top on send
- Update default models to grok-4.5
- Source login shell once for local bash (env + alias/function snapshot)
- Template hardcoded param names in server-native tool descriptions
- Fix System-Reminder XML tag injection in CLAUDE.md via agents_md
- Fix remote workspace-server hardcoding LSP trust (repo code execution risk)
- Clear orphaned tool-call updates at turn end
- Suppress task wake after cancel
- Send x-grok-client-identifier on direct API tool calls
- Harden dashboard peek lease transitions
- Host /btw side panel in live region (minimal mode)
- Bound scroll presentation latency
- Highlight multi-line constructs correctly in diffs and the file viewer
- Block web_fetch non-public IPs; local opt-in is explicit-host only
- Seed coding_data_retention_opt_out=false for OAuth e2es in pty-harness
- Follow up clipboard delivery feedback
- Use canonical editing in pickers
- Route TextArea through canonical editor
- Persistent "watching" status row; quieter turn markers
- Gate sensitive edit targets
- Expose agent registry counts and gate session churn on them
- Default coding data sharing to opt-out until server preference applies
- Wire chat attachment ids through gateway prompts
- On auth refresh failure, issue retry
- Forward preview provenance and computer lifecycle state
- Document independent privacy controls and scope /privacy output
- Strip SamplingError Display prefix on rate-limit UI copy
- Stop dumping Cloudflare HTML into Retry failed
- Disable in-place prompt edit (scroll jank on enter)
- Strip forced ANSI color from gh pr view JSON
- Plumb bash tool description onto ToolUsageCard wire

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

@@ -412,15 +412,20 @@ async fn run_browser_auth_flow(
     tokio::select! {
         result = callback_rx => {
             callback_server.abort();
-            let (code, csrf_state) = result
+            let callback = result
                 .map_err(|_| "Callback channel dropped".to_string())?
                 .map_err(|e| format!("OAuth callback failed: {e}"))?;
 
             // 6. Exchange code for tokens (auto-persists via CredentialStore).
+            // Pass RFC 9207 `iss` when present (required if the AS advertises it).
             let mgr = auth_manager.lock().await;
-            mgr.exchange_code_for_token(&code, &csrf_state)
-                .await
-                .map_err(|e| format!("Token exchange failed: {e}"))?;
+            mgr.exchange_code_for_token_with_issuer(
+                &callback.code,
+                &callback.state,
+                callback.issuer.as_deref(),
+            )
+            .await
+            .map_err(|e| format!("Token exchange failed: {e}"))?;
 
             tracing::info!(server = server_name, "MCP OAuth authentication successful");
         }
@@ -444,22 +449,54 @@ fn html_escape(s: &str) -> String {
         .replace('\'', "'")
 }
 
-/// Start a loopback HTTP server for the OAuth callback.
-///
-/// Returns the server task handle (for cleanup) and a oneshot receiver
-/// that resolves with `(code, state)` when the callback arrives.
-///
-/// The caller is responsible for aborting the server handle.
+#[derive(Debug, Clone, PartialEq, Eq)]
+struct OAuthCallbackPayload {
+    code: String,
+    state: String,
+    /// RFC 9207 `iss` (optional; required when the AS advertises support).
+    issuer: Option<String>,
+}
+
+fn parse_oauth_callback_params(
+    params: &HashMap<String, String>,
+) -> Result<OAuthCallbackPayload, String> {
+    if let Some(error) = params.get("error") {
+        let desc = params
+            .get("error_description")
+            .cloned()
+            .unwrap_or_else(|| "Unknown error".to_string());
+        return Err(format!("OAuth error: {error} - {desc}"));
+    }
+    let code = params
+        .get("code")
+        .filter(|s| !s.is_empty())
+        .cloned()
+        .ok_or_else(|| "Missing authorization code".to_string())?;
+    let state = params
+        .get("state")
+        .filter(|s| !s.is_empty())
+        .cloned()
+        .ok_or_else(|| "Missing state parameter".to_string())?;
+    let issuer = params.get("iss").cloned();
+    Ok(OAuthCallbackPayload {
+        code,
+        state,
+        issuer,
+    })
+}
+
+/// Loopback OAuth callback server. Returns (server task, oneshot for payload).
+/// Caller must abort the server task.
 #[allow(clippy::type_complexity)]
 fn start_oauth_callback_server(
     listener: tokio::net::TcpListener,
 ) -> (
     tokio::task::JoinHandle<()>,
-    oneshot::Receiver<Result<(String, String), String>>,
+    oneshot::Receiver<Result<OAuthCallbackPayload, String>>,
 ) {
     use axum::{Router, extract::Query, response::Html, routing::get};
 
-    let (tx, rx) = oneshot::channel::<Result<(String, String), String>>();
+    let (tx, rx) = oneshot::channel::<Result<OAuthCallbackPayload, String>>();
     let tx = Arc::new(tokio::sync::Mutex::new(Some(tx)));
 
     let handler = {
@@ -467,19 +504,7 @@ fn start_oauth_callback_server(
         move |Query(params): Query<HashMap<String, String>>| {
             let tx = tx.clone();
             async move {
-                let result = if let Some(error) = params.get("error") {
-                    let desc = params
-                        .get("error_description")
-                        .cloned()
-                        .unwrap_or_else(|| "Unknown error".to_string());
-                    Err(format!("OAuth error: {error} - {desc}"))
-                } else {
-                    match (params.get("code"), params.get("state")) {
-                        (Some(code), Some(state)) => Ok((code.clone(), state.clone())),
-                        (None, _) => Err("Missing authorization code".to_string()),
-                        (_, None) => Err("Missing state parameter".to_string()),
-                    }
-                };
+                let result = parse_oauth_callback_params(&params);
 
                 let html = match &result {
                     Ok(_) => {
@@ -520,3 +545,164 @@ fn start_oauth_callback_server(
 
     (server, rx)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::rmcp::transport::auth::{
+        AuthorizationManager, AuthorizationMetadata, OAuthClientConfig,
+    };
+
+    const TEST_ISSUER: &str = "https://auth.example.com";
+
+    fn params(pairs: &[(&str, &str)]) -> HashMap<String, String> {
+        pairs
+            .iter()
+            .map(|(k, v)| ((*k).to_string(), (*v).to_string()))
+            .collect()
+    }
+
+    #[test]
+    fn callback_parses_code_state_and_rfc9207_iss() {
+        let p = params(&[
+            ("code", "auth-code"),
+            ("state", "csrf"),
+            ("iss", TEST_ISSUER),
+        ]);
+        let got = parse_oauth_callback_params(&p).unwrap();
+        assert_eq!(got.code, "auth-code");
+        assert_eq!(got.state, "csrf");
+        assert_eq!(got.issuer.as_deref(), Some(TEST_ISSUER));
+    }
+
+    #[test]
+    fn callback_issuer_optional_for_legacy_servers() {
+        let p = params(&[("code", "c"), ("state", "s")]);
+        let got = parse_oauth_callback_params(&p).unwrap();
+        assert!(got.issuer.is_none());
+    }
+
+    #[test]
+    fn callback_requires_code_and_state() {
+        assert!(parse_oauth_callback_params(&params(&[("state", "s")])).is_err());
+        assert!(parse_oauth_callback_params(&params(&[("code", "c")])).is_err());
+    }
+
+    #[test]
+    fn callback_surfaces_oauth_error() {
+        let p = params(&[
+            ("error", "access_denied"),
+            ("error_description", "user said no"),
+        ]);
+        let err = parse_oauth_callback_params(&p).unwrap_err();
+        assert!(err.contains("access_denied"));
+        assert!(err.contains("user said no"));
+    }
+
+    fn require_iss_metadata(token_endpoint: String) -> AuthorizationMetadata {
+        // non_exhaustive: build via Default.
+        let mut meta = AuthorizationMetadata::default();
+        meta.authorization_endpoint = "https://auth.example.com/authorize".to_string();
+        meta.token_endpoint = token_endpoint;
+        meta.issuer = Some(TEST_ISSUER.to_string());
+        meta.additional_fields.insert(
+            "authorization_response_iss_parameter_supported".to_string(),
+            serde_json::json!(true),
+        );
+        meta
+    }
+
+    async fn manager_ready_for_exchange(token_endpoint: String) -> (AuthorizationManager, String) {
+        let mut mgr = AuthorizationManager::new("http://localhost/mcp")
+            .await
+            .unwrap();
+        mgr.set_metadata(require_iss_metadata(token_endpoint));
+        mgr.configure_client(
+            OAuthClientConfig::new("grok-test-client", "http://127.0.0.1:0/callback")
+                .with_application_type("native"),
+        )
+        .unwrap();
+        let auth_url = mgr.get_authorization_url(&[]).await.unwrap();
+        let state = url::Url::parse(&auth_url)
+            .unwrap()
+            .query_pairs()
+            .find(|(k, _)| k == "state")
+            .expect("auth URL must include state")
+            .1
+            .into_owned();
+        (mgr, state)
+    }
+
+    async fn start_mock_token_endpoint() -> String {
+        use axum::{Router, body::Body, http::Response, routing::post};
+        let app = Router::new().route(
+            "/token",
+            post(|| async {
+                Response::builder()
+                    .status(200)
+                    .header("content-type", "application/json")
+                    .body(Body::from(
+                        r#"{"access_token":"at-ok","token_type":"Bearer","expires_in":3600,"refresh_token":"rt-ok"}"#,
+                    ))
+                    .unwrap()
+            }),
+        );
+        let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap();
+        let addr = listener.local_addr().unwrap();
+        tokio::spawn(async move {
+            axum::serve(listener, app).await.unwrap();
+        });
+        format!("http://{addr}/token")
+    }
+
+    #[tokio::test]
+    async fn after_fix_passes_iss_and_token_exchange_succeeds() {
+        let token_ep = start_mock_token_endpoint().await;
+        let (mgr, state) = manager_ready_for_exchange(token_ep).await;
+
+        let callback = parse_oauth_callback_params(&params(&[
+            ("code", "auth-code"),
+            ("state", &state),
+            ("iss", TEST_ISSUER),
+        ]))
+        .unwrap();
+
+        let token = mgr
+            .exchange_code_for_token_with_issuer(
+                &callback.code,
+                &callback.state,
+                callback.issuer.as_deref(),
+            )
+            .await
+            .expect("with_issuer must succeed when callback iss matches AS");
+
+        use oauth2::TokenResponse as _;
+        assert_eq!(token.access_token().secret(), "at-ok");
+    }
+
+    #[tokio::test]
+    async fn callback_http_server_forwards_iss_query_param() {
+        let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap();
+        let addr = listener.local_addr().unwrap();
+        let (server, rx) = start_oauth_callback_server(listener);
+
+        let url = format!(
+            "http://{addr}/callback?code=c1&state=s1&iss={}",
+            urlencoding_encode(TEST_ISSUER)
+        );
+        let resp = reqwest::get(&url).await.unwrap();
+        assert!(resp.status().is_success());
+        let body = resp.text().await.unwrap();
+        assert!(body.contains("Authorization Complete"));
+
+        let payload = rx.await.unwrap().unwrap();
+        assert_eq!(payload.code, "c1");
+        assert_eq!(payload.state, "s1");
+        assert_eq!(payload.issuer.as_deref(), Some(TEST_ISSUER));
+        server.abort();
+    }
+
+    fn urlencoding_encode(s: &str) -> String {
+        s.replace(':', "%3A").replace('/', "%2F")
+    }
+}

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

@@ -1044,15 +1044,31 @@ pub fn parse_mcp_meta_config(
 /// here so existing call sites continue to work.
 pub use xai_grok_telemetry::enums::McpInitStrategy;
 
-/// Parse MCP tool name in format "server__tool"
-/// Returns (server_name, tool_name) if valid MCP tool, None otherwise
-pub fn parse_mcp_tool_name(name: &str) -> Option<(String, String)> {
-    let parts: Vec<&str> = name.splitn(2, MCP_TOOL_NAME_DELIMITER).collect();
-    if parts.len() == 2 {
-        Some((parts[0].to_string(), parts[1].to_string()))
-    } else {
-        None
+/// Parse a non-empty `server__tool` ID with one overlap-aware delimiter and
+/// valid [`xai_tool_protocol::ToolId`] syntax.
+pub fn parse_mcp_qualified_name(name: &str) -> Option<(xai_tool_protocol::ToolId, &str, &str)> {
+    let delimiter = MCP_TOOL_NAME_DELIMITER.as_bytes();
+    // Byte windows preserve both overlapping `__` boundaries in `___`.
+    let mut boundaries = name
+        .as_bytes()
+        .windows(delimiter.len())
+        .enumerate()
+        .filter_map(|(index, window)| (window == delimiter).then_some(index));
+    let boundary = boundaries.next()?;
+    if boundaries.next().is_some() {
+        return None;
     }
+    let (server, tool_with_delimiter) = name.split_at(boundary);
+    let tool = &tool_with_delimiter[MCP_TOOL_NAME_DELIMITER.len()..];
+    if server.is_empty() || tool.is_empty() {
+        return None;
+    }
+    Some((xai_tool_protocol::ToolId::new(name).ok()?, server, tool))
+}
+
+/// Parse an MCP tool name in `server__tool` format into owned segments.
+pub fn parse_mcp_tool_name(name: &str) -> Option<(String, String)> {
+    parse_mcp_qualified_name(name).map(|(_, server, tool)| (server.to_owned(), tool.to_owned()))
 }
 
 #[derive(Debug, thiserror::Error)]
@@ -1237,36 +1253,24 @@ impl McpTool {
 
     /// Convert into the data needed for `ToolBridge::register_erased()`.
     ///
-    /// Returns `None` if the tool name is invalid (doesn't match LLM API requirements).
-    /// Invalid tools are logged and skipped — fix the upstream connector.
-    ///
-    /// Also rejects qualified names that contain the delimiter
-    /// (`MCP_TOOL_NAME_DELIMITER`) more than once. The underlying tool-name
-    /// regex permits underscores in each segment, so a server like
-    /// `"foo__bar"`, a tool like `"my__thing"`, or even a `"foo_"`/`"_bar"`
-    /// pair (which concatenates to `"foo___bar"` — two valid `__`
-    /// positions) would produce a qualified name that downstream
-    /// `split_once("__")` consumers would split at the wrong boundary.
-    /// The "exactly one delimiter" check covers all three cases with a
-    /// single rule.
+    /// Invalid or ambiguous qualified IDs and provider-invalid names are logged
+    /// and skipped; the upstream connector must provide non-empty `server` and
+    /// `tool` segments separated by exactly one `__` boundary.
     pub fn into_registration(self) -> Option<McpToolRegistration> {
-        // Qualify MCP tool name with server name: "server__tool"
         let qualified_name = format!(
             "{}{}{}",
             self.server_name, MCP_TOOL_NAME_DELIMITER, self.name
         );
 
-        // Reject ambiguous qualified names — see doc-comment above.
-        if qualified_name.matches(MCP_TOOL_NAME_DELIMITER).count() != 1 {
+        if parse_mcp_qualified_name(&qualified_name).is_none() {
             tracing::error!(
                 server = %self.server_name,
                 tool = %self.name,
                 qualified = %qualified_name,
-                "Skipping MCP tool: qualified name contains '{MCP_TOOL_NAME_DELIMITER}' more than once (server, tool, or their boundary collides with the reserved delimiter)"
+                "Skipping MCP tool with invalid or ambiguous qualified name"
             );
             return None;
         }
-
         if let Err(reason) = validate_tool_name(&qualified_name) {
             tracing::error!(
                 tool_name = %qualified_name,
@@ -5720,28 +5724,83 @@ mod tests {
     }
 
     #[test]
-    fn into_registration_accepts_well_formed_segments() {
-        // Positive guard: the count check rejects `__`-anywhere-but-the-delimiter
-        // names but must not reject legitimate ones. If the rejection rule is
-        // ever tightened too far, this test breaks before any of the negative
-        // cases below.
-        let tool = make_mcp_tool("linear", "list_issues");
-        let reg = tool.into_registration().expect("should register");
-        assert_eq!(reg.name, "linear__list_issues");
+    fn qualified_mcp_name_parser_accepts_structurally_valid_tool_ids() {
+        for (name, expected) in [
+            ("linear__list_issues", ("linear", "list_issues")),
+            ("123__lookup", ("123", "lookup")),
+            ("server:scope__tool", ("server:scope", "tool")),
+        ] {
+            let (id, server, tool) = parse_mcp_qualified_name(name).expect("valid qualified ID");
+            assert_eq!(id.as_str(), name);
+            assert_eq!((server, tool), expected);
+            assert_eq!(
+                parse_mcp_tool_name(name),
+                Some((expected.0.to_owned(), expected.1.to_owned()))
+            );
+        }
     }
 
     #[test]
-    fn into_registration_rejects_boundary_ambiguity() {
-        // `"foo_"` + `"__"` + `"_bar"` => `"foo___bar"` has two valid
-        // `__` positions (indices 3 and 4), so `split_once("__")` would
-        // misparse it as `("foo", "_bar")` and silently auto-allow a
-        // future legitimate `"foo"` server. The naïve per-segment check
-        // (each side individually has no `__`) misses this — the count
-        // check catches it. Same rule also rejects "__-in-segment" cases
-        // (`"weird__server"` + `"list"`, `"linear"` + `"my__weird__tool"`)
-        // which are covered by the same `count() != 1` line of code.
-        let tool = make_mcp_tool("foo_", "_bar");
-        assert!(tool.into_registration().is_none());
+    fn qualified_mcp_name_parser_rejects_malformed_names() {
+        for name in [
+            "server__part__tool",
+            "server__tool__part",
+            "foo___bar",
+            "foo____bar",
+            "__tool",
+            "server__",
+            "server",
+            "",
+            "server__bad.tool",
+        ] {
+            assert!(
+                parse_mcp_qualified_name(name).is_none(),
+                "unexpectedly accepted {name:?}"
+            );
+        }
+    }
+
+    #[test]
+    fn into_registration_validates_qualified_name() {
+        let registration = make_mcp_tool("linear", "list_issues")
+            .into_registration()
+            .expect("should register");
+        assert_eq!(registration.name, "linear__list_issues");
+
+        for (server, tool) in [
+            ("server__part", "tool"),
+            ("server", "tool__part"),
+            ("foo_", "bar"),
+            ("foo", "_bar"),
+            ("foo_", "_bar"),
+            ("", "tool"),
+            ("server", ""),
+        ] {
+            assert!(
+                make_mcp_tool(server, tool).into_registration().is_none(),
+                "unexpectedly registered {server:?} and {tool:?}"
+            );
+        }
+    }
+
+    #[test]
+    fn into_registration_preserves_provider_name_policy() {
+        for qualified in ["123__lookup", "server:scope__tool"] {
+            assert!(parse_mcp_qualified_name(qualified).is_some());
+            let (server, tool) = qualified.split_once("__").unwrap();
+            assert!(make_mcp_tool(server, tool).into_registration().is_none());
+        }
+
+        let server_61 = format!("a{}", "b".repeat(60));
+        let server_62 = format!("a{}", "b".repeat(61));
+        let valid_64 = format!("{server_61}__b");
+        let invalid_65 = format!("{server_62}__b");
+        assert_eq!(valid_64.len(), 64);
+        assert_eq!(invalid_65.len(), 65);
+        assert!(parse_mcp_qualified_name(&valid_64).is_some());
+        assert!(parse_mcp_qualified_name(&invalid_65).is_some());
+        assert!(make_mcp_tool(&server_61, "b").into_registration().is_some());
+        assert!(make_mcp_tool(&server_62, "b").into_registration().is_none());
     }
 
     // ── is_retriable_transport_error tests ───────────────────────────