Skip to content

feat(codex): add OpenCode OAuth usage fallback - #1077

Merged
junhoyeo merged 5 commits into
junhoyeo:mainfrom
j-chmielewski:opencode-openai-token
Aug 9, 2026
Merged

feat(codex): add OpenCode OAuth usage fallback#1077
junhoyeo merged 5 commits into
junhoyeo:mainfrom
j-chmielewski:opencode-openai-token

Conversation

@j-chmielewski

@j-chmielewski j-chmielewski commented Aug 8, 2026

Copy link
Copy Markdown
Contributor
  • Use OpenCode's openai OAuth access token as a read-only Codex usage fallback when native sources return no successful result.
  • Preserve native diagnostics without importing, refreshing, or rewriting OpenCode-managed credentials.
  • Identify OpenCode-managed usage in JSON and the TUI, with setup and recovery documentation across README translations.
  • Add coverage for XDG discovery, fallback precedence, API-key exclusion, rejected tokens, diagnostics, and file immutability.

Summary by cubic

Adds a read-only OpenCode OAuth fallback for Codex usage when native sources return no result. Clearly labels OpenCode-managed credentials and preserves provider diagnostics in the CLI and TUI.

  • New Features

    • Codex fallback: read openai OAuth from OpenCode’s $XDG_DATA_HOME/opencode/auth.json only if store/file/keychain yield no usage; OpenAI API-key entries are ignored.
    • Read-only safety: never import, refresh, or rewrite OpenCode auth. If rejected, instruct users to reconnect OpenAI in OpenCode with /connect. Native diagnostics are preserved.
    • JSON/TUI: added credential_source to UsageOutput ("opencode" when used). TUI marks “managed by OpenCode” and hides account actions. Discovery honors XDG; has_credentials() includes the OpenCode candidate.
  • Bug Fixes

    • Malformed/unreadable OpenCode auth now emits a clear diagnostic with the file path; CLI surfaces active-but-failing providers as diagnostics; tests cover this in fetch_provider_report.
    • TUI: fixed inconsistent naming so the Actions row now also shows “Managed by OpenCode” for OpenCode-sourced credentials.

Written for commit 6dc1146. Summary will update on new commits.

Review in cubic

@vercel

vercel Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
tokscale Ignored Ignored Preview Aug 9, 2026 1:03am

Request Review

@j-chmielewski
j-chmielewski marked this pull request as ready for review August 8, 2026 12:52

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All reported issues were addressed across 19 files

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

Re-trigger cubic

Comment thread crates/tokscale-cli/src/commands/usage/codex.rs Outdated

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All reported issues were addressed across 1 file (changes from recent commits).

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

Re-trigger cubic

Comment thread crates/tokscale-cli/src/commands/usage/codex.rs Outdated
@junhoyeo

