Skip to content

fix: refuse duplicate Claude account directories - #153

Merged
andrei-hasna merged 3 commits into
mainfrom
task/b1bf2b66-accounts-husking-guard
Aug 10, 2026
Merged

fix: refuse duplicate Claude account directories#153
andrei-hasna merged 3 commits into
mainfrom
task/b1bf2b66-accounts-husking-guard

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Task: b1bf2b66-2d90-4faf-851c-6ddb259033bc

Summary

  • refuse by default before credential writes when switch-account would place one Claude OAuth identity into a second registered account directory
  • cover broker repoint, legacy restore, and deliberate live-default apply while preserving canonical same-destination no-op behavior
  • report registered directory owner separately from the current live occupant
  • keep a deliberate --allow-duplicate-account-dir override; --yes does not bypass the guard

Local verification

  • bun test src/switch-account.test.ts src/symlink-broker.test.ts: 56 pass, 0 fail, rc=0
  • bun run typecheck: rc=0
  • shield review: no security issues found, rc=0
  • gitleaks staged and origin/main..HEAD: no leaks found, rc=0

Required hosted gate

The station-local full bun test was interrupted under sustained machine contention and is incomplete, not passing. Do not begin adversarial review or merge until the hosted pull-request CI full matrix passes on this exact head SHA.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #153 @ 37d19f6 — lens: correctness+security+gates, reviewer Trebius (1 of 1)

Reviewed the full diff for src/cli.ts, src/lib/switch-account.ts, and src/switch-account.test.ts against origin/main 9f56682, plus the surrounding switch, apply, auth-store, identity-index, wrong-dir, and repoint paths.

Commands and gates:

  • bun install — exit 0; setup only, not a gate.
  • bun run typecheck — exit 0; 0 TypeScript errors.
  • bun run test — exit 1; 1655 pass, 1 skip, 2 fail across 1658 tests. The direct failure is "registered profile dir needs no override — repoint runs for it too"; "bare Bun tests make zero inherited network, database, keychain, or tool side effects" is the isolation meta-test propagating that same nested-suite failure.
  • Targeted live-default reachability fixture — exit 0; actual output: {"dirKind":"live-default","duplicateCreated":true}.

Blocking P0/P1 findings:

  1. P1, high confidence — src/lib/switch-account.ts:297 gates duplicate detection on destinationProfile !== undefined || opts.liveDefault === true, but dirKind === "live-default" is reachable through an explicit --dir <live-default> or CLAUDE_CONFIG_DIR=<live-default> without setting liveDefault. That supported path reaches applyProfile and creates a second live copy of the target OAuth identity while the target profile directory still serves it. Source: operator-controlled explicit live-default directory; flow: switchAccount classifies live-default but skips the duplicate guard; sink: applyProfile restores the same account into live-default; impact: two refresh sessions race and can husk one credential. Minimal fix: key the guard on dirKind === "live-default", and add explicit-dir plus env-selected live-default regression tests proving refusal before bytes change.
  2. P1 required-gate regression — the declared test gate is red because src/switch-account-repoint.test.ts still asserts that switching one registered account into another registered profile directory needs no override. Update that repoint-specific test to use the new deliberate duplicate-account override (and retain its repoint assertions), then rerun the affected switch-account lanes and the declared gates.

Non-blocking follow-ups: none.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #153 @ 91d0af4 — lens: correctness+security+gates, reviewer Trebius (1 of 1)

Focused remediation review of the named blockers and their direct regressions:

  • Read the rebased remediation diff and the affected switch/repoint paths. The duplicate guard now keys on the resolved destination classification (dirKind === "live-default"), so explicit --dir ~/.claude, CLAUDE_CONFIG_DIR=~/.claude, and the default live directory all take the same guard path.
  • The concurrent branch update at ae8adb9ccb47569c84a8a04b18f16d8b19f65fb9 made the broker compatibility test's duplicate intent explicit. The remediation was rebased onto it without overwriting that change.
  • bun run test -- src/switch-account.test.ts src/switch-account-repoint.test.ts — exit 0 — 49 pass, 0 fail, 149 expect() calls across 2 files.
  • bun run typecheck — exit 0 — 0 TypeScript errors.
  • bun run test — exit 0 — 1659 pass, 1 skip, 0 fail, 12418 expect() calls across 1660 tests in 100 files.
  • shield review on the exact staged remediation before commit — exit 0 — No security issues found in staged changes.
  • Push to task/b1bf2b66-accounts-husking-guard — exit 0, fast-forward ae8adb9..91d0af4.

