Fix Claude same-dir account-switch history mis-attribution (#1886)#1903
Conversation
|
Codex review: found issues before merge. Reviewed July 5, 2026, 8:42 AM ET / 12:42 UTC. Summary Reproducibility: yes. The linked issue and PR discussion give a live same-directory Claude Review metrics: 3 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Preserve established owner bindings once set, quarantine mismatched active-account observations, and allow new owners to bind only from corroborated evidence with focused regression coverage. Do we have a high-confidence way to reproduce the issue? Yes. The linked issue and PR discussion give a live same-directory Claude Is this the best way to solve the issue? Not yet. The overall design is a narrow maintainable fix, but the established-binding rebind path should be resolved before merge because it can weaken the quarantine that the fix relies on. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 780767953539. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (8 earlier review cycles)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b68895d1ac
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } else if map[owner] == nil, let currentUuid { | ||
| // First sighting of this owner: bind it to the account active right now. | ||
| // Never OVERWRITE on mismatch (that is the stale case handled above). | ||
| map[owner] = currentUuid | ||
| self.persistClaudeOAuthAccountUuidMap(map) |
There was a problem hiding this comment.
When this new ClaudeOAuthHistoryOwnerAccountUuidMapV1 map is empty (for example on the first run after upgrading) but planUtilizationHistory already contains a bucket for the old OAuth owner, a background poll after /login can still receive the stale old historyOwnerIdentifier while ~/.claude.json already reports the new account UUID. This branch treats that stale owner as a first sighting, stores oldOwner -> newUuid, and then records the sample under the old owner's history, so the same same-directory switch mis-attribution still happens for migrated users until the owner had previously been observed by this new code.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Thanks for the reviews. Addressed the P2 (empty-map first-sighting / map poisoning) flagged by both Codex and ClawSweeper in
Real-behavior proof (now in the PR body): ran the patched
@clawsweeper re-review |
|
@codex review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bf824dccb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } else if map[owner] == nil, | ||
| let currentUuid, | ||
| ProviderInteractionContext.current == .userInitiated |
There was a problem hiding this comment.
When a user already has Claude OAuth history from before this version, the new UUID map starts empty; if they then switch Claude accounts before any explicit user-initiated refresh, normal timer/startup refreshes still run with ProviderInteractionContext.current == .background (see UsageStore.swift around the refresh task setup), so this branch leaves effectiveOwner as the stale owner and appends the new account’s background sample to the previous account’s bucket. That leaves the account-switch contamination unfixed for the upgrade path unless the user happens to force a refresh first; consider quarantining or seeding a binding for known scoped owners instead of failing open on background first-sightings that already have history.
Useful? React with 👍 / 👎.
|
Re: the follow-up P2 on the empty-map upgrade window — this is a deliberate, documented tradeoff rather than an oversight, and I want to lay out why fail-open is the default (with the quarantine variant ready if maintainers prefer it). The genuine regression this PR introduced — map poisoning (binding a stale owner to the new UUID) — is fixed: background polls never create a binding. What remains is the narrow warm-up window where an upgraded user switches accounts before the map is warmed by any user-initiated refresh; there the background sample fails open, identical to pre-fix behavior. I considered the suggested quarantine-on-known-scoped-owner, but it has a broad downside: So: fail-open keeps history continuous and never poisons the map; the residual contamination is bounded to the pre-warm-up window and self-heals on the first user-initiated refresh. If the maintainers would rather guarantee no cross-account contamination at the cost of that background-history gap, the quarantine variant is a ~10-line follow-up (drop background first-sightings whose owner already has a scoped bucket) — happy to add it. That's the product call framed as ClawSweeper's option 1 vs 2. |
|
@clawsweeper re-run |
clawsweeper
Bot
added
proof: sufficient
|
clawsweeper's option 1 vs 2, condensed (full rationale in the PR description):
If you prefer guaranteed no-contamination, the quarantine variant (option 1) is ~10 lines — I can push it same-day. |
…1886) Background polls keep Claude-Keychain access gated (steipete#1848), so the credential cache can serve a previous account's stale credential after an in-place /login switch on the shared ~/.claude, mis-attributing usage to the previous account's plan-utilization history bucket. Corroborate the served history owner against the active account in ~/.claude.json (prompt-free, never gated) and quarantine the sample on a positive mismatch; recover on the next user-initiated refresh. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ds (steipete#1886) A background poll can serve a previous account's stale credential (the keychain freshness sync is gated off in background), so binding an unmapped owner to the currently-active account on a background first sighting could poison the owner->accountUuid map for upgraded users who already have owner-keyed history. Gate the first-sighting binding on ProviderInteractionContext == .userInitiated, where the freshness sync guarantees the served owner reflects the active account. Background polls still honor an existing binding (quarantine on mismatch) but never create one; unmapped background reads fail open. Addresses the Codex and ClawSweeper P2 on the empty-map upgrade path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
steipete
force-pushed
the
fix/claude-account-switch-same-dir-history
branch
from
8bf824d to
70dc617
Compare
|
@clawsweeper re-review Maintainer repair is now on current |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
rating: 🦞 diamond lobster
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfe9ccda2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return nil | ||
| } | ||
| // Two stable exact-Keychain observations repair a binding poisoned by a non-atomic login. | ||
| map[evidence.owner] = currentAccountIdentity |
There was a problem hiding this comment.
When an owner is already bound to account A, a non-atomic /login can make ~/.claude.json report account B while the current Keychain/cache still serves owner A; after two such stable observations this branch overwrites map[ownerA] with B and returns the owner, so future background stale-owner samples see mapped == currentAccountIdentity and append account B's usage into owner A's history bucket. Established owner mismatches should stay quarantined rather than repairing the binding to a different active UUID.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦐 gold shrimp
|
Landed as Exact-head proof for
This lands the history-isolation portion only. #1886 remains open for the separate visible-menu account-selection problem. Thanks @ss251! |
2 participants
Summary
Addresses the history-isolation portion of #1886.
Claude Code can update the shared Keychain item and
~/.claude.jsonduring/loginwhile~/.claude/.credentials.jsonremains stale. A background refresh could then attribute a successful OAuth sample to the wrong account-history lifecycle.Final design
This intentionally does not change visible menu snapshot selection. #1886 should remain open for that remaining behavior.
Proof
swift test --filter 'ClaudeOAuthHistoryCredentialRoutingTests|UsageStorePlanUtilizationClaudeIdentity'— 37 tests passed across 3 suites.make check— clean; SwiftFormat and strict SwiftLint passed.make test— all 47 shards passed./Users/steipete/Projects/agent-skills/skills/autoreview/scripts/autoreview --mode branch --base origin/main— clean; no accepted/actionable findings.Exact reviewed head:
bfe9ccda2ba7c07db6648ba246e1a63eed2cfe99Notes
/loginbehavior; maintainer hardening and isolated regression tests cover the final evidence boundary.