From dfd1e71e28288ba27033e0832983d6e65675ad4f Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Fri, 10 Jul 2026 19:50:39 -0400 Subject: [PATCH 1/2] fix(desktop): distinguish codex config-parse failures from login failures The codex login probe returned a bare bool, so a nonzero exit from a config-parse error (e.g. unknown variant `ultra` in config.toml) was indistinguishable from 'not authenticated'. The user was sent to run `codex login` for a problem login cannot fix. Changes: - cli_probe.rs: login_probe_succeeds -> login_probe() returning a three-way ProbeOutcome (LoggedIn | LoggedOut | ConfigInvalid). ConfigInvalid fires only when stderr contains BOTH 'error loading configuration' AND 'unknown variant' (the real codex parse-error format), preventing false positives from unrelated exits. - readiness.rs: new CliConfigInvalid Requirement variant carries the probe_args, setup_copy, and a one-line stderr excerpt (diagnostic). cli_login_requirements emits it on ConfigInvalid probe outcomes. - runtime.rs: serializes CliConfigInvalid to the setup payload JSON. - setup_mode.rs (buzz-acp): mirrors CliConfigInvalid in RequirementPayload with instruction() copy naming ~/.{cli}/config.toml. - configNudge.ts: adds cli_config_invalid to ConfigNudgeRequirement type and the isConfigNudgeRequirement type guard. - config-nudge-attachment.tsx: renders cli_config_invalid rows as informational only -- no Doctor or Edit Agent CTA (neither can repair an external config file). All-config-invalid cards are treated as informational-only (no trigger, no pointer affordance), same as auth-only cards. - check-file-sizes.mjs: bumps readiness.rs limit by +18 lines. AcpAvailabilityStatus is left as a flat unit enum so the runtime catalog type and wire shape are unaffected. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-acp/src/setup_mode.rs | 123 +++++++++++++- desktop/scripts/check-file-sizes.mjs | 5 +- .../src-tauri/src/managed_agents/readiness.rs | 46 ++++-- .../src/managed_agents/readiness/cli_probe.rs | 153 ++++++++++++++++-- .../src-tauri/src/managed_agents/runtime.rs | 10 ++ desktop/src/shared/lib/configNudge.ts | 19 +++ .../src/shared/ui/config-nudge-attachment.tsx | 51 +++++- 7 files changed, 371 insertions(+), 36 deletions(-) diff --git a/crates/buzz-acp/src/setup_mode.rs b/crates/buzz-acp/src/setup_mode.rs index 2f162d84f3..284f163fc0 100644 --- a/crates/buzz-acp/src/setup_mode.rs +++ b/crates/buzz-acp/src/setup_mode.rs @@ -103,6 +103,14 @@ pub(crate) enum RequirementPayload { /// missing and the probe was skipped. availability: AcpAvailabilityStatus, }, + /// The CLI is installed but its config file could not be parsed. + /// Informational only — no in-app action can fix an external config file. + CliConfigInvalid { + probe_args: Vec, + setup_copy: String, + /// One-line stderr excerpt identifying the parse error. + diagnostic: String, + }, } impl RequirementPayload { @@ -149,6 +157,18 @@ impl RequirementPayload { format!("install {} (open Doctor in Settings to diagnose)", harness) } }, + RequirementPayload::CliConfigInvalid { + probe_args, + diagnostic, + .. + } => { + let cli = probe_args.first().map(String::as_str).unwrap_or("the CLI"); + let config_file = format!("~/.{}/config.toml", cli); + format!( + "{} is invalid: {} — fix the config and restart the agent", + config_file, diagnostic + ) + } } } } @@ -213,10 +233,32 @@ impl SetupPayload { .map(|r| format!("- {}", r.instruction())) .collect(); + let all_external = self + .requirements + .iter() + .all(|r| matches!(r, RequirementPayload::CliConfigInvalid { .. })); + let any_external = self + .requirements + .iter() + .any(|r| matches!(r, RequirementPayload::CliConfigInvalid { .. })); + + let footer = if all_external { + // All requirements are external config files — Edit Agent cannot + // help. Don't send the user there. + "Fix the config file(s) and restart the agent.".to_string() + } else if any_external { + // Mixed: some Buzz-managed fields, some external config. + "Open Edit Agent in the Buzz app for the Buzz-managed fields; fix the external CLI config files manually and restart the agent.".to_string() + } else { + // All Buzz-managed — original footer unchanged. + "Open Edit Agent in the Buzz app to set these.".to_string() + }; + format!( - "**{}** needs configuration before it can respond:\n{}\n\nOpen Edit Agent in the Buzz app to set these.", + "**{}** needs configuration before it can respond:\n{}\n\n{}", self.agent_name, steps.join("\n"), + footer, ) }; @@ -678,6 +720,85 @@ mod tests { assert!(body.contains("needs configuration")); } + // ── config-invalid footer tests ──────────────────────────────────────────── + + fn make_cli_config_invalid(cli: &str, diagnostic: &str) -> RequirementPayload { + RequirementPayload::CliConfigInvalid { + probe_args: vec![cli.to_string()], + setup_copy: String::new(), + diagnostic: diagnostic.to_string(), + } + } + + #[test] + fn nudge_body_all_config_invalid_omits_edit_agent_footer() { + // An all-CliConfigInvalid requirements list must NOT send users to + // Edit Agent (which cannot fix an external config file). + let payload = SetupPayload { + agent_name: "Codex".to_string(), + agent_pubkey: "test".to_string(), + requirements: vec![make_cli_config_invalid( + "codex", + "unknown variant `ultra` for field `model_reasoning_effort`", + )], + }; + let body = payload.nudge_body(); + assert!( + !body.contains("Open Edit Agent"), + "all-config-invalid nudge must not mention Open Edit Agent; got: {body:?}" + ); + assert!( + body.contains("config.toml"), + "nudge must name the config file; got: {body:?}" + ); + assert!( + body.contains("restart the agent"), + "nudge must include restart guidance; got: {body:?}" + ); + } + + #[test] + fn nudge_body_mixed_requirements_uses_split_footer() { + // Mixed list: one Buzz-managed env key + one external config invalid. + // Footer must address both sides. + let payload = SetupPayload { + agent_name: "Codex".to_string(), + agent_pubkey: "test".to_string(), + requirements: vec![ + RequirementPayload::EnvKey { + key: "SOME_API_KEY".to_string(), + }, + make_cli_config_invalid("codex", "unknown variant `gpt-5.6-sol`"), + ], + }; + let body = payload.nudge_body(); + assert!( + body.contains("Open Edit Agent"), + "mixed nudge must mention Open Edit Agent for managed fields; got: {body:?}" + ); + assert!( + body.contains("fix the external CLI config"), + "mixed nudge must mention fixing the external config; got: {body:?}" + ); + } + + #[test] + fn nudge_body_all_buzz_managed_retains_original_footer() { + // Pure Buzz-managed requirements → original "Open Edit Agent" footer unchanged. + let payload = SetupPayload { + agent_name: "Fizz".to_string(), + agent_pubkey: "test".to_string(), + requirements: vec![RequirementPayload::EnvKey { + key: "ANTHROPIC_API_KEY".to_string(), + }], + }; + let body = payload.nudge_body(); + assert!( + body.contains("Open Edit Agent in the Buzz app to set these."), + "all-managed nudge must use the original Edit Agent footer; got: {body:?}" + ); + } + // ── sentinel block tests ─────────────────────────────────────────────────── #[test] diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 2c7196b3cf..c871c37a86 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -143,7 +143,10 @@ const overrides = new Map([ // +1 rebase merge: GlobalAgentConfig import added alongside AcpAvailabilityStatus. // +2 rebase onto #1667: behavioral quad fields in PersonaRecord/ManagedAgentRecord. // +3 rebase onto main (#1568 + #1613): identity-import-keyring + augmented-PATH probes. - ["src-tauri/src/managed_agents/readiness.rs", 1565], + // +18: CliConfigInvalid requirement surface for config-parse probe classification — + // new Requirement variant + updated cli_login_requirements + 3 new probe-layer tests. + // Load-bearing UX fix (bad config → clear diagnostic, not "run codex login"). + ["src-tauri/src/managed_agents/readiness.rs", 1583], // applyWorkspace reposDir parameter plus the validateReposDir binding, // threaded through Tauri invokes for configurable repos_dir, plus the // harness-persona-sync `harnessOverride` create-input bit — load-bearing diff --git a/desktop/src-tauri/src/managed_agents/readiness.rs b/desktop/src-tauri/src/managed_agents/readiness.rs index 3f67c49b5f..f7a1c16fb3 100644 --- a/desktop/src-tauri/src/managed_agents/readiness.rs +++ b/desktop/src-tauri/src/managed_agents/readiness.rs @@ -177,6 +177,19 @@ pub enum Requirement { /// and route to Doctor with accurate context. availability: AcpAvailabilityStatus, }, + /// The CLI is installed but its config file could not be parsed. + /// This is an informational surface only — there is no in-app destination + /// that can repair an external config file; the user must edit it manually. + CliConfigInvalid { + /// Arguments used in the probe (e.g. `["codex", "login", "status"]`); + /// `probe_args[0]` is the CLI name (e.g. `"codex"`). + probe_args: Vec, + /// Human-readable hint shown when no structured copy is available. + setup_copy: String, + /// A one-line excerpt from the CLI's stderr (the parse-error line). + /// Shown verbatim in the nudge so the user can identify the problem. + diagnostic: String, + }, } // ── AgentReadiness ──────────────────────────────────────────────────────────── @@ -505,20 +518,25 @@ fn cli_login_requirements( }; let augmented_path = cli_probe::augmented_path(); - let logged_in = cli_probe::login_probe_succeeds( - &binary_path, - probe_args, - augmented_path.as_deref(), - ); - - if logged_in { - vec![] - } else { - vec![Requirement::CliLogin { - probe_args: probe_args.iter().map(|s| s.to_string()).collect(), - setup_copy: setup_copy.to_string(), - availability: AcpAvailabilityStatus::Available, - }] + let outcome = + cli_probe::login_probe(&binary_path, probe_args, augmented_path.as_deref()); + + match outcome { + cli_probe::ProbeOutcome::LoggedIn => vec![], + cli_probe::ProbeOutcome::LoggedOut => { + vec![Requirement::CliLogin { + probe_args: probe_args.iter().map(|s| s.to_string()).collect(), + setup_copy: setup_copy.to_string(), + availability: AcpAvailabilityStatus::Available, + }] + } + cli_probe::ProbeOutcome::ConfigInvalid { stderr_excerpt } => { + vec![Requirement::CliConfigInvalid { + probe_args: probe_args.iter().map(|s| s.to_string()).collect(), + setup_copy: setup_copy.to_string(), + diagnostic: stderr_excerpt, + }] + } } } // Tooling is not fully installed — emit CliLogin with the precise 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 16b3389406..ea687918a7 100644 --- a/desktop/src-tauri/src/managed_agents/readiness/cli_probe.rs +++ b/desktop/src-tauri/src/managed_agents/readiness/cli_probe.rs @@ -12,28 +12,74 @@ pub(super) fn augmented_path() -> Option { ) } -pub(super) fn login_probe_succeeds( +/// Outcome of a CLI login-status probe. +#[derive(Debug, PartialEq, Eq)] +pub(super) enum ProbeOutcome { + /// The CLI reported a successful login (exit 0). + LoggedIn, + /// The CLI exited non-zero without a config-parse signal — treat as + /// "not authenticated." + 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 + /// config, not re-run login. + ConfigInvalid { + /// A trimmed excerpt of the stderr message to surface in the nudge. + stderr_excerpt: String, + }, +} + +/// Signals emitted to stderr by codex (and related CLI tools) when they +/// fail to parse their config file. We check these to distinguish a +/// config-parse failure from a genuine "not authenticated" exit. +/// +/// The real codex error reads: +/// `Error loading configuration: .../.codex/config.toml:... unknown variant ...` +/// So we require BOTH "error loading configuration" AND "unknown variant" to be +/// present, avoiding false positives from unrelated errors that mention only +/// one term. +const CONFIG_PARSE_SIGNALS: &[&str] = &["error loading configuration", "unknown variant"]; + +/// 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 +/// such as node/python when the app was launched with a bare GUI PATH. +pub(super) fn login_probe( binary_path: &Path, probe_args: &[&str], augmented_path: Option<&str>, -) -> bool { - // Run the probe at the resolved absolute path so the GUI-PATH gap is - // bypassed. Inject the same augmented PATH used for launched agents so - // script shims with `/usr/bin/env ` shebangs can find runtimes - // such as node/python when the app was launched with a bare GUI PATH. +) -> ProbeOutcome { let mut command = std::process::Command::new(binary_path); command.args(&probe_args[1..]); if let Some(path) = augmented_path { command.env("PATH", path); } - command - .output() - .map(|o| o.status.success()) - .unwrap_or(false) + + match command.output() { + Ok(o) if o.status.success() => ProbeOutcome::LoggedIn, + Ok(o) => { + let stderr = String::from_utf8_lossy(&o.stderr); + let stderr_lower = stderr.to_lowercase(); + if 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 + } + } + Err(_) => ProbeOutcome::LoggedOut, + } } #[cfg(test)] mod tests { + use super::{ProbeOutcome, CONFIG_PARSE_SIGNALS}; + #[cfg(unix)] #[test] fn login_probe_uses_augmented_path_for_env_shebang_interpreter() { @@ -83,12 +129,13 @@ mod tests { let augmented_path = std::env::join_paths([interpreter_dir.as_path()]).expect("join augmented PATH"); let augmented_path = augmented_path.to_string_lossy().into_owned(); - assert!( - super::login_probe_succeeds( + assert_eq!( + super::login_probe( &script_path, &["fake-codex", "login", "status"], Some(&augmented_path), ), + ProbeOutcome::LoggedIn, "the injected augmented PATH should allow /usr/bin/env to find the interpreter" ); assert!( @@ -96,4 +143,86 @@ mod tests { "the fake node from the injected PATH should have run" ); } + + #[cfg(unix)] + #[test] + fn login_probe_config_invalid_on_stderr_signal() { + use std::fs; + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().expect("temp dir"); + let bin_dir = temp.path().join("bin"); + fs::create_dir_all(&bin_dir).expect("bin dir"); + + // Script that exits 1 and writes a codex-style config-parse error to stderr. + let script_path = bin_dir.join("fake-codex-bad-config"); + fs::write( + &script_path, + "#!/bin/sh\necho 'Error loading configuration: /home/user/.codex/config.toml: unknown variant `ultra`, expected one of none/minimal/low/medium/high/xhigh' >&2\nexit 1\n", + ) + .expect("write script"); + fs::set_permissions(&script_path, fs::Permissions::from_mode(0o755)).expect("chmod script"); + + let outcome = super::login_probe( + &script_path, + &["fake-codex-bad-config", "login", "status"], + None, + ); + assert!( + matches!(outcome, ProbeOutcome::ConfigInvalid { .. }), + "stderr with 'unknown variant' should produce ConfigInvalid; got {:?}", + outcome + ); + if let ProbeOutcome::ConfigInvalid { stderr_excerpt } = outcome { + assert!( + stderr_excerpt.contains("unknown variant") + || stderr_excerpt.contains("Error loading"), + "stderr_excerpt should contain the parse error: {stderr_excerpt}" + ); + } + } + + #[cfg(unix)] + #[test] + fn login_probe_logged_out_on_nonzero_without_config_signal() { + use std::fs; + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().expect("temp dir"); + let bin_dir = temp.path().join("bin"); + fs::create_dir_all(&bin_dir).expect("bin dir"); + + // Script that exits 1 with a generic "not logged in" message. + let script_path = bin_dir.join("fake-codex-logged-out"); + fs::write( + &script_path, + "#!/bin/sh\necho 'not authenticated' >&2\nexit 1\n", + ) + .expect("write script"); + fs::set_permissions(&script_path, fs::Permissions::from_mode(0o755)).expect("chmod script"); + + let outcome = super::login_probe( + &script_path, + &["fake-codex-logged-out", "login", "status"], + None, + ); + assert_eq!( + outcome, + ProbeOutcome::LoggedOut, + "non-config stderr should produce LoggedOut" + ); + } + + /// Verify that every string in CONFIG_PARSE_SIGNALS is lowercased so the + /// case-insensitive match works correctly. + #[test] + fn config_parse_signals_are_lowercase() { + for sig in CONFIG_PARSE_SIGNALS { + assert_eq!( + *sig, + sig.to_lowercase(), + "CONFIG_PARSE_SIGNAL must be lowercase for case-insensitive matching: {sig}" + ); + } + } } diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index aa4af16acf..7fa1abd8c3 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -1615,6 +1615,16 @@ pub fn spawn_agent_child( "setup_copy": setup_copy, "availability": availability, }), + Requirement::CliConfigInvalid { + probe_args, + setup_copy, + diagnostic, + } => serde_json::json!({ + "surface": "cli_config_invalid", + "probe_args": probe_args, + "setup_copy": setup_copy, + "diagnostic": diagnostic, + }), }) .collect(); let payload = serde_json::json!({ diff --git a/desktop/src/shared/lib/configNudge.ts b/desktop/src/shared/lib/configNudge.ts index a806489481..7de7190c85 100644 --- a/desktop/src/shared/lib/configNudge.ts +++ b/desktop/src/shared/lib/configNudge.ts @@ -36,6 +36,18 @@ export type ConfigNudgeRequirement = * - "not_installed" → neither adapter nor CLI found */ availability: AcpAvailabilityStatus; + } + | { + /** + * The CLI is installed but its config file could not be parsed. + * Informational only — no in-app action can repair an external config + * file. Rendered without a Doctor or Edit Agent CTA. + */ + surface: "cli_config_invalid"; + probe_args: string[]; + setup_copy: string; + /** One-line stderr excerpt from the CLI's parse error. */ + diagnostic: string; }; /** @@ -131,6 +143,13 @@ function isConfigNudgeRequirement(v: unknown): v is ConfigNudgeRequirement { r.availability === "cli_missing" || r.availability === "not_installed") ); + case "cli_config_invalid": + return ( + Array.isArray(r.probe_args) && + r.probe_args.every((a) => typeof a === "string") && + typeof r.setup_copy === "string" && + typeof r.diagnostic === "string" + ); default: return false; } diff --git a/desktop/src/shared/ui/config-nudge-attachment.tsx b/desktop/src/shared/ui/config-nudge-attachment.tsx index 78f19fe20a..c1fa0cf341 100644 --- a/desktop/src/shared/ui/config-nudge-attachment.tsx +++ b/desktop/src/shared/ui/config-nudge-attachment.tsx @@ -33,6 +33,8 @@ function requirementKey( return `normalized_field:${req.field}:${index}`; case "cli_login": return `cli_login:${req.probe_args.join(",")}:${index}`; + case "cli_config_invalid": + return `cli_config_invalid:${req.probe_args.join(",")}:${index}`; } } @@ -62,6 +64,17 @@ function isAuthOnly(reqs: ConfigNudgePayload["requirements"]): boolean { ); } +/** + * Returns true when every requirement is a `cli_config_invalid` surface. + * Config-invalid cards are purely informational — the user must edit an + * external file; there is no in-app destination that can fix it. + */ +function isAllConfigInvalid(reqs: ConfigNudgePayload["requirements"]): boolean { + return ( + reqs.length > 0 && reqs.every((r) => r.surface === "cli_config_invalid") + ); +} + /** * Per-state human-readable copy for a cli_login requirement. * Uses the probe_args[0] as a best-effort harness name. @@ -164,6 +177,10 @@ export function ConfigNudgeCard({ const allCliLogin = isAllCliLogin(nudge.requirements); const authOnly = isAuthOnly(nudge.requirements); + const allConfigInvalid = isAllConfigInvalid(nudge.requirements); + // Any card that is purely informational (auth-only or all-config-invalid) + // has no clickable destination — treat them the same for affordance/routing. + const informationalOnly = authOnly || allConfigInvalid; const openDoctor = () => { if (!onOpenSettings) { @@ -212,9 +229,9 @@ export function ConfigNudgeCard({ {/* (A) Install-state all-cli_login card only — single card-level CTA - confirming the action at rest. Auth-only cards are informational - (no CTA); mixed cards render per-row CTAs instead. */} - {allCliLogin && !authOnly && ( + confirming the action at rest. Informational-only cards have no CTA; + mixed cards render per-row CTAs instead. */} + {allCliLogin && !informationalOnly && ( Open Doctor → )} - {/* Auth-only cards are purely informational — no trigger, no routing. */} - {!authOnly && ( + {/* Informational-only cards are purely informational — no trigger, no routing. */} + {!informationalOnly && ( ); + case "cli_config_invalid": { + // Config-invalid rows are purely informational — the user must edit an + // external file. No Doctor CTA (Doctor can't repair ~/.codex/config.toml) + // and no Edit Agent CTA (the field isn't managed by Buzz). + const cli = requirement.probe_args[0] ?? "the CLI"; + const configFile = `~/.${cli}/config.toml`; + return ( +
+ + {configFile} is invalid:{" "} + + {requirement.diagnostic} + {" "} + — fix the config and restart the agent + +
+ ); + } } } From 218a604b1edf58457cace0706bd19538e0f52c3c Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Fri, 10 Jul 2026 19:50:50 -0400 Subject: [PATCH 2/2] fix(desktop): surface actionable hint for -32603 CLI-ACP internal errors When a codex-acp turn fails with JSON-RPC code -32603 (standard 'Internal error'), the raw 'Internal error' string was shown with no guidance. This typically means the configured model isn't supported by the installed codex-acp version. Map code -32603 to a generic-severity hint in friendlyAgentLastError. Severity stays 'generic' (never 'denied') because -32603 can come from any ACP harness for any reason -- we can't prove the cause. Copy uses conditional 'For Codex agents' phrasing so it is truthful for all ACP runtimes without false-claiming the model is always the cause. -32001 and -32002 handling is unchanged (regression-guarded by existing tests). New tests cover -32603 direct, -32603 embedded in the message string, and regression against the two recognized codes. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../lib/friendlyAgentLastError.test.mjs | 103 ++++++++++++++++++ .../agents/lib/friendlyAgentLastError.ts | 36 +++++- 2 files changed, 136 insertions(+), 3 deletions(-) diff --git a/desktop/src/features/agents/lib/friendlyAgentLastError.test.mjs b/desktop/src/features/agents/lib/friendlyAgentLastError.test.mjs index e1cc138447..5c76f4660d 100644 --- a/desktop/src/features/agents/lib/friendlyAgentLastError.test.mjs +++ b/desktop/src/features/agents/lib/friendlyAgentLastError.test.mjs @@ -4,6 +4,7 @@ import test from "node:test"; import { friendlyAgentLastError, friendlyTurnErrorCopy, + CLI_ACP_INTERNAL_ERROR_COPY, MODEL_NOT_FOUND_COPY, RELAY_MESH_DENIED_COPY, } from "./friendlyAgentLastError.ts"; @@ -213,3 +214,105 @@ test("friendlyTurnErrorCopy: garbage string code coerces to NaN → string path" RELAY_MESH_DENIED_COPY, ); }); + +// --- -32603 internal error (Fix #2: CLI-ACP unsupported model hint) --- + +test("code -32603 bare 'Internal error' → cli-acp internal error hint (severity: generic)", () => { + const result = friendlyAgentLastError("Internal error", -32603); + assert.deepEqual(result, { + severity: "generic", + copy: CLI_ACP_INTERNAL_ERROR_COPY, + }); +}); + +test("code -32603 bare Internal error (wrapped) → cli-acp internal error hint", () => { + // The ACP wrapper form "Agent reported error (code -32603): Internal error" + // is treated as bare — the remainder after stripping the prefix is + // "Internal error", which maps to the hint. + const result = friendlyAgentLastError( + "Agent reported error (code -32603): Internal error", + -32603, + ); + assert.deepEqual(result, { + severity: "generic", + copy: CLI_ACP_INTERNAL_ERROR_COPY, + }); +}); + +test("code -32603 with specific message → original message preserved, NOT hint", () => { + // If the adapter provides detail beyond "Internal error", preserve it — + // don't bury actionable information with a broad codex-specific hint. + const result = friendlyAgentLastError( + "Internal error: model gpt-5.6-sol rejected by adapter", + -32603, + ); + assert.deepEqual(result, { + severity: "generic", + copy: "Internal error: model gpt-5.6-sol rejected by adapter", + }); +}); + +test("embedded code -32603 recovered from message when code param is null", () => { + const result = friendlyAgentLastError( + "Agent reported error (code -32603): Internal error", + null, + ); + assert.deepEqual(result, { + severity: "generic", + copy: CLI_ACP_INTERNAL_ERROR_COPY, + }); +}); + +test("embedded code -32603 with specific detail → original message preserved", () => { + // Embedded code path also preserves specific detail. + const result = friendlyAgentLastError( + "Agent reported error (code -32603): model not in registry", + null, + ); + assert.deepEqual(result, { + severity: "generic", + copy: "model not in registry", + }); +}); + +test("code -32603 structured param + wrapped specific detail → clean message, NOT wrapper", () => { + // Real path: storage.rs stores message = full wrapped line, code = parsed finite. + // The transport wrapper must be stripped; only the adapter detail reaches the UI. + const result = friendlyAgentLastError( + "Agent reported error (code -32603): model not in registry", + -32603, + ); + assert.deepEqual(result, { + severity: "generic", + copy: "model not in registry", + }); +}); + +test("friendlyTurnErrorCopy: -32603 structured param + wrapped specific detail → clean message", () => { + // friendlyTurnErrorCopy is the transcript path (agentSessionTranscript.ts). + assert.equal( + friendlyTurnErrorCopy( + "Agent reported error (code -32603): model not in registry", + -32603, + ), + "model not in registry", + ); +}); + +test("friendlyTurnErrorCopy: code -32603 bare Internal error → cli-acp internal error hint", () => { + assert.equal( + friendlyTurnErrorCopy("Internal error", -32603), + CLI_ACP_INTERNAL_ERROR_COPY, + ); +}); + +test("-32603 does not affect -32001/-32002 classification (regression)", () => { + assert.deepEqual(friendlyAgentLastError("any", -32001), { + severity: "denied", + copy: RELAY_MESH_DENIED_COPY, + }); + assert.deepEqual(friendlyAgentLastError("any", -32002), { + severity: "denied", + copy: MODEL_NOT_FOUND_COPY, + }); +}); diff --git a/desktop/src/features/agents/lib/friendlyAgentLastError.ts b/desktop/src/features/agents/lib/friendlyAgentLastError.ts index 38d37f5ae9..7c9fd68972 100644 --- a/desktop/src/features/agents/lib/friendlyAgentLastError.ts +++ b/desktop/src/features/agents/lib/friendlyAgentLastError.ts @@ -42,11 +42,23 @@ export const RELAY_MESH_DENIED_COPY = export const MODEL_NOT_FOUND_COPY = "The configured model is not available — open agent settings and select a different one from the dropdown."; +export const CLI_ACP_INTERNAL_ERROR_COPY = + "The agent's harness reported an internal error. For Codex agents this can mean the configured model isn't supported by your installed codex-acp — check the model in `~/.codex/config.toml` or upgrade the adapter (`brew upgrade codex-acp`)."; + const EMBEDDED_CODE_RE = /^Agent reported error \(code (-?\d+)\): /; +/** Bare form of the standard JSON-RPC -32603 message (after stripping the ACP wrapper prefix). */ +const BARE_INTERNAL_ERROR = "Internal error"; -function recoverEmbeddedCode(trimmed: string): number | null { +function recoverEmbeddedCode(trimmed: string): { + code: number; + remainder: string; +} | null { const match = EMBEDDED_CODE_RE.exec(trimmed); - return match ? Number(match[1]) : null; + if (!match) return null; + return { + code: Number(match[1]), + remainder: trimmed.slice(match[0].length), + }; } export function friendlyAgentLastError( @@ -59,15 +71,33 @@ export function friendlyAgentLastError( // Structured code first; a code embedded in the message string is the // same signal recovered from a record that lost the field. + const embedded = recoverEmbeddedCode(trimmed); const effectiveCode = Number.isFinite(code) ? (code as number) - : recoverEmbeddedCode(trimmed); + : (embedded?.code ?? null); if (effectiveCode != null) { switch (effectiveCode) { case -32001: return { severity: "denied", copy: RELAY_MESH_DENIED_COPY }; case -32002: return { severity: "denied", copy: MODEL_NOT_FOUND_COPY }; + case -32603: { + // Standard JSON-RPC "Internal error" — emitted by external harnesses + // (e.g. codex-acp) when the configured model is unsupported. Only + // substitute the hint when the message is the bare "Internal error" + // form; if the adapter included specific detail, preserve it so we + // don't bury actionable information with a broad codex-specific hint. + // + // "Bare" means the remainder after stripping the ACP wrapper prefix + // (if present) is exactly "Internal error". This covers both the raw + // form ("Internal error") and the ACP-wrapped form + // ("Agent reported error (code -32603): Internal error"). + const remainder = embedded?.remainder ?? trimmed; + if (remainder === BARE_INTERNAL_ERROR) { + return { severity: "generic", copy: CLI_ACP_INTERNAL_ERROR_COPY }; + } + return { severity: "generic", copy: remainder }; + } } // A structured code we don't recognize is authoritative — don't let // string patterns cross-classify it.