Blocking P0/P1 findings: none remain. The explicit and environment-selected live-default bypasses are covered by regression tests, and the repository-declared required gates pass.

Non-blocking follow-ups: none.

@andrei-hasna
andrei-hasna merged commit 0a71e91 into main Aug 10, 2026
5 checks passed
@andrei-hasna
andrei-hasna deleted the task/b1bf2b66-accounts-husking-guard branch August 10, 2026 10:31
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO

1. P0/P1 findings

none

I pressed hardest on one candidate before clearing it: the live-default test proves that applying a registered profile to the live default dir now refuses by default whenever that account holds a live door in its registered profile dir — which is every healthy registered profile. That is not a regression; it is the acceptance criterion itself. The husk mechanism named in the guard comment (one refresh token, two refresh sessions, loser blanked) arises precisely from the live-default-plus-profile-dir dual occupancy, the fleet's session model runs on profile dirs with --live-default as a deliberate exceptional path, and the override exists for the repair case. The hosted portable-claude Ubuntu and Windows lanes and the full hosted test gate (required=4, failures=0) passed on this exact head, so no production-shape flow regressed under it.

The other candidate I attacked and cleared: restoring an account into its own registered dir after a husk. accountLiveDoorsElsewhere(..., configDir) excludes the destination, so a switch of profile X into X's own dir where the account lives nowhere else proceeds — the guard cannot brick self-repair. And when the destination dir's occupant already matches the target (alreadyThisDir, case-insensitive), the guard is skipped entirely, preserving the no-op.

2. Evidence table

