fix(diagnostics): classify 401/403 as auth-failed so a wrong password stops the retry loop (#170)

AGE-107. The 498 `API Error: 401` events from one device were not a client
token-refresh loop. Sentry breadcrumbs on the surviving events show a `touch`
event immediately before every capture, at irregular human-paced intervals
(87s, 199s, 69s, 5s, 61s, 66s) — a person re-tapping Connect, not a backoff
timer. The app's automated loops were already correct: events.ts terminates
the SSE reconnect loop on ApiAuthError (issue #76).

What actually drove it: in v0.4.4 the connection probe counted any HTTP
response as a successful health check, so a 401 was classified `ok` and shown
to the user as "Health endpoint responded — connection actually works now"
while their password was wrong. The user retried for two months. `requireOk`
(#114, v0.4.8) stopped the false success, but 401 then fell into the generic
`health-failed` bucket — "Likely wrong path, auth, or an old server version" —
which still doesn't tell anyone to fix their password.

- New `auth-failed` classification: a 401/403 from /global/health means the
  server is up and reachable and rejected the credentials. Its summary names
  the status, points at the password and OPENCODE_SERVER_USERNAME, and says
  the server is fine. It flows straight into the existing failure Alert on
  both the add and edit connection screens — which is where the password
  field is, i.e. the re-auth prompt.
- It short-circuits before the root/internet probes can downgrade it: a 401
  already proves the server answered.
- `health-failed` copy no longer blames auth.
- `connect auth-failed` joins the noise-gate drop-list. A wrong password is
  user config, unactionable server-side, already visible in the UI and
  already trended in PostHog as connection_failed{error_class:"unauthorized"}.
  `health-failed` and `tls-error` still report.

Tests: 6 new (401/403 -> auth-failed, message content, root-unreachable does
not override, 404/500/502 stay health-failed, health-failed copy drops "auth",
noise gate drops `connect auth-failed` but not a raw `API Error: 401`).
263 pass, tsc --noEmit clean.

Co-authored-by: engineer <engineer@macbookpro.lan>
Co-authored-by: Paperclip <noreply@paperclip.ing>

docs/analytics.md

@@ -81,20 +81,32 @@ Consent decides *whether* we report; the noise gate in `src/lib/sentry-noise.ts`
 often*. It exists because this app became the org's #1 Sentry volume source (~4,500
 events/month against a 3,500/month org quota) while ~1,100 of those events were three
 non-defects: `connect timeout`, `connect server-unreachable`, and one device's
-`API Error: 401` token-refresh loop firing 498 times.
+`API Error: 401` firing 498 times.
+
+> **AGE-107 postscript.** That 401 storm was traced to a *human* retry loop, not a client
+> token-refresh loop. In v0.4.4 the connection probe scored **any** HTTP response as a
+> success, so a 401 was reported to the user as "Health endpoint responded — connection
+> actually works now" while their password was wrong. They re-tapped Connect for two months
+> (Sentry breadcrumbs show a `touch` event before every single capture, at irregular
+> human-paced intervals). `requireOk` in `diagnostics.ts` (v0.4.8) stopped the false
+> success; `auth-failed` now gives it its own actionable message and drop-list entry.
+> The client's automated loops were never at fault — `events.ts` already terminates the SSE
+> reconnect loop on `ApiAuthError` (issue #76).
 
 `beforeSend` applies three layers, cheapest first:
 
 | Layer | Rule | Effect |
 |---|---|---|
 | Always-send allowlist | OOM / ANR / native / `IllegalStateException` / `NullPointerException` / fatal level / unhandled mechanism | Bypasses every limit below — quota is worthless if it silences real crashes |
-| Transport drop-list | `connect timeout\|server-unreachable\|no-internet\|malformed-url`, `Network request failed`, `Request timed out after`, `ECONN*`/`ETIMEDOUT`… | Hard drop. Not sampled: the gate is per-install, so even 1/device/day multiplies by the install base back into thousands/month |
+| Transport drop-list | `connect timeout\|server-unreachable\|no-internet\|malformed-url\|auth-failed`, `Network request failed`, `Request timed out after`, `ECONN*`/`ETIMEDOUT`… | Hard drop. Not sampled: the gate is per-install, so even 1/device/day multiplies by the install base back into thousands/month |
 | Dedup + rate cap | per-fingerprint cooldown 6h, ≤6 new fingerprints/h, ≤10 events/h (mirrors the `openclaw-box-bot` shim, AGE-55) | Turns a retry loop into one report and caps any future regression |
 
 Nothing is lost by the transport drop: those failures are already shown to the user as
 connection UI **and** already trended, PII-free, as the PostHog `connection_failed` event with
-an `error_class` property (`src/lib/analytics-classify.ts`). Sentry was paying per event for a
+an `error_class` property (`src/lib/analytics-classify.ts` — a 401 lands in `unauthorized`).
-graph we already have.
+Sentry was paying per event for a graph we already have. `connect health-failed` and
+`connect tls-error` are deliberately **not** dropped: a box that answers but is unhealthy, or
+a broken certificate, is actionable.
 
 Dropped-event counts are not silent — the number dropped since the last delivered event rides
 along as a `noise.dropped_since_last` tag, so the saving is auditable from Sentry itself.

src/lib/diagnostics-classify.test.ts

@@ -116,13 +116,61 @@ test("root reachable but internet probe down still classifies as health-failed,
   // this must not be misreported as "no internet".
   const r = classify(
     okUrl,
-    probe({ ok: false, error: "HTTP 401", status: 401 }), // health
+    probe({ ok: false, error: "HTTP 404", status: 404 }), // health
     probe({ ok: false }), // internet (down)
     probe({ ok: true }), // root (reachable)
   )
   assert.equal(r.classification, "health-failed")
 })
 
+// --- AGE-107: a rejected credential must not read as anything else ---------
+//
+// One device sent 498 `API Error: 401` events over two months. The cause was
+// not an automated retry loop — it was a human re-tapping Connect because
+// v0.4.4's probe scored any HTTP response as ok:true and reported
+// "connection actually works now" while the password was wrong. `requireOk`
+// stopped that lie; these lock in the follow-up: 401/403 is its own,
+// actionable classification, not the generic health-failed bucket.
+
+test("health probe 401 -> auth-failed, and the summary tells the user to fix credentials", () => {
+  const r = classify(
+    okUrl,
+    probe({ ok: false, error: "HTTP 401", status: 401 }), // health
+    probe({ ok: true }), // internet
+    probe({ ok: true, status: 401 }), // root reachable
+  )
+  assert.equal(r.classification, "auth-failed")
+  assert.match(r.summary, /401/)
+  assert.match(r.summary, /credential|password/i)
+  // Must NOT tell the user the connection works (the v0.4.4 defect).
+  assert.doesNotMatch(r.summary, /actually works/i)
+})
+
+test("health probe 403 -> auth-failed too", () => {
+  const r = classify(okUrl, probe({ ok: false, status: 403 }), probe({ ok: true }), probe({ ok: true, status: 403 }))
+  assert.equal(r.classification, "auth-failed")
+  assert.match(r.summary, /403/)
+})
+
+test("auth-failed wins even when the server root probe is unreachable", () => {
+  // A 401 from /global/health already proves the server answered us; the
+  // root probe's outcome must not downgrade this to server-unreachable.
+  const r = classify(okUrl, probe({ ok: false, status: 401 }), probe({ ok: true }), probe({ ok: false }))
+  assert.equal(r.classification, "auth-failed")
+})
+
+test("non-auth failure statuses stay health-failed", () => {
+  for (const status of [404, 500, 502]) {
+    const r = classify(okUrl, probe({ ok: false, status }), probe({ ok: true }), probe({ ok: true, status }))
+    assert.equal(r.classification, "health-failed", `status ${status}`)
+  }
+})
+
+test("health-failed copy no longer blames auth now that auth-failed exists", () => {
+  const r = classify(okUrl, probe({ ok: false, status: 500 }), probe({ ok: true }), probe({ ok: true, status: 500 }))
+  assert.doesNotMatch(r.summary, /auth/i)
+})
+
 test("server-unreachable adds MagicDNS hint only for hostnames, not IPs", () => {
   const fail = { internet: probe({ ok: true }), root: probe({ ok: false }) }
   const hostR = classify(parseUrl("http://box.ts.net:8080"), probe({ error: "refused" }), fail.internet, fail.root)

src/lib/diagnostics-classify.ts

@@ -7,6 +7,7 @@ export type Classification =
   | "malformed-url"
   | "no-internet"
   | "server-unreachable"
+  | "auth-failed"
   | "health-failed"
   | "tls-error"
   | "timeout"
@@ -55,6 +56,27 @@ export function classify(
     return { classification: "ok", summary: "Health endpoint responded — connection actually works now." }
   }
 
+  // 401/403 means the server is up, reachable, and talking to us — it just
+  // rejected our credentials. That needs the opposite instruction from every
+  // other failure below (fix the password, not the network), so it gets its
+  // own classification rather than being folded into the generic
+  // "health-failed / likely wrong path, auth, or an old server version".
+  //
+  // This is the tail of AGE-107: one device produced 498 `API Error: 401`
+  // events over two months because v0.4.4's probe counted any HTTP response
+  // as ok:true and told the user "connection actually works now" while the
+  // password was wrong. `requireOk` (v0.4.8) stopped the lie; this makes the
+  // remaining message actionable so the user stops retrying blind.
+  if (health.status === 401 || health.status === 403) {
+    return {
+      classification: "auth-failed",
+      summary:
+        `The server rejected your credentials (HTTP ${health.status}). ` +
+        `Check the password, and the username if you set OPENCODE_SERVER_USERNAME on the server ` +
+        `(it defaults to "opencode"). The server itself is running and reachable.`,
+    }
+  }
+
   const txt = `${health.error ?? ""} ${health.errorCause ?? ""}`.toLowerCase()
   const isTls = /ssl|tls|certificate|trust|handshake/.test(txt)
   const isTimeout = /timeout|timed out/.test(txt)
@@ -67,7 +89,7 @@ export function classify(
   // probe (captive portal, no WAN but Tailscale LAN still up, etc.) must not
   // override it and misreport a reachable server as "no internet".
   if (root.ok) {
-    return { classification: "health-failed", summary: `Server is reachable but /global/health failed (HTTP ${health.status ?? "error"}). Likely wrong path, auth, or an old server version.` }
+    return { classification: "health-failed", summary: `Server is reachable but /global/health failed (HTTP ${health.status ?? "error"}). Likely a wrong path or an old server version.` }
   }
   if (!internet.ok) {
     return { classification: "no-internet", summary: "The device has no working internet/network at all (public check also failed). Check Wi-Fi/data and Tailscale (VPN) status." }

src/lib/sentry-noise.test.ts

@@ -34,6 +34,16 @@ test("isTransportNoise: server-side and app-side failures are NOT noise", () =>
   assert.equal(isTransportNoise(""), false)
 })
 
+test("isTransportNoise: AGE-107 — a rejected credential is user config, not an app defect", () => {
+  // The server answered 401/403. Nothing on our side can fix a wrong password;
+  // it is already shown in the connection screen and already trended in
+  // PostHog as connection_failed{error_class:"unauthorized"}.
+  assert.equal(isTransportNoise("connect auth-failed"), true)
+  // ...but a genuine 401 from anywhere else still reports — only the
+  // classified diagnostic is dropped.
+  assert.equal(isTransportNoise("API Error: 401 - "), false)
+})
+
 test("isAlwaysSend: genuine crash classes bypass the gate", () => {
   assert.equal(isAlwaysSend("OutOfMemoryError (okio.Buffer.readByteArray)"), true)
   assert.equal(isAlwaysSend("IllegalStateException: no activity"), true)

src/lib/sentry-noise.ts

@@ -15,8 +15,15 @@
 // `connection_failed` event with an `error_class` property
 // (see analytics-classify.ts + stores/connections.ts), so dropping them from
 // Sentry loses no trend visibility — it just stops paying per-event for a graph
-// we already have. The 401 storm is one device's token-refresh retry loop:
+// we already have.
-// 498 copies of one problem, not 498 problems.
+//
+// The 401 storm was NOT an automated retry loop (AGE-107 traced it): it was one
+// user manually re-tapping Connect for two months because v0.4.4's probe scored
+// any HTTP response as a success and told them "connection actually works now"
+// while their password was wrong. A wrong password is user config, not an app
+// defect — it is already surfaced in the connection screen (now as
+// `connect auth-failed`) and already trended in PostHog as
+// `connection_failed{error_class:"unauthorized"}`, so it is dropped here too.
 //
 // Three layers, cheapest first (`admit()` applies them in order):
 //   1. ALWAYS-SEND allowlist — genuine crash classes (OOM/ANR/native/fatal)
@@ -41,12 +48,15 @@ export type NoiseEventLike = {
   exception?: { values?: Array<{ type?: string; value?: string; mechanism?: { handled?: boolean } }> }
 }
 
-/** Client-side network conditions. Unactionable server-side, already surfaced to
+/** Client-side network and credential conditions. Unactionable server-side,
- *  the user as connection UI, and already trended in PostHog as
+ *  already surfaced to the user as connection UI, and already trended in
- *  `connection_failed{error_class}`. Dropped outright. */
+ *  PostHog as `connection_failed{error_class}`. Dropped outright. */
 export const TRANSPORT_NOISE_PATTERNS: RegExp[] = [
   // captureDiagnostic() → new Error(`connect ${classification}`)
-  /^connect (?:timeout|server-unreachable|no-internet|malformed-url)$/i,
+  // `auth-failed` = the server answered 401/403: wrong password, user config.
+  // `health-failed` and `tls-error` are deliberately NOT here — a box that
+  // answers but is unhealthy, or a broken cert, is actionable.
+  /^connect (?:timeout|server-unreachable|no-internet|malformed-url|auth-failed)$/i,
   // RN fetch failures surfacing through the global handler / rejection hook.
   /network request failed/i,
   /request timed out after/i,

src/lib/sentry.ts

@@ -267,11 +267,12 @@ export function captureException(
 export function captureDiagnostic(report: DiagnosticReport) {
   log.info("sentry", "capture", report.classification, enabled ? "(uploading)" : "(local only)")
   if (!enabled) return
-  // Client-side network conditions (timeout / unreachable / no internet) are
+  // Client-side network conditions (timeout / unreachable / no internet) and
-  // already shown to the user and already trended in PostHog as
+  // wrong credentials (auth-failed) are already shown to the user and already
-  // `connection_failed{error_class}`; they were the single largest consumer of
+  // trended in PostHog as `connection_failed{error_class}`; they were the
-  // the org Sentry quota (AGE-105). Skip them here so we don't even build the
+  // single largest consumer of the org Sentry quota (AGE-105 / AGE-107). Skip
-  // event. `health-failed` / `tls-error` are genuinely actionable and still go.
+  // them here so we don't even build the event. `health-failed` / `tls-error`
+  // are genuinely actionable and still go.
   if (isTransportNoise(`connect ${report.classification}`)) {
     log.info("sentry", "skipped diagnostic upload (client-side network condition)", report.classification)
     return