fix(agents): harden structured error-code classification edge cases (#1663)

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:?}"),
+        }
+    }
 }

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);
             }

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<i64>,
 }
 
@@ -557,6 +562,21 @@ pub fn meaningful_agent_error_from_log(path: &Path) -> Option<AgentLogError> {
                 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]

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,
+  );
+});

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;
 }

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,
 };