fix(agents): require positive 401 evidence before auth_invalid_token (review feedback from #77394)

jeffrey701 · jeffrey701 · commit 1a5dffdd5953 · 2026-05-14T11:42:36.000-04:00
diff --git a/src/agents/pi-embedded-helpers.formatassistanterrortext.test.ts b/src/agents/pi-embedded-helpers.formatassistanterrortext.test.ts
@@ -404,6 +404,53 @@ describe("formatAssistantErrorText", () => {
     );
   });
 
+  it("does not claim HTTP 401 for plain 403 errors that fall through to the generic auth reason (#77394 review)", () => {
+    // `classifyFailoverClassificationFromHttpStatus` returns the same
+    // `auth` reason for both `status === 401` AND `status === 403` after
+    // the billing/`auth_permanent` precedence at line 661 of errors.ts.
+    // A real 403 (key revoked, scope-missing) without an HTML body or
+    // `permission_error` JSON falls through to that generic `auth` branch.
+    // Before the 401-evidence gate, my new `auth_invalid_token` branch
+    // would pick those up and tell the user "HTTP 401" when the provider
+    // actually returned 403. The gate now requires `status === 401` or
+    // (status unknown AND message embeds 401 but not 403); a plain 403
+    // satisfies neither leg and falls through to the existing copy.
+    // Pin both directions: the 403 must NOT get the new "HTTP 401" copy,
+    // and it must still produce a non-empty user-facing message.
+    const plain403 = makeAssistantError("403 Forbidden");
+    const friendly = formatAssistantErrorText(plain403);
+    expect(friendly).toBeDefined();
+    expect(friendly).not.toContain("HTTP 401");
+    expect(friendly).not.toBe(
+      "Authentication failed (provider returned HTTP 401). " +
+        "Your provider token may have expired — try the request again in a moment. " +
+        "If the failure persists, re-authenticate this provider.",
+    );
+  });
+
+  it("does not claim HTTP 401 for message-only auth errors with no HTTP status prefix (#77394 review)", () => {
+    // `classifyFailoverSignal`'s message-only path at line 848 of errors.ts
+    // returns the `auth` reason via `isAuthErrorMessage(raw)` for payloads
+    // that have no leading HTTP status at all — for example a plain
+    // `{"error":{"code":"invalid_api_key"}}` envelope. Before the 401-
+    // evidence gate, the new `auth_invalid_token` branch would catch those
+    // too and surface "HTTP 401" copy when no HTTP status was ever present.
+    // `status` is `undefined` here and the message contains no `401`, so
+    // the second leg of the gate (`messageMentions401 && !messageMentions403`)
+    // is false and the gate falls through. Pin the negative behavior so a
+    // future widening that drops the gate fails this test rather than
+    // silently shipping the regression.
+    const messageOnly = makeAssistantError('{"error":{"code":"invalid_api_key"}}');
+    const friendly = formatAssistantErrorText(messageOnly);
+    expect(friendly).toBeDefined();
+    expect(friendly).not.toContain("HTTP 401");
+    expect(friendly).not.toBe(
+      "Authentication failed (provider returned HTTP 401). " +
+        "Your provider token may have expired — try the request again in a moment. " +
+        "If the failure persists, re-authenticate this provider.",
+    );
+  });
+
   it("returns a proxy-specific message for proxy misroutes", () => {
     const msg = makeAssistantError("407 Proxy Authentication Required");
     expect(formatAssistantErrorText(msg)).toBe(
diff --git a/src/agents/pi-embedded-helpers/errors.ts b/src/agents/pi-embedded-helpers/errors.ts
@@ -990,14 +990,39 @@ export function classifyProviderRuntimeFailureKind(
   // Plain HTTP 401 / "Invalid token" / "Unauthorized" — surface a friendly
   // re-auth hint rather than the raw provider error. Distinct from
   // `auth_refresh` (explicit OAuth-refresh failure messages) and
-  // `auth_html_403` (HTML 403 bodies). The `classifyFailoverSignal` path
-  // at status 401/403 already returns the `auth` reason for non-permanent /
-  // non-billing 401s, so we reuse it here instead of duplicating the
+  // `auth_html_403` (HTML 403 bodies). Reuses the existing
+  // `classifyFailoverSignal` `auth` reason instead of duplicating the
   // message regex set. Placed AFTER replay-invalid / schema so 401s with
   // structurally-meaningful bodies (e.g. "401 input item ID does not belong
   // to this conversation" → `replay_invalid`) keep their existing copy.
-  // See [Github #56197].
-  if (failoverClassification?.kind === "reason" && failoverClassification.reason === "auth") {
+  //
+  // The 401-evidence gate is load-bearing: `classifyFailoverSignal` returns
+  // the same `auth` reason for two non-401 shapes that must NOT get the
+  // "HTTP 401" copy:
+  //   1. Plain HTTP 403 fall-through (key revoked, scope-missing) without
+  //      an HTML body or `permission_error` JSON — same branch at status
+  //      401/403 in `classifyFailoverClassificationFromHttpStatus`.
+  //   2. Message-only auth errors with no HTTP status prefix at all
+  //      (e.g. `{"error":{"code":"invalid_api_key"}}`) classified by
+  //      `isAuthErrorMessage(raw)`.
+  // We require positive 401 evidence: either `status === 401` (parsed from
+  // a leading HTTP status), or `status` is unknown AND the message embeds
+  // `401` at a word boundary while not also embedding `403`. The latter
+  // covers real-world variants like `'status code: 401, message: ...'`
+  // where `extractLeadingHttpStatus` cannot pick up the code because it
+  // is not at the start. Billing 401s (e.g. `{"code":401,"message":"Key
+  // limit exceeded"}`) are already caught upstream by the billing reason
+  // taking precedence over `auth` in the 401/403 branch, so they never
+  // reach this gate. See [Github #56197] and PR #77394 review.
+  const messageMentions401 = /\b401\b/.test(message);
+  const messageMentions403 = /\b403\b/.test(message);
+  const has401Evidence =
+    status === 401 || (status === undefined && messageMentions401 && !messageMentions403);
+  if (
+    failoverClassification?.kind === "reason" &&
+    failoverClassification.reason === "auth" &&
+    has401Evidence
+  ) {
     return "auth_invalid_token";
   }
   if (
