fix: scope session-equivalent forecast history by account - #276
Conversation
|
Thanks for the PR, I will review it ASAP. |
Finesssee
left a comment
There was a problem hiding this comment.
Requesting changes: for the provider this PR is about, the account discriminator is structurally always None, so the headline bug remains.
Trace of the blocker, all at head 98e397b:
spawn_provider_refreshes(apps/desktop-tauri/src-tauri/src/commands/providers.rs:327) sources the discriminator fromProviderRefreshInputs::load()'sTokenAccountStoreread (token-accounts.json).TokenAccountSupport::for_providerexplicitly excludes Codex (rust/src/core/token_accounts.rs; pinned byassert!(!TokenAccountSupport::is_supported(ProviderId::Codex))), and the normal writers (codexbar account add/list/active, the usage env-override path) all reject unsupported providers — so a Codex entry can never exist in that store through any supported flow.- Codex accounts live in the separate
codex_accountsAccountStore(accounts.json under the app-support dir, the ADR 0003 lanes), which this refresh path never consults. - The ambient Codex provider publishes neither
account_emailnoraccount_organization(nothing in rust/src/providers/codex sets them) — the new comment infrom_fetch_resultitself concedes this.
Net effect: token_account_id = None and forecast_account_key = None on every real Codex refresh, so record_provider_windows / forecast_for_provider keep keying every Codex sample as (codex, None). Switching accounts still blends burn history into one series — the exact bug this PR claims to fix.
Required before merge:
- Derive the forecast identity from the same resolved
CODEX_HOME/auth.jsonthe fetch path uses (rust/src/providers/codex/api.rs), and have the Codex provider publish it: email first, falling back toprovider_account_id/auth_subjectfrom the auth.json identity. Thecodex_accountsmanager already parses exactly those fields viaload_identity, so the parsing precedent exists in-repo. - A real refresh-boundary regression test proving two Codex accounts on one machine yield two distinct history keys end-to-end. The current targeted tests are insufficient: they inject synthetic ids (
token-account:aaaaaaaa-…) directly intofrom_fetch_result, so they cannot observe the TokenAccountStore gap. - One canonical account-identity helper instead of duplicated precedence:
forecast_account_keydeliberately mirrorsquota_notification_account_identity's token-account → email → organization ordering (its own doc comment says so). Collapse both into a single shared helper so the notification and forecast subsystems cannot drift apart again.
Explicit guidance on the identity source: do not substitute CodexAccount.id as the discriminator. Undiscovered ambient/managed accounts receive fresh Uuid::new_v4() ids — candidate_account and build_discovered_account in rust/src/codex_accounts/account_manager.rs fall back to Uuid::new_v4() whenever identity matching finds no existing account — so one real account would keep bifurcating into new history keys across discoveries.
Problem
Session-equivalent forecast history is keyed by
provider_idalone, so it is shared across every account on that provider.SessionEquivalentHistoryStoreandLastFullSessionEstimateStoreare bothHashMap<String, _>keyed by provider, andbridge.rspassesid.cli_name()— the literal string"codex"or"claude"— with no account discriminator:Fetch itself is single-account, so nothing mixes concurrently. The problem is switching:
switch_active_account()changes which account is fetched, while the ring buffer — sizedsample_limit * 8, minimum 24 entries — still holds the previous account's observations.After a switch the burn median is computed from a mixture of two accounts' samples until the old entries age out. Moving between plans of different sizes produces a silently wrong "estimated windows to exhaust" for the following several windows, with nothing in the UI signalling the number is unreliable.
The existing source comment already anticipates the correct key:
This is the same class of fix as #203, which scoped quota notifications by account identity. That change never reached forecast history.
Change
Introduce
ForecastScope { provider_id, account_key }and key both stores by it.SessionEquivalentHistoryStore.by_provider→by_scopeLastFullSessionEstimateStore.by_provider→by_scoperecord_provider_windows(),forecast_for_provider(),remember_full_session_estimate(),record(),histories()take the account discriminatorSourcing the discriminator
Claude publishes
account_email, but Codex publishes neither email nor organization — under the ADR 0003 ambient/managed lane split its only stable discriminator is the managed token-account id. Keying off the email alone would have left this bug live for Codex, which is the provider with first-class multi-account support.So the identity is resolved where it actually lives and threaded down:
spawn_provider_refreshes(hasinputs.token_accounts) →refresh_provider→fetch_provider_snapshot→from_fetch_resultA new
forecast_account_key()deliberately mirrorsquota_notification_account_identityprecedence —token-account:{uuid}→ email →org:{org}. It is a separate function only because that one consumes an already-builtProviderUsageSnapshotwhile the forecast needs the key while the snapshot is being built.Behaviour for single-account users is unchanged: one account means one scope.
Notes
UsageSnapshot. A comment marks that it should be hashed rather than written in the clear if this history is later persisted to disk.account_key: None, which is its own bucket rather than a catch-all shared with identified accounts.Tests
history_is_isolated_per_account— two accounts on one provider; asserts neither account's samples appear in the other's history and that theNonebucket stays empty.forecast_account_key_matches_notification_identity— pinsforecast_account_keytoquota_notification_account_identityacross all 8 combinations of email/org/token, so the two subsystems can never see one account as two identities. If they diverged, an account's burn history would split and silently halve the sample count behind every forecast.history_ring_retains_latest_samplesupdated for the new signature.Verification
Note on
cli::tty_runner::tests::test_run_sends_script_through_pty: it failed on onefull-suite run here and passed on others. I checked it against untouched
main(
116af24b), where it also fails under full-suite parallel load, so it is pre-existingflakiness in this environment and unrelated to this change.
No CUA Driver proof included: this changes no UI, tray, or Settings surface. The forecast line in
MenuCardDetailsrenders exactly as before for a single-account user.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.