Skip to content

fix(onboarding) [BRNS-DESK-024]: declining sign-in writes nothing to the claude config - #86

Open
sebastian-ssvlabs wants to merge 1 commit into
mainfrom
fix/desk-024-decline-writes-nothing
Open

fix(onboarding) [BRNS-DESK-024]: declining sign-in writes nothing to the claude config#86
sebastian-ssvlabs wants to merge 1 commit into
mainfrom
fix/desk-024-decline-writes-nothing

Conversation

@sebastian-ssvlabs

Copy link
Copy Markdown

Summary

  • Declining sign-in silently modified the user's Claude Code CLI config. "Skip for now" → click nothingbrains@brains was enabled in their live ~/.claude/settings.json with no credentials, the brains marketplace was registered, and the Mission Control skill was written into ~/.claude/skills and ~/.agents/skills.
  • The damage is outside this app: their own claude sessions then loaded a brains MCP that could only fail to authenticate, on every session — and uninstalling the desktop app never undid it.
  • Declining is now a no-op on the user's machine: two independent guards, plus a boot-time heal for machines already polluted in the field.
  • The sign-in path is unchanged — plugin + config + skill are still provisioned and the user still lands in the app.
  • Closes BRNS-DESK-024 (P2, effort M, onboarding / desktop / provisioning).

Which writes happened, from which unguarded path

runConnectSetup() fires from an $effect the instant stage === "connect", and skipLogin() sets exactly that — so merely landing on the second screen ran setup. Inside ensureBrains() only the middle step was token-gated:

Step External write Was gated?
ensureBrainsPlugin() claude plugin marketplace add + install + enableenabledPlugins["brains@brains"] = true no
provisionBrainsPlugin() pluginConfigs["brains@brains"].options.{endpoint,token} yes — if (tok)
installMissionControlSkill() ~/.claude/skills/brains-mission-control/, ~/.agents/skills/brains-mission-control/ no

Net result of declining: enabled: true, config: undefinedbrains_is_provisioned() returned false, so the app knew the state was incomplete after having already mutated the user's config.

The two guards

  1. ensureBrains() resolves the token first and returns before touching anything external, so both unguarded steps now sit behind the same check that already guarded provisioning. The if (tok) conditional and the tok ? … : … error branch collapse away, since past that point a token is guaranteed. The other two installMissionControlSkill() call sites were already covered and are untouched: ensureBrainsCodex() returns on !tok above its call, and bootRoute()'s idempotent refresh sits inside if (ready), below the !brainsTok early return.
  2. runConnectSetup() stops after the read-only vendor probe when there is no token, so navigation alone can no longer provision. refreshVendor() only reads CLI/auth state.

Deliberately narrower than "explicit user action only": the connect screen's own CTA is finishConnect, which is gated on setup already being complete, so wiring setup to it would deadlock, and a returning signed-in user must still auto-resume the step they left. The token — minted only by an explicit brains sign-in — is the consent signal, and guard 1 makes the writer itself refuse regardless of caller.

The visible end state of declining is unchanged: today's decline path already ends on the brains card in error with "Sign in to brains first, then finish setup." — it now reaches that state without the writes. No copy or label changes, so no overlap with the in-flight #27 / #28.

Rollback / heal

New brains_revert_orphaned_setup command, called fire-and-forget from bootRoute() only on the no-token branch. It acts on one signature — enabledPlugins["brains@brains"] === true with no token in pluginConfigs — which a provisioned install, a disabled plugin, and an untouched config can never match. On a match it removes both settings.json keys and both skill dirs (Rust), then best-effort unwinds the plugin and its marketplace through the same claude CLI commands that registered them, and logs everything reverted via client_log. Empty result = clean machine, nothing runs.

Two consequences worth flagging for review:

  • Logout leaves that same signature (brains_deprovision_plugin scrubs the token but leaves the plugin enabled), so a logged-out user's next launch reverts it too. That is the same broken state by any other name, and re-login re-installs it — but it is a behaviour change beyond the literal ticket.
  • The command is registered in lib.rs only, not in web_server/dispatch.rs — deliberately, to avoid colliding with PR fix(onboarding) [BRNS-DESK-019]: the brains step completes remotely, and stops leaking method names #63, which owns that table. Over a remote transport the call rejects with unknown method and the heal is skipped; the frontend swallows that.