junhoyeo commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Reviewed as part of a sweep across the open PRs. This is the largest change in the batch and it holds up well — the read-only discipline is real (no write, refresh, or lock anywhere on OpenCode's auth.json), the token never reaches --json output or a diagnostic string, and credential_source is additive with skip_serializing_if, so existing consumers are unaffected.

I want to flag one thing in particular because it looks alarming in the diff and is not: the -144 in usage/mod.rs removes fetch_all, fetch_all_with_errors, partition_results, and ProviderError. I checked for surviving callers and there are none — run() now goes through fetch_all_report_with_intent(CliReadOnly), which maps a provider Err to a diagnostic on the same path and additionally upgrades the old silent handle.join().ok() drop into a ProviderPanicked diagnostic. So that deletion is genuine dead-code removal, not a regression.

Please restore the coverage that deletion took with it

The two deleted tests were the regression guard from #759 — "an active-but-failing provider must be surfaced, not silently dropped", with the expired SAKANA_SESSION_COOKIE case named explicitly. The behavior survives; the guard does not. rg finds no test anywhere touching fetch_provider_report, and no integration test asserts run()'s — skipped stderr line. So a future edit turning that Err arm into a default() reintroduces #759 with the suite fully green.

Roughly:

#[test]
fn fetch_provider_report_surfaces_provider_errors_instead_of_dropping_them() {
    let report = fetch_provider_report(
        "Sakana",
        Err(anyhow::anyhow!(
            "Sakana session expired or invalid. Refresh SAKANA_SESSION_COOKIE."
        )),
    );

    assert!(report.outputs.is_empty());
    assert_eq!(report.diagnostics.len(), 1);
    assert!(report.diagnostics[0].message.contains("SAKANA_SESSION_COOKIE"));
}

Smaller items

  • The TUI names the same credential two ways. credential_detail was updated but selected_account_actions_line was not, so an OpenCode entry renders Credential managed by OpenCode on one row and Managed externally two rows below. Worth branching that label on credential_source.
  • Two assertions in the new TUI test are vacuous. selected_opencode_fallback_names_its_external_credential_manager asserts the account buttons are absent, but that whole block is already gated on selected.account.is_some() and the fixture sets account: None — so both negatives pass on main regardless. The managed by OpenCode assertion is the only load-bearing one; the "hides account actions" claim in the body is not actually pinned.
  • One error message omits the path. The empty-access-token bail! is the only OpenCode error that does not name the file, which undercuts the stated goal — {"openai":{"type":"oauth","refresh":"r","access":"","expires":1}} parses fine, so this branch is reachable and currently prints no path. Untested too.
  • expires is in the schema but never deserialized, so a token known to be stale still costs a live 401 round-trip before the user is told anything. Optional, but cheap.
  • has_credentials() fully reads and parses the auth file, then the fallback path reads and parses the identical file again.

Note that no build or test leg has run on this branch — the first-time-contributor gate meant only cubic executed — so nothing here has been compiled in CI. Worth approving a CI run before merge given the size.

@junhoyeo

junhoyeo commented Aug 8, 2026

Copy link
Copy Markdown
Owner

CI update: I approved the blocked workflow runs, so this branch has now been compiled for the first time. Lint, Tests (ubuntu-latest) and Tests (windows-latest) all pass at ac2ad80. So the review comments above are the remaining items rather than anything structural — the restored fetch_provider_report coverage is the one I would not skip.

An active provider whose fetch returns Err must surface a diagnostic rather than vanishing from the output. The tests that pinned this behavior covered partition_results, which was removed along with the fetch_all path, so the invariant was left unguarded even though the behavior moved intact into fetch_provider_report. Retarget the coverage at the current entry point, asserting the error message keeps its auth-refresh guidance and that a successful fetch stays diagnostic-free.
@junhoyeo

junhoyeo commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Thanks @j-chmielewski — I pushed 802197a3 restoring the #759 regression guard, since that was the one item I did not want to lose. Everything else from my review is left to you.

First I confirmed the guard was genuinely gone rather than relocated. Running control patterns first so an empty result means something: fetch_provider_report appears exactly twice in crates/, both non-test (the definition at mod.rs:387 and the scope.spawn call at mod.rs:422) — zero test coverage. — skipped appears twice, neither in a test. SAKANA_SESSION_COOKIE appears only in sakana.rs. So the behavior survived the -144; the guard did not.

Restored coverage against the current shape rather than resurrecting the deleted tests, since they referenced the removed partition_results/ProviderError:

  • fetch_provider_report_surfaces_provider_errors_instead_of_dropping_them — feeds the expired-cookie Err and asserts outputs are empty, exactly one diagnostic, provider Sakana, kind FetchFailed, severity Error, and the message retaining SAKANA_SESSION_COOKIE. I assert kind and severity too, because the CLI only prints diagnostics that reach it — a silent severity downgrade would be fix(usage): surface active-provider fetch errors instead of dropping them #759 again in a new shape.
  • fetch_provider_report_reports_no_diagnostics_when_fetch_succeeds — carries over the intent of the deleted partition_results_reports_no_errors_when_all_succeed.

Red-green verified: patching the Err arm to UsageFetchReport::default() fails the guard with expected exactly one diagnostic for the failing provider, got: [], while the success-path test stays green. Reverted, and confirmed the diff is +68/-0.

cargo test -p tokscale-cli — 1034 unit and 153 integration tests pass, 0 failures. cargo fmt --all --check clean. CI is green per-job at 802197a3: Lint, Tests (ubuntu-latest), Tests (windows-latest), and both Windows native builds. (I approved the blocked fork runs after confirming the commit touches no .github/, build.rs, Cargo.toml, or package.json path — it is one Rust file, purely additive.)

One follow-up I did not do: the -144 also removed run()'s only end-to-end coverage of the — skipped stderr line, and there is still no integration test for it. My test pins the diagnostic at the aggregation boundary, which is where #759 actually regressed, but a run()-level test would additionally pin the user-visible output.

Still yours to decide: the TUI naming the same credential two ways (credential_detail vs selected_account_actions_line), the two vacuous assertions in the new TUI test, the empty-access-token bail! omitting the path, expires never being deserialized, and the has_credentials() double-parse.

…ount panel

The Credential detail row rendered "managed by OpenCode" while the Actions row two lines below still rendered "Managed externally" for the same account, so one credential was described two different ways.

Both rows now derive their wording from a single external_credential_manager helper. Also pairs the OpenCode fallback test with a saved-account case so the "no account buttons" assertions are exercised against a render that does show them, instead of passing vacuously on a fixture where the button block is unreachable.
@junhoyeo

junhoyeo commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Pushed one more small follow-up (6dc1146) on top of the regression-guard commit.

Consistent naming for an OpenCode-sourced credential. credential_detail was updated in this PR to render managed by OpenCode, but selected_account_actions_line still returned the unconditional Managed externally string, so the very same account rendered both labels in the selected-account panel:

Credential  managed by OpenCode
...
Actions
Managed externally

Both rows now go through one external_credential_manager(output) -> Option<&'static str> helper, so the detail row renders managed by OpenCode and the actions row renders Managed by OpenCode. Adding a new externally-managed source in future only needs the one match arm.

Made the fallback test's negative assertions load-bearing. selected_opencode_fallback_names_its_external_credential_manager asserted !body.contains("Use Account") and !body.contains("Remove"), but the account-button block is gated on selected.account.is_some() and the fixture sets account: None, so both negatives passed regardless of this PR. They are now paired with a saved-account render in selected_account_actions_appear_only_for_saved_accounts, which asserts the buttons do appear before asserting they disappear for the externally-managed output. The fallback test itself now pins the labelling instead: managed by OpenCode present, Managed externally absent. It fails on the pre-fix code with the full rendered panel showing both labels.

cargo fmt --all -- --check clean; cargo test -p tokscale-cli passes (1035 unit + 153 CLI tests).

Deliberately left alone as author's call: the empty-access-token bail! message, the undeserialized expires field, and the double parse in has_credentials().

@junhoyeo
junhoyeo merged commit d4b5c2a into junhoyeo:main Aug 9, 2026
20 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.

2 participants