Scope quota warning state to accounts#2130
Conversation
|
CI follow-up: macOS shard 2 failed only in the unchanged Evidence:
The harness configures a 10s PTY timeout, while the runner also maps a short-lived command that leaves no captured output to I attempted a failed-only rerun through both |
|
Codex review: needs maintainer review before merge. Reviewed July 13, 2026, 4:03 PM ET / 20:03 UTC. Summary Reproducibility: yes. The supplied base/head sequence reproduces duplicate warnings for account A and a suppressed warning for account B on the current base, while the exact-head fresh bundle produces one warning per synthetic account through production paths. Review metrics: 3 noteworthy metrics.
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
Security Review detailsBest possible solution: Land the account-scoped reducer fix, then require the second-landed hooks branch to carry the documented stable-discriminator union and its email-less-account regression. Do we have a high-confidence way to reproduce the issue? Yes. The supplied base/head sequence reproduces duplicate warnings for account A and a suppressed warning for account B on the current base, while the exact-head fresh bundle produces one warning per synthetic account through production paths. Is this the best way to solve the issue? Yes. Extending the existing reducer key with an opaque stable owner while keeping display identity separate is the narrowest maintainable fix and follows the repository's provider-silo and privacy guidance. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c852c135e10c. 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
|
|
Added the requested redacted current-head runtime proof to the PR body. The identical disposable harness drives the production ordinary-refresh path with two synthetic selected accounts:
Both traces passed one focused test in one suite. The harness used isolated settings, stubbed outcomes, synthetic labels/tokens, and The separate CI failure remains the unchanged PTY harness timeout documented above; a maintainer rerun is still needed because contributor reruns return 403. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Added the requested fresh-bundle runtime proof to the PR body for exact head
@clawsweeper re-review |
clawsweeper
Bot
added
proof: sufficient
be7f11c to
746b488
Compare
![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: 746b488c38
ℹ️ 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 self.warningClaudeAccountDiscriminator( | ||
| strategyKind: result.strategyKind, | ||
| observation: context.claudeOAuthActiveAccountObservation, | ||
| oauthHistoryOwnerIdentifier: result.claudeOAuthHistoryOwnerIdentifier) |
There was a problem hiding this comment.
For Claude OAuth results this new quota-warning path reuses warningClaudeAccountDiscriminator, which prefers the active .claude.json account identity and only falls back to claudeOAuthHistoryOwnerIdentifier when that metadata is unavailable. If a refresh first records a warning under claude-oauth-owner:<owner> while the active account file is absent/unreadable, and a later refresh for the same OAuth credential sees the active account identity, the key changes to claude-account:<identity>; because the reducer fires on a first observation already below the threshold, the same account can get a duplicate low-quota notification without crossing again. Use the OAuth owner identifier consistently for OAuth quota-warning state when it is present, or migrate the fallback key.
Useful? React with 👍 / 👎.
2 participants
Summary
Root cause
QuotaWarningStateKeypreviously contained only the provider, warning window, and optional extra-window ID. When users switched accounts, one account's observation could clear, suppress, or re-arm another account's threshold episode. The stacked selected-Codex publication path also did not run the static threshold reducer.This change separates the opaque state discriminator from the optional display identity. Stable token-account UUIDs and hashed provider ownership keys scope reducer state; notification copy still follows the existing Hide Personal Info setting.
Runtime proof
I ran an identical disposable harness at base
c852c135and current headbe7f11c4. It drives the productionUsageStore.refreshProviderpath through six ordinary selected-account refreshes, with two synthetic token accounts and fixture-only provider outcomes. A notifier spy records the sameQuotaWarningEventpassed to the notification layer.Sequence: A 40%, B 40%, A 55%, B 30%, A 55%, B 55%, with a 50%-remaining threshold.
Base shared-state trace:
Account B's recovery re-armed A, so A emitted twice; B's own crossing was then suppressed.
Current-head account-scoped trace:
Each account retained an independent episode: no duplicate A warning and no suppressed B warning. Both runs passed one test in one suite using:
CODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 swift test --filter QuotaWarningAccountRuntimeProofTestsThe proof used isolated settings, synthetic labels/tokens, and stubbed provider outcomes. It did not read a real account store, browser data, provider session, or Keychain item; the disposable harness was not added to either branch.
Validation
CODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 OS_ACTIVITY_MODE=disable swift test --filter 'UsageStoreAccountQuotaWarningTests|UsageStoreSessionQuotaTransitionTests|CodexSessionQuotaFalseRestoreTests|ClaudeExtraWindowQuotaWarningTests|PredictivePaceWarningTests'- 86 tests in 5 suites passedmake check- passed; SwiftFormat reported 0 files requiring formatting and SwiftLint reported 0 violations across 1,426 filesmake teston this commit combined with the test-only isolation from Isolate Codex CLI account test from local data #2128 - all 639 selections passed across 54/54 groups, with zero first-pass failures, retries, or timeoutsb14629bd- 51 focused tests in 5 suites passed;make checkreported 0/1,437 files requiring formatting and 0 SwiftLint violations across 1,436 files;git diff --checkpassedgit diff --check- passedCoordination with #2001
#2001 now has a dedicated
QuotaLowHookUsageKeyat headb14629bd, but derives its account key fromsnapshot.accountEmail. That compiles independently, yet is not a sufficient stable discriminator: Claude OAuth accounts can be email-less, generic token accounts can fall back to mutable or duplicate labels, and Codex owners can share an email across workspace/source/auth contexts.The refreshed disposable merge simulation produced exactly one conflicted file,
UsageStore+QuotaWarnings.swift, with two adjacent regions. The minimal correct union is to keep #2001's dedicated hook key, notification-independent hook dispatch, and payload behavior while feeding this PR'saccountContext.discriminatorintodispatchQuotaLowHooks(accountKey:);accountContext.displayNameremains presentation-only.The combined tree passed 51 focused warning/hook tests across five suites, including a simulation-only regression proving two email-less Claude owners keep separate hook baselines while native notifications are disabled. It also passed
make checkandgit diff --check. No published branch was changed.Whichever PR lands second must carry that union and the email-less hook regression. If #2001 lands first, this PR will be rebased and the integration included before merge.
A recording was not available because the host was locked; the exact-head packaged-app runtime log is included above.
Fresh-bundle runtime proof
I also ran the freshly packaged current-head app, not the disposable harness:
The exact unmodified bundle ran with an isolated HOME/CODEX_HOME/config, both Keychain-access guards set, and only sub2api enabled against a loopback HTTP fixture. The two token accounts had synthetic labels and fixed UUIDs. This does not claim authenticated-provider behavior and did not read a real provider account, browser store, or Keychain item.
Because the host was locked, I drove the same production operations used by the segmented account switcher under LLDB:
setActiveTokenAccountIndex, cached-account activation, andrefreshProvider(.sub2api). Provider outcomes were not stubbed: the packaged app used its production Sub2API URLSession fetcher against the loopback server, then its production reducer,SessionQuotaNotifier, andQuotaWarningAlertOverlayController.The redacted server trace was:
The real notifier breakpoint was hit exactly twice:
The debug bundle was not authorized for Notification Center, so its native UNUserNotificationCenter posts were skipped; the enabled on-screen alert path did present twice. The final in-process state contained exactly two independent owner keys:
The source worktree remained clean. The temporary server/app were stopped, the pre-run debug preference snapshot was restored exactly, and the installed release app was left running.