refactor: ai and tests#137
Merged
Merged
Conversation
…hase 1 - Replace createOpenAI+v1 with createOllama (ai-sdk-ollama) for Ollama - Extract provider.ts: getResolvedProvider, getProvider, default models - Add POST /ai/generate: single prompt, plain text SSE, CLI-friendly - Phase 1: chat uses provider, plain chat only (tools disabled for Phase 3) - OLLAMA_BASE_URL default https://ollama.gaboesquivel.com - Contract tests (schema, auth) run always; remote gated by OLLAMA_REMOTE_TESTS - Remove throttleAiRequests from chat tests - Regenerate OpenAPI and @repo/core client - Update self-hosted-llm docs with route split
…EMOTE_TESTS - Delete db.spec.ts, db-integration.spec.ts, reference.test.ts, reference.spec.ts - Remove it.skip from revoke.test.ts (API key auth test) - Extract AI skip helpers to test/utils/ai-remote.ts - Run AI remote tests by default (remove OLLAMA_REMOTE_TESTS flag) - Remove DEBUG_TEST, OLLAMA_REMOTE_TESTS from turbo.json, .env.test.example - Use logger: silent always in fastify test util - Consolidate auth-helpers extractToken/extractMagicLinkData (next e2e)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors AI provider resolution to prefer Open Router (with Ollama as a configurable self-host option), adds a new /ai/generate endpoint, centralizes provider/model logic and tools, introduces upstream timeout and abort handling, expands test helpers and many Fastify tests, and updates OpenAPI/docs and test config. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Fastify as Fastify Route
participant Resolver as Provider Resolver
participant Model as Language Model
participant Upstream as Upstream AI Service
Client->>Fastify: POST /ai/chat or /ai/generate (prompt, model?, stream?)
Fastify->>Resolver: getResolvedProvider()
Resolver-->>Fastify: 'openrouter' / 'ollama' / null
Fastify->>Resolver: getProvider(provider, model?)
Resolver-->>Model: configured LanguageModel
Fastify->>Fastify: getMergedTools(userId)
Fastify->>Upstream: generateText()/streamText() (options + abortSignal)
Upstream-->>Fastify: chunks / full text or error
alt stream
Fastify->>Client: 200, text/event-stream chunks
else success
Fastify->>Client: 200, { text }
else upstream error
Fastify->>Client: 502 UPSTREAM_SERVICE_ERROR
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (6)
apps/fastify/src/routes/account/email/change/request.test.ts (1)
25-36: Consider verifying the error code for consistency.Good security test for blocking
javascript:URL injection. For consistency with the 401 test (which checks.code), consider also verifying the validation error code in the response body.💡 Optional enhancement
}) expect(res.statusCode).toBe(400) + expect(JSON.parse(res.body).code).toBe('VALIDATION_ERROR') // or appropriate code })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/account/email/change/request.test.ts` around lines 25 - 36, The test currently only asserts statusCode 400; extend it to also assert the response body error code for consistency with the 401 test that checks `.code`. After obtaining `res` from `fastify.inject` in the `should return 400 for invalid callback URL` test, parse the body with `res.json()` and assert that `.code` equals the same validation error code used elsewhere (use the identical string/constant used in the 401 test), e.g., expect(res.json().code).toBe(<validation_error_code>), so the test verifies both HTTP status and the error code in the POST /account/email/change/request response.apps/fastify/src/routes/account/link/oauth/unlink.test.ts (2)
12-31: Consider extractinguserIdto describe scope to avoid duplication.The pattern of calling PATCH
/account/profilewith an empty payload to retrieveuserIdis repeated in bothbeforeAlland the success test case (lines 59-65). StoringuserIdat the describe scope level would reduce duplication and improve readability.♻️ Proposed refactor
describe('DELETE /account/link/oauth/:providerId', () => { let jwt: string + let userId: string beforeAll(async () => { jwt = await getOrCreateSession(fastify, 'phase2-oauth@test.ai') const profileRes = await fastify.inject({ method: 'PATCH', url: '/account/profile', headers: { Authorization: `Bearer ${jwt}` }, payload: {}, }) - const userId = (JSON.parse(profileRes.body) as { user: { id: string } }).user?.id + userId = (JSON.parse(profileRes.body) as { user: { id: string } }).user?.id if (!userId) throw new Error('No user id') const db = await getDb() await db.insert(account).values({ id: randomUUID(), userId, accountId: `oauth-test-${randomUUID()}`, providerId: 'github', accessToken: null, refreshToken: null, }) })Then in the success test, remove lines 59-66 and use
userIddirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/account/link/oauth/unlink.test.ts` around lines 12 - 31, Extract the userId into the outer describe scope so both beforeAll and the success test reuse it: declare a let userId variable above beforeAll, assign it inside beforeAll by parsing the response from the PATCH /account/profile call (the same logic currently in beforeAll), and remove the duplicated PATCH+parse block from the success test; ensure code that inserts into the DB (the db.insert(account).values call) and any assertions reference this shared userId variable and keep existing uses of jwt/getOrCreateSession and fastify.inject unchanged.
68-69: Consider filtering byproviderIdfor clarity and robustness.The test setup creates exactly one account entry with
providerId='github', so the assertion will pass as written. However, the query retrieves all accounts without filtering, which makes the test less explicit about its intent. Filtering byproviderIdwould make it clearer that only the GitHub OAuth link is being verified as removed, and would prevent the assertion from becoming fragile if future auth changes add additional account entries.🛡️ Suggested improvement
- const remaining = await db.select().from(account).where(eq(account.userId, userId)) - expect(remaining).toHaveLength(0) + const remaining = await db + .select() + .from(account) + .where(eq(account.userId, userId)) + const githubAccounts = remaining.filter((a) => a.providerId === 'github') + expect(githubAccounts).toHaveLength(0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/account/link/oauth/unlink.test.ts` around lines 68 - 69, The test currently selects all accounts for the user using db.select().from(account).where(eq(account.userId, userId)); update this query to also filter by providerId (e.g., eq(account.providerId, 'github')) so it explicitly checks that the GitHub OAuth link was removed; keep the existing variable names (remaining) and the final assertion expect(remaining).toHaveLength(0). Ensure you import/use the same eq helper and the account table fields already referenced in the test.apps/fastify/src/plugins/error-handler.ts (1)
114-118: Minor: Unnecessary intermediate variable and blank line.The extraction of
messageto a local variable doesn't add value here since it's used only once and unchanged. The blank line (115) also adds unnecessary whitespace.💡 Simpler: Use property directly
- const message = catalogError.message - reply.status(statusCode).send({ code: catalogError.code, - message, + message: catalogError.message, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/plugins/error-handler.ts` around lines 114 - 118, Remove the unnecessary intermediate variable and blank line: don't assign catalogError.message to a separate const; instead use catalogError.message directly in the payload passed to reply.status(statusCode).send({ code: catalogError.code, message: catalogError.message }). Update the block in the error handler where catalogError and reply.status(...).send(...) are used to eliminate the extra variable and whitespace.apps/fastify/test/utils/fastify.ts (1)
15-17: Consider preserving debug toggle for test troubleshooting.Hardcoding
'silent'removes the ability to enable logs when debugging failing tests. A simple environment check would preserve this flexibility:💡 Optional: Restore debug toggle
const fastify = Fastify({ - logger: { level: 'silent' }, + logger: { level: process.env.DEBUG_TEST ? 'info' : 'silent' }, }).withTypeProvider<TypeBoxTypeProvider>()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/test/utils/fastify.ts` around lines 15 - 17, The fastify instance creation hardcodes logger: { level: 'silent' } which prevents enabling logs when debugging tests; change the Fastify(...) call that creates the fastify variable (the Fastify(...) with .withTypeProvider<TypeBoxTypeProvider> chain) to read the log level from an environment toggle (e.g., process.env.TEST_LOG_LEVEL or a DEBUG boolean) and pass that value into the logger option so tests can enable verbose logging when needed.apps/fastify/test/utils/auth-helper.ts (1)
121-131: Use a read-only endpoint to resolveuserId.This helper sends an empty
PATCH /account/profilepurely to obtainuser.id. That couples the setup utility to profile-update semantics and can start failing as soon as that route rejects empty updates or adds side effects./auth/session/useris the safer source here.Suggested refactor
- const profileRes = await app.inject({ - method: 'PATCH', - url: '/account/profile', - headers: { Authorization: `Bearer ${jwt}` }, - payload: {}, - }) - if (profileRes.statusCode !== 200) - throw new Error(`Profile failed: ${profileRes.statusCode} ${profileRes.body}`) - const body = JSON.parse(profileRes.body) as { user?: { id: string } } + const sessionRes = await app.inject({ + method: 'GET', + url: '/auth/session/user', + headers: { Authorization: `Bearer ${jwt}` }, + }) + if (sessionRes.statusCode !== 200) + throw new Error(`session/user failed: ${sessionRes.statusCode} ${sessionRes.body}`) + const body = JSON.parse(sessionRes.body) as { user?: { id: string } } const userId = body.user?.id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/test/utils/auth-helper.ts` around lines 121 - 131, Replace the PATCH /account/profile call used to fetch the user id with a read-only GET to /auth/session/user: change the app.inject call that currently sets method 'PATCH' and url '/account/profile' (producing profileRes) to a GET against '/auth/session/user' (still passing Authorization: `Bearer ${jwt}`), assert a 200 response, and parse the returned JSON to extract the user id into the existing userId variable (adjust the expected shape if the endpoint returns { id } vs { user: { id } }). This keeps auth-helper.ts's logic read-only and avoids side effects from profile updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/docu/content/docs/deployment/self-hosted-llm.mdx`:
- Line 240: The docs incorrectly state that setting OLLAMA_BASE_URL alone opts
into Ollama; update the text to clarify that when both Open Router and Ollama
are configured, you must explicitly set AI_PROVIDER=ollama (because
getResolvedProvider() will prefer Open Router if OPEN_ROUTER_API_KEY is
present). Edit the sentences at the referenced spots to state that privacy-first
routing requires both OLLAMA_BASE_URL and AI_PROVIDER=ollama when
OPEN_ROUTER_API_KEY exists, and add a short note referencing
getResolvedProvider() behavior.
In `@apps/fastify/src/lib/username.ts`:
- Around line 48-56: The final fallback return builds an unchecked username
(`${candidate.slice(0, 41)}_${randomBytes(3).toString('hex')}`) and can produce
duplicates; instead, generate that fallback into a variable and call
isUsernameTaken(client, fallback) and only return it if the check passes—if it’s
taken, loop/regenerate (e.g., repeat randomBytes and re-check until
isUsernameTaken returns false) so generateFunnyUsername() only returns a
username verified free; reference randomBytes, isUsernameTaken, randomSuffix and
candidate when making the change.
In `@apps/fastify/src/routes/account/account.spec.ts`:
- Line 2: The test suite currently only clears the fake inbox in a beforeEach
hook, which allows the previous suite's last email to persist until the next
beforeEach; add an afterEach hook that performs the same cleanup so each
test/suite leaves no mail behind. Locate the existing beforeEach that clears
fastify.fakeEmail (or calls the same cleanup helper) and add a corresponding
afterEach calling that same cleanup for fastify.fakeEmail so inboxes are cleared
both before and after tests.
In `@apps/fastify/src/routes/account/link/totp/verify.test.ts`:
- Around line 8-15: This test suite must use a dedicated TOTP test user instead
of sharing 'phase2-totp@test.ai'; update the beforeAll setup where
getOrCreateSession is called (the jwt variable assignment) to use a unique email
like 'phase2-totp-verify@test.ai' and keep the rest of the setup (the POST to
'/account/link/totp/setup' via fastify.inject) unchanged so this suite no longer
depends on the state left by setup.test.ts.
In `@apps/fastify/src/routes/ai/chat.ts`:
- Around line 223-245: The route's streaming branch (guarded by shouldStream and
returning result.toUIMessageStreamResponse() from streamText with smoothStream)
is declared as Type.String() in the 200 schema but actually emits SSE frames
carrying structured JSON messages (objects with a "type" field like "start",
"text-delta", "text-end", etc.); update the route's 200 response schema to
accurately model the SSE payload (e.g., a stream of JSON event objects or a
union/array of Type.Object shapes with a required "type" enum and corresponding
fields) and ensure the response content type matches text/event-stream so
generated OpenAPI/clients reflect the structured SSE format returned by
streamText/toUIMessageStreamResponse.
- Around line 223-245: The UI-message stream response is returned without a
consumeSseStream handler, so upstream provider calls may not be drained on
client disconnect; update the call to result.toUIMessageStreamResponse to pass
consumeSseStream: consumeStream (keeping baseOptions and experimental_transform
as-is) so the SDK can drain the SSE stream and honor the existing abortSignal;
locate the streamText invocation and the subsequent
result.toUIMessageStreamResponse call and add the consumeSseStream option there.
In `@apps/fastify/src/routes/ai/generate.ts`:
- Around line 29-30: The handler currently treats Accept: text/event-stream as
SSE but calls toTextStreamResponse(), which emits plain UTF-8 text chunks;
update the streaming branch in generate.ts so that when Accept:
text/event-stream is negotiated it returns proper SSE frames by using
toUIMessageStreamResponse() (same approach as chat.ts) and ensure the OpenAPI
summary/description and tests expect SSE, or if you intend plain text streaming
instead, change the negotiation to Accept: text/plain and update the OpenAPI
schema and tests accordingly; specifically, replace the call to
toTextStreamResponse() with toUIMessageStreamResponse() (or change the
accept-condition and OpenAPI entries) so the content-type and framing match the
declared SSE behavior.
In `@apps/fastify/src/routes/ai/provider.ts`:
- Around line 38-45: The code currently maps explicit model IDs (e.g.,
'qwen2.5:3b', 'qwen3:8b', 'meta-llama/llama-3.3-70b-instruct:free') to the
runtime default by including them in the useDefault condition; update the logic
so useDefault only triggers when the request has no model (m is undefined/null),
when m is the explicit 'default' token, or when it equals defaultOllamaModel if
intended, and do not treat concrete model strings as aliases — remove those
literal model checks from the useDefault expression that sets modelId (variable
names: m, useDefault, defaultOllamaModel, defaultModel, modelId), and make the
same change in the other analogous block referenced (the block around lines
53-59).
In `@apps/fastify/test/utils/ai-remote.ts`:
- Around line 1-15: The current heuristics in isInsufficientCredits and
skipIfInsufficientCredits are too broad and can mask real failures; tighten them
by only treating a response as "insufficient credits" when there is a specific
provider-indicative status or message pattern (e.g., exact 402 status combined
with provider error key/text like "insufficient_quota" or "insufficient_credits"
or an OpenRouter-specific error field), avoid matching generic words like
"credits", "network", or provider names alone, and do not skip on generic 5xx or
non-SSE responses; update isInsufficientCredits to check both statusCode === 402
and a strict regex or JSON-parsed error.code/message, and have
skipIfInsufficientCredits call that stricter predicate (functions:
isInsufficientCredits, skipIfInsufficientCredits).
In `@apps/fastify/test/utils/auth-helper.ts`:
- Around line 7-24: getOrCreateSession currently returns a cached JWT before
honoring options.clearBefore; update the function so that if
options?.clearBefore is true it first removes any existing entry for the email
(or skips reading the cache) before attempting to return the cached token, then
proceeds to call getSessionToken and stores the new token in sessionPool;
reference the getOrCreateSession function and the sessionPool Map when making
this change.
In `@apps/fastify/test/utils/fake-email.ts`:
- Around line 117-128: extractTokenFromUrls currently only parses HTML hrefs, so
tokens present only in the email's text body are missed and tests that relied on
the previous extractMagicLink fallback break; update extractTokenFromUrls (or
call the original extractMagicLink) to also inspect the plain-text email body
(e.g., targetEmail.text) for token patterns (same regex used for HTML or the
previous /(\d{6})/ and token= patterns) and return that match if no href token
is found, ensuring extractTokenFromUrls returns tokens from either hrefs or the
text body.
In `@apps/next/e2e/auth-helpers.ts`:
- Around line 91-92: The problem is that extractMagicLinkData() currently
returns null until both token and verificationId exist, which prevents
extractToken() (and loginAsTestUser()) from working when only token is
available; change extractMagicLinkData() so it returns the token as soon as it's
present (i.e., do not gate returning { token } on presence of verificationId),
or alternatively adjust extractToken() to independently parse and return the
token from the page response even if extractMagicLinkData() omits
verificationId; update extractMagicLinkData() to still include verificationId
when available but allow token-only results so extractToken() can return
data?.token without waiting for verificationId.
In `@apps/next/e2e/chat-assistant.spec.ts`:
- Around line 37-38: The failing 60s assertions in chat-assistant.spec.ts are
aborted by Playwright's 30s default because the 'chromium' project in the
Playwright config has no timeout override; fix by either adding timeout: 60_000
to the 'chromium' project definition in the Playwright config (the project named
"chromium") or by calling test.setTimeout(60_000) at the top of the
chat-assistant.spec.ts test file (before the awaits) so the long waits for
sheet.locator('[data-role="assistant"]') and sheet.getByTestId('user-info-card')
can complete.
- Around line 23-38: Replace the single immediate sample of chat-error with a
race that waits for either the assistant reply or the error element to appear
within the shared timeout: use the existing locators chatError
(sheet.getByTestId('chat-error')) and assistant locator
(sheet.locator('[data-role="assistant"]')) and await whichever becomes visible
first (e.g., Promise.race of their visible/waitFor calls with the same timeout),
then if the chatError won the race read its text and apply the existing skip
logic (checking /402|insufficient|credits/i and /fetch
failed|ENOTFOUND|unreachable|connection|ECONNREFUSED|ETIMEDOUT/i); otherwise
continue to the current expect checks for assistant and user-info-card.
---
Nitpick comments:
In `@apps/fastify/src/plugins/error-handler.ts`:
- Around line 114-118: Remove the unnecessary intermediate variable and blank
line: don't assign catalogError.message to a separate const; instead use
catalogError.message directly in the payload passed to
reply.status(statusCode).send({ code: catalogError.code, message:
catalogError.message }). Update the block in the error handler where
catalogError and reply.status(...).send(...) are used to eliminate the extra
variable and whitespace.
In `@apps/fastify/src/routes/account/email/change/request.test.ts`:
- Around line 25-36: The test currently only asserts statusCode 400; extend it
to also assert the response body error code for consistency with the 401 test
that checks `.code`. After obtaining `res` from `fastify.inject` in the `should
return 400 for invalid callback URL` test, parse the body with `res.json()` and
assert that `.code` equals the same validation error code used elsewhere (use
the identical string/constant used in the 401 test), e.g.,
expect(res.json().code).toBe(<validation_error_code>), so the test verifies both
HTTP status and the error code in the POST /account/email/change/request
response.
In `@apps/fastify/src/routes/account/link/oauth/unlink.test.ts`:
- Around line 12-31: Extract the userId into the outer describe scope so both
beforeAll and the success test reuse it: declare a let userId variable above
beforeAll, assign it inside beforeAll by parsing the response from the PATCH
/account/profile call (the same logic currently in beforeAll), and remove the
duplicated PATCH+parse block from the success test; ensure code that inserts
into the DB (the db.insert(account).values call) and any assertions reference
this shared userId variable and keep existing uses of jwt/getOrCreateSession and
fastify.inject unchanged.
- Around line 68-69: The test currently selects all accounts for the user using
db.select().from(account).where(eq(account.userId, userId)); update this query
to also filter by providerId (e.g., eq(account.providerId, 'github')) so it
explicitly checks that the GitHub OAuth link was removed; keep the existing
variable names (remaining) and the final assertion
expect(remaining).toHaveLength(0). Ensure you import/use the same eq helper and
the account table fields already referenced in the test.
In `@apps/fastify/test/utils/auth-helper.ts`:
- Around line 121-131: Replace the PATCH /account/profile call used to fetch the
user id with a read-only GET to /auth/session/user: change the app.inject call
that currently sets method 'PATCH' and url '/account/profile' (producing
profileRes) to a GET against '/auth/session/user' (still passing Authorization:
`Bearer ${jwt}`), assert a 200 response, and parse the returned JSON to extract
the user id into the existing userId variable (adjust the expected shape if the
endpoint returns { id } vs { user: { id } }). This keeps auth-helper.ts's logic
read-only and avoids side effects from profile updates.
In `@apps/fastify/test/utils/fastify.ts`:
- Around line 15-17: The fastify instance creation hardcodes logger: { level:
'silent' } which prevents enabling logs when debugging tests; change the
Fastify(...) call that creates the fastify variable (the Fastify(...) with
.withTypeProvider<TypeBoxTypeProvider> chain) to read the log level from an
environment toggle (e.g., process.env.TEST_LOG_LEVEL or a DEBUG boolean) and
pass that value into the logger option so tests can enable verbose logging when
needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d88764ef-9a97-4fd6-ae7b-57a08e778c6f
⛔ Files ignored due to path filters (9)
apps/fastify/.env-sampleis excluded by!**/.env*apps/fastify/.env.test.exampleis excluded by!**/.env*apps/fastify/tsconfig.jsonis excluded by!**/tsconfig*.jsonpackages/core/src/api-client.gen.tsis excluded by!**/*.gen.tspackages/core/src/api-wrapper.gen.tsis excluded by!**/*.gen.tspackages/core/src/gen/index.tsis excluded by!**/gen/**,!**/gen/**packages/core/src/gen/sdk.gen.tsis excluded by!**/gen/**,!**/gen/**,!**/*.gen.tspackages/core/src/gen/types.gen.tsis excluded by!**/gen/**,!**/gen/**,!**/*.gen.tspnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (50)
apps/docu/content/docs/deployment/self-hosted-llm.mdxapps/docu/content/docs/testing/index.mdxapps/fastify/openapi/openapi.jsonapps/fastify/package.jsonapps/fastify/src/lib/env.tsapps/fastify/src/lib/username.tsapps/fastify/src/plugins/error-handler.tsapps/fastify/src/routes/account/account.spec.tsapps/fastify/src/routes/account/apikeys/create.test.tsapps/fastify/src/routes/account/apikeys/list.test.tsapps/fastify/src/routes/account/apikeys/revoke.test.tsapps/fastify/src/routes/account/email/change/request.test.tsapps/fastify/src/routes/account/email/change/verify.test.tsapps/fastify/src/routes/account/link/email/request.test.tsapps/fastify/src/routes/account/link/email/verify.test.tsapps/fastify/src/routes/account/link/oauth/unlink.test.tsapps/fastify/src/routes/account/link/passkey/delete.test.tsapps/fastify/src/routes/account/link/passkey/finish.test.tsapps/fastify/src/routes/account/link/passkey/start.test.tsapps/fastify/src/routes/account/link/totp/setup.test.tsapps/fastify/src/routes/account/link/totp/unlink.test.tsapps/fastify/src/routes/account/link/totp/verify.test.tsapps/fastify/src/routes/account/link/wallet/unlink.test.tsapps/fastify/src/routes/account/link/wallet/verify.test.tsapps/fastify/src/routes/account/passkeys/list.test.tsapps/fastify/src/routes/account/profile/update.test.tsapps/fastify/src/routes/ai/ai.spec.tsapps/fastify/src/routes/ai/chat.test.tsapps/fastify/src/routes/ai/chat.tsapps/fastify/src/routes/ai/generate.test.tsapps/fastify/src/routes/ai/generate.tsapps/fastify/src/routes/ai/provider.tsapps/fastify/src/routes/auth/magiclink/request.tsapps/fastify/src/routes/auth/session/user.test.tsapps/fastify/src/routes/reference.spec.tsapps/fastify/src/routes/reference.test.tsapps/fastify/src/routes/test/authed.test.tsapps/fastify/src/routes/test/totp.test.tsapps/fastify/test/db-integration.spec.tsapps/fastify/test/utils/ai-remote.tsapps/fastify/test/utils/auth-helper.tsapps/fastify/test/utils/db.spec.tsapps/fastify/test/utils/db.tsapps/fastify/test/utils/fake-email.tsapps/fastify/test/utils/fastify.tsapps/fastify/vitest.config.tsapps/fastify/vitest.setup.tsapps/next/e2e/auth-helpers.tsapps/next/e2e/chat-assistant.spec.tsturbo.json
💤 Files with no reviewable changes (4)
- apps/fastify/src/routes/reference.test.ts
- apps/fastify/src/routes/reference.spec.ts
- apps/fastify/test/utils/db.spec.ts
- apps/fastify/test/db-integration.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/fastify/src/routes/ai/chat.ts (2)
163-167:⚠️ Potential issue | 🟠 MajorDescribe JSON and SSE as separate 200 content types.
The handler returns
{ text }for non-streaming requests and atext/event-streambody for streaming requests, but the schema still advertises one unioned 200 response.@fastify/swaggerexpects different media types to be modeled undercontent; otherwise the autogenerated OpenAPI stays out of sync with the actual route contract. (raw.githubusercontent.com)Suggested fix
response: { - 200: Type.Union([ - ChatResponseSchema, - Type.String({ description: 'Streaming text response' }), - ]), + 200: { + content: { + 'application/json': { + schema: ChatResponseSchema, + }, + 'text/event-stream': { + schema: Type.String({ description: 'SSE stream of UI message events' }), + }, + }, + },As per coding guidelines, "CRITICAL: never edit db/migrations, openapi schema files directly. All migrations must be created through the typical Drizzle flow and OpenAPI is autogenerated. Always validate no discrepancies."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/ai/chat.ts` around lines 163 - 167, The route's response currently unions ChatResponseSchema and a plain string which misrepresents media types; update the route options' response for status 200 to specify content by media type instead of a union — e.g., set response: { 200: { content: { "application/json": ChatResponseSchema, "text/event-stream": Type.String({ description: "Streaming text response" }) } } } so the OpenAPI generated by the ai/chat.ts route correctly documents JSON vs SSE; change the response block referencing ChatResponseSchema and Type.String accordingly.
214-246:⚠️ Potential issue | 🟠 MajorThe stream branch still misses the SDK abort path.
The only downstream abort signal here is
AbortSignal.timeout(...), and the UI stream response is returned withoutconsumeSseStream. The AI SDK docs call for forwarding a request abort signal intostreamText()and passingconsumeSseStream: consumeStreamfor UI-message streams; without that, a dropped SSE client can leave provider work running until the timeout expires. (raw.githubusercontent.com)Minimal fix for the UI stream side
import { + consumeStream, convertToModelMessages, generateText, @@ - const response = result.toUIMessageStreamResponse() + const response = result.toUIMessageStreamResponse({ + consumeSseStream: consumeStream, + })As per coding guidelines, "Verify Fastify functional patterns and TypeBox schema usage".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/ai/chat.ts` around lines 214 - 246, The stream branch currently only uses AbortSignal.timeout and doesn't forward the request abort to the SDK or provide the SSE consumer; update the streamText call in the shouldStream branch to (1) forward a combined abort signal that races the existing AbortSignal.timeout(env.AI_UPSTREAM_TIMEOUT_MS) with the incoming request abort (e.g., request.raw?.signal or a created AbortController that listens to request close) and pass that as abortSignal, and (2) include the SSE consumer option by adding consumeSseStream: consumeStream (or consumeStream as the value expected by streamText). Modify the code around streamText(...) so the options include the raced abortSignal and consumeSseStream: consumeStream and ensure consumeStream is imported/available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/fastify/src/routes/ai/chat.ts`:
- Around line 235-258: Remove the stable user identifier from the
request.log.info calls in the chat handler: stop including
session.user.id.slice(0, 8) in the logged payload for both the streaming branch
(before reply.send) and the non-streaming branch where generateText(baseOptions)
returns; instead remove the userId field entirely (or replace it with a
non-identifying flag like authenticated: true) so request.log.info no longer
emits PII from session.user.id while preserving the existing route, provider,
model, stream, and durationMs fields.
- Around line 34-38: The request schema ChatRequestSchema currently materializes
defaultOpenRouterModel for the model field which causes Fastify/Ajv to set model
on the request and bypass the Ollama fallback in getProvider(); remove the
default from the model property in ChatRequestSchema (make it
Type.Optional(Type.String()) with no default) so that omitted model remains
undefined, and ensure any OpenRouter defaulting logic is applied only inside
getProvider() (or the code path that chooses provider) where
defaultOpenRouterModel can be used when provider-specific logic determines it.
---
Duplicate comments:
In `@apps/fastify/src/routes/ai/chat.ts`:
- Around line 163-167: The route's response currently unions ChatResponseSchema
and a plain string which misrepresents media types; update the route options'
response for status 200 to specify content by media type instead of a union —
e.g., set response: { 200: { content: { "application/json": ChatResponseSchema,
"text/event-stream": Type.String({ description: "Streaming text response" }) } }
} so the OpenAPI generated by the ai/chat.ts route correctly documents JSON vs
SSE; change the response block referencing ChatResponseSchema and Type.String
accordingly.
- Around line 214-246: The stream branch currently only uses AbortSignal.timeout
and doesn't forward the request abort to the SDK or provide the SSE consumer;
update the streamText call in the shouldStream branch to (1) forward a combined
abort signal that races the existing
AbortSignal.timeout(env.AI_UPSTREAM_TIMEOUT_MS) with the incoming request abort
(e.g., request.raw?.signal or a created AbortController that listens to request
close) and pass that as abortSignal, and (2) include the SSE consumer option by
adding consumeSseStream: consumeStream (or consumeStream as the value expected
by streamText). Modify the code around streamText(...) so the options include
the raced abortSignal and consumeSseStream: consumeStream and ensure
consumeStream is imported/available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a666941-2209-4d57-bc17-7a92f2321e95
📒 Files selected for processing (1)
apps/fastify/src/routes/ai/chat.ts
- docu: clarify AI_PROVIDER required when both Open Router and Ollama configured - fastify: username fallback loop with isUsernameTaken; provider useDefault fix - fastify: afterEach fake inbox cleanup; unique emails for totp/verify, expired-token tests - fastify: tighten isInsufficientCredits; getOrCreateSession honor clearBefore - fastify: fake-email extractToken from text body; insertTestPasskey use GET session/user - fastify: error-handler inline message; change/request assert code; oauth/unlink extract userId - fastify: TEST_LOG_LEVEL env; turbo.json env declarations - next: auth-helpers return token without verificationId; chat-assistant timeout and race
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
apps/docu/content/docs/deployment/self-hosted-llm.mdx (1)
255-258: Consider clarifying the "Required" column annotations.The annotations "(one required)" and "(for privacy)" have different meanings that could confuse readers:
OPEN_ROUTER_API_KEY: "No (one required)" implies one of the two API configs must be presentOLLAMA_BASE_URL: "No (for privacy)" suggests it's only for privacy use casesA cleaner approach might be to use consistent terminology like "Conditional" or add a footnote explaining "At least one of
OPEN_ROUTER_API_KEYorOLLAMA_BASE_URLmust be set."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docu/content/docs/deployment/self-hosted-llm.mdx` around lines 255 - 258, The "Required" column is ambiguous for environment vars; update the table row annotations for OPEN_ROUTER_API_KEY and OLLAMA_BASE_URL to use consistent wording (e.g., "Conditional") and add a short footnote clarifying the condition: "At least one of OPEN_ROUTER_API_KEY or OLLAMA_BASE_URL must be set; OLLAMA_BASE_URL is used for self-hosted/private setups." Also ensure AI_PROVIDER row mentions it will be inferred unless both vars are present (then must be set explicitly), and keep AI_DEFAULT_MODEL as-is but align wording with the new "Conditional" convention (refer to OPEN_ROUTER_API_KEY, OLLAMA_BASE_URL, AI_PROVIDER, AI_DEFAULT_MODEL).apps/fastify/src/lib/username.ts (3)
32-36: No uniqueness verification for deterministic test usernames.This function returns a hash-based username without checking the database. While 12 hex characters (48 bits) make collisions unlikely, they're theoretically possible with enough distinct test emails. Consider whether the deterministic property is more important than guaranteed uniqueness, or add a verification step if collisions could cause test failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/lib/username.ts` around lines 32 - 36, The generateTestUsername function returns a deterministic hash-based username but does not verify uniqueness; modify it to either (A) keep determinism but increase hash length (e.g., slice to 16+ chars) to reduce collision risk, or (B) enforce uniqueness by checking the data store and adjusting on collision: call an existing lookup helper (e.g., usernameExists or UserRepository.findByUsername) using the base from generateTestUsername(email) and, if it already exists, append a short deterministic suffix/counter (e.g., _1, _2) or rehash with a salt until an unused username is found before returning; update any calling code to await the new async uniqueness-check variant if you implement the DB check.
68-70: Redundant type check and OAuth flow inconsistency.
typeof email === 'string'is redundant since the parameter is already typed asstring.Also, per
oauth-user.ts, the OAuth user creation flow callsgenerateFunnyUsernamedirectly without theALLOW_TESTcheck. This means OAuth-based test users won't get deterministic usernames even whenALLOW_TESTis enabled. If consistent test behavior is needed across both flows, consider usinggenerateUsernameForMagicLinkin the OAuth path as well.Remove redundant check
- if (env.ALLOW_TEST === true && typeof email === 'string' && email.endsWith('@test.ai')) + if (env.ALLOW_TEST === true && email.endsWith('@test.ai')) return generateTestUsername(email)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/lib/username.ts` around lines 68 - 70, Remove the redundant typeof email === 'string' check in the IF that decides test usernames and make the OAuth path consistent by using the same deterministic generator: update the branch that currently returns generateTestUsername(email) / generateFunnyUsername(client) to simply check env.ALLOW_TEST and email.endsWith('@test.ai') to call generateTestUsername(email) otherwise generateFunnyUsername(client), and modify the OAuth creation flow (where generateFunnyUsername is called directly) to call the same logic (e.g., use generateUsernameForMagicLink or the shared helper you create) so OAuth-based test users receive deterministic test usernames when ALLOW_TEST is enabled; reference generateTestUsername, generateFunnyUsername, generateUsernameForMagicLink and the function that builds the username in the OAuth flow (oauth-user.ts) to locate where to change.
27-30: Consider clarifying themaxLen = 0behavior.When
maxLenis 0, the function returns 4 characters rather than an empty string. This works because callers handle overflow withslice(0, 48), but the semantics are confusing. Consider renaming or documenting the actual behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/lib/username.ts` around lines 27 - 30, The function randomSuffix currently returns 4 hex chars when maxLen is 0 which is surprising; update randomSuffix to either (a) clarify behavior via a JSDoc comment explaining that maxLen<=0 yields a 4-char default suffix, or (b) change the API to be explicit (e.g. rename maxLen to maxOrDefaultLen or add a defaultMinLen parameter) and implement: if maxLen <= 0 then return hex.slice(0, 4), otherwise slice(0, maxLen); update any callers that rely on slice(0, 48) to reflect the clearer API or behavior; refer to the randomSuffix function to locate and modify the implementation and its callers.apps/fastify/test/utils/ai-remote.ts (2)
32-34:isProviderUnavailableis still broad enough to mask real application errors.Using
statusCode >= 500will skip tests on any 500 Internal Server Error, which could hide genuine bugs in route handlers. The regex pattern also matches generic terms like "network" and "ollama" in any context.Consider narrowing to gateway-specific errors that indicate upstream provider issues:
Suggested refinement
export const isProviderUnavailable = (res: ResponseLike) => - res.statusCode >= 500 || - /ECONNREFUSED|fetch failed|ENOTFOUND|ETIMEDOUT|ECONNRESET|network|ollama/i.test(res.body) + [502, 503, 504].includes(res.statusCode) || + /\b(ECONNREFUSED|fetch failed|ENOTFOUND|ETIMEDOUT|ECONNRESET)\b/i.test(res.body)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/test/utils/ai-remote.ts` around lines 32 - 34, The helper isProviderUnavailable is too broad and currently masks real application 500s; change its status check to only treat upstream-gateway failures (e.g., statusCode === 502 || statusCode === 503 || statusCode === 504) instead of statusCode >= 500, and tighten the regex to only match explicit transport/provider connection errors (keep ECONNREFUSED, ENOTFOUND, ETIMEDOUT, ECONNRESET and explicit phrases like "fetch failed" but remove generic tokens such as "network" and broad provider names like "ollama" unless they are exact provider error signatures). Update the function (isProviderUnavailable and its ResponseLike usage) so tests will fail on genuine 500 errors while still recognizing real upstream provider failures.
47-56: Streaming fallback can mask route failures unrelated to provider issues.When
expectStreamis true and content-type isn'ttext/event-stream, the test passes unconditionally. This could hide real application bugs where the route returns wrong content-type due to code errors, not upstream failures.Consider only skipping when there's evidence of provider unavailability:
Suggested refinement
if (opts?.expectStream) { const ct = String(res.headers?.['content-type'] ?? '').toLowerCase() - if (!ct.includes('text/event-stream')) { + if (!ct.includes('text/event-stream') && isProviderUnavailable(res)) { process.stderr.write( `[AI test] ${name}: Expected event-stream, got ${ct || 'unknown'} - passing without validation\n`, ) return true } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/test/utils/ai-remote.ts` around lines 47 - 56, The current early-return when opts?.expectStream is true unconditionally passes the test if content-type isn't text/event-stream; change this to only skip validation when there is clear evidence of upstream/provider failure: inside the expectStream branch, replace the unconditional return with a check of res.statusCode (e.g., 502, 503, 504) or an error marker in the response body, and only log-and-return true in those failure cases; otherwise, treat the unexpected content-type as a test failure (throw or return false) and include the request name (name) and content-type (ct) in the error message so route bugs aren't silently masked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/fastify/test/utils/fake-email.ts`:
- Around line 112-118: The HTML URL extraction only looks for a "token" query
param, but extractToken() expects either "token" or "verificationId"; update
extractTokenFromUrls to parse hrefs' query strings and return the value of
"token" or, if missing, "verificationId" (preserve same precedence), and also
update the regex/html fallback used later (the code referenced around the other
occurrence) to include "verificationId" as a recognized query param so HTML-only
verificationId links return the URL value instead of falling through to the
regex path; reference functions extractToken and extractTokenFromUrls (and the
HTML/regex fallback helper used later) when making the change.
In `@apps/next/e2e/chat-assistant.spec.ts`:
- Around line 18-29: The test currently races on global locators (assistantLoc,
chatError) which may match stale messages; instead, after clicking the Who am I?
button, first wait for and capture the new user turn (the locator that matches
the new user text "Who am I?" — e.g., the element produced by
sheet.locator('[data-role="user"]').filter({ hasText: 'Who am I?' }) or the
enclosing turn element), then scope the assistant and error locators to that
specific turn (use locator.locator(...) or filter on the turn) and run the
Promise.race against those scoped locators so winner and the subsequent
user-info-card assertion only consider the fresh chat turn.
- Around line 31-38: Update the skip logic around chatError in
chat-assistant.spec.ts to match the shared AI helper patterns: compute errorText
from chatError as currently done, then replace the two regex checks with the
same patterns used in apps/fastify/test/utils/ai-remote.ts so the "insufficient
credits" branch uses the same (narrower) matcher and the "provider unreachable"
branch includes missing cases like ECONNRESET, network, and ollama (and any
other tokens present in the ai-remote.ts helper); locate and update the checks
around the errorText variable in the block that calls test.skip to ensure both
branches are identical to the helper's regexes.
---
Nitpick comments:
In `@apps/docu/content/docs/deployment/self-hosted-llm.mdx`:
- Around line 255-258: The "Required" column is ambiguous for environment vars;
update the table row annotations for OPEN_ROUTER_API_KEY and OLLAMA_BASE_URL to
use consistent wording (e.g., "Conditional") and add a short footnote clarifying
the condition: "At least one of OPEN_ROUTER_API_KEY or OLLAMA_BASE_URL must be
set; OLLAMA_BASE_URL is used for self-hosted/private setups." Also ensure
AI_PROVIDER row mentions it will be inferred unless both vars are present (then
must be set explicitly), and keep AI_DEFAULT_MODEL as-is but align wording with
the new "Conditional" convention (refer to OPEN_ROUTER_API_KEY, OLLAMA_BASE_URL,
AI_PROVIDER, AI_DEFAULT_MODEL).
In `@apps/fastify/src/lib/username.ts`:
- Around line 32-36: The generateTestUsername function returns a deterministic
hash-based username but does not verify uniqueness; modify it to either (A) keep
determinism but increase hash length (e.g., slice to 16+ chars) to reduce
collision risk, or (B) enforce uniqueness by checking the data store and
adjusting on collision: call an existing lookup helper (e.g., usernameExists or
UserRepository.findByUsername) using the base from generateTestUsername(email)
and, if it already exists, append a short deterministic suffix/counter (e.g.,
_1, _2) or rehash with a salt until an unused username is found before
returning; update any calling code to await the new async uniqueness-check
variant if you implement the DB check.
- Around line 68-70: Remove the redundant typeof email === 'string' check in the
IF that decides test usernames and make the OAuth path consistent by using the
same deterministic generator: update the branch that currently returns
generateTestUsername(email) / generateFunnyUsername(client) to simply check
env.ALLOW_TEST and email.endsWith('@test.ai') to call
generateTestUsername(email) otherwise generateFunnyUsername(client), and modify
the OAuth creation flow (where generateFunnyUsername is called directly) to call
the same logic (e.g., use generateUsernameForMagicLink or the shared helper you
create) so OAuth-based test users receive deterministic test usernames when
ALLOW_TEST is enabled; reference generateTestUsername, generateFunnyUsername,
generateUsernameForMagicLink and the function that builds the username in the
OAuth flow (oauth-user.ts) to locate where to change.
- Around line 27-30: The function randomSuffix currently returns 4 hex chars
when maxLen is 0 which is surprising; update randomSuffix to either (a) clarify
behavior via a JSDoc comment explaining that maxLen<=0 yields a 4-char default
suffix, or (b) change the API to be explicit (e.g. rename maxLen to
maxOrDefaultLen or add a defaultMinLen parameter) and implement: if maxLen <= 0
then return hex.slice(0, 4), otherwise slice(0, maxLen); update any callers that
rely on slice(0, 48) to reflect the clearer API or behavior; refer to the
randomSuffix function to locate and modify the implementation and its callers.
In `@apps/fastify/test/utils/ai-remote.ts`:
- Around line 32-34: The helper isProviderUnavailable is too broad and currently
masks real application 500s; change its status check to only treat
upstream-gateway failures (e.g., statusCode === 502 || statusCode === 503 ||
statusCode === 504) instead of statusCode >= 500, and tighten the regex to only
match explicit transport/provider connection errors (keep ECONNREFUSED,
ENOTFOUND, ETIMEDOUT, ECONNRESET and explicit phrases like "fetch failed" but
remove generic tokens such as "network" and broad provider names like "ollama"
unless they are exact provider error signatures). Update the function
(isProviderUnavailable and its ResponseLike usage) so tests will fail on genuine
500 errors while still recognizing real upstream provider failures.
- Around line 47-56: The current early-return when opts?.expectStream is true
unconditionally passes the test if content-type isn't text/event-stream; change
this to only skip validation when there is clear evidence of upstream/provider
failure: inside the expectStream branch, replace the unconditional return with a
check of res.statusCode (e.g., 502, 503, 504) or an error marker in the response
body, and only log-and-return true in those failure cases; otherwise, treat the
unexpected content-type as a test failure (throw or return false) and include
the request name (name) and content-type (ct) in the error message so route bugs
aren't silently masked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 227afafa-facb-447d-86da-6b2578f88e77
📒 Files selected for processing (16)
apps/docu/content/docs/deployment/self-hosted-llm.mdxapps/fastify/src/lib/username.tsapps/fastify/src/plugins/error-handler.tsapps/fastify/src/routes/account/account.spec.tsapps/fastify/src/routes/account/email/change/request.test.tsapps/fastify/src/routes/account/link/email/verify.test.tsapps/fastify/src/routes/account/link/oauth/unlink.test.tsapps/fastify/src/routes/account/link/totp/verify.test.tsapps/fastify/src/routes/ai/provider.tsapps/fastify/test/utils/ai-remote.tsapps/fastify/test/utils/auth-helper.tsapps/fastify/test/utils/fake-email.tsapps/fastify/test/utils/fastify.tsapps/next/e2e/auth-helpers.tsapps/next/e2e/chat-assistant.spec.tsturbo.json
💤 Files with no reviewable changes (1)
- apps/fastify/src/plugins/error-handler.ts
✅ Files skipped from review due to trivial changes (1)
- apps/fastify/src/routes/account/link/totp/verify.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/fastify/src/routes/account/email/change/request.test.ts
- apps/fastify/test/utils/fastify.ts
- apps/fastify/src/routes/ai/provider.ts
- apps/next/e2e/auth-helpers.ts
- turbo.json
- Add tsconfig.build.json excluding tests, fix @test/utils resolution - Add typescript to devDependencies, set main to dist/server.js - Chat: remove userId from logs, drop model default, response by content-type - Chat: forward request abort, add consumeSseStream for stream drain - Generate: use toUIMessageStreamResponse for proper SSE - Username: loop until fallback verified free, throw on exhaustion - OAuth unlink test: use GET /auth/session/user instead of PATCH profile - Playwright: add 60s timeout for chromium project - Docs: clarify Ollama privacy-first routing with getResolvedProvider
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/docu/content/docs/deployment/self-hosted-llm.mdx`:
- Line 247: Update the description for the `POST /ai/generate` route in the
docs: locate the table row containing the literal "`POST /ai/generate`" and
replace the phrase "plain text SSE" with wording that makes clear the endpoint
uses JSON-formatted events over SSE (e.g., "JSON events over SSE") so
integrators know the event framing and payload are JSON objects sent via
Server-Sent Events.
In `@apps/fastify/src/lib/username.ts`:
- Around line 68-69: The test-only username branch currently gates on
env.ALLOW_TEST but not environment, so change the condition in the username
generation flow to also ensure non-production before calling
generateTestUsername; specifically update the if that references env.ALLOW_TEST
and email.endsWith('@test.ai') to require env.NODE_ENV !== 'production' (in
addition to env.ALLOW_TEST === true and typeof email === 'string') so the
test-only path (generateTestUsername) cannot run in production.
In `@apps/fastify/src/routes/ai/generate.ts`:
- Around line 7-15: The GenerateRequestSchema currently sets a default model
value (defaultOpenRouterModel) which forces model into the parsed request and
prevents provider-level fallbacks; update GenerateRequestSchema by removing the
default from the model property so the model key can be undefined (i.e., change
Type.Optional(Type.String({ default: defaultOpenRouterModel })) to
Type.Optional(Type.String()) in the GenerateRequestSchema definition) so that
getProvider()/getResolvedProvider() can see undefined and allow Ollama or other
providers to apply their own default selection logic.
- Around line 34-40: The response schema for the /ai/generate route currently
uses Type.Union (resulting in an anyOf under application/json) so the
text/event-stream content-type is missing; update the response object in
apps/fastify/src/routes/ai/generate.ts to mirror /ai/chat by declaring two
separate content-type entries: "application/json" with GenerateResponseSchema
and "text/event-stream" with a Type.String (description: 'Streaming SSE
(text/event-stream) with JSON event objects'), ensuring the stream parameter
behavior remains unchanged and preserving the same response shape identifiers
(GenerateResponseSchema) used in the union.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce1391a5-e80e-4597-a285-bb4892285b10
⛔ Files ignored due to path filters (4)
apps/fastify/tsconfig.build.jsonis excluded by!**/tsconfig*.jsonpackages/core/src/gen/sdk.gen.tsis excluded by!**/gen/**,!**/gen/**,!**/*.gen.tspackages/core/src/gen/types.gen.tsis excluded by!**/gen/**,!**/gen/**,!**/*.gen.tspnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/docu/content/docs/deployment/self-hosted-llm.mdxapps/fastify/openapi/openapi.jsonapps/fastify/package.jsonapps/fastify/src/lib/username.tsapps/fastify/src/routes/account/link/oauth/unlink.test.tsapps/fastify/src/routes/ai/chat.tsapps/fastify/src/routes/ai/generate.tsapps/next/playwright.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/fastify/package.json
- apps/fastify/src/routes/account/link/oauth/unlink.test.ts
- run-e2e-local: use stdio inherit for fastify/next so server logs appear in CI - chat route: log full error (cause, stack) when upstream fails - chat-assistant spec: surface actual error in assertion message, skip on AI provider upstream failure
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
apps/fastify/src/lib/username.ts (1)
65-70:⚠️ Potential issue | 🟠 MajorKeep the
@test.aishortcut disabled in production.Line 69 only checks
env.ALLOW_TEST. If that flag is ever enabled in a production deploy, this deterministic test path becomes active in live auth flows, andapps/fastify/src/lib/oauth-user.tsLine 21 now broadens that impact beyond magic-link signup. Gate it onenv.NODE_ENV !== 'production'as well.Suggested fix
export async function generateUsernameForMagicLink( client: Db | Tx, email: string, ): Promise<string> { - if (env.ALLOW_TEST === true && email.endsWith('@test.ai')) return generateTestUsername(email) + if ( + env.ALLOW_TEST === true && + env.NODE_ENV !== 'production' && + email.endsWith('@test.ai') + ) { + return generateTestUsername(email) + } return generateFunnyUsername(client) }As per coding guidelines, "SECURITY: Ensure server-side validation (never trust client-side validation alone)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/lib/username.ts` around lines 65 - 70, The current generateUsernameForMagicLink function lets the `@test.ai` shortcut run whenever env.ALLOW_TEST is true; change the conditional to require both env.ALLOW_TEST and that env.NODE_ENV !== 'production' so the deterministic test path (generateTestUsername) is only enabled in non-production environments; update the guard in generateUsernameForMagicLink to check env.ALLOW_TEST && env.NODE_ENV !== 'production' before returning generateTestUsername(email), otherwise fall back to generateFunnyUsername(client).apps/fastify/test/utils/ai-remote.ts (1)
32-67:⚠️ Potential issue | 🟠 MajorSkipping on bare
502still hides route regressions.
isProviderUnavailable()now narrows the heuristic, but502is still too broad for these callers: the route handlers return502for every caught upstream exception, not just transient provider outages. That means auth/config/provider-contract bugs in the integration path can still turn into green tests here. Only skip on a distinct retryable error marker, or have the route emit one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/test/utils/ai-remote.ts` around lines 32 - 67, The tests are skipping on bare 502 which hides route regressions; update isProviderUnavailable and the expectStream branch in skipIfProviderUnavailable to only treat true transient/provider outages as skip conditions: remove 502 from the unconditional status checks in isProviderUnavailable (keep 503/504 and the network-error regex), and in skipIfProviderUnavailable's hasUpstreamFailure logic require an explicit retryable/upstream marker in res.body (or only treat 503/504 or network-error regex as upstream) instead of treating any 502 as upstream failure; adjust both isProviderUnavailable and the hasUpstreamFailure usage so they share the same stricter predicate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/docu/content/docs/deployment/self-hosted-llm.mdx`:
- Around line 257-260: The docs currently claim the Ollama default model is
`qwen3:8b` in the environment table (the AI_DEFAULT_MODEL row) but the
walkthrough only instructs users to pull `qwen2.5:3b` and set `OLLAMA_BASE_URL`,
causing a mismatch; update the MDX so the AI_DEFAULT_MODEL entry matches the
walkthrough (change `qwen3:8b` to `qwen2.5:3b`) or alter the walkthrough steps
to pull `qwen3:8b` instead—ensure the `AI_DEFAULT_MODEL` value and the model
referenced in the Ollama install/pull instructions are identical and mention
`OLLAMA_BASE_URL` as the server URL for the chosen model.
In `@apps/fastify/test/utils/ai-remote.ts`:
- Around line 2-23: The current isInsufficientCredits/skipIfInsufficientCredits
logic is unreachable because upstream failures are collapsed into 502
UPSTREAM_SERVICE_ERROR; update isInsufficientCredits to also accept
res.statusCode === 502 and inspect the JSON body or error fields for provider
messages indicating depleted credits (e.g., look for the same
insufficient_quota/insufficient_credits/quota_exceeded/credits_exceeded patterns
or a provider-specific indicator) so skipIfInsufficientCredits can still detect
and skip tests when remote credits are exhausted; alternatively, if you prefer
route-level change, preserve and return a distinct 402 response from the route
handlers that currently collapse errors (the handlers around the logic in
ai/chat.ts and ai/generate.ts) so the existing isInsufficientCredits behavior
remains valid.
In `@apps/fastify/test/utils/fake-email.ts`:
- Around line 112-138: The extractToken(email?: Email) method must stop
returning verificationId; update its URL/text regexes to only match token
parameters and remove verificationId fallbacks: in extractToken change the
textUrlMatch pattern from /[?&](?:token|verificationId)=/i to /[?&]token=/i
(only capture token) and in extractTokenFromUrls(html: string) remove the
verificationIdMatch fallback so the function only attempts to extract token=
from hrefs and returns null when no token is found; ensure extractToken still
tries the existing mono/code regexes but never returns a verificationId.
---
Duplicate comments:
In `@apps/fastify/src/lib/username.ts`:
- Around line 65-70: The current generateUsernameForMagicLink function lets the
`@test.ai` shortcut run whenever env.ALLOW_TEST is true; change the conditional to
require both env.ALLOW_TEST and that env.NODE_ENV !== 'production' so the
deterministic test path (generateTestUsername) is only enabled in non-production
environments; update the guard in generateUsernameForMagicLink to check
env.ALLOW_TEST && env.NODE_ENV !== 'production' before returning
generateTestUsername(email), otherwise fall back to
generateFunnyUsername(client).
In `@apps/fastify/test/utils/ai-remote.ts`:
- Around line 32-67: The tests are skipping on bare 502 which hides route
regressions; update isProviderUnavailable and the expectStream branch in
skipIfProviderUnavailable to only treat true transient/provider outages as skip
conditions: remove 502 from the unconditional status checks in
isProviderUnavailable (keep 503/504 and the network-error regex), and in
skipIfProviderUnavailable's hasUpstreamFailure logic require an explicit
retryable/upstream marker in res.body (or only treat 503/504 or network-error
regex as upstream) instead of treating any 502 as upstream failure; adjust both
isProviderUnavailable and the hasUpstreamFailure usage so they share the same
stricter predicate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20788e4e-d5c6-43a1-ac3d-6d3e0f45d795
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/docu/content/docs/deployment/self-hosted-llm.mdxapps/fastify/package.jsonapps/fastify/src/lib/oauth-user.tsapps/fastify/src/lib/username.tsapps/fastify/test/utils/ai-remote.tsapps/fastify/test/utils/fake-email.tsapps/next/e2e/chat-assistant.spec.tspackage.json
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/fastify/package.json
- Add @ai-sdk/anthropic, ANTHROPIC_API_KEY, AI_PROVIDER=anthropic - Provider precedence (unset): Anthropic > OpenRouter > Ollama - Default model: claude-3-5-sonnet-20241022 when Anthropic - Update chat/generate descriptions and error messages - Tests remain on Ollama (vitest.setup, run-e2e-local unset Anthropic) - turbo.json: add ANTHROPIC_API_KEY to env arrays - Docs: self-hosted-llm.mdx provider table and precedence
- Anthropic: claude-sonnet-4-20250514 - OpenRouter: anthropic/claude-sonnet-4 - Keep openrouter/free alias for free tier - Update env samples and docs
- Add defaultProvider constant (anthropic) - Env sample: ANTHROPIC_API_KEY primary, Open Router commented - AI_PROVIDER enum: anthropic first - Error messages: ANTHROPIC_API_KEY (default) - Docs: Anthropic direct API preferred over Open Router
… build - Replace @test/utils/auth-helper.js with relative paths in all test files - tsgo does not resolve path aliases for test imports, causing TS2307 on Vercel - Fix account/email/change depth (5 levels up to fastify root) fix(next): skip Chat Assistant E2E when Ollama returns 522/timeout - Add Connection timed out, Error code 522, status_code: 522 to skip conditions - CI cannot reach external ollama.gaboesquivel.com (Cloudflare 522)
- Switch vitest.setup, run-e2e-local, and CI workflows from Ollama to Anthropic - Add skip condition for AI 401/invalid API key in chat-assistant E2E - Fix authenticator E2E: use data-slot locator for card-title (not heading) - Fix api-keys E2E: revoke keys from prior tests before expecting empty state - Restore security project in playwright config (run with --project=security) - Keep default E2E run as auth+chromium (security flaky in headless)
- ai-remote: broaden ResponseLike headers to accept OutgoingHttpHeaders (Fastify inject) - fastify test: add pluginTimeout 30s to fix autoload timeout in parallel runs - chat-assistant e2e: make user-info-card assertion resilient when model uses text only
Avoid flaky toContainText failure when AI responds without getAccountInfo
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation