Skip to content

Fix empty cloud API key routing#379

Open
Farhan (fkb032) wants to merge 1 commit into
developfrom
farhankhalaf/eng-558-clearing-a-cloud-api-key-stores-an-empty-string-that-still
Open

Fix empty cloud API key routing#379
Farhan (fkb032) wants to merge 1 commit into
developfrom
farhankhalaf/eng-558-clearing-a-cloud-api-key-stores-an-empty-string-that-still

Conversation

@fkb032

Copy link
Copy Markdown
Contributor

Summary

  • Treat empty or whitespace cloud API key writes as clears so keychain-backed secrets are deleted instead of saved as empty strings.
  • Normalize stored fallback secrets before backend credential routing so an empty OpenRouter key cannot beat a valid OpenAI key.
  • Add a production-path regression test for the clear-then-route path and include minimal upstream Rust compile cleanup required on origin/develop.

Fixes ENG-558
Fixes #368
Supersedes draft PR #370; this keeps the same backend direction but avoids the weaker duplicated routing test and preserves Scott's committed import ordering.

Test Plan

  • rustfmt --check --edition 2021 --config style_edition=2024,skip_children=true apps/native/src-tauri/src/storage/store.rs apps/native/src-tauri/src/state/mod.rs apps/native/src-tauri/src/evolve/edit_nix_file.rs apps/native/src-tauri/src/evolve/tools/edit_nix_file.rs
  • cargo test --manifest-path apps/native/src-tauri/Cargo.toml storage::store::tests -- --nocapture
  • cargo check --manifest-path apps/native/src-tauri/Cargo.toml
  • git diff --check

Docs

  • No docs update needed; this is a backend credential-routing bugfix with regression coverage.

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

🎨 Storybook preview

Open Storybook preview

Updated for 04f7db3

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

📋 PR Overview

Lines changed 22 (+13 / -9)
Files 0 added, 1 modified, 0 deleted
Draft / WIP no
Has Test Plan yes
No Test Plan Needed no
New UI components no
New Storybook stories no
New Rust modules no
New TS source files no
New tests no
package.json touched no
Cargo.toml touched no
Infra / CI touched no

🔬 Coverage

Report Lines Statements Functions Branches
apps/native/coverage/coverage-summary.json 26.8% 26.9% 25.1% 19.3%

Generated by 🚫 dangerJS against 04f7db3

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 889282b629

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

return Ok(Some((key, OPENAI_BASE_URL)));
Ok(select_openai_compatible_credential(
get_effective_openrouter_api_key(app)?,
get_effective_openai_api_key(app)?,

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 Badge Preserve short-circuiting when OpenRouter credentials win

When OPENROUTER_API_KEY or a stored OpenRouter key is present, this eager argument evaluation still calls get_effective_openai_api_key(app)?, which can touch the OpenAI keychain and return an error or trigger an OS prompt before select_openai_compatible_credential ever sees that OpenRouter should win. The previous inline if let returned immediately after the OpenRouter lookup, matching the env-first comment that avoids unnecessary keychain access; keep this branch lazy so OpenRouter-only environments do not fail because the unrelated OpenAI credential store is unavailable.

Useful? React with 👍 / 👎.

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.

This normalizes values on write. This means that existing broken configurations stay broken. It is also not so robust (what happens if you write the config from a different place, forgetting about the "normalization".

We should instead normalize the values on read (e.g. by having a get_secret_pref_non_empty or get_secret_pref_normalized that is used when reading the keys from the config).

fn set_secret_pref<R: Runtime>(app: &AppHandle<R>, key: &'static str, value: &str) -> Result<()> {
if e2e_mock_system_enabled() {
return set_string_pref(app, key, value);
let normalized = value.trim();

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.

Should this use normalize_secret so if the rules changes this stays consistent?

@fkb032 Farhan (fkb032) force-pushed the farhankhalaf/eng-558-clearing-a-cloud-api-key-stores-an-empty-string-that-still branch from 889282b to 04f7db3 Compare June 13, 2026 04:05
@fkb032

Copy link
Copy Markdown
Contributor Author

Rebased onto current origin/develop and narrowed the PR to the read-normalization feedback.

What changed:

  • Added get_secret_pref_normalized as the named read-side helper Juan suggested.
  • Routed OpenRouter, direct OpenAI, vLLM, and sync-secret reads through that helper so existing whitespace/empty stored secrets normalize on read.
  • Kept the provider split from develop, so the old combined OpenRouter/OpenAI eager lookup thread is now outdated.

Local verification:

  • cargo test --manifest-path apps/native/src-tauri/Cargo.toml storage::store -> 9 passed
  • cargo check --manifest-path apps/native/src-tauri/Cargo.toml --tests -> passed
  • cargo clippy --manifest-path apps/native/src-tauri/Cargo.toml --tests -> passed with existing warnings outside this diff

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.

Clearing a cloud API key stores an empty string that still wins credential routing

2 participants