Skip to content

fix(onboarding) [BRNS-DESK-031]: validate the brains credential before reporting ready - #87

Open
sebastian-ssvlabs wants to merge 2 commits into
mainfrom
fix/desk-031-credential-probe
Open

fix(onboarding) [BRNS-DESK-031]: validate the brains credential before reporting ready#87
sebastian-ssvlabs wants to merge 2 commits into
mainfrom
fix/desk-031-credential-probe

Conversation

@sebastian-ssvlabs

Copy link
Copy Markdown

Summary

  • A false green light: the app told the user brains was connected and ready while every MCP call 401'd. Readiness reported "provisioned" from a token string being present — it never asked the server whether that string still works — so a revoked credential looked identical to a live one and bootRoute() sent the user straight to stage app with brains effectively dead. Undiagnosable from the product; it hid a stale token for weeks on a real machine.
  • The structural check stays as the cheap pre-filter, but a live whoami is now the answer. Only a 401/403 may flip readiness — a network failure must not, because offline is not signed out.
  • On a rejection the boot router lands on the connect step with copy distinct from the never-signed-in case ("Your brains sign-in expired"), an action that restarts sign-in, and the dead token scrubbed from every mirror.
  • Closes BRNS-DESK-031 (P2, effort S, onboarding / desktop / readiness).

The mechanism it replaces

brains_is_provisioned() was purely structural and had no network step anywhere in the readiness path:

enabled   = cfg.enabledPlugins["brains@brains"] == true
has_token = cfg.pluginConfigs["brains@brains"].options.token is a non-empty string
return enabled && has_token

Which is why the reported state and the observed state disagreed completely on a real config (2026-08-12): enabled = true, has_token = true (len 47), brains_is_provisioned() = true — and the same token on the server 401 {"error":"invalid token"}. The control experiment (same endpoint, same body) showed the desktop's real token landing in the identical bucket as a fabricated one, while an empty/absent bearer got a distinct missing bearer token. The server does distinguish "missing" from "invalid"; nothing in the app was asking.

