diff --git a/crates/buzz-acp/src/acp.rs b/crates/buzz-acp/src/acp.rs index b52754262..78e23c8f8 100644 --- a/crates/buzz-acp/src/acp.rs +++ b/crates/buzz-acp/src/acp.rs @@ -105,6 +105,19 @@ pub enum AcpError { AgentError { code: i64, message: String }, } +/// Build an [`AcpError::AgentError`] from a JSON-RPC error object, +/// preserving the numeric code. When the `message` field is missing or +/// non-string, fall back to the full JSON object so provider-specific +/// detail (e.g. a `data` field) is not lost. +fn agent_error_from_json(error: &serde_json::Value) -> AcpError { + let code = error.get("code").and_then(|c| c.as_i64()).unwrap_or(-32000); + let message = match error.get("message").and_then(|m| m.as_str()) { + Some(m) => m.to_string(), + None => error.to_string(), + }; + AcpError::AgentError { code, message } +} + /// ACP client that owns an agent subprocess and communicates over its stdio. /// /// One `AcpClient` per agent process. Multiple sessions can be created on the @@ -863,13 +876,7 @@ impl AcpClient { if let Some(id) = msg.get("id") { if *id == serde_json::json!(expected_id) && msg.get("method").is_none() { if let Some(error) = msg.get("error") { - let code = error.get("code").and_then(|c| c.as_i64()).unwrap_or(-32000); - let message = error - .get("message") - .and_then(|m| m.as_str()) - .unwrap_or("unknown error") - .to_string(); - return Err(AcpError::AgentError { code, message }); + return Err(agent_error_from_json(error)); } return Ok(msg["result"].clone()); } @@ -1191,16 +1198,7 @@ impl AcpClient { let _ = ack_tx .send(crate::pool::SteerAck::PromptCompletedNeutral); } - let code = error - .get("code") - .and_then(|c| c.as_i64()) - .unwrap_or(-32000); - let message = error - .get("message") - .and_then(|m| m.as_str()) - .unwrap_or("unknown error") - .to_string(); - return Err(AcpError::AgentError { code, message }); + return Err(agent_error_from_json(error)); } if let Some((_, ack_tx)) = pending_steer.take() { let _ = @@ -3022,4 +3020,33 @@ mod tests { client.handle_goose_usage_update(&bad2); assert!(client.take_turn_usage().is_none()); } + + #[test] + fn agent_error_from_json_falls_back_to_full_json_when_message_missing() { + // Errors without a string `message` field (e.g. only a `data` field) must + // not be silently truncated to "unknown error" — the full JSON is preserved. + let error = serde_json::json!({"code": -32000, "data": "quota exceeded"}); + match super::agent_error_from_json(&error) { + AcpError::AgentError { code, message } => { + assert_eq!(code, -32000); + assert!( + message.contains("quota exceeded"), + "expected full JSON in message, got: {message}" + ); + } + other => panic!("expected AgentError, got {other:?}"), + } + } + + #[test] + fn agent_error_from_json_uses_message_field_when_present() { + let error = serde_json::json!({"code": -32001, "message": "auth denied"}); + match super::agent_error_from_json(&error) { + AcpError::AgentError { code, message } => { + assert_eq!(code, -32001); + assert_eq!(message, "auth denied"); + } + other => panic!("expected AgentError, got {other:?}"), + } + } } diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index c70c66c28..8627d1d42 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -2875,6 +2875,10 @@ fn handle_prompt_result( | acp::AcpError::Timeout(_) | acp::AcpError::Protocol(_) ); + let error_code = match &e { + acp::AcpError::AgentError { code, .. } => Some(*code), + _ => None, + }; if is_transport_error { tracing::warn!( agent = agent_index, @@ -2884,10 +2888,6 @@ fn handle_prompt_result( error = %e, "transport/protocol error — respawning agent" ); - let error_code = match &e { - acp::AcpError::AgentError { code, .. } => Some(*code), - _ => None, - }; emit_turn_error(&e.to_string(), error_code); let index = result.agent.index; @@ -2914,10 +2914,6 @@ fn handle_prompt_result( error = %e, "agent_returned (application error — pipe intact)" ); - let error_code = match &e { - acp::AcpError::AgentError { code, .. } => Some(*code), - _ => None, - }; emit_turn_error(&e.to_string(), error_code); pool.return_agent(result.agent); } diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index 166766d86..fa5281f88 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/desktop/src-tauri/src/managed_agents/storage.rs @@ -532,8 +532,13 @@ fn bytecount_newlines(buf: &[u8]) -> usize { buf.iter().filter(|&&b| b == b'\n').count() } +/// A meaningful error recovered from an exited agent's log tail. pub struct AgentLogError { + /// The full log line, wrapped as `Agent reported error…` for display. pub message: String, + /// JSON-RPC error code parsed from the line's `(code N)` marker, or a + /// synthetic code for known bare prefixes. `None` for legacy-format + /// lines that carry no code (or when the code fails to parse as i64). pub code: Option, } @@ -557,6 +562,21 @@ pub fn meaningful_agent_error_from_log(path: &Path) -> Option { code: None, }); } + // Bare prefixes emitted by older agent binaries whose Display still leaks + // unwrapped errors. Promote these so they surface instead of the generic + // "harness exited with status N" fallback. + if line.starts_with("llm auth:") { + return Some(AgentLogError { + message: format!("Agent reported error: {line}"), + code: Some(-32001), + }); + } + if line.starts_with("llm model not found:") { + return Some(AgentLogError { + message: format!("Agent reported error: {line}"), + code: Some(-32002), + }); + } None }) } @@ -884,7 +904,28 @@ mod tests { #[test] fn meaningful_agent_error_from_log_promotes_unwrapped_llm_auth() { let file = write_log("noise\nllm auth: denied\n"); - assert!(super::meaningful_agent_error_from_log(file.path()).is_none()); + let result = super::meaningful_agent_error_from_log(file.path()).unwrap(); + assert_eq!(result.message, "Agent reported error: llm auth: denied"); + assert_eq!(result.code, Some(-32001)); + } + + #[test] + fn meaningful_agent_error_from_log_promotes_bare_model_not_found() { + let file = write_log("noise\nllm model not found: (some-model) 404\n"); + let result = super::meaningful_agent_error_from_log(file.path()).unwrap(); + assert_eq!( + result.message, + "Agent reported error: llm model not found: (some-model) 404" + ); + assert_eq!(result.code, Some(-32002)); + } + + #[test] + fn meaningful_agent_error_from_log_promotes_legacy_format() { + let file = write_log("noise\nAgent reported error: llm: 500 internal\n"); + let result = super::meaningful_agent_error_from_log(file.path()).unwrap(); + assert_eq!(result.message, "Agent reported error: llm: 500 internal"); + assert_eq!(result.code, None); } #[test] diff --git a/desktop/src/features/agents/lib/friendlyAgentLastError.test.mjs b/desktop/src/features/agents/lib/friendlyAgentLastError.test.mjs index 4f36ea518..e1cc13844 100644 --- a/desktop/src/features/agents/lib/friendlyAgentLastError.test.mjs +++ b/desktop/src/features/agents/lib/friendlyAgentLastError.test.mjs @@ -148,3 +148,68 @@ test("friendlyTurnErrorCopy: missing code falls back to raw text", () => { test("friendlyTurnErrorCopy: unknown code passes raw text through", () => { assert.equal(friendlyTurnErrorCopy("raw error", 12345), "raw error"); }); + +// --- structured-code hardening --- + +test("unknown code prevents string-pattern cross-classification", () => { + // code -32003 is structured and unrecognized — must NOT fall through to + // the legacy string path that would wrongly promote this to denied. + const result = friendlyAgentLastError( + "llm auth: rate limiter denial", + -32003, + ); + assert.deepEqual(result, { + severity: "generic", + copy: "llm auth: rate limiter denial", + }); +}); + +test("NaN code param treated as absent — string path applies", () => { + // NaN is not finite; falls back to string matching. + const result = friendlyAgentLastError("llm auth: denied", NaN); + assert.deepEqual(result, { + severity: "denied", + copy: RELAY_MESH_DENIED_COPY, + }); +}); + +test("embedded code -32001 recovered from message when code param is null", () => { + const result = friendlyAgentLastError( + "Agent reported error (code -32001): llm auth: 401", + null, + ); + assert.deepEqual(result, { + severity: "denied", + copy: RELAY_MESH_DENIED_COPY, + }); +}); + +test("embedded code -32002 recovered from message when code param is undefined", () => { + const result = friendlyAgentLastError( + "Agent reported error (code -32002): llm model not found: x", + undefined, + ); + assert.deepEqual(result, { + severity: "denied", + copy: MODEL_NOT_FOUND_COPY, + }); +}); + +test("embedded unknown code is authoritative — no cross-classification", () => { + const result = friendlyAgentLastError( + "Agent reported error (code -32099): llm auth: weird", + null, + ); + assert.deepEqual(result, { + severity: "generic", + copy: "Agent reported error (code -32099): llm auth: weird", + }); +}); + +test("friendlyTurnErrorCopy: garbage string code coerces to NaN → string path", () => { + // "garbage" → NaN → not finite → null → string prefix matches "llm auth:". + assert.equal( + friendlyTurnErrorCopy("llm auth: denied", "garbage"), + RELAY_MESH_DENIED_COPY, + ); +}); diff --git a/desktop/src/features/agents/lib/friendlyAgentLastError.ts b/desktop/src/features/agents/lib/friendlyAgentLastError.ts index 57a9b3fbb..38d37f5ae 100644 --- a/desktop/src/features/agents/lib/friendlyAgentLastError.ts +++ b/desktop/src/features/agents/lib/friendlyAgentLastError.ts @@ -1,27 +1,29 @@ /** * Promote certain machine-readable `lastError` strings to user-facing copy. * - * The mesh-llm seam (Max's commit `5196203…`) flows like this: - * buzz-agent — gets HTTP 401/403 from the OpenAI-compatible mesh endpoint - * → raises `AgentError::LlmAuth("…")` (json_rpc_code `-32001`, - * Display prefix `"llm auth: …"`) - * buzz-acp — wraps it as `AcpError::AgentError("Agent reported error: llm auth: …")` - * desktop managed-agent supervisor — on nonzero exit, scans `read_log_tail` - * for `"Agent reported error:"` / `"llm auth:"` and persists - * that line into `ManagedAgent.lastError` instead of the - * generic `"harness exited with status …"`. + * The error classification seam flows like this: + * buzz-agent — classifies LLM failures into `AgentError` variants with + * JSON-RPC codes (`-32001` auth, `-32002` model-not-found, + * `-32000` generic), defined in `crates/buzz-agent/src/types.rs`. + * buzz-acp — preserves the code structurally in + * `AcpError::AgentError { code, message }`, whose Display is + * `"Agent reported error (code N): message"`, and includes + * `code` in `turn_error` observer events. + * desktop supervisor — on nonzero exit, recovers `{ message, code }` from + * the log tail (`managed_agents/storage.rs`) into + * `ManagedAgent.lastError` / `lastErrorCode`. * - * v1 caveat (named, not hidden): the typed `-32001` code never reaches - * desktop structurally — desktop only supervises the child process and ACP's - * `ObserverHandle` is in-process inside that child. So `lastError` is the - * recovered string, not the original code. The follow-up to make this 9/10 - * structural is an ACP status file or desktop-owned observer sink; that lives - * elsewhere. For now we match the string this function exists to render. + * This function dispatches on the numeric code first (works for any harness), + * then recovers a code embedded in the message string (handles records where + * the `lastErrorCode` field was lost, e.g. downgrade or pre-code records with + * new-format strings), then falls back to legacy string prefixes for records + * written before structured codes existed. * * Returns: * - null when there's nothing to show (null/empty lastError). * - A `{ severity: "denied"; copy: string }` object for the auth-failure - * case, so the UI can render with the right visual weight (destructive). + * and model-not-found cases, so the UI can render with the right visual + * weight (destructive). * - A `{ severity: "generic"; copy: string }` pass-through for any other * lastError, so generic harness exits still surface their text instead of * being swallowed. @@ -40,6 +42,13 @@ 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."; +const EMBEDDED_CODE_RE = /^Agent reported error \(code (-?\d+)\): /; + +function recoverEmbeddedCode(trimmed: string): number | null { + const match = EMBEDDED_CODE_RE.exec(trimmed); + return match ? Number(match[1]) : null; +} + export function friendlyAgentLastError( raw: string | null, code?: number | null, @@ -48,18 +57,25 @@ export function friendlyAgentLastError( const trimmed = raw.trim(); if (trimmed.length === 0) return null; - // Structured code path — no string matching, works across all providers. - if (code != null) { - switch (code) { + // Structured code first; a code embedded in the message string is the + // same signal recovered from a record that lost the field. + const effectiveCode = Number.isFinite(code) + ? (code as number) + : recoverEmbeddedCode(trimmed); + 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 }; } + // A structured code we don't recognize is authoritative — don't let + // string patterns cross-classify it. + return { severity: "generic", copy: trimmed }; } - // Match either the unwrapped buzz-agent prefix or the buzz-acp wrap. - // The desktop supervisor recovers whichever appears first in the log tail. + // Legacy string fallback for records written before codes existed. + // Match either the unwrapped buzz-agent prefix or the buzz-acp v0 wrap. if ( trimmed.startsWith("Agent reported error: llm auth:") || trimmed.startsWith("llm auth:") @@ -77,5 +93,6 @@ export function friendlyAgentLastError( */ export function friendlyTurnErrorCopy(raw: string, code: unknown): string { const numeric = code == null ? null : Number(code); - return friendlyAgentLastError(raw, numeric)?.copy ?? raw; + const safe = Number.isFinite(numeric) ? (numeric as number) : null; + return friendlyAgentLastError(raw, safe)?.copy ?? raw; } diff --git a/desktop/tests/e2e/agent-error-state-screenshots.spec.ts b/desktop/tests/e2e/agent-error-state-screenshots.spec.ts index 51cb93e27..a55823c62 100644 --- a/desktop/tests/e2e/agent-error-state-screenshots.spec.ts +++ b/desktop/tests/e2e/agent-error-state-screenshots.spec.ts @@ -26,7 +26,7 @@ const MODEL_NOT_FOUND_AGENT = { name: "Databricks Agent", status: "stopped" as const, lastError: - "Agent reported error: llm: (goose-databricks-llama-3-3-70b) 404 Not Found: model not found", + "Agent reported error (code -32002): llm model not found: (goose-databricks-llama-3-3-70b) 404 Not Found: model not found", lastErrorCode: -32002, };