Skip to content

feat(codex): add subscription usage provider#7

Merged
ogrodev merged 7 commits into
mainfrom
task/005-provider-codex
Jun 3, 2026
Merged

feat(codex): add subscription usage provider#7
ogrodev merged 7 commits into
mainfrom
task/005-provider-codex

Conversation

@ogrodev
Copy link
Copy Markdown
Owner

@ogrodev ogrodev commented Jun 3, 2026

Summary

  • Adds Codex subscription usage via per-account local OAuth discovery.
  • Hardens shipping gaps: no Codex refresh downscope, bounded HTTP probes, read-only WAL fallback, overflow-safe timestamps.
  • Adds deterministic OMP discovery coverage and Vitest UI state coverage for siloed stale/error handling.

Verification

  • make check
  • make tasks
  • make deps
  • CI=true make qa
  • pre-push cargo test --workspace

Summary by cubic

Adds Codex subscription usage with multi-account discovery (Oh My Pi SQLite + Codex CLI), showing session and weekly windows with percent used and reset countdowns alongside Claude. Generalizes per-account discovery and OAuth refresh, making Claude Code multi-account; UI keeps snapshots and errors siloed per account and auto-refreshes.

  • New Features

    • Codex provider: per-account sources (codex:<account_id>) discovered from Oh My Pi and the CLI, deduped by account id; JWT-based identity, ChatGPT-Account-Id header; lossy wham/usage parsing.
    • Shared per-account discovery used by Claude Code (claude-code:<account_id>); each account has its own namespaced OAuth cache key.
    • fetch_usage(id) command and per-provider usage/error events so one provider’s failure doesn’t affect another; tabs disambiguate same-named accounts by email.
    • UI: per-account tiles, countdown labels, stale/error states; vitest coverage; CI runs pnpm test.
  • Refactors

    • Generalized OAuth refresher into providers::oauth::OAuthRefresher (configurable endpoint/client/cache/scope) used by Codex and Claude; read vendor tokens, store refreshed copies only under our keychain.
    • Hardened probes: 15s HTTP timeout; bounded local discovery with backoff; relaxed Claude scope check when scopes are unknown; overflow-safe timestamps.
    • Security: Codex chatgpt_base_url must be HTTPS; invalid bases fall back to the secure default. UI clears cached usage/error on API-key disconnect. GitHub Actions uses workflow-level least-privilege (permissions: contents: read).
    • Dependencies: added rusqlite (bundled) for read-only Oh My Pi SQLite access; added toml; added vitest.

Written for commit 4996f49. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • New Features

    • Codex (ChatGPT/OpenAI) subscription usage tracking
    • Multi-account support for reused-login OAuth providers with per-account sources
    • Unified on-demand usage fetch (per-provider/per-account) from the UI
  • Improvements

    • Per-provider siloed usage state and structured error events
    • Discovery resiliency and HTTP request timeouts to avoid stalls
  • Documentation

    • ADR and docs added/updated for multi-account discovery and Codex behavior

ogrodev added 4 commits June 2, 2026 17:56
Reuse the Codex CLI's ~/.codex/auth.json OAuth login and poll the private
chatgpt.com/backend-api/wham/usage endpoint, rendering Codex's session and
weekly windows next to Claude's in the popover.

core:
- codex provider: lossy wham/usage parser (primary/secondary windows
  classified by length, additive model-specific limits), JWT-claims account
  identity, ChatGPT-Account-Id header, 401/429 handling.
- generalize the Claude token refresher into a shared
  providers::oauth::OAuthRefresher (configurable endpoint/client/cache/scope)
  used by both Claude and Codex; add OAuthTokens.account_id.
- register Codex in the source catalog.