brains_probe_credential now runs the structural check, then makes one cheap authenticated tools/call for whoami through the existing reqwest seam in commands/brains.rs (shared keep-alive client, its own 5s timeout so the boot path can't stall). The status line is the whole answer, so the body is never read.

The tri-state verdict

Verdict Trigger Effect on readiness UI
ok 2xx ready normal connect / straight to app
auth-rejected 401 / 403 not provisioned connect step, "Your brains sign-in expired — sign in again" + sign-in action; token scrubbed
unreachable timeout, DNS, connection refused, 5xx, 404 unchanged — last known verdict stands non-blocking connectivity note; the user stays signed in

verdict is null when the structural pre-filter short-circuited (nothing provisioned to test), which keeps "never signed in" distinct from "signed in and rejected". A throwing IPC call fails soft to no verdict, so a broken probe can never sign anyone out.

The verdict is cached in-process for 5 minutes and invalidated by every token write (brains_token_save, brains_provision_plugin, codex_provision_brains_mcp, and the scrub). Readiness is asked on boot and again on entering connect, so a normal session makes exactly one call.

The token mirrors

On auth-rejected the dead credential is scrubbed mirrors first, keychain last. That order is the point: keychain_token()'s macOS no-prompt migration re-seeds an empty v2 keychain item from ~/.claude/settings.json / ~/.codex/config.toml, so clearing the keychain first would let the dead token heal straight back in. Each location is best-effort — one failure must not leave the others intact.

The probe reads the credential from whichever mirror the spawned agent actually sends (Claude mirror preferred; Codex mirror as fallback, probed against its own registered url so a self-hosted endpoint is never checked against the default host — that 401 would be indistinguishable from a revoked token).

credentialExpired is deliberately sticky for the visit: the scrub empties the mirrors, so the next probe reports "not provisioned" and the reason we routed here would otherwise vanish out from under the copy.

Scope and overlaps

  • BRNS-DESK-032 (spawn-time token observability, MCP auth failure inside a live chat) — this PR owns the readiness path only, and introduces the user-facing expired-sign-in copy (onboarding_signInExpired*) plus the route-to-connect action for 032 to reuse.
  • BRNS-DESK-024 / 027 / 028 also touch the onboarding screens. The connect-stage change here is confined to the header block (badge / title / body / action) and two new state vars; the setup checklist, model picker and login stage are untouched.
  • Deliberately not changed: preflightSwitch / classifySwitch still ignore the credential verdict. A dead token would report ready at vendor-switch time, but SwitchVerdict has no expired case and inventing one belongs with the switch UX, not here. Worth a follow-up ticket.

Verification

  • cargo fmt --manifest-path src-tauri/Cargo.toml — clean.
  • npx prettier --check, npx eslint on every changed file — clean (0 errors; the 35 no-unused-vars warnings are pre-existing on main).
  • npm run i18n:check — 0 errors. Both new-string files updated (en.json, zh-CN.json).
  • npx vitest run1888 passed / 82 files, including 16 in brains-setup.test.ts (7 new). The one reported error is a missing jsdom in this environment's node_modules, unrelated to this change.
  • svelte-check ran and passed via the pre-commit hook, so the frontend types are genuinely checked.
  • cargo test was NOT run locally — no cargo target in this environment. The 5 new Rust tests are written to compile by inspection, copying the crate's existing idioms exactly: a raw tokio::net::TcpListener stub HTTP server (as in commands/diagnostics.rs), #[tokio::test], and no new dev-dependency. CI runs them.
  • No live server was contacted. Every HTTP assertion goes to a loopback stub; the unreachable case targets 127.0.0.1:1. The tri-state mapping is derived from the ticket's recorded control experiment, not from a call to mcp.mybrains.ai.
  • Needs a real device / CI to prove: the acceptance run itself — a fabricated token in both mirrors routing to connect with the expired message, a valid token still booting to the app, and pulling the network not logging the user out. The keychain scrub is also untested by unit tests on purpose: exercising it would mutate the developer's real OS keychain.

…e reporting ready

Readiness reported brains as connected from a token STRING being present, so a
revoked credential passed every check while every MCP call 401'd — a false green
light that made a stale token invisible for weeks on a real machine.

The structural check stays as the cheap pre-filter; a live `whoami` against the
brains MCP endpoint is now the answer. `brains_probe_credential` returns a
tri-state verdict (ok / auth-rejected / unreachable) behind a 5-minute in-process
cache that any token write invalidates, so a session makes at most one call.
Only 401/403 flips readiness: an unreachable server keeps the last known verdict
and shows a connectivity note, because offline is not signed out.

On a rejection the boot router lands on connect with distinct "your brains
sign-in expired" copy plus a sign-in action, and the dead token is scrubbed from
the keychain and both CLI mirrors — mirrors first, keychain last, so the macOS
no-prompt migration cannot heal it back in.
…rovision call

codex_deprovision_brains_mcp is synchronous; awaiting it failed to compile.

@nir-ssvlabs nir-ssvlabs left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The invariant that matters — offline must not read as signed out — holds end to end, and I traced the whole chain rather than the status mapping alone: verdict_for_status sends only 401|403 to AuthRejected and everything else to Unreachable; resolve_verdict returns unreachable_probe() without calling remember_verdict, so a transient failure cannot overwrite the last good answer; a transport Err lands in the same place; and the TS side sets credentialRejected from verdict === "auth-rejected" alone, with readiness gated on !credentialRejected. A 500 or a dead network therefore leaves readiness exactly where it was.

Worth calling out specifically because it would have been silent: scrubbing mirrors first and the keychain last. The macOS no-prompt migration re-seeds an empty keychain item from those mirrors, so the obvious order would have let the dead token heal straight back in after a successful scrub.

Checked: the status→verdict mapping, cache behaviour on the unreachable path, the transport error path, and the readiness expression that consumes it. Not read: the whoami call's own wire format — the status line is the whole answer here, as the description says.

Merge: ✅ into main.

@nir-ssvlabs nir-ssvlabs left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at 0a603675. One line — dropping .await from a synchronous codex_deprovision_brains_mcp(), i.e. a compile fix. Rust is now green on all three platforms, which it could not have been at the head I approved; my earlier pass verified the logic but leaned on CI for the build, and CI hadn't answered yet. Worth noting rather than glossing.

The scrub ordering I called out is untouched: claude mirror (573), codex mirror (576), keychain last (579), with each failure collected rather than short-circuiting. That was the property worth protecting here and the change doesn't move it.

Merge: ✅ into main.

Comment thread src/routes/+page.svelte
/** Record the live credential verdict the connect stage renders. A rejected
* credential is also scrubbed from the keychain and both CLI mirrors so it
* can't be healed back in from a mirror after the user signs in again. */
function applyCredentialVerdict(signal: CredentialSignal) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 BRNS-DESK-031: the scrub is defeated and a rejected credential can still enter the app. bootRoute keeps the dead token in brainsTok before applyCredentialVerdict scrubs the mirrors; runConnectSetup then runs the unconditional checklist, ensureBrains() re-provisions brainsTok ?? brainsToken() into settings.json, isBrainsProvisioned() turns green, and finishConnect ("Enter brains", gated only on setupReady) routes to stage app with the server-rejected token — which the keychain migration also re-seeds on next boot. Clear brainsTok in applyCredentialVerdict and gate ensureBrains/finishConnect on !credentialExpired so only reauthBrains can re-provision.

Comment thread src/routes/+page.svelte
style="margin:18px auto 0;height:44px;padding:0 22px;border:none;border-radius:12px;background:#1E5C43;color:#FBF8F1;font-family:inherit;font-size:15px;font-weight:700;cursor:pointer;display:flex;align-items:center;justify-content:center;gap:9px;"
>{#if loginBusy}<span
style="width:15px;height:15px;border-radius:50%;border:2px solid rgba(255,255,255,0.4);border-top-color:#FBF8F1;display:inline-block;animation:bspin .7s linear infinite;"
></span>Waiting for browser…{:else}{t(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Hardcoded English "Waiting for browser…" in the new expired-credential button while every sibling string in the block goes through t(); a zh-CN user mid-reauth sees untranslated text. CONTRIBUTING.md requires new user-facing UI text in BOTH messages/en.json and messages/zh-CN.json — add an onboarding_signInExpiredWaiting key to both catalogs and use t().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants