M3: All providers + auth #4

2026-07-10T17:10:43+02:00

darman commented

2026-07-10 17:10:43 +02:00

OpenAI chat + responses codecs, provider, registry wiring
auth.json credential storage
model pricing + per-step cost accumulation
models.dev catalog + cost display wiring in the TUI
Copilot device-flow login, token exchange with direct-Bearer fallback, provider

- OpenAI chat + responses codecs, provider, registry wiring - `auth.json` credential storage - model pricing + per-step cost accumulation - models.dev catalog + cost display wiring in the TUI - Copilot device-flow login, token exchange with direct-Bearer fallback, provider

darman changed target branch from develop to feature/m2

2026-07-10 17:12:20 +02:00

darman added 5 commits 2026-07-10 17:12:20 +02:00

M3: OpenAI chat and responses codecs, provider, registry wiring 607bd88ae3

Adds both OpenAI codecs — chat completions and the newer responses API — plus the OpenAiProvider and registry wiring, so the same session can run against OpenAI in addition to Anthropic.

M3: auth.json credential storage a3f6fd6378

Adds harness-providers::auth, a persisted auth.json credential store for provider tokens, laying the groundwork for Copilot's device-flow login and token refresh.

M3: model pricing and per-step cost accumulation a99edc116a

Extends the model catalog with per-token pricing and accumulates cost per engine step, threading the running total through the store and up into harness-app so it can be surfaced in the UI.

M3: models.dev catalog and cost display wiring ab0269df1a

Adds a bundled models.dev snapshot plus harness-providers::modelsdev to resolve model metadata, and wires live cost display into the TUI's input bar and message state, updating the render snapshots accordingly.

M3: Copilot device-flow, token exchange, provider 8ed17bf091

Adds the Copilot provider: device-flow login modal support, copilot_internal/v2/token exchange with a direct-Bearer fallback, and routing across the three codecs, per the dual-path mitigation noted for Copilot token variance.

darman requested review from claude-reviewer 2026-07-11 14:42:03 +02:00

claude-reviewer commented

2026-07-11 14:47:26 +02:00

Review: M3 — All providers + auth

Lots of surface here and it's mostly in great shape: the cost math (ModelCost::cost_of) is correct and explicitly documents the reasoning-tokens-are-in-output subtlety, models.dev has a clean cache→baked-snapshot fallback, the device-flow state machine is pure and thoroughly tested, and the Copilot codec routing is nicely factored. Two things worth fixing, one security-flavored.

Copilot: transient network error permanently disables token exchange (medium)

TokenProvider::token (copilot/token.rs:2321):

let fresh = match exchange(&self.client, &self.oauth_token).await {
    Ok(tok) => tok,
    Err(_) => CopilotToken { token: self.oauth_token.clone(), expires_at: 0 },
};
*guard = Some(fresh.clone());

The Err(_) arm catches all errors, including TokenError::Network (timeout, DNS blip), and caches a never-expiring (expires_at: 0) direct-Bearer token. Because needs_refresh is always false for expires_at == 0, a single transient failure during the first exchange pins the provider to direct-Bearer mode for the entire process lifetime — even though the exchange endpoint is actually supported. The sticky fallback should be reserved for TokenError::ExchangeUnsupported; on Network/Unexpected, don't cache — let the next token() call retry the exchange. The doc comment ("when the exchange endpoint rejects the request") already describes the intended narrower behavior.

auth.json secret is briefly world-readable during write (low-medium)

AuthStorage::write (auth.rs:624):

std::fs::write(&tmp, &json)?;   // created at umask default, typically 0644
set_owner_only(&tmp)?;          // chmod 0600 *after* the secret is on disk
std::fs::rename(&tmp, &self.path)?;

There's a window where the temp file containing the OAuth/API secret exists at 0644 before the chmod lands. Create it 0600 from the start via OpenOptions::new().write(true).create_new(true).mode(0o600) (unix), and consider create_dir_all + chmod 0700 on the parent directory so the file isn't sitting in a world-readable dir either.

Minor

openai.rs: flavor_for(&req.model) is computed twice (once to pick the path/body, once to pick the decoder). Harmless but easy to compute once and reuse.
Copilot exchange returns TokenError::Network for any non-success status that isn't 401/403/404 (e.g. 500). Combined with the fix above, make sure a 5xx from the exchange endpoint retries rather than sticking to fallback.

Nice test coverage throughout (auth 0600 assertion, cost-of-usage, cache TTL, poll state machine).

— automated review (Claude)

## Review: M3 — All providers + auth Lots of surface here and it's mostly in great shape: the cost math (`ModelCost::cost_of`) is correct and explicitly documents the reasoning-tokens-are-in-output subtlety, models.dev has a clean cache→baked-snapshot fallback, the device-flow state machine is pure and thoroughly tested, and the Copilot codec routing is nicely factored. Two things worth fixing, one security-flavored. ### Copilot: transient network error permanently disables token exchange (medium) `TokenProvider::token` (`copilot/token.rs:2321`): ```rust let fresh = match exchange(&self.client, &self.oauth_token).await { Ok(tok) => tok, Err(_) => CopilotToken { token: self.oauth_token.clone(), expires_at: 0 }, }; *guard = Some(fresh.clone()); ``` The `Err(_)` arm catches **all** errors, including `TokenError::Network` (timeout, DNS blip), and caches a never-expiring (`expires_at: 0`) direct-Bearer token. Because `needs_refresh` is always false for `expires_at == 0`, a single transient failure during the first exchange pins the provider to direct-Bearer mode for the entire process lifetime — even though the exchange endpoint is actually supported. The sticky fallback should be reserved for `TokenError::ExchangeUnsupported`; on `Network`/`Unexpected`, don't cache — let the next `token()` call retry the exchange. The doc comment ("when the exchange endpoint rejects the request") already describes the intended narrower behavior. ### auth.json secret is briefly world-readable during write (low-medium) `AuthStorage::write` (`auth.rs:624`): ```rust std::fs::write(&tmp, &json)?; // created at umask default, typically 0644 set_owner_only(&tmp)?; // chmod 0600 *after* the secret is on disk std::fs::rename(&tmp, &self.path)?; ``` There's a window where the temp file containing the OAuth/API secret exists at 0644 before the chmod lands. Create it 0600 from the start via `OpenOptions::new().write(true).create_new(true).mode(0o600)` (unix), and consider `create_dir_all` + chmod `0700` on the parent directory so the file isn't sitting in a world-readable dir either. ### Minor - `openai.rs`: `flavor_for(&req.model)` is computed twice (once to pick the path/body, once to pick the decoder). Harmless but easy to compute once and reuse. - Copilot `exchange` returns `TokenError::Network` for any non-success status that isn't 401/403/404 (e.g. 500). Combined with the fix above, make sure a 5xx from the exchange endpoint retries rather than sticking to fallback. Nice test coverage throughout (auth 0600 assertion, cost-of-usage, cache TTL, poll state machine). — automated review (Claude)

You are not authorized to merge this pull request.

This pull request can be merged automatically.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin feature/m3:feature/m3

git checkout feature/m3

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: darman/ai-harness#4