feat(providers): support custom providers in local anonymous chat via file store - #103
Conversation
… file store Allow custom provider instances (custom+/openai-chat-completions+) to be created and registered on local non-DB accounts by persisting them in a gitignored per-user file store at .fleet/providers.json instead of requiring pi_user_providers. The store uses atomic temp-file+rename writes with a per-process mutation lock, degrades to a diagnostic when unreadable, and scopes files per account. Loopback http base URLs are now allowed for OCC-family instances on local dev surfaces only; https stays enforced elsewhere. Registration/route logic shares normalized allocation and URL validation, and local-store reads are mocked in unit tests for hygiene. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 26 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughCustom providers now support local account-scoped storage and Postgres storage. Shared provider normalization, ID allocation, URL validation, atomic writes, route handlers, runtime registration, diagnostics, schemas, tests, and local development guidance were updated. ChangesCustom provider storage and runtime
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant providersServerHandlers
participant ProviderStore
participant custom-provider-registry
participant ModelRuntime
Client->>providersServerHandlers: Save custom provider
providersServerHandlers->>ProviderStore: Create or update instance
custom-provider-registry->>ProviderStore: List instances and load API keys
custom-provider-registry->>ModelRuntime: Register validated models
ModelRuntime-->>Client: Provide registered custom model
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds first-class support for custom provider instances in local anonymous / non-DB chat by introducing a gitignored on-disk provider store, while keeping deployed/DB-backed behavior using pi_user_providers. This aligns the Settings providers surface and Pi runtime registration so locally-created custom providers can be persisted and re-registered across sessions.
Changes:
- Introduces shared custom-provider utilities in
@workspace/pi-protocol(API family list/guards, consistent id allocation, instance normalization). - Updates
/api/chat/providersand the Pi runtime custom-provider registry to select between Postgres storage and a local.fleet/file store. - Adds atomic file-writing utilities and comprehensive unit tests covering local-store CRUD, route behavior, and registration behavior.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/pi-protocol/src/provider-catalog.ts | Adds custom-provider API family helpers, consistent id allocation, and instance normalization utilities. |
| apps/web/src/routes/api/chat/providers.ts | Routes provider CRUD to Postgres vs local file store and centralizes base URL policy. |
| apps/web/src/routes/api/chat/tests/providers-custom-route.test.ts | Tests route handler behavior for local anonymous custom providers and storage selection. |
| apps/web/src/lib/pi/server-shared.test.ts | Mocks the local provider store for server-shared tests. |
| apps/web/src/lib/pi/runtime/openai-chat-completions-url.ts | Consolidates custom-provider base URL validation and tightens local-http gating to deployed surfaces. |
| apps/web/src/lib/pi/runtime/custom-provider-registry.ts | Registers custom providers from either Postgres or the local file store and surfaces store-read diagnostics. |
| apps/web/src/lib/pi/runtime/tests/session-factory.test.ts | Mocks local provider store in session-factory tests. |
| apps/web/src/lib/pi/runtime/tests/openai-chat-completions-registration.test.ts | Ensures DB-backed OCC registration tests don’t accidentally use the local store. |
| apps/web/src/lib/pi/runtime/tests/custom-provider-local-registration.test.ts | Adds coverage for registering local-store providers (including unreadable-store degradation). |
| apps/web/src/lib/fs-atomic.ts | New helper for atomic same-directory temp-file + rename writes. |
| apps/web/src/lib/env-manager.ts | Switches .env.local persistence to the shared atomic write helper. |
| apps/web/src/lib/db/user-settings.ts | Uses shared chat DB configuration predicate. |
| apps/web/src/lib/db/user-providers.ts | Uses shared chat DB configuration predicate. |
| apps/web/src/lib/db/occ-instances.ts | Reuses shared id allocation + instance normalization; widens input shape for upserts. |
| apps/web/src/lib/db/local-provider-instances.ts | Implements the local .fleet/ plaintext provider instance store with serialized writes. |
| apps/web/src/lib/db/chat-db-config.ts | Adds a shared isChatDatabaseConfigured() helper. |
| apps/web/src/lib/db/tests/local-provider-instances.test.ts | Adds test coverage for local-store scoping, atomic writes, and concurrent creates. |
| AGENTS.md | Updates contributor documentation to reflect the new local custom-provider storage behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…dable store in providers route
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (3)
apps/web/src/routes/api/chat/providers.ts:80
- When
useLocalProviderStore(userId)is true, an unreadable/malformed local provider store will throw fromlistLocalProviderInstancesWithApiKeyand currently bubbles up as a 500 forGET /api/chat/providers(and also after POST/DELETE when returningupdatedProviders). That breaks the Providers UI even though session creation intentionally degrades to a warning + empty list. Consider catching local-store read errors here and falling back to[]so Settings still loads and the runtime diagnostics remain the primary surfacing mechanism.
// One query + one decrypt pass for DB-backed accounts; local anonymous/dev
// accounts read the gitignored file store. DB errors propagate (route returns
// 500) instead of a misleading "not configured" row.
const instances = useLocalProviderStore(userId)
? await listLocalProviderInstancesForRoute(userId)
: await listOccInstancesWithApiKey(userId)
AGENTS.md:148
- This bullet says the local file store is at
<projectRoot>/.fleet/providers.json, but the implementation writes per-scope files (e.g.providers.anonymous.jsonandproviders.user-<hash>.json). This mismatch can mislead contributors when debugging local provider persistence.
- **Custom providers (native Pi mechanism).** Users can register any OpenAI/Anthropic/Google-compatible endpoint, each `{ displayName, apiKey, baseUrl, api, modelIds }` with `api` one of `openai-completions`, `openai-responses`, `anthropic-messages`, `google-genai`. General custom providers get a slug id `custom+<slug>`; legacy named OCC instances keep `openai-chat-completions+<slug>` — both register through the same path. The reserved `openai-chat-completions` id stays the default/Neon AI Gateway slot. Storage reuses `pi_user_providers` (`provider_id` = slug, encrypted `encrypted_key` for the apiKey, encrypted `encrypted_payload` JSON `{displayName, baseUrl, api, modelIds}` — legacy payloads with `modelId` normalize on read); no Neon migration. `apps/web/src/lib/db/occ-instances.ts` is the persistence layer; `custom-provider-registry.ts` (`registerCustomProviders`) registers each instance as its own native Pi provider (`<instanceId>/<modelId>` in the model picker; `google-genai` maps to Pi's `google-generative-ai`) with a per-host `max_tokens` cap (25k + gateway compat on `*.neon.tech` OCC endpoints, else 32k). Custom instances pick a storage backend at runtime: Postgres `pi_user_providers` when the account is DB-backed (`userId` + `FLEET_PI_CHAT_DATABASE_URL`), otherwise a gitignored per-scope local file store at `<projectRoot>/.fleet/providers.anonymous.json` (anonymous chat) or `<projectRoot>/.fleet/providers.user-<hash>.json` (local signed-in accounts; plaintext, atomic temp-file+rename) that enables anonymous/local non-DB chat — `POST /api/chat/providers` with `createOccInstance: true` or an existing `…+<slug>` id. OCC-family instances may use loopback `http://localhost` base URLs in local dev; everything else stays https-only. The reserved default OCC slot remains the env-backed `OPENAI_CHAT_COMPLETIONS_{API_KEY,BASE_URL,MODEL}`; `apps/web/src/lib/db/local-provider-instances.ts` is the local store (and `useLocalProviderStore` picks the backend). Sandbox credential sync covers only the reserved default OCC slot for now (custom instances are not yet pushed to Daytona).
apps/web/src/lib/fs-atomic.ts:14
tempPathis derived fromprocess.pid+Date.now(). Two writes to the samefilePathwithin the same millisecond in the same process can collide on the same temp filename, causing intermittent write failures or clobbering. Using a random suffix (e.g.randomUUID()) makes the atomic write helper robust under concurrent callers.
export async function writeFileAtomic(filePath: string, content: string) {
await mkdir(dirname(filePath), { recursive: true })
const tempPath = `${filePath}.${process.pid}.${Date.now()}.tmp`
try {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (3)
apps/web/src/lib/db/local-provider-instances.ts:255
createLocalProviderInstancealso writesmodelIdfromnormalized.modelIds[0]without guarding against an empty model list. A bad caller would persist an invalid row and brick future reads. Adding a local invariant check here keeps the store format self-consistent.
modelId: normalized.modelIds[0],
apps/web/src/lib/db/local-provider-instances.ts:206
modelIdis derived fromnormalized.modelIds[0]without checking that at least one model id exists. If an emptymodelIds/missingmodelIdever reaches this function, it will writemodelId: undefinedto disk and make the store unreadable on the next load (failsisLocalProviderInstance). Consider enforcing the invariant here to avoid corrupting the local store.
This issue also appears on line 255 of the same file.
modelId: normalized.modelIds[0],
apps/web/src/routes/api/chat/providers.ts:235
- When
useLocalStoreis true, failures reading/writing the local provider store (e.g. malformed JSON or futureversion) will currently bubble up and 500 the POST update/create path. Since GET explicitly degrades unreadable stores to[], it’s better to also catch local-store failures here and return a 400 with a clear message so the Settings UI doesn’t hard-fail on save/remove attempts.
if (!isExistingInstance) {
instanceId = useLocalStore
? await createLocalProviderInstance(
userId,
instanceBody,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (4)
apps/web/src/routes/api/chat/providers.ts:487
- DELETE has the same failure mode as POST for local instances: if the local store JSON is malformed,
getLocalProviderInstance/removeLocalProviderInstancewill throw and the route returns a 500. That makes it impossible to remove broken instances via Settings, which is the primary recovery path for a bad store.
if (useLocalProviderStore(userId)) {
const existing = await getLocalProviderInstance(
userId,
body.providerId
)
apps/web/src/lib/fs-atomic.ts:17
writeFileAtomiccan generate the same temp filename for concurrent writes to the same target within a single process (samepid+ sameDate.now()millisecond). That race can cause one writer to rename away the temp file and the other to fail with ENOENT, making env/provider updates flaky under parallel requests.
await mkdir(dirname(filePath), { recursive: true })
const tempPath = `${filePath}.${process.pid}.${Date.now()}.tmp`
try {
await writeFile(tempPath, content, "utf8")
apps/web/src/routes/api/chat/providers.ts:242
- When using the local provider store,
createLocalProviderInstancewill throw if.fleet/providers.*.jsonis malformed/future-version. That currently bubbles up as a 500 for POST, even though unreadable stores are treated as recoverable elsewhere, and it prevents users from repairing the store via Settings.
This issue also appears on line 483 of the same file.
if (!isExistingInstance) {
instanceId = useLocalStore
? await createLocalProviderInstance(
userId,
instanceBody,
apps/web/src/lib/db/local-provider-instances.ts:37
- The PR description mentions a single
<projectRoot>/.fleet/providers.json, but the implementation uses scoped filenames (.fleet/providers.anonymous.jsonand.fleet/providers.user-<hash>.json). Please update the PR description (or any other external docs) to match the actual on-disk paths to avoid confusion for local users.
/**
* Resolves the gitignored local provider store path for a project root. One
* file per account: anonymous local chat shares `providers.anonymous.json`,
* signed-in local accounts get `providers.user-<hash>.json`, so an anonymous
* user and a local Better Auth account on the same machine never read each
* other's instances (or plaintext API keys). `.fleet/` is already gitignored
* (see root `.gitignore`).
Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/lib/db/occ-instances.ts (1)
295-303: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftClose the allocate-then-upsert window on the Postgres create path.
allocateInstanceIdreadspi_user_providersin one transaction, then the caller allocates a second time and runsupsertOccInstanceas anON CONFLICT DO UPDATE. Concurrent new-provider creates from the same user can both pick the same id and cause one write to overwrite the other. For new providers, use a create-only insert, retry on conflict for the next generated id, and fail after exhausting generated options.🤖 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 `@apps/web/src/lib/db/occ-instances.ts` around lines 295 - 303, Update the new-provider create flow around allocateInstanceId and upsertOccInstance to eliminate separate allocation and write steps. Insert the provider with create-only semantics, retrying with the next generated ID when a uniqueness conflict occurs so concurrent creates cannot overwrite one another. Preserve update behavior for existing providers, and return an explicit failure after all generated ID options are exhausted.
🧹 Nitpick comments (5)
apps/web/src/lib/db/__tests__/local-provider-instances.test.ts (1)
138-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for an instance with no model ids.
This test covers the legacy
modelIdtomodelIdsnormalization. It does not cover the input where bothmodelIdsandmodelIdare absent. That input producesmodelId: undefinedand makes every later read of the store throw. See the comment onapps/web/src/lib/db/local-provider-instances.tsLines 189-217.Add a test that calls
upsertLocalProviderInstancewith no models and asserts the store rejects it, then asserts a followinglistLocalProviderInstancesstill succeeds.🤖 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 `@apps/web/src/lib/db/__tests__/local-provider-instances.test.ts` around lines 138 - 155, Extend the local provider instance tests near the legacy normalization case with an instance whose input omits both modelIds and modelId. Assert upsertLocalProviderInstance rejects the invalid instance, then verify listLocalProviderInstances still completes successfully afterward without throwing.apps/web/src/lib/db/local-provider-instances.ts (1)
284-307: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that the allocated id is stale once the lock is released.
allocateLocalInstanceIdreturns the id afterwithStoreLockresolves. The caller then writes in a separate lock acquisition. Two concurrent creates that use this helper can therefore receive the same id, and the laterupsertLocalProviderInstanceoverwrites the earlier instance in place at Line 211.
createLocalProviderInstanceexists to close that window, and its doc says so. This helper's doc does not state the caveat. Add the caveat, and direct new callers tocreateLocalProviderInstance.♻️ Proposed doc change
* Allocates an available custom provider id for a display name from the local * dev store, appending `-2`, `-3`, … or a timestamp suffix on collisions. * + * The id is only guaranteed unique at the moment of allocation. The store lock + * is released before the caller writes, so concurrent creates can receive the + * same id. Use {`@link` createLocalProviderInstance} to allocate and write under + * one lock. + * * `@param` userId - The account, or `undefined` for anonymous local chat🤖 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 `@apps/web/src/lib/db/local-provider-instances.ts` around lines 284 - 307, Update the JSDoc for allocateLocalInstanceId to state that its returned id may become stale after withStoreLock releases because callers write under a separate lock, allowing concurrent creates to collide. Direct new callers that need allocation and persistence to be atomic to createLocalProviderInstance, while leaving the implementation unchanged.packages/pi-protocol/src/provider-catalog.ts (1)
6-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnforce the exhaustiveness the doc comment promises.
The comment states that adding a family updates every consumer in one place. The annotation
ReadonlyArray<PiCustomProviderApi>does not enforce that. A subset also satisfies the type, so adding a member toPiCustomProviderApicompiles without adding it here. The runtime guardisPiCustomProviderApithen silently rejects the new family, andlocal-provider-instances.tsdrops stored rows that use it as malformed.Derive the array from a keyed record so a missing member fails type checking.
♻️ Proposed change to make the list exhaustive
-export const PI_CUSTOM_PROVIDER_APIS: ReadonlyArray<PiCustomProviderApi> = [ - "openai-completions", - "openai-responses", - "anthropic-messages", - "google-genai", -] +const PI_CUSTOM_PROVIDER_API_SET: Record<PiCustomProviderApi, true> = { + "openai-completions": true, + "openai-responses": true, + "anthropic-messages": true, + "google-genai": true, +} + +export const PI_CUSTOM_PROVIDER_APIS: ReadonlyArray<PiCustomProviderApi> = + Object.keys(PI_CUSTOM_PROVIDER_API_SET) as Array<PiCustomProviderApi>🤖 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 `@packages/pi-protocol/src/provider-catalog.ts` around lines 6 - 16, Update PI_CUSTOM_PROVIDER_APIS to use a keyed record or equivalent exhaustive mapped type over PiCustomProviderApi, then derive the exported array from that record. Ensure every union member requires an entry so adding a new API family fails type checking until it is registered here, while preserving the existing array consumers and runtime validation behavior.apps/web/src/routes/api/chat/__tests__/providers-custom-route.test.ts (2)
249-276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the local update branch.
The suite covers local create, Postgres create, and local delete. It does not cover an update against an existing
custom+<slug>id with the local store selected. That path is the only caller ofupsertLocalProviderInstancein the route, and it must not callcreateLocalProviderInstance. Add a test that posts an existing${CUSTOM_PROVIDER_ID_PREFIX}my-endpointid and assertsupsertLocalProviderInstanceMockreceives the id whilecreateLocalProviderInstanceMockis not called.🤖 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 `@apps/web/src/routes/api/chat/__tests__/providers-custom-route.test.ts` around lines 249 - 276, The custom provider route tests need coverage for the local update path. Add a test alongside the existing local create/Postgres cases that selects the local store, posts an existing `${CUSTOM_PROVIDER_ID_PREFIX}my-endpoint` provider id, asserts `upsertLocalProviderInstanceMock` receives that id, and verifies `createLocalProviderInstanceMock` is not called.
205-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the deployed-surface state for the loopback tests.
These two tests depend on
isDeployedChatRuntimeSurface()returningfalse. That function readsprocess.env.VERCELandgetChatAuthSurface(). Neither is stubbed here. If the test process runs withVERCEL=1or with a Neon Function auth surface configured, the loopback acceptance test fails. Mock@/lib/pi/runtime/deployed-chat-runtimeto make the policy branch explicit.♻️ Proposed mock for the deployed-surface module
vi.mock("`@/lib/deployment/environment`", () => ({ isVercelDeployment: () => false, })) + +vi.mock("`@/lib/pi/runtime/deployed-chat-runtime`", () => ({ + isDeployedChatRuntimeSurface: () => false, +}))🤖 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 `@apps/web/src/routes/api/chat/__tests__/providers-custom-route.test.ts` around lines 205 - 247, Mock the "`@/lib/pi/runtime/deployed-chat-runtime`" module in the providers-custom-route tests so isDeployedChatRuntimeSurface() consistently returns false. Apply this explicit local-development runtime state to both loopback URL tests while preserving their existing assertions.
🤖 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 `@apps/web/src/lib/db/local-provider-instances.ts`:
- Around line 189-217: In upsertLocalProviderInstance, stop writing a row when
normalizeCustomProviderInstance returns an empty modelIds list: read
normalized.modelIds[0] into a local, validate that it exists before building the
LocalProviderInstance, and only assign the checked value to modelId. Apply the
same guard in createLocalProviderInstance so both write paths reject invalid
input consistently, and add the test in local-provider-instances.test.ts to
cover an upsert with neither modelIds nor modelId and confirm the store remains
readable afterward.
- Around line 332-347: Update isLocalProviderInstance so an omitted modelIds
field is accepted and normalized to an empty list, matching the optional
modelIds contract in LocalProviderInstance; continue validating every provided
modelIds value as an array of strings.
- Around line 58-72: Update useLocalProviderStore to disable the local file
store on both Vercel and the Neon Function chat surface, using
getChatAuthSurface alongside isVercelDeployment. Ensure Neon Function users
never fall back to the ephemeral .fleet store, regardless of chat database
configuration.
In
`@apps/web/src/lib/pi/runtime/__tests__/custom-provider-local-registration.test.ts`:
- Around line 48-52: Update the afterEach cleanup to restore
NEON_AI_GATEWAY_TOKEN and NEON_AI_GATEWAY_BASE_URL accurately: assign each
original value when defined, but delete the corresponding process.env property
when its original value is undefined, preventing test state from leaking.
In `@apps/web/src/lib/pi/runtime/custom-provider-registry.ts`:
- Around line 180-182: Update registerCustomProviders around
listLocalProviderInstances and loadLocalProviderInstanceApiKey to catch API-key
store read failures, return an empty registration set with the existing
storeError diagnostic, and allow session creation to continue; add a regression
test in
apps/web/src/lib/pi/runtime/__tests__/custom-provider-local-registration.test.ts
covering successful discovery followed by a rejected
loadLocalProviderInstanceApiKey and asserting the diagnostic result.
---
Outside diff comments:
In `@apps/web/src/lib/db/occ-instances.ts`:
- Around line 295-303: Update the new-provider create flow around
allocateInstanceId and upsertOccInstance to eliminate separate allocation and
write steps. Insert the provider with create-only semantics, retrying with the
next generated ID when a uniqueness conflict occurs so concurrent creates cannot
overwrite one another. Preserve update behavior for existing providers, and
return an explicit failure after all generated ID options are exhausted.
---
Nitpick comments:
In `@apps/web/src/lib/db/__tests__/local-provider-instances.test.ts`:
- Around line 138-155: Extend the local provider instance tests near the legacy
normalization case with an instance whose input omits both modelIds and modelId.
Assert upsertLocalProviderInstance rejects the invalid instance, then verify
listLocalProviderInstances still completes successfully afterward without
throwing.
In `@apps/web/src/lib/db/local-provider-instances.ts`:
- Around line 284-307: Update the JSDoc for allocateLocalInstanceId to state
that its returned id may become stale after withStoreLock releases because
callers write under a separate lock, allowing concurrent creates to collide.
Direct new callers that need allocation and persistence to be atomic to
createLocalProviderInstance, while leaving the implementation unchanged.
In `@apps/web/src/routes/api/chat/__tests__/providers-custom-route.test.ts`:
- Around line 249-276: The custom provider route tests need coverage for the
local update path. Add a test alongside the existing local create/Postgres cases
that selects the local store, posts an existing
`${CUSTOM_PROVIDER_ID_PREFIX}my-endpoint` provider id, asserts
`upsertLocalProviderInstanceMock` receives that id, and verifies
`createLocalProviderInstanceMock` is not called.
- Around line 205-247: Mock the "`@/lib/pi/runtime/deployed-chat-runtime`" module
in the providers-custom-route tests so isDeployedChatRuntimeSurface()
consistently returns false. Apply this explicit local-development runtime state
to both loopback URL tests while preserving their existing assertions.
In `@packages/pi-protocol/src/provider-catalog.ts`:
- Around line 6-16: Update PI_CUSTOM_PROVIDER_APIS to use a keyed record or
equivalent exhaustive mapped type over PiCustomProviderApi, then derive the
exported array from that record. Ensure every union member requires an entry so
adding a new API family fails type checking until it is registered here, while
preserving the existing array consumers and runtime validation behavior.
🪄 Autofix
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 Plus
Run ID: 5f89f999-f18e-44bd-8da5-1d3a603163c8
📒 Files selected for processing (20)
.pi/settings.jsonAGENTS.mdapps/web/openapi.jsonapps/web/src/lib/db/__tests__/local-provider-instances.test.tsapps/web/src/lib/db/chat-db-config.tsapps/web/src/lib/db/local-provider-instances.tsapps/web/src/lib/db/occ-instances.tsapps/web/src/lib/db/user-providers.tsapps/web/src/lib/db/user-settings.tsapps/web/src/lib/env-manager.tsapps/web/src/lib/fs-atomic.tsapps/web/src/lib/pi/runtime/__tests__/custom-provider-local-registration.test.tsapps/web/src/lib/pi/runtime/__tests__/openai-chat-completions-registration.test.tsapps/web/src/lib/pi/runtime/__tests__/session-factory.test.tsapps/web/src/lib/pi/runtime/custom-provider-registry.tsapps/web/src/lib/pi/runtime/openai-chat-completions-url.tsapps/web/src/lib/pi/server-shared.test.tsapps/web/src/routes/api/chat/__tests__/providers-custom-route.test.tsapps/web/src/routes/api/chat/providers.tspackages/pi-protocol/src/provider-catalog.ts
💤 Files with no reviewable changes (1)
- .pi/settings.json
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (3)
apps/web/src/lib/db/local-provider-instances.ts:258
- When creating a new local provider instance,
modelIdis set fromnormalized.modelIds[0]without guarding against an empty model list. If an invalid call site passes no models, the store can be written in a shape that will fail validation on the next read.
baseUrl: input.baseUrl,
modelId: normalized.modelIds[0],
api: normalized.api,
modelIds: normalized.modelIds,
apiKey,
apps/web/src/lib/db/local-provider-instances.ts:206
modelIdis written asnormalized.modelIds[0]without verifying the normalized model list is non-empty. If a caller ever upserts an instance with nomodelIds/modelId, JSON serialization will dropmodelId, making the store unreadable on the next load and effectively disabling local custom providers.
This issue also appears on line 254 of the same file.
baseUrl: instance.baseUrl,
modelId: normalized.modelIds[0],
api: normalized.api,
modelIds: normalized.modelIds,
apiKey,
apps/web/src/lib/pi/runtime/custom-provider-registry.ts:162
instance.apiis trusted from persisted data. If the DB/local store ever contains an unexpected string (corruption, manual edit, older bug), it will be passed through as the provider API and can produce invalid Pi registrations. Consider validating the value against the supported API families and defaulting (or skipping) when it is not recognized.
for (const instance of instances) {
const api: PiCustomProviderApi = instance.api ?? "openai-completions"
// Legacy single-model rows carry `modelId` only; normalized rows carry
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (3)
apps/web/src/routes/api/chat/providers.ts:487
- DELETE for local custom provider instances will 500 if the local provider store is malformed/unreadable (e.g.
getLocalProviderInstancethrows). That makes it hard to remove broken instances via the Settings UI. Consider catching local-store errors here and returning a 400 with an actionable message, while still letting DB errors propagate on the Postgres path.
if (useLocalProviderStore(userId)) {
const existing = await getLocalProviderInstance(
userId,
body.providerId
)
apps/web/src/routes/api/chat/providers.ts:236
- Local custom provider mutations can still 500 if the local provider store is malformed/unreadable.
createLocalProviderInstance/upsertLocalProviderInstancewill throw, and this route-level handler catches it as a generic 500, which prevents users from recovering via the Settings UI (contradicts the intent inlistLocalProviderInstancesForRoute). Consider translating local-store read/write errors into a 400 with an actionable message (or otherwise handling them distinctly from DB errors).
This issue also appears on line 483 of the same file.
instanceId = useLocalStore
? await createLocalProviderInstance(
userId,
instanceBody,
isNamedOccTarget ? toOccInstanceId : toCustomProviderId,
apps/web/src/lib/db/local-provider-instances.ts:35
- PR description mentions a single
<projectRoot>/.fleet/providers.json, but the implementation (and this docstring) uses per-scope files:.fleet/providers.anonymous.jsonand.fleet/providers.user-<hash>.json. Consider updating the PR description to match the actual on-disk paths to avoid confusion when validating the feature.
/**
* Resolves the gitignored local provider store path for a project root. One
* file per account: anonymous local chat shares `providers.anonymous.json`,
* signed-in local accounts get `providers.user-<hash>.json`, so an anonymous
* user and a local Better Auth account on the same machine never read each
- Pin the deployed surface in loopback provider route tests and cover the local update branch (upsert, no new id allocation) - Document that allocateLocalInstanceId's id can go stale after its lock releases; point new callers at createLocalProviderInstance - Derive PI_CUSTOM_PROVIDER_APIS from a keyed record so adding an API family fails type checking until registered here - Use a random temp suffix in writeFileAtomic so concurrent writes in the same millisecond cannot collide on the same temp path Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/pi-protocol/src/provider-catalog.ts:273
- New shared helpers in this file (e.g. allocateProviderId, normalizeCustomProviderInstance, isPiCustomProviderApi / isOccFamilyApi) aren’t covered by the existing pi-protocol unit tests (provider-catalog.test.ts currently only covers slug/id helpers). Adding focused tests here would reduce the risk of silent regressions in both the Postgres and local-file provider stores.
export function allocateProviderId(
baseSlug: string,
existingIds: ReadonlySet<string>,
toId: (slug: string) => string
): string {
apps/web/src/routes/api/chat/providers.ts:102
- The comment says the unreadable local store degrades to "no instances" with a diagnostic, but this code path only logs a console warning and returns an empty list (no user-visible diagnostic from this route). This mismatch can mislead future maintainers when debugging missing instances in Settings.
/**
* A malformed/unreadable local store must not 500 the Settings providers list
* (the UI is how users fix or remove broken instances); degrade to "no
* instances" with a diagnostic, matching the registration path. DB errors on
* the Postgres branch still propagate.
*/
Summary
Enables custom provider instances (
custom+<slug>andopenai-chat-completions+<slug>) for local anonymous / non-DB chat instead of returning "Custom provider instances require a database-backed account (deployed chat)."pi_user_providerswhen DB-backed (userId+FLEET_PI_CHAT_DATABASE_URL), otherwise a gitignored per-user file store at<projectRoot>/.fleet/providers.json(plaintext, atomic temp-file+rename).[]+ a diagnostic instead of bricking chat session creation.http://localhostbase URLs are allowed for OCC-family instances on local dev surfaces only (!isDeployedChatRuntimeSurface()); https stays enforced everywhere else.normalizeCustomProviderInstance,assertCustomProviderBaseUrl,allocateProviderIdin@workspace/pi-protocol).GET /api/chat/providersand Pi runtime registration now use symmetric keyed-list APIs; tests mock the local store for hygiene.Test plan
pnpm --filter web typecheck— cleanpnpm --filter pi-protocol lint/test— clean / 19 passedpnpm --filter web lint— 0 errorsresource-expectations.spec.tsfailures from unrelated.pi/settings.jsondrift).fleet/providers.jsonfor anonymous local chatMade with Cursor