feat(settings): /settings page skeleton + agent config (#554)#587
feat(settings): /settings page skeleton + agent config (#554)#587
Conversation
Adds Phase 5.1 settings UI: - Backend: settings_v2 router with GET/PUT /api/v2/settings, AgentSettings models, EnvironmentConfig.max_cost_usd + agent_type_models fields persisted to .codeframe/config.yaml. - Frontend: /settings page with Tabs (Agent functional; API Keys / PROOF9 / Workspace stubs), settingsApi client, Settings sidebar entry. Closes #554
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
WalkthroughAdds end-to-end agent settings: new EnvironmentConfig fields, Pydantic API models, a v2 FastAPI router with GET/PUT /api/v2/settings, frontend settings page and API client, sidebar navigation, and backend/frontend tests for validation and persistence. Changes
Sequence DiagramsequenceDiagram
participant Client as Web Client
participant Server as FastAPI Server
participant ConfigFile as Config File (/.codeframe/config.yaml)
rect rgba(100,150,200,0.5)
Note over Client,ConfigFile: GET /api/v2/settings
Client->>Server: GET /api/v2/settings?workspace_path=...
Server->>ConfigFile: load_environment_config(workspace_path)
ConfigFile-->>Server: EnvironmentConfig or None
Server->>Server: _config_to_response(EnvironmentConfig)
Server-->>Client: AgentSettingsResponse (200)
end
rect rgba(150,200,100,0.5)
Note over Client,ConfigFile: PUT /api/v2/settings
Client->>Server: PUT /api/v2/settings?workspace_path=... (UpdateAgentSettingsRequest)
Server->>ConfigFile: load_environment_config(workspace_path)
ConfigFile-->>Server: EnvironmentConfig or default
Server->>Server: merge request -> config (agent_type_models, max_cost_usd, agent_budget.max_iterations)
Server->>ConfigFile: save_environment_config(workspace_path, EnvironmentConfig)
Server->>Server: _config_to_response(EnvironmentConfig)
Server-->>Client: AgentSettingsResponse (200)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 unit tests (beta)
Comment |
|
PR Review posted (see below for full text - file write restricted by sandbox) |
|
PR Review: feat(settings) - /settings page skeleton + agent config Overall: Clean, well-structured implementation. The architecture rules are respected - UI router wraps core, core stays headless, CLI is untouched. Test coverage is solid. Three bugs/input-safety gaps should be fixed before merge. Bugs / Input Safety 1. agent_budget null safety (settings_v2.py line 119) config.agent_budget.max_iterations is accessed without a null check. A legacy config file written without this field will cause GET /api/v2/settings to throw a 500. Use a safe fallback: 2. No agent_type validation in PUT (settings_v2.py line 164) The PUT handler writes any string as an agent_type key to config without checking against AGENT_TYPES. A malformed client can pollute .codeframe/config.yaml with arbitrary keys. Filter the dict comprehension against AGENT_TYPES, or add a Pydantic field_validator on AgentTypeModelConfig.agent_type to reject unknown types at the model layer. 3. Empty-string model values written to config (settings_v2.py line 164) When a user leaves all models at the default value, the config gets agent_type_models with empty string values. This diverges from the key-not-present semantics that _config_to_response already handles. Filter out empty default_model values and unknown agent_type values at save time. Items 2 and 3 can be combined into one dict comprehension. Minor / Non-blocking 4. Issue reference in core comment (config.py line 9): The comment couples core to a UI issue number. Something like 'Per-agent-type model overrides, managed via web UI' ages better. 5. Identical subclasses (models.py lines 60-65): AgentSettingsResponse and UpdateAgentSettingsRequest subclass AgentSettings with no overrides. If the intent is room for future divergence, a short comment makes it explicit so the next reader does not assume it is accidental. 6. Hardcoded model lists (page.tsx lines 619-624): MODEL_OPTIONS_BY_AGENT will go stale as models ship. A TODO comment to replace with an API-driven model list would prevent it from being forgotten. What is working well
Summary: Items 1-3 are bugs or input-safety gaps - fix before merge. Items 4-6 are polish suggestions and non-blocking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
web-ui/src/__tests__/components/settings/SettingsPage.test.tsx (1)
59-67: These tests bypass the load-on-mount path.
mockSWR()handsSettingsPagepreloaded data, so the suite never verifies that the page actually callssettingsApi.get()with the selected workspace. That leaves the core fetch behavior untested even though it is part of the feature contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/__tests__/components/settings/SettingsPage.test.tsx` around lines 59 - 67, The tests currently bypass the load-on-mount fetch by using mockSWR(data...) which preloads SettingsPage; change the tests to simulate the real mount path by having mockSWR return data = undefined (isLoading true) for the initial render so SettingsPage triggers the fetch, then assert that settingsApi.get was called with the selected workspace; you can then resolve the mocked settingsApi.get (or call the returned mutate) to provide data and continue existing assertions. Target symbols: mockSWR, SettingsPage, settingsApi.get, useSWR, and mutate.codeframe/ui/routers/settings_v2.py (1)
53-73: Consider removing the unusedrequestparameter from both endpoints.The
request: Requestparameter appears in the function signatures (lines 56 and 79) but is never used. While this is a consistent pattern across v2 router endpoints, slowapi'slimiter.limit()decorator accesses the request through FastAPI's context rather than requiring it as an explicit function parameter. Removing it would simplify the signatures without affecting functionality.Note: The
workspace_pathquery parameter requirement from the coding guidelines is properly satisfied through theget_v2_workspacedependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/ui/routers/settings_v2.py` around lines 53 - 73, The request parameter is unused in the get_settings endpoint; remove the unused request: Request parameter from the function signature of get_settings (and the other similar v2 endpoint in this module) and keep the workspace: Workspace = Depends(get_v2_workspace) dependency intact; ensure any references to request inside these functions are also removed (or replaced) and update any type hints/imports if Request is no longer used so there are no unused imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@codeframe/core/config.py`:
- Around line 166-168: EnvironmentConfig.validate() currently omits the newly
persisted fields max_cost_usd and agent_type_models; update validate() to ensure
max_cost_usd is either None or a non-negative float and to ensure
agent_type_models is a dict with string keys and string values (no empty
keys/values), and if your codebase exposes a canonical set of agent types (e.g.,
an AgentType enum or AGENT_TYPES registry) also assert that all keys in
agent_type_models are members of that set; on failure raise ValueError (or use
existing validation error pattern in EnvironmentConfig.validate()) so
invalid/migrated configs are rejected.
In `@codeframe/ui/models.py`:
- Around line 932-934: The declared default for max_turns in the UI model
(max_turns: int = Field(... default=20 ...)) is inconsistent with the core
config default (EnvironmentConfig.agent_budget.max_iterations default=100);
update one so both match: either change the UI model default to 100 or change
EnvironmentConfig.agent_budget.max_iterations default in
codeframe/core/config.py to 20 (and update any related docs/tests), and ensure
the GET serialization uses the same source
(EnvironmentConfig.agent_budget.max_iterations) so new workspaces load the
intended default.
- Around line 917-923: The agent_type Field in the model schema currently
accepts any string; replace its loose definition with a constrained type so
invalid values fail validation upfront: change agent_type to use a Python Enum
or typing.Literal that enumerates the allowed values from AGENT_TYPES (or derive
the Enum from AGENT_TYPES), update any import for Enum/Literal and Pydantic
typing, and ensure the model class uses that Enum/Literal type instead of str
(keep the Field description), so PUT payloads are validated against the explicit
set rather than accepting arbitrary strings.
In `@web-ui/src/app/settings/page.tsx`:
- Around line 34-39: Replace the hardcoded MODEL_OPTIONS_BY_AGENT with dynamic
data fetched from the backend: call GET /api/v2/agents/config in a useEffect (or
equivalent load function) within page.tsx, parse the response into the same
shape (Record<AgentTypeKey, string[]>), store it in component state (e.g.,
modelOptionsByAgent) and use that state wherever MODEL_OPTIONS_BY_AGENT was
referenced; ensure the loader merges or preserves any currently selected/saved
model values so they remain selectable even if not returned by the backend, and
fall back to empty arrays or sensible defaults on fetch errors.
---
Nitpick comments:
In `@codeframe/ui/routers/settings_v2.py`:
- Around line 53-73: The request parameter is unused in the get_settings
endpoint; remove the unused request: Request parameter from the function
signature of get_settings (and the other similar v2 endpoint in this module) and
keep the workspace: Workspace = Depends(get_v2_workspace) dependency intact;
ensure any references to request inside these functions are also removed (or
replaced) and update any type hints/imports if Request is no longer used so
there are no unused imports.
In `@web-ui/src/__tests__/components/settings/SettingsPage.test.tsx`:
- Around line 59-67: The tests currently bypass the load-on-mount fetch by using
mockSWR(data...) which preloads SettingsPage; change the tests to simulate the
real mount path by having mockSWR return data = undefined (isLoading true) for
the initial render so SettingsPage triggers the fetch, then assert that
settingsApi.get was called with the selected workspace; you can then resolve the
mocked settingsApi.get (or call the returned mutate) to provide data and
continue existing assertions. Target symbols: mockSWR, SettingsPage,
settingsApi.get, useSWR, and mutate.
🪄 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: 79b17b79-274c-457b-af88-3049075cbf2b
📒 Files selected for processing (10)
codeframe/core/config.pycodeframe/ui/models.pycodeframe/ui/routers/settings_v2.pycodeframe/ui/server.pytests/ui/test_settings_v2.pyweb-ui/src/__tests__/components/settings/SettingsPage.test.tsxweb-ui/src/app/settings/page.tsxweb-ui/src/components/layout/AppSidebar.tsxweb-ui/src/lib/api.tsweb-ui/src/types/index.ts
| const MODEL_OPTIONS_BY_AGENT: Record<AgentTypeKey, string[]> = { | ||
| claude_code: ['claude-opus-4', 'claude-sonnet-4', 'claude-haiku-4'], | ||
| codex: ['gpt-4o', 'gpt-4o-mini', 'o3', 'o3-mini'], | ||
| opencode: ['claude-opus-4', 'claude-sonnet-4', 'gpt-4o'], | ||
| react: ['claude-opus-4', 'claude-sonnet-4', 'gpt-4o', 'qwen2.5-coder:7b'], | ||
| }; |
There was a problem hiding this comment.
Load model options from the backend instead of hardcoding them.
Issue #554 calls for these choices to come from GET /api/v2/agents/config. Keeping a static table here will drift from server-supported models, and any already-saved model outside these arrays becomes impossible to display/reselect correctly in the form.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/app/settings/page.tsx` around lines 34 - 39, Replace the hardcoded
MODEL_OPTIONS_BY_AGENT with dynamic data fetched from the backend: call GET
/api/v2/agents/config in a useEffect (or equivalent load function) within
page.tsx, parse the response into the same shape (Record<AgentTypeKey,
string[]>), store it in component state (e.g., modelOptionsByAgent) and use that
state wherever MODEL_OPTIONS_BY_AGENT was referenced; ensure the loader merges
or preserves any currently selected/saved model values so they remain selectable
even if not returned by the backend, and fall back to empty arrays or sensible
defaults on fetch errors.
- AgentType is now a Literal in the Pydantic model so unknown agent_type values fail validation up front (claude #2, coderabbit #2). - PUT no longer persists empty default_model strings — those are semantically equivalent to "key not present" via the response builder and only added YAML noise (claude #3). - Null-guard for legacy YAML where agent_budget was hand-removed/nulled (claude #1) — both GET (defaults) and PUT (re-create budget) paths. - EnvironmentConfig.validate() now rejects negative max_cost_usd and unknown agent_type_models keys (coderabbit #1). - AgentSettings.max_turns Pydantic default raised from 20 → 100 to match EnvironmentConfig.agent_budget.max_iterations default (coderabbit #3).
|
Pushed 3cf0b8b addressing review feedback. Summary: Claude review:
CodeRabbit:
Tests added:
Skipped nitpicks: identical Pydantic subclasses (intentional for future divergence), unused |
Follow-up ReviewAll three bugs from the previous review are confirmed fixed in the diff: 1. 2. Unknown agent_type rejection — 3. Empty model strings filtered — the dict comprehension One new observation (low priority)
config = load_environment_config(...) # read
config.agent_budget.max_iterations = body.max_turns
save_environment_config(...) # writeTwo concurrent PUT requests could interleave and lose one set of changes. For a single-user local tool this is acceptable, but worth a comment in the handler noting that last-write-wins is intentional. Deferred items (confirmed as intentional)
StatusAll blocking items from the first review are resolved. The test suite covers the key paths (round-trip, config preservation, null budget, empty strings, unknown types). The TOCTOU note above is non-blocking. LGTM — ready to merge pending any final CI check. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/ui/test_settings_v2.py (1)
236-257: Legacy YAML compatibility test is valuable but misplaced.The
test_get_handles_null_agent_budgettest verifies GET behavior but is located in theTestSettingsV2Putclass. Consider moving it toTestSettingsV2Getfor better organization.The test itself is valuable - manually nulling
agent_budgetin YAML to simulate legacy/hand-edited files ensures the router handles this gracefully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ui/test_settings_v2.py` around lines 236 - 257, The test test_get_handles_null_agent_budget is verifying GET behavior but currently lives in the TestSettingsV2Put test class; move the entire test function into the TestSettingsV2Get class so it lives with other GET-related tests (locate test_get_handles_null_agent_budget and relocate it from TestSettingsV2Put to TestSettingsV2Get) to improve organization and keep GET behavior tests together.tests/core/test_environment_config.py (1)
1-15: Consider adding v2 marker for new tests.The new tests at lines 111-123 validate v2 functionality (
EnvironmentConfigvalidation for settings page fields). Per coding guidelines, new v2 tests should be marked with@pytest.mark.v2decorator orpytestmark = pytest.mark.v2at module level.Since this file has existing tests without markers, you may want to either:
- Add
pytestmark = pytest.mark.v2at module level if all tests should run in the v2 suite, or- Add
@pytest.mark.v2decorators only to the new test methodsAs per coding guidelines: "New v2 tests must be marked with
@pytest.mark.v2decorator orpytestmark = pytest.mark.v2at module level"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/test_environment_config.py` around lines 1 - 15, Add v2 test marking to the new EnvironmentConfig validation tests: either add a module-level pytest mark by importing pytest and adding pytestmark = pytest.mark.v2 at the top of the file, or decorate the specific new test functions that validate EnvironmentConfig settings page fields (the tests around lines 111-123 that assert EnvironmentConfig validation for settings page fields) with `@pytest.mark.v2`; ensure you import pytest if you add the decorator or module-level marker.codeframe/ui/routers/settings_v2.py (1)
91-108: PUT endpoint correctly merges settings without overwriting unrelated fields.The implementation properly:
- Loads existing config to preserve other fields (line 92)
- Guards against
Noneagent_budgetfor legacy compatibility (lines 93-94)- Skips empty model strings to avoid YAML noise (line 104)
- Saves and returns the updated config
One consideration:
EnvironmentConfig.validate()is not called beforesave_environment_config(). SinceUpdateAgentSettingsRequestuses Pydantic validation withAgentTypeLiteral andmax_turns: int = Field(..., gt=0), input validation is handled at the request layer. However, if you want defense-in-depth against invalid config states from merging with potentially malformed legacy configs, you could add validation before save.💡 Optional: Add validation before save
config.agent_type_models = { entry.agent_type: entry.default_model for entry in body.agent_models if entry.default_model } + errors = config.validate() + if errors: + raise HTTPException( + status_code=400, + detail=api_error( + "Invalid configuration", ErrorCodes.VALIDATION_FAILED, "; ".join(errors) + ), + ) + save_environment_config(workspace.repo_path, config)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/ui/routers/settings_v2.py` around lines 91 - 108, The code merges and mutates an EnvironmentConfig but doesn't validate the final config before persisting; call EnvironmentConfig.validate() (or equivalent validation method) on the merged config after setting agent_budget, agent_budget.max_iterations, max_cost_usd, and agent_type_models and before calling save_environment_config(workspace.repo_path, config) to catch malformed legacy/merged state; reference the functions/classes load_environment_config, EnvironmentConfig, AgentBudgetConfig, save_environment_config, and _config_to_response when locating where to insert the validation step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@codeframe/ui/routers/settings_v2.py`:
- Around line 91-108: The code merges and mutates an EnvironmentConfig but
doesn't validate the final config before persisting; call
EnvironmentConfig.validate() (or equivalent validation method) on the merged
config after setting agent_budget, agent_budget.max_iterations, max_cost_usd,
and agent_type_models and before calling
save_environment_config(workspace.repo_path, config) to catch malformed
legacy/merged state; reference the functions/classes load_environment_config,
EnvironmentConfig, AgentBudgetConfig, save_environment_config, and
_config_to_response when locating where to insert the validation step.
In `@tests/core/test_environment_config.py`:
- Around line 1-15: Add v2 test marking to the new EnvironmentConfig validation
tests: either add a module-level pytest mark by importing pytest and adding
pytestmark = pytest.mark.v2 at the top of the file, or decorate the specific new
test functions that validate EnvironmentConfig settings page fields (the tests
around lines 111-123 that assert EnvironmentConfig validation for settings page
fields) with `@pytest.mark.v2`; ensure you import pytest if you add the decorator
or module-level marker.
In `@tests/ui/test_settings_v2.py`:
- Around line 236-257: The test test_get_handles_null_agent_budget is verifying
GET behavior but currently lives in the TestSettingsV2Put test class; move the
entire test function into the TestSettingsV2Get class so it lives with other
GET-related tests (locate test_get_handles_null_agent_budget and relocate it
from TestSettingsV2Put to TestSettingsV2Get) to improve organization and keep
GET behavior tests together.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: acb49066-b1a8-418e-a1c9-43359df13b16
📒 Files selected for processing (5)
codeframe/core/config.pycodeframe/ui/models.pycodeframe/ui/routers/settings_v2.pytests/core/test_environment_config.pytests/ui/test_settings_v2.py
🚧 Files skipped from review as they are similar to previous changes (1)
- codeframe/ui/models.py
Follow-up (Round 3)Confirming my previous LGTM still stands. The current diff reflects all three blocking fixes cleanly. Weighing in on the remaining CodeRabbit nitpicks from the second pass: Worth fixing before merge:
Low priority / defer:
Status: Ready to merge once the |
* feat(settings): API key management tab (#555) 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> * fix: address CodeRabbit feedback on PR #588 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> * docs: mark issue #555 done in roadmap (5.1) * fix: address claude-review feedback on PR #588 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> --------- Co-authored-by: Test User <test@example.com> Co-authored-by: Claude <noreply@anthropic.com>
1 participant
Summary
/settingspage (Phase 5.1) — sidebar nav entry, Radix Tabs (Agent functional; API Keys / PROOF9 / Workspace stubs).settings_v2router withGET/PUT /api/v2/settings, persisted to.codeframe/config.yamlvia existing helpers.EnvironmentConfigwithmax_cost_usdandagent_type_models(per-agent-type model overrides).Closes #554
Test plan
uv run pytest tests/ui/test_settings_v2.py(6/6 pass)uv run pytest tests/ui/(199/199 pass)uv run ruff check .(clean)npm test(790/790 pass; 7 new tests for SettingsPage)npm run build(/settingsroute in output)/settings, change Claude Code model + max turns, save, reload, verify persistenceAcceptance criteria (from issue)
/settingsrenders with sidebar navnpm testandnpm run buildpassSummary by CodeRabbit
New Features
Tests
Documentation