feat(settings): API key management tab (#555)#588
Conversation
Adds the API Keys tab to /settings so users can store, verify, and remove the three core credentials without touching environment variables. Builds on the settings page skeleton from #587. Backend (codeframe/ui/routers/settings_v2.py) - GET /api/v2/settings/keys — status for the 3 providers (no plaintext) - PUT /api/v2/settings/keys/{provider} — store with format validation - DELETE /api/v2/settings/keys/{provider} — idempotent removal - POST /api/v2/settings/verify-key — live check (Anthropic / OpenAI / GitHub) Storage delegates to existing CredentialManager (keyring with encrypted-file fallback). Env vars continue to take precedence at runtime; the status endpoint reports the source so the UI can surface that. Frontend - ApiKeysTab + KeySlot components with masked input, last-4 display, Verify / Save / Remove flows, source badge, env-var read-only mode - settingsApi extended with getKeys / storeKey / removeKey / verifyKey - /settings page renders tabs even without a workspace so the API Keys flow works for first-time users hitting the env-var dead end Tests - 20 new pytest cases covering store/status/delete/verify, plaintext-leak guards, env-precedence, format validation, unknown-provider rejection - 9 new jest tests for ApiKeysTab covering render, save/verify/remove, invalid verification, env-source disabling Closes #555 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds machine-wide API key management: backend endpoints and verification helpers, Pydantic/TS types, frontend client + ApiKeysTab/KeySlot UI, page integration, comprehensive backend/frontend tests, and a small roadmap text update. ChangesAPI Key Management
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant settingsApi
participant Server
participant CredentialManager
participant ProviderAPI
User->>UI: open Settings > API Keys
UI->>settingsApi: getKeys() / storeKey / verifyKey / removeKey
settingsApi->>Server: HTTP request to /api/v2/settings/*
Server->>CredentialManager: read/store/delete credentials
Server->>ProviderAPI: verification call (async/sync)
ProviderAPI-->>Server: verification result
Server-->>settingsApi: response (no plaintext)
settingsApi-->>UI: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
PR #588 — API Key Management TabGood implementation overall. The security-first design (no plaintext in responses, env-var precedence, format validation before storage) is exactly right. The test coverage is solid across both layers. A few things worth addressing: Security / Correctness
except Exception as e:
return False, f"GitHub verification failed: {e}"If
return False, f"Anthropic verification failed: {e}"SDK auth errors are usually safe, but the pattern is worth making explicit. Log Verify endpoint rate limit — ArchitectureDirect
Minor Issues
if "no such" in msg or "not found" in msg:
return Response(status_code=204)If
FrontendThe
Test CoverageThe 20 new Python tests and 9 frontend tests are comprehensive for the happy path and most error branches. One gap: no test for the Summary: The security fundamentals are solid. The main items worth a follow-up are (1) tightening the verify rate limit, (2) sanitizing error messages in verification helpers to avoid token leakage, and (3) either moving verification logic to the adapter layer or adding |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codeframe/ui/routers/settings_v2.py (1)
146-355: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftMove machine-scope key behavior out of the UI router.
This block makes
codeframe/uiown provider resolution, status shaping, live verification, and the first workspace-less routes in this router. Please push that behavior into a core credential service and keep this module as a thin HTTP adapter, or split these machine-scope routes into a module with an explicit non-workspace contract.As per coding guidelines, "codeframe/ui/**/*.py: FastAPI server and web UI are thin adapters over core — do NOT implement domain logic in
codeframe/ui/routers" and "codeframe/ui/routers/**/*.py: Web UI API client must requireworkspace_pathquery parameter for all endpoints"🤖 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 `@codeframe/ui/routers/settings_v2.py` around lines 146 - 355, The router currently implements domain logic (provider mapping, key status shaping, live verification) in functions like _PROVIDER_MAP/_resolve_provider, _build_status, _check_github_token, _verify_anthropic_sync, _verify_openai_sync and the verify_key/store_key/delete_key endpoints; move that logic into a core credential service (eg. CredentialService) in the core package that exposes methods such as resolve_provider(provider: str) -> CredentialProvider, get_key_status(provider: KeyProvider, manager: CredentialManager) -> KeyStatusResponse, verify_key(provider: CredentialProvider, key: str) -> (bool,str), store_credential(cp, value), delete_credential(cp) and use CredentialManager only as a persistence dependency inside the service; then update this UI router so store_key/delete_key/list_key_status/verify_key simply call the new service methods and return results (or split these machine-scope routes into a dedicated non-workspace module with an explicit non-workspace contract if you prefer), removing provider mapping and verification helpers from codeframe/ui and keeping the router a thin HTTP adapter.
🧹 Nitpick comments (1)
tests/ui/test_settings_v2.py (1)
461-478: 💤 Low valueConsider a clearer pattern for raising exceptions in mocks.
The generator throw pattern
(_ for _ in ()).throw(Exception(...))on line 466 works but is obscure. A straightforward helper function would improve readability.♻️ Suggested improvement
class FakeAnthropicClient: def __init__(self, api_key): + def raise_auth_error(): + raise Exception("401 unauthorized") self.models = type( "M", (), - {"list": lambda self_: (_ for _ in ()).throw(Exception("401 unauthorized"))}, + {"list": lambda self_: raise_auth_error()}, )()🤖 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 `@tests/ui/test_settings_v2.py` around lines 461 - 478, The mock in FakeAnthropicClient uses an obscure generator throw to raise an exception; replace it with a clear helper that raises the error and assign that to the models.list attribute: inside FakeAnthropicClient.__init__ create a small function (e.g., raise_unauthorized) that raises Exception("401 unauthorized") and set self.models = type("M", (), {"list": raise_unauthorized})(), then keep the monkeypatch on settings_v2._AnthropicClient and the keys_client.post assertions unchanged so the test behavior is identical but readable.
🤖 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 `@codeframe/ui/routers/settings_v2.py`:
- Around line 307-313: The _verify_anthropic_sync function currently calls the
non-existent client.models.list(); update it to call client.messages.create(...)
(e.g., send a minimal test message like role="system", content="ping") to
validate the API key and catch auth errors instead. Replace the models.list()
call with a call to client.messages.create and preserve the same try/except
behavior so failures return False with the error message and success returns
True with "Anthropic key accepted".
In `@web-ui/src/__tests__/components/settings/ApiKeysTab.test.tsx`:
- Around line 213-214: The test currently checks
slot.querySelector('button[aria-label]') which always passes because the KeySlot
buttons lack aria-labels; update the assertion to verify that no visible
"Remove" button is rendered for environment-source keys (e.g., assert that there
is no button whose accessible name or innerText matches "Remove" /
case-insensitive) so the env-source restriction is actually tested; locate the
check in the ApiKeysTab.test.tsx around the KeySlot usage and replace the
selector-based assertion with one that queries by button text/accessible name
"Remove".
---
Outside diff comments:
In `@codeframe/ui/routers/settings_v2.py`:
- Around line 146-355: The router currently implements domain logic (provider
mapping, key status shaping, live verification) in functions like
_PROVIDER_MAP/_resolve_provider, _build_status, _check_github_token,
_verify_anthropic_sync, _verify_openai_sync and the
verify_key/store_key/delete_key endpoints; move that logic into a core
credential service (eg. CredentialService) in the core package that exposes
methods such as resolve_provider(provider: str) -> CredentialProvider,
get_key_status(provider: KeyProvider, manager: CredentialManager) ->
KeyStatusResponse, verify_key(provider: CredentialProvider, key: str) ->
(bool,str), store_credential(cp, value), delete_credential(cp) and use
CredentialManager only as a persistence dependency inside the service; then
update this UI router so store_key/delete_key/list_key_status/verify_key simply
call the new service methods and return results (or split these machine-scope
routes into a dedicated non-workspace module with an explicit non-workspace
contract if you prefer), removing provider mapping and verification helpers from
codeframe/ui and keeping the router a thin HTTP adapter.
---
Nitpick comments:
In `@tests/ui/test_settings_v2.py`:
- Around line 461-478: The mock in FakeAnthropicClient uses an obscure generator
throw to raise an exception; replace it with a clear helper that raises the
error and assign that to the models.list attribute: inside
FakeAnthropicClient.__init__ create a small function (e.g., raise_unauthorized)
that raises Exception("401 unauthorized") and set self.models = type("M", (),
{"list": raise_unauthorized})(), then keep the monkeypatch on
settings_v2._AnthropicClient and the keys_client.post assertions unchanged so
the test behavior is identical but readable.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84e1ad79-ab12-429a-8f78-ee4a4015b672
📒 Files selected for processing (9)
codeframe/ui/models.pycodeframe/ui/routers/settings_v2.pytests/ui/test_settings_v2.pyweb-ui/src/__tests__/components/settings/ApiKeysTab.test.tsxweb-ui/src/app/settings/page.tsxweb-ui/src/components/settings/ApiKeysTab.tsxweb-ui/src/components/settings/KeySlot.tsxweb-ui/src/lib/api.tsweb-ui/src/types/index.ts
1. Anthropic verification: switch from client.models.list() to client.messages.create() with max_tokens=1. The codebase pins anthropic>=0.18.0 and the .models attribute was added later, so models.list() was unreliable. messages.create() has been the stable surface since v0.18 and AuthenticationError lets us distinguish a bad key from a transient/network failure. 2. Test assertion fix: the env-source remove-button assertion in ApiKeysTab.test.tsx was vacuous (it queried for buttons with aria-label, but the component renders none). Replaced with a text-based search for a button labelled Remove. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
PR #588 Review — Follow-upMy previous comment was truncated mid-sentence. Picking up and completing it here. Security / Correctness
except Exception as e:
return False, f"GitHub verification failed: {e}"If the underlying except Exception:
return False, "GitHub verification failed: network or server error"
Code Quality
return _build_status(provider, manager) # type: ignore[arg-type]The Redundant aria-label={`${displayName} API key`}
Unnecessary const refresh = async () => {
await mutate();
};This is just Hardcoded model ID in The code uses TestingThe fixture bypasses manager = CredentialManager.__new__(CredentialManager)
manager._store = storeThis is fragile — if
SummaryThe implementation is solid and the security fundamentals (no plaintext in responses, env-var precedence, format validation, idempotent delete) are all correct. The two items above worth fixing before merge are:
The remaining points are style/quality improvements that can be addressed here or as a follow-up. |
PR #588 - Follow-up ReviewReferencing my two earlier comments. The code has not changed since those were posted, so this is a status summary of what still needs attention before merge. Still Outstanding (critical)1. Token leakage in The bare Minimal fix: catch Exception without binding 2. No specific auth-error handling in Anthropic catches Still Outstanding (style/quality, lower priority)Covered in my second comment; can be addressed as a fast follow-up after merge:
VerdictThe two security items (token leakage, OpenAI auth-error discrimination) should be fixed before merge. Everything else is non-blocking. The overall design is solid: no plaintext in responses, env-var precedence, format validation, idempotent delete, isolated test fixtures. |
Critical (security): - _check_github_token, _verify_anthropic_sync, _verify_openai_sync no longer echo raw exception detail to the client. aiohttp errors can carry the request URL and headers (which include the bearer token); the detail is now logged server-side and a generic message returned. - _verify_openai_sync gained a specific openai.AuthenticationError branch so users see "key rejected" vs "network error". - POST /verify-key uses rate_limit_ai (20/min) instead of standard (100/min) — the endpoint makes outbound API calls and can be used to enumerate key validity or burn quota. Quality: - _last_four guards against values shorter than 4 chars. - store_key uses cast(KeyProvider, provider) instead of type:ignore. - Anthropic verification uses the full model id "claude-haiku-4-5-20251001". - KeySlot aria-label drops the redundant " API key" suffix. - ApiKeysTab passes mutate inline instead of wrapping it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
PR #588 — Follow-up ReviewBuilding on my earlier review: several items from the first pass have been addressed in the latest commits. Noting what is resolved and what is still open. Resolved from prior review
Still open
Architecture concern (also flagged by CodeRabbit): Minor remaining gapNo test covers OverallThe security-critical items from the first review are addressed. The three issues above are real but none is a blocker — the |
1 participant
Summary
Adds the API Keys tab to
/settingsso users can store, verify, and remove the three core credentials (Anthropic, OpenAI, GitHub PAT) without touching environment variables. Builds on the settings page skeleton from #587.Closes #555.
Backend (
codeframe/ui/routers/settings_v2.py)GET /api/v2/settings/keys— status for the 3 providers (never returns plaintext)PUT /api/v2/settings/keys/{provider}— store with format validationDELETE /api/v2/settings/keys/{provider}— idempotent removalPOST /api/v2/settings/verify-key— live check (Anthropic models.list / OpenAI models.list / GitHub/user)Storage delegates to the existing
CredentialManager(keyring with encrypted-file fallback). Env vars continue to take precedence at runtime; the status endpoint reports the source (environment/stored/none) so the UI can render env-loaded keys read-only.A new
get_credential_managerFastAPI dependency lets tests inject an isolated, file-backed store so they don't touch the developer's real keyring.Frontend
ApiKeysTab+KeySlotcomponents with: masked input, last-4 display, Verify / Save / Remove flows, source badge, env-var read-only modesettingsApiextended withgetKeys/storeKey/removeKey/verifyKey(machine-wide — noworkspace_path)/settingsnow renders its tab shell even without a workspace so the API Keys flow works for first-time users hitting the env-var dead end. The Agent tab still requires a workspace and shows the existing prompt.Test plan
uv run pytest tests/ui/test_settings_v2.py— 29 passed (9 existing + 20 new)uv run pytest tests/ui/ tests/core/test_credentials.py— 227 passeduv run ruff check .— all checks passednpm test— 799 passed (includes 9 new ApiKeysTab tests + 7 unchanged SettingsPage tests)npm run build—/settingsstatic route emitted/settings→ API Keys tab → store/verify/remove a real key (covered by demo verification phase)Acceptance criteria (from issue)
test_status_never_returns_plaintext,test_store_response_excludes_plaintext)test_delete_removes_credential)npm testandnpm run buildpassOut of scope (per issue)
OAuth flows, key rotation/expiry, sidebar
hasWorkspacegating refactor.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation