diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index f9e7bf1ed8..f9636f4483 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -708,9 +708,26 @@ pub(crate) fn normalize_agent_command_identity(command: &str) -> String { .collect() } +const GROK_DEFAULT_MODEL: &str = "grok-4.6"; + +fn grok_agent_args(model: &str) -> Vec { + vec![ + "agent".to_string(), + "--model".to_string(), + model.to_string(), + "stdio".to_string(), + ] +} + fn default_agent_args(command: &str) -> Option> { match normalize_agent_command_identity(command).as_str() { "goose" => Some(vec!["acp".to_string()]), + // Grok Build speaks ACP directly from its CLI: + // `grok agent --model stdio`. The model defaults to the + // subscription-backed grok-4.6 release; an explicit model (from the + // managed-agent config) is substituted as a discrete argument by + // `effective_agent_args` below — never shell-interpolated. + "grok" => Some(grok_agent_args(GROK_DEFAULT_MODEL)), "codex" | "codex-acp" | "claude-agent-acp" | "claude-code-acp" | "claude-code" | "claudecode" | "buzz-agent" => Some(Vec::new()), _ => None, @@ -789,6 +806,43 @@ pub fn codex_network_env(agent_command: &str, relay_url: &str) -> Option<(String )) } +/// Build the effective agent argv for a managed agent launch. +/// +/// For Grok Build the managed model is part of the ACP argv +/// (`grok agent --model stdio`), so it is substituted here as a +/// discrete argument from the resolved model — never via a shell string. +/// A non-empty model is required; an absent model falls back to the pinned +/// subscription default `grok-4.6`. Explicitly supplied `agent_args` for +/// Grok are normalized and used as-is (they win over the default). +/// +/// Every other runtime keeps the existing [`normalize_agent_args`] behaviour. +pub fn effective_agent_args( + command: &str, + agent_args: Vec, + model: Option<&str>, +) -> Result, String> { + if normalize_agent_command_identity(command).as_str() != "grok" { + return Ok(normalize_agent_args(command, agent_args)); + } + let normalized = agent_args + .into_iter() + .map(|arg| arg.trim().to_string()) + .filter(|arg| !arg.is_empty()) + .collect::>(); + // Clap's historical cross-runtime default is a single `acp` argument. + // That is not a valid Grok launch override and must not shadow the managed + // model. Real operator overrides provide Grok's full argv and still win. + let is_legacy_acp_default = normalized.len() == 1 && normalized[0].eq_ignore_ascii_case("acp"); + if !normalized.is_empty() && !is_legacy_acp_default { + return Ok(normalized); + } + let resolved_model = model.unwrap_or(GROK_DEFAULT_MODEL); + if resolved_model.trim().is_empty() { + return Err("grok agent model must be a non-empty string".to_string()); + } + Ok(grok_agent_args(resolved_model.trim())) +} + pub fn normalize_agent_args(command: &str, agent_args: Vec) -> Vec { let normalized = agent_args .into_iter() @@ -923,7 +977,9 @@ impl Config { )); } - let agent_args = normalize_agent_args(&agent_command, args.agent_args); + let agent_args = + effective_agent_args(&agent_command, args.agent_args, args.model.as_deref()) + .map_err(ConfigError::ConfigFile)?; if let Some(ref channels) = args.channels { for ch in channels { @@ -1590,6 +1646,103 @@ mod tests { ); } + #[test] + fn grok_default_args_are_the_subscription_acp_argv() { + // Pinned subscription-backed contract: `grok agent --model grok-4.6 stdio`. + assert_eq!( + normalize_agent_args("grok", Vec::new()), + vec![ + "agent".to_string(), + "--model".to_string(), + "grok-4.6".to_string(), + "stdio".to_string() + ] + ); + } + + #[test] + fn grok_effective_args_substitute_the_managed_model_as_discrete_arg() { + assert_eq!( + effective_agent_args("grok", Vec::new(), Some("grok-4.6")).unwrap(), + vec![ + "agent".to_string(), + "--model".to_string(), + "grok-4.6".to_string(), + "stdio".to_string() + ] + ); + // Absent model falls back to the pinned default. + assert_eq!( + effective_agent_args("grok", Vec::new(), None).unwrap(), + vec![ + "agent".to_string(), + "--model".to_string(), + "grok-4.6".to_string(), + "stdio".to_string() + ] + ); + // Clap's legacy cross-runtime default must not shadow the Grok model. + assert_eq!( + effective_agent_args("grok", vec!["acp".into()], Some("grok-5")).unwrap(), + vec![ + "agent".to_string(), + "--model".to_string(), + "grok-5".to_string(), + "stdio".to_string() + ] + ); + } + + #[test] + fn grok_fails_closed_on_empty_model_and_windows_stem_normalization() { + assert!(effective_agent_args("grok", Vec::new(), Some(" ")).is_err()); + // `grok.exe` normalizes to the same identity as `grok`. + assert_eq!( + effective_agent_args("grok.exe", Vec::new(), Some("grok-4.6")).unwrap(), + vec![ + "agent".to_string(), + "--model".to_string(), + "grok-4.6".to_string(), + "stdio".to_string() + ] + ); + } + + #[test] + fn grok_explicit_agent_args_win_over_the_default_argv() { + assert_eq!( + effective_agent_args( + "grok", + vec![ + "agent".into(), + "--model".into(), + "grok-5".into(), + "stdio".into() + ], + Some("grok-4.6") + ) + .unwrap(), + vec![ + "agent".to_string(), + "--model".to_string(), + "grok-5".to_string(), + "stdio".to_string() + ] + ); + } + + #[test] + fn non_grok_runtimes_keep_existing_args_behavior() { + assert_eq!( + effective_agent_args("goose", Vec::new(), Some("whatever")).unwrap(), + vec!["acp".to_string()] + ); + assert_eq!( + effective_agent_args("codex-acp", Vec::new(), None).unwrap(), + Vec::::new() + ); + } + #[test] fn preserves_explicit_nonempty_agent_args() { assert_eq!( @@ -2841,6 +2994,113 @@ channels = "ALL" ); } + // --- Grok managed-model bridge regression (PR #5742 fix) --- + // + // Desktop emits BUZZ_ACP_AGENT_COMMAND=grok, BUZZ_ACP_AGENT_ARGS (empty for + // the catalog default) and BUZZ_ACP_MODEL=; clap maps those + // env vars onto the same CliArgs fields these flags drive, so Config::from_args + // here exercises the identical resolution chain: desktop env → clap → + // effective_agent_args → real child argv. + + #[test] + fn grok_managed_model_reaches_child_argv_end_to_end() { + // Catalog default path: no --agent-args (desktop emits none for grok), + // managed model grok-4.6 → exact subscription argv. + let args = CliArgs::try_parse_from([ + "buzz-acp", + "--private-key", + TEST_PRIVATE_KEY, + "--agent-command", + "grok", + "--model", + "grok-4.6", + ]) + .expect("clap should parse args"); + let config = Config::from_args(args).expect("from_args should succeed"); + assert_eq!( + config.agent_args, + vec![ + "agent".to_string(), + "--model".to_string(), + "grok-4.6".to_string(), + "stdio".to_string() + ] + ); + } + + #[test] + fn grok_alternate_managed_model_is_not_shadowed() { + // A non-default managed model must reach the argv — the desktop emits no + // default agent_args, so the model drives the child command. + let args = CliArgs::try_parse_from([ + "buzz-acp", + "--private-key", + TEST_PRIVATE_KEY, + "--agent-command", + "grok", + "--model", + "grok-5", + ]) + .expect("clap should parse args"); + let config = Config::from_args(args).expect("from_args should succeed"); + assert_eq!( + config.agent_args, + vec![ + "agent".to_string(), + "--model".to_string(), + "grok-5".to_string(), + "stdio".to_string() + ] + ); + } + + #[test] + fn grok_blank_model_fails_closed() { + let args = CliArgs::try_parse_from([ + "buzz-acp", + "--private-key", + TEST_PRIVATE_KEY, + "--agent-command", + "grok", + "--model", + " ", + ]) + .expect("clap should parse args"); + let result = Config::from_args(args); + assert!( + result.is_err(), + "from_args should reject a blank grok model: {result:?}" + ); + } + + #[test] + fn grok_explicit_agent_args_override_wins() { + // An operator-supplied explicit --agent-args is distinguishable from the + // generated catalog default (which the desktop now omits) and wins. + let args = CliArgs::try_parse_from([ + "buzz-acp", + "--private-key", + TEST_PRIVATE_KEY, + "--agent-command", + "grok", + "--model", + "grok-5", + "--agent-args", + "agent,--model,grok-9,stdio", + ]) + .expect("clap should parse args"); + let config = Config::from_args(args).expect("from_args should succeed"); + assert_eq!( + config.agent_args, + vec![ + "agent".to_string(), + "--model".to_string(), + "grok-9".to_string(), + "stdio".to_string() + ] + ); + } + // --- max_turn_duration ceiling gate --- #[test] diff --git a/desktop/src-tauri/src/managed_agents/discovery.rs b/desktop/src-tauri/src/managed_agents/discovery.rs index bc0e3a6cda..6921657cba 100644 --- a/desktop/src-tauri/src/managed_agents/discovery.rs +++ b/desktop/src-tauri/src/managed_agents/discovery.rs @@ -23,6 +23,8 @@ pub(crate) use runtime_metadata::KnownAcpRuntime; const GOOSE_AVATAR_URL: &str = "https://goose-docs.ai/img/logo_dark.png"; const CLAUDE_CODE_AVATAR_URL: &str = "https://anthropic.gallerycdn.vsassets.io/extensions/anthropic/claude-code/2.1.77/1773707456892/Microsoft.VisualStudio.Services.Icons.Default"; const CODEX_AVATAR_URL: &str = "https://openai.gallerycdn.vsassets.io/extensions/openai/chatgpt/26.5313.41514/1773706730621/Microsoft.VisualStudio.Services.Icons.Default"; +// Generic x.ai pointer; a dedicated Grok Build logo asset URL is a follow-up. +const GROK_AVATAR_URL: &str = "https://x.ai"; const BUZZ_AGENT_AVATAR_URL: &str = "https://raw.githubusercontent.com/block/buzz/refs/heads/main/crates/buzz-agent/buzz-agent.png"; fn common_binary_paths() -> &'static [PathBuf] { @@ -47,6 +49,8 @@ fn common_binary_paths() -> &'static [PathBuf] { home.join(".volta/bin"), home.join(".asdf/shims"), home.join(".bun/bin"), + // Grok Build's official CLI installs to ~/.grok/bin/grok. + home.join(".grok/bin"), ]); } // Windows well-known dirs for npm global shims and standalone installer targets. @@ -211,6 +215,47 @@ const KNOWN_ACP_RUNTIMES: &[KnownAcpRuntime] = &[ login_hint: None, auth_probe_args: None, }, + KnownAcpRuntime { + id: "grok", + label: "Grok Build", + commands: &["grok"], + aliases: &[], + avatar_url: GROK_AVATAR_URL, + mcp_command: None, + mcp_hooks: false, + underlying_cli: Some("grok"), + cli_install_commands: &[], + cli_install_commands_windows: &[], + adapter_install_commands: &[], + cli_install_instructions_url: "https://x.ai", + adapter_install_instructions_url: "", + cli_install_hint: "Buzz talks to Grok through the official Grok Build CLI, which must be installed and logged into a grok.com subscription.", + adapter_install_hint: "", + skill_dir: None, + supports_acp_model_switching: false, + // Grok Build takes the model as an argv argument + // (`grok agent --model stdio`), not an env var. The desktop + // therefore emits NO default `agent_args` for Grok: buzz-acp's + // `effective_agent_args` builds the argv from the managed model + // (`BUZZ_ACP_MODEL`, default grok-4.6) when the args vector is empty. + // Explicit record `agent_args` remain an operator override that wins. + model_env_var: None, + provider_env_var: None, + provider_locked: true, + default_env: &[], + config_file_path: None, + config_file_format: None, + supports_acp_native_config: false, + thinking_env_var: None, + max_tokens_env_var: None, + context_limit_env_var: None, + max_rounds_env_var: None, + required_normalized_fields: &["model"], + login_hint: Some("Run the Grok Build CLI and log in to grok.com to use your subscription."), + // `grok models` prints an explicit auth status but exits 0 in both states; + // cli_probe classifies that stdout before applying generic exit-code logic. + auth_probe_args: Some(&["grok", "models"]), + }, ]; /// Skill discovery directories declared by known runtimes. @@ -443,8 +488,12 @@ pub fn try_record_agent_command( fn default_agent_args(command: &str) -> Option> { match normalize_command_identity(command).as_str() { "goose" => Some(vec!["acp".to_string()]), + // No default agent_args for Grok: emitting the pinned argv here would + // shadow the managed model (`BUZZ_ACP_MODEL`) at the harness — buzz-acp + // builds `agent --model stdio` from the model instead. Explicit + // record `agent_args` still pass through as an operator override. "codex" | "codex-acp" | "claude-agent-acp" | "claude-code-acp" | "claude-code" - | "claudecode" | "buzz-agent" => Some(Vec::new()), + | "claudecode" | "buzz-agent" | "grok" => Some(Vec::new()), _ => None, } } @@ -1035,6 +1084,7 @@ fn probe_auth_status(binary_path: &Path, probe_args: &[&str]) -> AuthStatus { if let Some(mut pipe) = stdout_pipe { let _ = pipe.read_to_end(&mut buf); } + buf }); let stderr_thread = std::thread::spawn(move || { let mut buf = Vec::new(); @@ -1086,10 +1136,10 @@ fn probe_auth_status(binary_path: &Path, probe_args: &[&str]) -> AuthStatus { }; let _ = wait_thread.join(); - let _ = stdout_thread.join(); + let stdout_bytes = stdout_thread.join().unwrap_or_default(); let stderr_bytes = stderr_thread.join().unwrap_or_default(); - match cli_probe::classify_probe_output(&stderr_bytes, exit_status.success()) { + match cli_probe::classify_probe_output(&stdout_bytes, &stderr_bytes, exit_status.success()) { cli_probe::ProbeOutcome::LoggedIn => AuthStatus::LoggedIn, cli_probe::ProbeOutcome::LoggedOut => AuthStatus::LoggedOut, cli_probe::ProbeOutcome::ConfigInvalid { stderr_excerpt } => AuthStatus::ConfigInvalid { diff --git a/desktop/src-tauri/src/managed_agents/discovery/runtime_metadata.rs b/desktop/src-tauri/src/managed_agents/discovery/runtime_metadata.rs index 34edecdcd9..49f90828b2 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/runtime_metadata.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/runtime_metadata.rs @@ -123,4 +123,46 @@ mod tests { assert!(codex.adapter_install_instructions_url.contains("codex-acp")); assert!(codex.cli_install_hint.contains("Codex CLI")); } + + #[test] + fn grok_metadata_is_a_direct_acp_runtime_with_locked_provider() { + let grok = known_acp_runtime_exact("grok").unwrap(); + assert_eq!(grok.commands, &["grok"]); + assert!(grok.provider_locked); + // The model travels in the argv (`grok agent --model stdio`), + // not an env var — no model/provider env bridge for Grok. + assert_eq!(grok.model_env_var, None); + assert_eq!(grok.provider_env_var, None); + assert_eq!(grok.required_normalized_fields, &["model"]); + assert_eq!(grok.auth_probe_args, Some(&["grok", "models"][..])); + } + + #[test] + fn grok_emits_no_default_args_and_passes_explicit_overrides() { + // The desktop must NOT emit a pinned default argv for Grok — that would + // shadow the managed model at the harness. Empty args mean buzz-acp + // builds `agent --model stdio` from `BUZZ_ACP_MODEL`. + assert_eq!( + super::normalize_agent_args("grok", Vec::new()), + Vec::::new() + ); + // Explicit record agent_args remain an operator override that wins. + assert_eq!( + super::normalize_agent_args( + "grok", + vec![ + "agent".to_string(), + "--model".to_string(), + "grok-5".to_string(), + "stdio".to_string() + ] + ), + vec![ + "agent".to_string(), + "--model".to_string(), + "grok-5".to_string(), + "stdio".to_string() + ] + ); + } } diff --git a/desktop/src-tauri/src/managed_agents/readiness/cli_probe.rs b/desktop/src-tauri/src/managed_agents/readiness/cli_probe.rs index 513da4e2a8..40fcb8b8aa 100644 --- a/desktop/src-tauri/src/managed_agents/readiness/cli_probe.rs +++ b/desktop/src-tauri/src/managed_agents/readiness/cli_probe.rs @@ -24,10 +24,10 @@ pub(crate) fn augmented_path() -> Option { /// Outcome of a CLI login-status probe. #[derive(Debug, PartialEq, Eq)] pub(crate) enum ProbeOutcome { - /// The CLI reported a successful login (exit 0). + /// The CLI reported a successful login. LoggedIn, - /// The CLI exited non-zero without a config-parse signal — treat as - /// "not authenticated." + /// The CLI explicitly reported a logged-out state, or exited non-zero + /// without a config-parse signal. LoggedOut, /// The CLI exited non-zero and its stderr contains a config-parse error /// (e.g. from `~/.codex/config.toml`). The user needs to fix their @@ -49,6 +49,10 @@ pub(crate) enum ProbeOutcome { /// one term. const CONFIG_PARSE_SIGNALS: &[&str] = &["error loading configuration", "unknown variant"]; +/// Some CLIs report an explicit logged-out state while still exiting successfully. +/// Grok Build's `grok models` command currently uses this exact message. +const LOGGED_OUT_OUTPUT_SIGNALS: &[&str] = &["you are not authenticated"]; + /// Run the probe at the resolved absolute path so the GUI-PATH gap is /// bypassed. Injects the same augmented PATH used for launched agents so /// script shims with `/usr/bin/env ` shebangs can find runtimes @@ -66,8 +70,7 @@ pub(crate) fn login_probe( crate::util::configure_no_window(&mut command); match command.output() { - Ok(o) if o.status.success() => ProbeOutcome::LoggedIn, - Ok(o) => classify_probe_output(&o.stderr, false), + Ok(o) => classify_probe_output(&o.stdout, &o.stderr, o.status.success()), Err(_) => ProbeOutcome::LoggedOut, } } @@ -75,30 +78,79 @@ pub(crate) fn login_probe( /// Classify collected probe output into a `ProbeOutcome`. /// /// Shared between `login_probe` (which has the full `Output`) and the -/// process-level timeout path in `probe_auth_status` (which drains stderr -/// on a background thread and collects it separately). -pub(crate) fn classify_probe_output(stderr_bytes: &[u8], exit_success: bool) -> ProbeOutcome { - if exit_success { - return ProbeOutcome::LoggedIn; - } +/// process-level timeout path in `probe_auth_status` (which drains both output +/// streams on background threads and collects them separately). +pub(crate) fn classify_probe_output( + stdout_bytes: &[u8], + stderr_bytes: &[u8], + exit_success: bool, +) -> ProbeOutcome { let stderr = String::from_utf8_lossy(stderr_bytes); let stderr_lower = stderr.to_lowercase(); - if CONFIG_PARSE_SIGNALS - .iter() - .all(|sig| stderr_lower.contains(sig)) + if !exit_success + && CONFIG_PARSE_SIGNALS + .iter() + .all(|sig| stderr_lower.contains(sig)) { let excerpt = stderr.trim().lines().next().unwrap_or("").to_string(); ProbeOutcome::ConfigInvalid { stderr_excerpt: excerpt, } } else { - ProbeOutcome::LoggedOut + let stdout = String::from_utf8_lossy(stdout_bytes); + let output_lower = format!("{stdout}\n{stderr}").to_lowercase(); + if LOGGED_OUT_OUTPUT_SIGNALS + .iter() + .any(|signal| output_lower.contains(signal)) + { + ProbeOutcome::LoggedOut + } else if exit_success { + ProbeOutcome::LoggedIn + } else { + ProbeOutcome::LoggedOut + } } } #[cfg(test)] mod tests { - use super::{ProbeOutcome, CONFIG_PARSE_SIGNALS}; + use super::{classify_probe_output, ProbeOutcome, CONFIG_PARSE_SIGNALS}; + + #[test] + fn successful_grok_models_output_can_report_logged_out() { + assert_eq!( + classify_probe_output(b"You are not authenticated.\n", b"", true), + ProbeOutcome::LoggedOut + ); + } + + #[test] + fn successful_grok_models_output_reports_logged_in() { + assert_eq!( + classify_probe_output(b"You are logged in with grok.com.\n", b"", true), + ProbeOutcome::LoggedIn + ); + } + + #[test] + fn generic_success_without_auth_markers_remains_logged_in() { + assert_eq!( + classify_probe_output(b"", b"", true), + ProbeOutcome::LoggedIn + ); + } + + #[test] + fn nonzero_config_error_takes_precedence_over_logged_out_marker() { + assert!(matches!( + super::classify_probe_output( + b"You are not authenticated.\n", + b"Error loading configuration: unknown variant `bad`", + false, + ), + ProbeOutcome::ConfigInvalid { .. } + )); + } #[cfg(unix)] #[test]