Overlap with PR #63

#63 (fix/desk-019) fixes the transport side of the same area and states it does not touch routes/+page.svelte. This PR touches +page.svelte, lib/brains-setup.ts, commands/brains_setup.rs and one lib.rs line — no shared hunks, and nothing here re-introduces a raw RPC name into the UI (the tokenless path sets a fixed human string, never an error body). #63's repro note — "installMissionControlSkill() is the first WS-missing call because skipLogin sets stage = "connect" with no brains token" — describes the very call this PR stops making on that path; its parity fix still stands for the signed-in path, which does call it.

Verification

  • npx vitest run src/lib/brains-setup.test.ts — 14 passed (3 new); full suite npx vitest run — 82 files / 1886 passed (the one error is pre-existing: print-sanitize.test.ts wants the jsdom env, not installed locally)
  • New Rust unit tests: the orphan signature matches only enabled-without-token (provisioned / disabled / absent / endpoint-only cases pinned), and skill removal exactly undoes install_mission_control_into and is a no-op the second time
  • npx prettier --check, npx eslint on all changed files — clean (0 errors; the 35 warnings are pre-existing unused-symbol ones)
  • cargo fmt --check — clean; svelte-check0 errors (129 pre-existing warnings) via the pre-commit hook, which passed in full
  • Not verified: the FTU repro itself (quit app → reset the three token sources → Skip → inspect config) was not run — no local build env, so no cargo build / npm run build / GUI launch. The Rust command's real-$HOME IO path, the claude plugin uninstall / marketplace remove exit behaviour on a polluted machine, and the pre-polluted self-heal are reasoned from the code, not observed. Worth a manual pass on a real machine before merge.

…the claude config

"Skip for now" navigated to the connect stage, whose $effect ran setup
immediately, and only the middle step there was token-gated. So declining
enabled brains@brains in the user's live ~/.claude/settings.json with no
credentials, registered the marketplace, and wrote the Mission Control skill
into ~/.claude/skills and ~/.agents/skills — breaking their own CLI sessions,
and surviving an app uninstall.

Two independent guards, both required:

- ensureBrains() resolves the token first and returns before touching anything
  external, so ensureBrainsPlugin() and installMissionControlSkill() are behind
  the same check that already guarded provisionBrainsPlugin(). The Codex analog
  and the boot-time skill refresh were already token-gated.
- runConnectSetup() stops after the read-only vendor probe when there is no
  token, so landing on the connect screen can no longer provision by navigation.

Adds brains_revert_orphaned_setup to heal machines already polluted in the
field: at boot with no token, an enabled-but-tokenless plugin entry is reverted
(settings.json keys, plugin, marketplace, both skill dirs) and logged. A
provisioned install never matches that signature.

@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.

Both halves check out. The gate in ensureBrains() sits ahead of all three external writes — ensureBrainsPlugin, provisionBrainsPlugin and installMissionControlSkill are now unreachable without a token, which is the right place for it given the middle step was the only one previously gated.

The heal is the part that worried me, since it mutates a user's ~/.claude/settings.json unprompted at boot, and is_orphaned_decline_artifact is correctly narrow: plugin_enabled && token.is_none(). A provisioned install carries a token so it can't match; a disabled plugin can't match; an untouched config can't match. The one shape I went looking for — someone signed out of the desktop app but with a working CLI setup — still has the token in pluginConfigs, so it reads as provisioned and is left alone. The signature only fires on state that is genuinely non-functional.

Checked: the guard's position relative to each write, the heal's signature against the provisioned / disabled / untouched cases, and that save_cli_config is only reached behind a match. Not read: the Rust-side second guard's own callers — the signature check stands on its own regardless of who reaches it.

Merge: ✅ into main.

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.

2 participants