From ec0ce5f2afa6505741b073db71cdf83a52d7aacb Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Wed, 8 Jul 2026 18:07:54 -0400 Subject: [PATCH] fix(agents): harden structured error-code classification edge cases Post-merge review of #1653 found gaps in the error-code seam: unknown codes could cross-classify via string prefixes, NaN slipped past the null guard, new-format log strings were unclassifiable when the code field was lost, non-standard JSON-RPC errors were truncated to "unknown error", and bare llm-auth log lines from older agent binaries regressed to the generic exit message. Unknown structured codes are now authoritative (generic, no string matching), codes embedded in the message are recovered, the full JSON error object is preserved when message is absent, and bare-prefix promotion is restored with synthetic codes. --- crates/buzz-acp/src/acp.rs | 61 ++++++++++++----- crates/buzz-acp/src/lib.rs | 12 ++-- .../src-tauri/src/managed_agents/storage.rs | 43 +++++++++++- .../lib/friendlyAgentLastError.test.mjs | 65 +++++++++++++++++++ .../agents/lib/friendlyAgentLastError.ts | 61 ++++++++++------- .../e2e/agent-error-state-screenshots.spec.ts | 2 +- 6 files changed, 195 insertions(+), 49 deletions(-) diff --git a/crates/buzz-acp/src/acp.rs b/crates/buzz-acp/src/acp.rs index b527542628..78e23c8f8d 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 c70c66c288..8627d1d42a 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 166766d865..fa5281f888 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 4f36ea5183..e1cc138447 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 57a9b3fbb1..38d37f5ae9 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 51cb93e27f..a55823c62a 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, };