server/routes/agent.js (1)

16-31: External agent endpoint wiring looks solid; cleanup guard could be a bit stricter

• validateExternalApiKey correctly enforces API key auth, ties keys to users via apiKeysDb.validateApiKey, and populates req.user so later logic (Claude userId, GitHub token lookup) has proper context.
• The main / handler cleanly parses boolean flags, validates provider, wires req.user.id into queryClaudeSDK for Claude, and uses githubTokensDb.getActiveGithubToken(req.user.id) for GitHub operations; the SSE vs non‑streaming writers match the queryClaudeSDK/spawnCursor interface.
• cleanupProject only deletes paths containing .claude/external-projects, which is a good safety net for auto‑cloned repos; if you ever allow more flexible clone locations, consider tightening this to a resolved‑path prefix check rather than a substring match, but it’s adequate for the current usage pattern.

Also applies to: 439-476, 867-1351

server/database/db.js (1)

78-95: Model provider DB layer is sound; consider tightening the API surface

• The migration and modelProvidersDb CRUD/activation behavior look correct, and getProviders’s use of api_key_preview with api_key: undefined is a good safeguard against leaking full secrets in API responses.
• getActiveProvider currently does SELECT *, returning the raw api_key. That’s fine for internal use from claude-sdk.js, but to reduce the chance of future handlers accidentally exposing full keys, it’s worth either:
  • Narrowing the select to only needed columns in getActiveProvider, or
  • Keeping getActiveProvider internal and enforcing that any HTTP‑facing code goes through getProviders.
• Minor perf nit: createProvider calls getProviders(userId) just to check whether this is the first provider. A lightweight SELECT COUNT(*) would avoid reading unnecessary columns.

Also applies to: 372-460, 462-470

server/database/init.sql (1)

52-69: Model providers schema matches usage; consider uniqueness on provider names per user

The table structure and indexes align with modelProvidersDb (FK, timestamps, is_active flag, indexes on user_id and is_active), so the schema looks sound.

If you want to prevent confusing duplicates like multiple "OpenRouter" entries for the same user, consider a unique index on (user_id, provider_name) later on.

server/routes/settings.js (4)

16-93: API key routes are correct and consistent with existing patterns

The /api-keys handlers look good: per-user scoping via req.user.id, masking keys in list responses, and clear error handling. Using a strict boolean check for isActive on the toggle endpoint is also fine.

If you want to harden slightly, you could:

• Add a radix to parseInt(keyId, 10) and handle NaN explicitly.
• Optionally accept "true"/"false" strings in addition to booleans for isActive for more tolerant clients.

---

100-193: Credentials routes mirror API key behavior cleanly

The credentials endpoints reuse the same pattern as API keys: user scoping, field validation, and not exposing credential_value at all (DB query already omits it), which is good from a security perspective. Toggle/delete logic is straightforward.

Similar to the API keys routes, you could optionally:

• Guard parseInt(credentialId, 10) against NaN.
• Loosen isActive validation if you expect non-boolean JSON from other clients.

---

199-257: Model provider list/create flow looks correct; minor API-shape nit

Listing and creation of model providers are wired correctly:

• Providers are scoped by req.user.id.
• The first provider auto-activates in modelProvidersDb.createProvider, and you correctly trigger updateClaudeSettingsForProvider only when result.isActive is true.
• Required fields (providerName, apiBaseUrl, apiKey) are validated up front.

One small nit: GET /model-providers returns snake_case fields (provider_name, api_base_url) while POST /model-providers returns camelCase in the provider payload. The current UI ignores the create response and re-fetches, so it works, but aligning naming conventions across these responses will make the API easier to consume for other clients.

---

259-294: Activation route correctly updates DB and settings.json

The activation handler correctly:

• Calls modelProvidersDb.setActiveProvider in a user-scoped way.
• Re-reads the active provider and passes it to updateClaudeSettingsForProvider.
• Treats settings write failures as non-fatal (provider remains active in DB), which matches the "settings sync is best-effort" design.

If you want to reduce an extra round-trip from the UI, you could return the activated provider (or its id) instead of just { success: true }, but the current re-fetch approach is fine.

src/components/CredentialsSettings.jsx (2)

52-56: Provider fetch wiring is correct; you may want to capture activeProviderId too

Fetching /api/settings/model-providers and storing providersData.providers into modelProviders matches the backend response.

Since the backend also returns activeProviderId, consider capturing it in state if you ever want to distinguish between “active by DB” vs. provider.is_active locally, or use it for client-side behaviors.

---

161-216: Model provider create/activate/delete logic matches the API

The three handler functions look correct:

• createModelProvider validates required fields, sets a creatingProvider flag, posts to the backend, and fully resets form state on success.
• setActiveProvider and deleteModelProvider delegate to the corresponding PATCH/DELETE endpoints and refresh via fetchData.

A couple of small optional improvements:

• Check res.ok before res.json() and surface server-side error messages to the user (e.g., toast) instead of only logging to the console.
• For deleteModelProvider, you could reuse the existing confirm text style from API keys for consistency (“Are you sure…”).

server/utils/claude-settings.js (2)

10-67: Settings read/write helpers are solid; parse-error handling could be more forgiving

The combination of:

• Ensuring the ~/.claude directory exists before reads/writes.
• Returning a sensible default { env: {}, permissions: { allow: [], deny: [] } } when the file is missing.
• Pretty-printing JSON on write.

is all good.

Consider optionally treating JSON parse errors (corrupted settings.json) similarly to ENOENT—e.g., log the issue, fall back to the default structure, and overwrite on the next write—so a bad file doesn’t permanently break settings reads.

---

162-182: Backup helper is straightforward and safe

backupClaudeSettings creates a timestamped copy next to the main file and gracefully no-ops when settings.json doesn’t exist. That’s a nice utility to have before doing risky writes.

You might optionally call this from higher-level flows before significant changes, but keeping it as an explicit tool is fine for now.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

server/utils/claude-settings.js (1)

136-150: Consider separate configuration for small/fast model.

Setting ANTHROPIC_SMALL_FAST_MODEL to the same value as ANTHROPIC_MODEL (line 139) may not align with the intended purpose. The "small/fast" model typically refers to a lighter, faster model (e.g., haiku) used for operations that don't require the full capabilities of the main model. Using the same model for both defeats this optimization.

Consider either:

1. Allowing providers to specify a separate small_fast_model_id field, or
2. Only setting ANTHROPIC_MODEL and leaving ANTHROPIC_SMALL_FAST_MODEL to use its default value.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro