Proj/rework llm billing#653
Conversation
Adds the billing transport layer (BillingEvents + team:list) backed by an HttpBillingService against BRV_BILLING_BASE_URL and a file-backed pinned-team store. The header trigger drops
…der.json, non-interactive UX Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…pinned team from suggestions, preserve unknown config keys
[ENG-2512] rework billing chain: DTO + storage + non-interactive UX
…h race - Remove daemon-side workspace fallback from `resolveBillingTeamId`; it's now a pure prediction helper (pin / single-paid / undefined). - Add `projectPath` to `billing:getPinnedTeam`, `billing:setPinnedTeam`, and `auth:getState` request bodies so the daemon resolves the project explicitly per request instead of via per-client state. - Add `computeTeamPreselection` so the team picker can visually pre-select a row without writing the pin to disk until the user clicks Confirm. - `useBillingDisplay`: fall through to auto-pick when the pin is stale; ignore pin entirely when the user has no paid teams (free monthly always wins). - Header: drop the workspace fallback; show the team that's actually being billed. - `team-select-step`: disable Confirm when the current selection isn't in the team list (stale workspace id or removed team). - `BrvApiClient.request`: pre-check `socket.connected` and fast-fail on disconnect so requests don't silently wait the full 5s timeout during project-switch socket teardown.
…orkspace fallback in provider list, harden tests - billing-handler: reject empty `projectPath` for `billing:getPinnedTeam` and `billing:setPinnedTeam` so a stray empty string from the client can't end up writing the pin to the daemon's CWD. - provider-select-step: drop the `?? teamId` workspace fallback to match the header (was the only remaining surface still showing the old behavior). - set-pinned-team: read `projectPath` at mutate-call time via `useTransportStore.getState()` so a project switch mid-dialog can't write the pin to the previous project. - Tests: add behavioral coverage for `BrvApiClient.request` (not-connected pre-check, disconnect-before-ack, normal ack) and for empty-projectPath rejection in both billing handlers.
…queries, assert listener cleanup
- get-pinned-team / get-auth-state: gate react-query on `projectPath !== ''` so we don't fire a doomed request on initial mount (cached error envelope is now skipped entirely).
- api-client tests: assert `socket.off('disconnect', ...)` runs on both the ack-success and timeout paths so a future regression that drops the cleanup is caught.
…rojectPath check Reverse the spread order in `useGetAuthState` (was queryConfig last) so the projectPath gate was being silently overridden by `auth-initializer.tsx`'s `enabled: apiClient !== null`. Switch both hooks to an explicit AND-merge form (`base.enabled !== false && (queryConfig?.enabled ?? true)`) so neither the gate nor a caller's enable check can clobber the other.
feat: [ENG-2512] rework billing-team resolution and fix project-switch race
|
Claude finished @ncnthien's task in 7m 16s —— View job PR Review: Rework LLM Billing
Overall: 4 / 5 — solid, ships-ready with one UX bug worth fixing before mergeThis is a thoughtful rework. The discriminated-union Inline commentspraise:
issue (UX, worth fixing pre-merge): issue (UX): issue (non-blocking): issue (low/security): issue (low): nitpick: nitpick: Other observations (not inlined, no action needed)
RecommendationAddress the "Personal free credits" UX bug (the multi-paid-org case) and the free-tier pin guardrail before shipping; the rest are non-blocking and can land as follow-ups. Pin-write authentication and atomic write are worth doing soon but don't gate this PR. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| public async getUsages(sessionKey: string): Promise<BillingUsageDTO[]> { | ||
| const [rawUsages, tiers] = await Promise.all([this.fetchRawUsages(sessionKey), this.getTiers(sessionKey)]) | ||
| const tierByOrg = new Map(tiers.map((t) => [t.organizationId, t])) | ||
| return rawUsages.map((usage) => { | ||
| const tier = tierByOrg.get(usage.organizationId) | ||
| return {...usage, isTrialing: tier?.isTrialing ?? false, tier: tier?.tier ?? 'FREE'} | ||
| }) | ||
| } |
There was a problem hiding this comment.
issue (non-blocking): Promise.all rejects if either /billing/usages or /billing/organizations/tiers fails, so a transient tiers outage will fail the whole getUsages call even though the raw usages alone would still be a useful response (every org would default to tier: 'FREE', which is the same fallback path you already implement for missing tier entries).
Consider Promise.allSettled so a tiers failure degrades gracefully:
const [rawResult, tiersResult] = await Promise.all([
this.fetchRawUsages(sessionKey),
this.getTiers(sessionKey).catch(() => [] as BillingOrganizationTierDTO[]),
])That keeps the resilient billing-pill UX (status falls back to "FREE" tier) instead of silently swallowing the whole resolve into undefined upstream in resolveBillingForProject.
| private setupGetPinnedTeam(): void { | ||
| this.transport.onRequest<BillingGetPinnedTeamRequest, BillingGetPinnedTeamResponse>( | ||
| BillingEvents.GET_PINNED_TEAM, | ||
| async (data) => { | ||
| if (!data.projectPath) return {error: 'projectPath is required'} | ||
| try { | ||
| const store = this.billingConfigStoreFactory(data.projectPath) | ||
| const teamId = await store.getPinnedTeamId() | ||
| return teamId === undefined ? {} : {teamId} | ||
| } catch (error) { | ||
| return {error: getErrorMessage(error)} | ||
| } | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
suggestion (non-blocking): setupGetPinnedTeam and setupSetPinnedTeam are the only billing handlers that don't gate on authStateStore.getToken()?.isValid(). They only validate projectPath. Since the pin is consumed at the next byterover task to decide which team to bill, an unauthenticated client can quietly write a pin that the user wouldn't normally see.
Even though the daemon is localhost-only, the rest of the handlers in this file follow the "must be signed in to touch billing state" rule, so the inconsistency is the bug. Adding if (!token?.isValid()) return {error: NOT_AUTHENTICATED_ERROR ...} to both endpoints keeps the contract uniform and prevents stale pins from outliving a logout.
| public async setPinnedTeamId(teamId: string | undefined): Promise<void> { | ||
| const json = await this.readJson() | ||
| const billing = {...json.billing} | ||
| if (teamId === undefined) { | ||
| delete billing.pinnedTeamId | ||
| } else { | ||
| billing.pinnedTeamId = teamId | ||
| } | ||
|
|
||
| const next: ProviderConfigJson = {...json, billing} | ||
| if (Object.keys(billing).length === 0) delete next.billing | ||
| await this.writeJson(next) | ||
| } |
There was a problem hiding this comment.
issue (low): setPinnedTeamId performs a non-atomic read → modify → writeFile. If two clients (e.g. webui dialog and brv providers connect --team) race on the same project, the later writer's view of json will not include the earlier writer's mutation and the earlier write is silently lost.
The blast radius is small (only one key in billing), but the file also preserves arbitrary unknown keys from the same JSON, so a concurrent write to the pin can also clobber unrelated keys an outside tool added. A simple fix: write to a temp path next to the file then fs.rename (atomic on POSIX/Win). The added cost is negligible since this is rare (~user-initiated).
| const isByteRoverActive = activeProvider?.id === BYTEROVER_PROVIDER_ID | ||
| const providerLabel = buildProviderLabel(activeProvider, activeConfig) | ||
|
|
||
| const {data: teamsData} = useListTeams() |
There was a problem hiding this comment.
nitpick: useListTeams() here fires unconditionally, even before sign-in. The server returns {error: NOT_AUTHENTICATED_ERROR} (not a rejection), so the query "succeeds" but the response carries an error string the UI never surfaces — net effect is a wasted round trip on every unauthenticated render and a noisy log channel.
useListTeams already accepts {enabled}:
| const {data: teamsData} = useListTeams() | |
| const isAuthorized = useAuthStore((s) => s.isAuthorized) | |
| const {data: teamsData} = useListTeams({enabled: isAuthorized}) |
Same pattern as useListBillingUsage in useBillingDisplay.
| teams.map((team) => { | ||
| const teamUsage = usageByTeam[team.id] | ||
| const roleLabel = team.id === workspaceTeamId ? 'Workspace' : team.isDefault ? 'Default' : undefined | ||
| return ( | ||
| <TeamRow | ||
| avatar={<TeamAvatar avatarUrl={team.avatarUrl} name={team.displayName} />} | ||
| badges={ | ||
| <> | ||
| {teamUsage && <TierBadge isTrialing={teamUsage.isTrialing} tier={teamUsage.tier} />} | ||
| {roleLabel && <RowBadge>{roleLabel}</RowBadge>} | ||
| </> | ||
| } | ||
| credits={<CreditsPill tone={getBillingTone(teamUsage)} usage={teamUsage} />} | ||
| key={team.id} | ||
| name={team.displayName} | ||
| onSelect={() => setSelection(team.id)} | ||
| selected={selection === team.id} | ||
| /> | ||
| ) | ||
| }) |
There was a problem hiding this comment.
issue (UX): This list renders every team the user belongs to, including FREE-tier teams. Selecting one of those teams persists a pin that the backend then resolves to a paid usage entry that doesn't exist — build-status-billing.ts:35 returns {organizationId: resolved, source: 'paid'} with no remaining/tier, which formatBillingLine then renders as Billing: <name> (usage unavailable). Same trap exists in the CLI's --team flag (no tier filter on matchTeam).
Two reasonable options:
- Hide / disable rows where
usageByTeam[team.id]?.tier === 'FREE'so users can't pick a tier that can't be billed against. - Validate in
setBillingPin(server-side) that the team is in the user's paid-org list, and surface "Pick a paid team" if not.
The free-tier message you already show in showFreeTierView only fires when all the user's teams are free — mixed accounts can still hit this footgun.
| const paidUsages = usagesResult.usages.filter((u) => u.tier !== 'FREE') | ||
| const freeUserLimit = | ||
| paidUsages.length === 0 | ||
| ? await deps.billingService.getFreeUserLimit(sessionKey).catch((): undefined => undefined) | ||
| : undefined | ||
|
|
||
| return buildStatusBilling({ | ||
| activeProvider, | ||
| freeUserLimit, | ||
| isAuthenticated: true, | ||
| paidUsages, | ||
| pinnedTeamId, | ||
| }) |
There was a problem hiding this comment.
issue (UX): When the user has 2+ paid orgs and no pin set, paidUsages.length === 0 is false so freeUserLimit stays undefined. buildStatusBilling then sees resolved === undefined (no pin, >1 paid org) and falls through to freeSource(undefined) which returns {source: 'free'} with no remaining/total.
The CLI then prints Billing: Personal free credits even though the user actually has multiple paid teams to choose from — that string is genuinely misleading. The web UI already handles this with needsPickPrompt and an "amber" warn pill, but the CLI side surfaces it as "you're on free credits."
Either fetch freeUserLimit unconditionally for the byterover branch (one extra call) and surface a 'needs-pick' source variant on StatusBillingDTO so the CLI can print "Pick a billing team — run brv providers connect byterover --team <name>." or keep the data layer the same but special-case paidUsages.length > 1 && pinnedTeamId === undefined in formatBillingLine to print the prompt. The current state silently misrepresents what the user is being billed against.
| private setupGetUsage(): void { | ||
| this.transport.onRequest<BillingGetUsageRequest, BillingGetUsageResponse>( | ||
| BillingEvents.GET_USAGE, | ||
| async (data) => { | ||
| const token = this.authStateStore.getToken() | ||
| if (!token?.isValid()) { | ||
| return {error: NOT_AUTHENTICATED_ERROR} | ||
| } | ||
|
|
||
| try { | ||
| const usages = await this.billingService.getUsages(token.sessionKey) | ||
| const usage = usages.find((u) => u.organizationId === data.organizationId) | ||
| if (!usage) { | ||
| return {error: `No billing usage found for organization ${data.organizationId}`} | ||
| } | ||
|
|
||
| return {usage} | ||
| } catch (error) { | ||
| return {error: getErrorMessage(error)} | ||
| } | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
nitpick: setupGetUsage fetches every organization the user belongs to and then filters client-side. If the IAM service exposes the per-org endpoint (/billing/usages/{orgId}), pushing the filter down avoids transferring/joining usage data the caller will discard, especially for users with many orgs.
If keeping the bulk fetch is intentional (single round-trip, simpler caching), consider removing this endpoint entirely — LIST_USAGE already gives the caller the full map keyed by organizationId, so callers can just index into it. The current shape is "list, then return one entry" which the consumer is going to repeat anyway when it later wants the full list.
TUI sends no body for auth:getState, so the {projectPath} destructure
added in #653 threw and the existing catch-all swallowed it into
{isAuthorized: false} — forcing the TUI into the provider chooser and
re-prompting byterover login on every startup. The valid token already
proves the user is logged in; treat projectPath as optional and pull it
via optional chaining so a missing body returns the correct auth state.
3 participants
Summary
Type of change
Scope (select all touched areas)
Linked issues
Root cause (bug fixes only, otherwise write
N/A)Test plan
User-visible changes
List user-visible changes (including defaults, config, or CLI output).
If none, write
None.Evidence
Attach at least one:
Checklist
npm test)npm run lint)npm run typecheck)npm run build)mainRisks and mitigations
List real risks for this PR. If none, write
None.