Acceptance item Evidence in diff / gates
1. Guard before broker repoint, legacy restore, live-default mutation Guard block inserted at the top of the mutation phase (immediately after warnings init), before the SINGLE-INODE BROKER section; the old const targetUuid at the broker site is removed and hoisted above the guard. Pre-mutation proven by tests: profile-dir refusal asserts dirEmail/dirAccessToken unchanged; live-default refusal asserts credential bytes equals(live.bytes). Hosted exact-head gates 4/4 green.
2. Reuse package identity index, no brittle literals Guard calls buildIdentityIndex(profiles, tool) + accountLiveDoorsElsewhere from ./identity-index.js; production code contains no account/profile name literals. The UUID literals (OWNER_UUID etc.) are test-only fixtures.
3. Canonical same-destination/current-account no-op preserved alreadyThisDir short-circuit (case-insensitive UUID compare) + test "keeps same-destination current-account no-op allowed even if another registered dir matches" → alreadyActive: true.
4. Registered owner vs current occupant kept separate in refusal profileAccountLabel(destinationProfile, …) vs occupantLabel(configDir, …) rendered as distinct facts; test writes a third UUID as occupant and asserts both Destination owner "destination-owner" … and current occupant current-occupant@example.com independently.
5. --yes cannot bypass Guard condition reads only opts.allowDuplicateAccountDir; message states "--yes does not override this"; dedicated test passes yes: true and asserts refusal.
6. Explicit, deliberate override New --allow-duplicate-account-dir CLI flag with its own help line, threaded as allowDuplicateAccountDir; used in exactly one condition (this guard); override path pushes a visible warning. Positive-override test passes.
7. Unregistered session-dir repoint preserved `duplicateGuardRequired = destinationProfile !== undefined
8. No credential exposure; rollback intact Refusal message carries emails/UUIDs (identifiers, not secrets); no token values touched. Code-only change, no migration; remediation commit touched only a test file. Staged shield, gitleaks, and origin/main..HEAD scans clean.
Regression instrument New refusal test failed pre-implementation (31 pass / 2 fail) and passes post (69/0 focused; hosted full test green) — the instrument demonstrated both states.
Compatibility remediation scope Diff to switch-account-repoint.test.ts only adds allowDuplicateAccountDir: true and renames the test to state the intent; production behavior unweakened. The rewritten switch-account.test.ts "needs no override" test was converted to a refusal test, which is the intended behavioral change, with the old permissive assertion removed rather than weakened.

3. Non-blocking P2/P3 follow-ups

  1. P3 — symlink alias of a registered dir. registeredProfileForDir uses resolve(), not realpathSync, so a symlink alias to a registered profile dir classifies as unregistered and skips the guard. Reachable only through the separate explicit --allow-unregistered-dir override, so already behind a deliberate gate.
  2. P2 — target identity missing skips the guard silently. A target profile with no persisted accountUuid bypasses the guard with no signal. This is also why the legacy tests (no accountUuid fixture) were unaffected. Consider a one-line warning when the guard is skipped for want of identity.
  3. P3 — override warning wording. duplicate-account-dir override used: "${profile.name}" also lives in … names the profile; it is the account that lives elsewhere.
  4. P3 — UUID case normalization inside the index. The guard compares toLowerCase() for alreadyThisDir, but whether accountLiveDoorsElsewhere matches case-insensitively is not visible in this diff; confirm the index lookup matches the guard's normalization so a case-variant UUID cannot dodge the elsewhere-check.

4. Exact head and merge-tree statement

Reviewed head ae8adb9ccb47569c84a8a04b18f16d8b19f65fb9 against origin/main 9f56682a6c3456d53bcb8a23336f1809df390f85. Head tree 1175f1ce919b3295b102b9cd2e42d3f07dd3f416 equals the merge-result tree (TREE_DIFF_RC=0): main has not moved beyond the branch base, so the tree reviewed here is byte-identical to the tree that lands on merge. This verdict is void if main moves before merge — re-run the merge-tree check in that case.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO

Exact candidate identities

  • Reviewed base: 9f56682a6c3456d53bcb8a23336f1809df390f85 (unchanged from initial review)
  • Initial GO head: ae8adb9ccb47569c84a8a04b18f16d8b19f65fb9
  • Update commit: 91d0af45e71d2d7774a19a95df60482aa7cf8127 — verified parent is ae8adb9, so the diff I reviewed is exactly the one added commit
  • Landing squash: 0a71e91b854d2553320393918cb7a8120631e221 — verified sole parent 9f56682
  • Tree identity: git rev-parse 91d0af4^{tree} and 0a71e91^{tree} both return 1af552844d08c97f11c148b94b21df855eafa5a0 — the landed bytes are byte-identical to the final head reviewed here. Hosted checks at that head: test, secret-scan, portable Ubuntu, portable Windows all terminal success.

P0/P1 findings

none

Acceptance assessment

The one-line production change replaces the caller's intent flag with the resolver's destination classification, and that is the correct predicate. dirKind is computed before the guard (switch-account.ts:218) as resolve(configDir) === resolve(liveClaudePaths().configDir), so it is true for every route by which the live default becomes the destination — explicit --dir ~/.claude, env-selected CLAUDE_CONFIG_DIR, deliberate --live-default, and the default fallthrough. The pre-fix predicate opts.liveDefault === true guarded only the flagged route: on a machine where the live default is not a registered profile dir, an explicit --dir or env-selected switch into it bypassed the duplicate guard entirely and wrote credentials. That was a real reachable gap in the husking guard, and this closes it.

The change is strictly widening — no supported path lost protection, and the paths that must still pass, still pass:

  • Same-destination no-op: unaffected; alreadyThisDir short-circuits before the predicate is consulted.
  • Override: allowDuplicateAccountDir still bypasses only this guard, on all live-default routes.
  • --yes: still cannot bypass (guard reads only the override flag).
  • Empty-registry live default: with no registered profiles, liveElsewhere is empty and the switch proceeds — a standalone ~/.claude machine is not blocked.
  • Registered-fleet live default: where the live default is itself registered, the guard already fired pre-fix via destinationProfile; no behavior change there.

Direct-regression assessment

Instrument proven in both directions, measured on temp extractions of both trees (repo untouched):

  • New tests overlaid on the pre-fix tree (ae8adb9): old_rc=1, 34 pass / 2 fail — the two failures are exactly the two new regressions (explicitly selected live-default dir, env-selected live-default dir). The tests detect the gap.
  • Fixed tree (91d0af4): new_rc=0, 36 pass / 0 fail / 101 expect() calls — the fix closes the gap and all 34 pre-existing tests in the file (refusal, --yes, override, no-op, live-default-flag, unregistered-dir controls) still pass unchanged.

Both new tests also assert the live credential bytes are unmodified after the refusal (live.bytes equality), so pre-mutation refusal is verified, not just the thrown message. The full-suite coverage at the exact landing tree is the hosted test gate, terminal success.

Non-blocking follow-ups

  1. P3, carried from the initial GO, unchanged in scope: live-default classification uses resolve(), not realpathSync, so a symlink alias to the live default dir still classifies as external — already fail-closed behind the separate --allow-unregistered-dir gate, so not a guard hole.
  2. P3: the opts.liveDefault flag now influences only the wrong-dir fallthrough guard, not the duplicate guard; a one-line doc touch on the option's comment would keep the two guards' responsibilities legible.

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.

1 participant