adapters:
- CodexCredentials (auth.json reader + metadata-only presence; expiry from
  the access token's JWT exp), codex_strategy wiring, LocalSourceProbe entry,
  and a [ignore]-style codex_live example.

app:
- generic fetch_usage(id) command + codex in fetch_for; the usage-error event
  now carries the provider id so a failure is siloed to its own tile.

ui:
- per-provider usage/error state: each provider shows only its own windows,
  keeps last-known values with a stale banner on a failed fetch, and never
  renders another provider's data. Existing providers are unchanged.

Tests use recorded fixtures + fakes (no live accounts, no Keychain prompts);
the live check is the [ignore]d codex_live example. Completes task 005.
Codex logins aren't only in ~/.codex/auth.json — when Codex is driven through Oh My Pi,
each profile keeps its own OAuth token in a per-profile SQLite store
(~/.omp[/profiles/*]/agent/agent.db, provider 'openai-codex'). Single-path discovery missed
all of them and only ever found the (often stale) CLI file. Discover both stores, dedupe by
ChatGPT account id (freshest token wins), and surface each distinct account as its own
connectable source.

core:
- sources: SourceDescriptor owns its strings; add CodexAccount + codex_account_descriptor +
  codex_source_id; discover_sources/active_sources take the discovered accounts and treat them
  as present-by-discovery (no file probe), seeding each account's email from discovery.
- codex: account_cache_key(account_id) namespaces each account's refreshed-token cache
  (oauth.codex.<id>) so two logins never collide; the catalog and the strategy derive the key
  from one place.

adapters:
- codex: enumerate the Codex CLI auth.json + every Oh My Pi profile DB (read-only rusqlite),
  dedupe by account id, and resolve per-account credentials (freshest token, re-read each load
  so MLT always uses Oh My Pi's latest token). Never writes either store.

app:
- per-account routing: fetch_for handles codex:<id>; descriptor_for / discover_all / active_all
  thread the discovered accounts through every command and the refresh loop; disconnect purges
  only the targeted account's namespaced token.

ui:
- per-account tiles: reportsUsage covers codex:<id>; the switcher disambiguates the otherwise
  identical \"Codex\" tabs by account email; Codex gets its own icon. Each account's usage,
  identity, and stale/error state stay siloed by its unique source id.

Verified live against two real logins (a personal Plus and a Team account) through the
[ignore]-style codex_live example: both discovered, both fetch their own windows. Unit tests
cover parsing, dedup, dynamic discovery, and per-account disconnect; no live accounts in tests.
…he same multi-account treatment

The multi-account model built for Codex was Codex-specific. Generalize it into one shared
mechanism and apply it to Claude Code, so every reused-login (OAuth-subscription) provider gets
the same treatment — and a new one plugs in with a registry row plus a strategy, no bespoke code.

core:
- CodexAccount -> DiscoveredAccount { base, account_id, email, origin }; codex_source_id /
  codex_account_descriptor -> account_source_id / account_cache_key / account_descriptor, driven
  by an ACCOUNT_PROVIDERS registry (codex, claude-code). discover_sources/active_sources expand
  any provider's accounts uniformly.
- claude: the usage scope guard now fires only when scopes are *known and insufficient*; an
  Oh My Pi credential omits scopes, so such a token is trusted and the endpoint is the authority.

adapters:
- new accounts.rs: the provider-agnostic Oh My Pi reader (one PROVIDERS table maps base ->
  Oh My Pi provider id), dedup, and AccountCredentials shared by every provider. codex.rs keeps
  only its CLI auth.json reader + strategy; claude.rs adds claude_account_strategy. anthropic
  OAuth logins are read the same way Codex's are.

app + ui:
- descriptor_for / fetch_for route any <base>:<account_id> (codex:, claude-code:) to the right
  strategy; the front-end recognizes per-account ids and disambiguates same-named tabs by email.

docs:
- ADR 0019 records the pattern; the shared Definition of Done now requires every reused-login
  provider to use it (so the next provider is multi-account by default).

Verified live (accounts_live example): 4 accounts discovered — 2 Codex + 2 Claude Code, across
the gmail and bigshotpictures logins — each fetching its own windows through its provider's real
strategy. Tests cover the generic discovery, dedup, per-account disconnect, and the relaxed scope
guard; no live accounts in tests.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 3, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR implements per-account OAuth discovery and credential loading, a shared OAuth refresher, Codex provider and adapter wiring (usage parsing and identity), resilient blocking probes, Tauri/backend per-account routing and fetch command, frontend per-provider usage state/UI, and matching docs/tests/CI updates.

Changes

Multi-account discovery, Codex provider, and shared OAuth refresh

Layer / File(s) Summary
Build and CI / UI test gate
Makefile, .github/workflows/ci.yml, package.json
Adds ui-test to Makefile check, runs frontend unit tests in CI, and adds vitest + test script to package.json.
Adapter resilience & HTTP timeout
crates/adapters/src/resilience.rs, crates/adapters/src/http.rs
Introduces bounded blocking probe helpers and command_stdout for CLI/file probes; adds a 15s default HTTP timeout to adapter requests.
Account discovery & credential source
crates/adapters/src/accounts.rs, crates/adapters/Cargo.toml, crates/adapters/examples/accounts_live.rs
Discovers OAuth accounts from Oh My Pi SQLite and vendor CLI, parses/normalizes blobs, deduplicates by account_id keeping freshest token, exposes discovered_accounts() and AccountCredentials source; includes live example and tests.
Codex adapter and strategy wiring
crates/adapters/src/codex.rs, crates/adapters/src/lib.rs
Discovers Codex CLI auth, normalizes OAuth/API-key shapes, builds per-account CodexStrategy with per-account cache key and user-agent detection; exports codex strategy.
Claude adapter migration
crates/adapters/src/claude.rs
Migrates Claude Code to use shared OAuthRefresher, uses resilience probes for file/CLI reads, sets Claude tokens' account_id = None, and adjusts scope handling/tests.
Shared OAuth refresher
crates/core/src/providers/oauth.rs
Implements OAuthRefresher that fast-paths fresh vendor tokens, prefers freshest cached/vendor token, performs network refresh when stale, and caches refreshed tokens under its cache key with tests.
Codex provider core
crates/core/src/providers/codex.rs, crates/core/src/providers/testdata/codex_usage.json
Adds Codex provider: lossy usage JSON parsing into usage windows, JWT identity/expiry helpers, authenticated usage fetch with header mapping and status handling, identity caching, and extensive tests/fixture.
Core sources per-account expansion
crates/core/src/sources.rs, crates/core/src/domain.rs
Adds DiscoveredAccount, per-account helpers (account_source_id, account_cache_key, account_descriptor), switches SourceDescriptor.oauth_cache_key to owned Option<String>, refactors discover_sources/active_sources to accept accounts and expand per-account descriptors, and adds OAuthTokens.account_id field.
Tauri backend routing & commands
src-tauri/src/lib.rs, src-tauri/Cargo.toml
Adds descriptor_for, discover_all/active_all, replaces fetch_claude_usage with fetch_usage(id), routes per-account usage (codex/claude), emits structured UsageErrorEvent, updates handlers/tests, and adds serde dependency.
Frontend usage API & state
src/lib/usage.ts, src/lib/usageState.ts, src/lib/usageState.test.ts
Adds fetchUsage(id) and UsageErrorEvent typed payload; introduces per-provider UsageRecords (snapshots/errors), helpers for activity, labels, reset countdowns, usageWindowKey, record/clear helpers, and Vitest tests validating per-provider isolation and state transitions.
Page component per-provider refactor
src/routes/+page.svelte
Refactors UI to keep snapshots/errors per provider id, derives selected provider display state, concurrently fetches usage for connected providers, updates tab labels/icons for account-qualified ids, and renders per-provider windows/errors/timestamps.
Docs, ADR, fixtures, dev deps
docs/adr/0019-multi-account-discovery.md, docs/*, crates/core/Cargo.toml, src-tauri/Cargo.toml
Adds ADR 0019 documenting per-account discovery, updates ADR index and provider docs, marks Codex task done, adds parking_lot dev-dep for tests, and documents read-only refresh behavior and multi-account Definition-of-Done.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Possibly related PRs

  • ogrodev/MLT#5: Related changes touching provider disconnect and cached secret purging.
  • ogrodev/MLT#2: Extends earlier source discovery patterns to per-account namespaced sources.

Poem

🐰 I dug through creds and found each key,

Namespaced accounts, refreshed carefully,
Codex and Claude now sing per id,
Usage blooms tidy, errors gently hid. 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(codex): add subscription usage provider' directly and accurately describes the main feature introduced in this PR—adding Codex as a new subscription usage provider.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch task/005-provider-codex

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
crates/adapters/src/claude.rs (1)

186-206: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Avoid spawning claude --version once per account per fetch.

Both constructors eagerly call detect_user_agent(). src-tauri/src/lib.rs Lines 383-409 rebuild a Claude strategy on every fetch, so the new per-account path now shells out once per discovered Claude account on each poll/open. Because that probe is blocking and unbounded, one hung CLI lookup can stall the refresh path. Cache the user agent once, or route this lookup through the same shared timeout/failure-gate wrapper as the other blocking adapter probes.

Based on learnings: ADR 0015 mandates a per-probe timeout/failure-gate that must be applied uniformly across all blocking adapter calls as a dedicated resilience change, not spot-fixed on individual probes.

Also applies to: 217-241

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/adapters/src/claude.rs` around lines 186 - 206, The code calls
detect_user_agent() eagerly inside claude_strategy (and the other Claude
constructor) causing a blocking shell probe per-account; change this to use a
shared cached user-agent or accept a user_agent: String parameter so the
expensive detect_user_agent() is computed once (e.g., during app init) and
reused, and/or invoke the probe through the existing shared timeout/failure-gate
wrapper used for other blocking adapter calls; update claude_strategy and the
other constructor to stop calling detect_user_agent() directly and instead
consume the precomputed/cached user agent or the wrapped probe.
src/routes/+page.svelte (1)

180-186: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clear cached usage when disconnecting an API-key source.

This path updates sources but leaves snapshots/errors[source.id] intact. After reconnecting the same provider, the page can render the previous session’s usage or error until the new fetch completes, unlike the local-login disable path at Lines 141-145.

Proposed fix
 async function disconnectKeySource(source: SourceState): Promise<void> {
   keyError = null;
   try {
     sources = await disconnectSource(source.id);
+    clearUsage(usageRecords, source.id);
   } catch (e) {
     keyError = String(e);
   }
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/routes/`+page.svelte around lines 180 - 186, In disconnectKeySource,
after updating sources, clear cached usage and error for that source so the UI
doesn't show stale data: remove snapshots.errors[source.id] and
snapshots.usage[source.id] (or set them to undefined) and then reassign
snapshots = { ...snapshots } to trigger Svelte reactivity; perform this inside
the try block after sources = await disconnectSource(source.id) in the
disconnectKeySource function.
🧹 Nitpick comments (1)
src/lib/usageState.ts (1)

63-69: ⚡ Quick win

Guard selectedAccount against cross-provider snapshots.

This helper returns snapshot.account without checking snapshot.provider === selected.id, so a mismatched caller can render one provider's identity under another provider. Encode the siloing rule here instead of relying on every call site to pass a pre-filtered snapshot.

Proposed fix
 export function selectedAccount(
   snapshot: UsageSnapshot | null,
   selected: SourceState | null,
 ): string | null {
   if (!selected) return null;
-  if (snapshot?.account) return snapshot.account.email ?? snapshot.account.organization;
+  if (snapshot?.provider === selected.id && snapshot.account) {
+    return snapshot.account.email ?? snapshot.account.organization;
+  }
   return selected.account?.email ?? selected.account?.organization ?? null;
 }

As per coding guidelines, "Provider data must be siloed; never render one provider's identity or plan under another provider".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/usageState.ts` around lines 63 - 69, The selectedAccount helper
currently returns snapshot.account without verifying the snapshot is for the
same provider as the selected source, allowing cross-provider identity leaks;
update selectedAccount (and its checks of UsageSnapshot and SourceState) to
first verify snapshot?.provider === selected.id before using snapshot.account,
and only if providers match return snapshot.account.email ??
snapshot.account.organization; otherwise fall back to selected.account?.email ??
selected.account?.organization ?? null so provider data is always siloed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/adapters/src/accounts.rs`:
- Around line 41-54: discovered_accounts() and AccountCredentials::load()
perform unbounded local probes (enumerate_for/dedup_freshest and inline
SQLite/vendor-store reads) and must be wrapped by the shared resilience gate
with per-probe timeouts per ADR-0015; update these functions to run each
provider probe (the loop over PROVIDERS calling enumerate_for and the
DB/vendor-store opens) through the common resilience gate/failure-gate API and
apply a per-probe deadline/timeout, ensuring any probe that times out or fails
returns/continues cleanly and surfaces a controlled error instead of blocking
discovery; reference and protect the calls to enumerate_for, dedup_freshest, and
the code paths in AccountCredentials::load that open SQLite/vendor-store files.

In `@crates/adapters/src/codex.rs`:
- Around line 147-152: CodexStrategy currently hard-codes usage_url to
DEFAULT_USAGE_URL so installs that set chatgpt_base_url never get used; change
the construction of CodexStrategy to resolve a base URL from the Codex config
(e.g., creds.chatgpt_base_url or the provided http/client base) and then build
usage_url from that base (e.g., "{base}/backend-api/wham/usage"), falling back
to DEFAULT_USAGE_URL only when no configured base is present; update the
CodexStrategy initializer that sets usage_url (and related places that reference
DEFAULT_USAGE_URL) to use this resolved base.
- Around line 33-39: The codex_cli_accounts() path currently performs blocking
std::fs::read_to_string(&path) and the codex "--version" probe without any
per-probe timeout or shared failure gate; wrap these blocking operations with
the project's failure-gate/timeout pattern from ADR0015 (same mechanism used for
HTTP probes) so they cannot hang forever: replace direct read_to_string calls in
codex_cli_accounts() and the inline process probe (the codex --version Command
invocation around lines ~112-127) with timed executions (spawn blocking tasks or
use a dedicated thread + join with timeout or the existing probe helper that
returns a Result::Err on timeout), ensure errors/timeouts are logged and
propagated back as Err so discovery/strategy construction short-circuits via the
shared gate.

In `@crates/core/src/providers/oauth.rs`:
- Around line 67-71: The is_fresh method currently treats OAuthTokens with
expires_at == None as permanently fresh, which lets malformed refresh responses
(where expires_in overflowed or was missing) become evergreen; change the logic
in is_fresh (and the refresh/cache path that sets OAuthTokens.expires_at to None
around the refresh handling code) so that tokens with expires_at == None are
considered expired (return false) and are not cached as valid long-term;
instead, when parsing refresh responses in the refresh handler (the code that
sets expires_at to None on parse/overflow errors), either set a short fallback
expiry (e.g., now + small skew) or reject/circuit the response and log an error,
and ensure the caching path (where tokens are stored) skips storing tokens that
lack a sane expires_at to avoid wedging future refreshes.

In `@src/routes/`+page.svelte:
- Around line 78-81: The tabLabel function (for SourceState) can produce
identical labels when an account has an organization but no email; update
tabLabel to include s.account?.organization as a fallback before s.display_name
so per-account tabs remain disambiguated. Concretely, in the function
tabLabel(s: SourceState) check s.label first, then if s.id.includes(':') return
s.account?.email ?? s.account?.organization ?? s.display_name; otherwise return
s.display_name; this ensures organization is used when email is absent.

---

Outside diff comments:
In `@crates/adapters/src/claude.rs`:
- Around line 186-206: The code calls detect_user_agent() eagerly inside
claude_strategy (and the other Claude constructor) causing a blocking shell
probe per-account; change this to use a shared cached user-agent or accept a
user_agent: String parameter so the expensive detect_user_agent() is computed
once (e.g., during app init) and reused, and/or invoke the probe through the
existing shared timeout/failure-gate wrapper used for other blocking adapter
calls; update claude_strategy and the other constructor to stop calling
detect_user_agent() directly and instead consume the precomputed/cached user
agent or the wrapped probe.

In `@src/routes/`+page.svelte:
- Around line 180-186: In disconnectKeySource, after updating sources, clear
cached usage and error for that source so the UI doesn't show stale data: remove
snapshots.errors[source.id] and snapshots.usage[source.id] (or set them to
undefined) and then reassign snapshots = { ...snapshots } to trigger Svelte
reactivity; perform this inside the try block after sources = await
disconnectSource(source.id) in the disconnectKeySource function.

---

Nitpick comments:
In `@src/lib/usageState.ts`:
- Around line 63-69: The selectedAccount helper currently returns
snapshot.account without verifying the snapshot is for the same provider as the
selected source, allowing cross-provider identity leaks; update selectedAccount
(and its checks of UsageSnapshot and SourceState) to first verify
snapshot?.provider === selected.id before using snapshot.account, and only if
providers match return snapshot.account.email ?? snapshot.account.organization;
otherwise fall back to selected.account?.email ?? selected.account?.organization
?? null so provider data is always siloed.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a523b8a6-701b-4469-b6f4-350d78ea2dbf

📥 Commits

Reviewing files that changed from the base of the PR and between 8c5d8df and 15da8e1.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (30)
  • Makefile
  • crates/adapters/Cargo.toml
  • crates/adapters/examples/accounts_live.rs
  • crates/adapters/src/accounts.rs
  • crates/adapters/src/claude.rs
  • crates/adapters/src/codex.rs
  • crates/adapters/src/http.rs
  • crates/adapters/src/lib.rs
  • crates/adapters/src/sources.rs
  • crates/core/Cargo.toml
  • crates/core/src/domain.rs
  • crates/core/src/providers/claude.rs
  • crates/core/src/providers/codex.rs
  • crates/core/src/providers/mod.rs
  • crates/core/src/providers/oauth.rs
  • crates/core/src/providers/testdata/codex_usage.json
  • crates/core/src/sources.rs
  • docs/PRD.md
  • docs/adr/0019-multi-account-discovery.md
  • docs/adr/README.md
  • docs/research/PROVIDERS.md
  • docs/tasks/005-provider-codex.md
  • docs/tasks/README.md
  • package.json
  • src-tauri/Cargo.toml
  • src-tauri/src/lib.rs
  • src/lib/usage.ts
  • src/lib/usageState.test.ts
  • src/lib/usageState.ts
  • src/routes/+page.svelte

Comment thread crates/adapters/src/accounts.rs
Comment thread crates/adapters/src/codex.rs
Comment thread crates/adapters/src/codex.rs Outdated
Comment thread crates/core/src/providers/oauth.rs
Comment thread src/routes/+page.svelte Outdated
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 6 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/adapters/src/lib.rs">

<violation number="1" location="crates/adapters/src/lib.rs:33">
P2: `std::sync::Mutex` is poisoning: if a test panics while holding this lock, all subsequent tests that try to acquire it will receive `Err(PoisonError)`. Using `.unwrap()` on the lock result will cause cascading test failures that are confusing to debug. Recover from poisoning with `.unwrap_or_else(|e| e.into_inner())` instead, or use a non-poisoning alternative like `parking_lot::Mutex`.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread crates/adapters/src/lib.rs Outdated
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="Makefile">

<violation number="1" location="Makefile:16">
P2: `check` target comment claims `## all gates (matches CI)` but CI is not updated to run `ui-test`, creating a mismatch: local `make check` will run vitest tests but CI will not catch test failures.</violation>
</file>

Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Re-trigger cubic

Comment thread Makefile
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)

56-79: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Explicitly set least-privilege GITHUB_TOKEN permissions in this workflow.

.github/workflows/ci.yml has no workflow/job permissions: block, so the frontend job runs with default token scopes (potentially broader than needed). Add least-privilege permissions at the workflow level.

Suggested hardening diff
 name: CI
 
 on:
   push:
     branches: [main]
   pull_request:
 
+permissions:
+  contents: read
+
 concurrency:
   group: ci-${{ github.ref }}
   cancel-in-progress: true
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/ci.yml around lines 56 - 79, Add an explicit
least-privilege GITHUB_TOKEN permissions block at the workflow top level (not
relying on defaults) so the frontend job named "frontend" doesn't get broad
scopes; add a top-level permissions: mapping granting only the minimal scopes
your CI needs (for example permissions: contents: read and any other specific
scopes required), and if the "frontend" job needs additional rights, add a
job-level permissions override for that job name ("frontend") with only those
extra scopes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 56-79: Add an explicit least-privilege GITHUB_TOKEN permissions
block at the workflow top level (not relying on defaults) so the frontend job
named "frontend" doesn't get broad scopes; add a top-level permissions: mapping
granting only the minimal scopes your CI needs (for example permissions:
contents: read and any other specific scopes required), and if the "frontend"
job needs additional rights, add a job-level permissions override for that job
name ("frontend") with only those extra scopes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2c3c2095-5b6d-4f55-a3cc-e3bc879b0519

📥 Commits

Reviewing files that changed from the base of the PR and between 15da8e1 and af0c86f.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (13)
  • .github/workflows/ci.yml
  • crates/adapters/Cargo.toml
  • crates/adapters/src/accounts.rs
  • crates/adapters/src/claude.rs
  • crates/adapters/src/codex.rs
  • crates/adapters/src/lib.rs
  • crates/adapters/src/resilience.rs
  • crates/core/src/providers/oauth.rs
  • crates/core/src/sources.rs
  • src-tauri/src/lib.rs
  • src/lib/usageState.test.ts
  • src/lib/usageState.ts
  • src/routes/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (8)
  • crates/adapters/Cargo.toml
  • crates/core/src/providers/oauth.rs
  • src/lib/usageState.ts
  • src-tauri/src/lib.rs
  • crates/adapters/src/lib.rs
  • crates/adapters/src/codex.rs
  • crates/core/src/sources.rs
  • src/routes/+page.svelte

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 12 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread crates/adapters/src/codex.rs
Address remaining PR #7 review feedback:

- codex: usage_url_from_base now rejects non-HTTPS or scheme-less bases, so
  the OAuth bearer token is never sent to a plaintext http:// endpoint; a
  malformed chatgpt_base_url falls back to the secure default.
- ci: add workflow-level least-privilege `permissions: contents: read`
  (every job only checks out and runs read-only gates).
- ui: clear cached usage/error when disconnecting an API-key source, matching
  the local-login disable path so a later reconnect starts clean.
- ui: enforce provider siloing inside selectedAccount() rather than relying
  on callers, per the per-provider siloing invariant.

The Makefile ui-test/CI-mismatch thread was already closed in af0c86f (CI
runs pnpm test, matching make ui-test); resolved.
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 5 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/adapters/src/codex.rs">

<violation number="1" location="crates/adapters/src/codex.rs:151">
P2: The HTTPS check is case-sensitive, so valid mixed/upper-case `HTTPS://` bases are incorrectly rejected and silently fall back to the default usage endpoint.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

// HTTPS only: this request carries the OAuth bearer token, so a plaintext
// `http://` (or scheme-less) base would leak it on the wire. Reject anything
// non-HTTPS so we fall back to the secure default endpoint.
if trimmed.is_empty() || !trimmed.starts_with("https://") {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The HTTPS check is case-sensitive, so valid mixed/upper-case HTTPS:// bases are incorrectly rejected and silently fall back to the default usage endpoint.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/adapters/src/codex.rs, line 151:

<comment>The HTTPS check is case-sensitive, so valid mixed/upper-case `HTTPS://` bases are incorrectly rejected and silently fall back to the default usage endpoint.</comment>

<file context>
@@ -145,7 +145,10 @@ fn resolve_codex_usage_url() -> Option<String> {
+    // HTTPS only: this request carries the OAuth bearer token, so a plaintext
+    // `http://` (or scheme-less) base would leak it on the wire. Reject anything
+    // non-HTTPS so we fall back to the secure default endpoint.
+    if trimmed.is_empty() || !trimmed.starts_with("https://") {
         return None;
     }
</file context>
Suggested change
if trimmed.is_empty() || !trimmed.starts_with("https://") {
if trimmed.is_empty() || !trimmed.to_ascii_lowercase().starts_with("https://") {

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/routes/+page.svelte (1)

179-188: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Scope API-key errors by source id.

Line 187 still writes disconnect failures into the shared keyError state. Because that same value is rendered in every non-editing API-key row at Lines 405-409, one provider's failure can show up under another provider and a later action can clear the wrong message. Store API-key errors per source.id here as well so the rows stay siloed.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/routes/`+page.svelte around lines 179 - 188, disconnectKeySource
currently writes failures to the shared keyError variable causing messages to
leak across rows; change it to use a per-source error mapping (e.g.,
keyErrors[sourceId] keyed by source.id) so errors are siloed: on entry clear
keyErrors[source.id], on success remove/clear keyErrors[source.id] after calling
disconnectSource and clearUsage(usageRecords, source.id), and on catch set
keyErrors[source.id] = String(e). Update references to keyError elsewhere to
read from the per-id map (use source.id) so each API-key row shows only its own
error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/routes/`+page.svelte:
- Around line 179-188: disconnectKeySource currently writes failures to the
shared keyError variable causing messages to leak across rows; change it to use
a per-source error mapping (e.g., keyErrors[sourceId] keyed by source.id) so
errors are siloed: on entry clear keyErrors[source.id], on success remove/clear
keyErrors[source.id] after calling disconnectSource and clearUsage(usageRecords,
source.id), and on catch set keyErrors[source.id] = String(e). Update references
to keyError elsewhere to read from the per-id map (use source.id) so each
API-key row shows only its own error.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: db2379df-27ea-44e8-9387-c64a6baf215d

📥 Commits

Reviewing files that changed from the base of the PR and between af0c86f and 4996f49.

📒 Files selected for processing (5)
  • .github/workflows/ci.yml
  • crates/adapters/src/codex.rs
  • src/lib/usageState.test.ts
  • src/lib/usageState.ts
  • src/routes/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (4)
  • .github/workflows/ci.yml
  • src/lib/usageState.test.ts
  • src/lib/usageState.ts
  • crates/adapters/src/codex.rs

@ogrodev ogrodev merged commit d8df605 into main Jun 3, 2026
5 checks passed
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