Skip to content

feat: support more AI providers for BYOK (JEF-52) - #155

Merged
mankatcheung merged 1 commit into
mainfrom
worktree-jef-52-multi-provider-byok
Jul 30, 2026
Merged

mankatcheung merged 1 commit into
mainfrom
worktree-jef-52-multi-provider-byok

Conversation

@mankatcheung

@mankatcheung mankatcheung commented Jul 30, 2026 •

Copy link
Copy Markdown
Owner

Summary

Extends JEF-6's bring-your-own-key support beyond OpenRouter/Google AI to the rest of the mainstream field, plus a custom-endpoint escape hatch (Linear JEF-52):

  • New direct providers: OpenAI, Anthropic (Claude), Mistral, Groq, xAI (Grok), DeepSeek
  • Custom (OpenAI-compatible) provider: user supplies their own base URL + key + model — covers anything not explicitly listed (Fireworks, Together, self-hosted, etc.)

Architecture:

  • OpenAICompatibleLLMProvider generalizes the old OpenRouterLLMProvider ({apiKey, baseUrl, model}) — covers every provider that implements OpenAI's /chat/completions shape: OpenAI, Mistral, Groq, xAI, DeepSeek, OpenRouter, and custom.
  • AnthropicLLMProvider is new and bespoke (Messages API: /v1/messages, x-api-key/anthropic-version headers, system prompt as a separate top-level field, content[0].text response shape).
  • PROVIDER_REGISTRY (keyed by provider id) replaces the old if/else in UserLLMProviderFactory — adding a provider is now a registry entry, not new branching logic.

Data model: two new nullable User columns — llmModel (optional override for named providers, falls back to a per-provider default; required for custom) and llmBaseUrl (only used, and required, for custom).

Validation (SaveLlmApiKeyUseCase): custom requires both a well-formed http(s) base URL and a model; named providers reject a baseUrl if one is given (prevents stale/confusing state).

Bug found and fixed during manual verification: ClearLlmApiKeyUseCase only nulled llmProvider/llmApiKey, leaving a stale model/baseUrl behind after clearing a custom-provider key. Now clears all four fields.

Frontend: Account settings' provider <select> lists all 9 providers; selecting custom reveals required Base URL/Model fields, and named providers get an optional "override the default" Model field.

Test plan

  • pnpm --filter @job-finder/api typecheck && pnpm --filter @job-finder/api test (793 tests passing, including new coverage for OpenAICompatibleLLMProvider, AnthropicLLMProvider, PROVIDER_REGISTRY, and updated factory/use-case tests)
  • pnpm --filter @job-finder/web typecheck
  • pnpm --filter @job-finder/api build && pnpm --filter @job-finder/web build
  • pnpm lint (both packages)
  • Manual end-to-end verification against a running dev API for all 8 named providers (save → llmKeyStatus reflects provider/model/baseUrl correctly) plus the custom provider's validation paths (missing base URL, malformed base URL, missing model, base URL rejected on a named provider) and the clear flow (this is where the stale-field bug was caught and fixed).
  • Manual browser verification (Playwright) of the Account settings UI: all 9 providers listed correctly, Base URL/Model fields appear only when Custom is selected and disappear when switching back to a named provider, optional model-override hint shown for named providers.

🤖 Generated with Claude Code

https://claude.ai/code/session_01N2PBmsuzPhrmNnfZf6C3BM

Summary by CodeRabbit

  • New Features
    • Added support for multiple AI providers, including OpenAI, Anthropic, OpenRouter and others.
    • Added custom OpenAI-compatible provider configuration with model and base URL fields.
    • Account settings now display configured provider, model and base URL details.
    • Added provider-specific validation and clearer configuration status information.
  • Bug Fixes
    • Clearing AI credentials now also removes saved model and base URL settings.
    • Improved handling of provider API requests, responses and errors.

Extends JEF-6's bring-your-own-key support beyond OpenRouter/Google AI
to OpenAI, Anthropic, Mistral, Groq, xAI, DeepSeek, and a custom
OpenAI-compatible endpoint (own base URL + key + model) for anything
else.

- OpenAICompatibleLLMProvider generalizes the old OpenRouterLLMProvider
  (same request/response shape covers OpenAI, Mistral, Groq, xAI,
  DeepSeek, OpenRouter, and custom); AnthropicLLMProvider is new and
  bespoke (Messages API, x-api-key/anthropic-version headers, separate
  system field).
- PROVIDER_REGISTRY replaces the old if/else in UserLLMProviderFactory
  with a lookup table — adding a provider is a registry entry now.
- New llmModel/llmBaseUrl columns: model is an optional override for
  named providers (falls back to a per-provider default), and required
  for the custom provider along with a validated http(s) base URL.
- Fixed a bug found during manual verification: ClearLlmApiKeyUseCase
  only nulled llmProvider/llmApiKey, leaving a stale model/baseUrl
  behind after clearing.
- Frontend: Account settings' provider dropdown now lists all
  supported providers, with Base URL/Model fields that appear only for
  the custom provider.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N2PBmsuzPhrmNnfZf6C3BM
@coderabbitai

coderabbitai Bot commented Jul 30, 2026 •

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Adds multi-provider LLM BYOK support with persisted model and base URL fields, custom-provider validation, Anthropic and OpenAI-compatible providers, registry-based resolution, expanded GraphQL status/configuration, and account UI updates.

Changes

Multi-provider LLM configuration

Layer / File(s) Summary
Persist LLM configuration
apps/api/drizzle/*, apps/api/src/domain/user/User.ts, apps/api/src/infrastructure/db/*, apps/api/src/use-cases/ports/IUserRepository.ts, apps/api/src/__tests__/helpers/*
Adds nullable llmModel and llmBaseUrl fields to the database, User entity, repository mapping, and test fixtures.
Validate and expose LLM settings
apps/api/src/use-cases/user/*, apps/api/src/http/schema/*, apps/api/src/interface-adapters/resolvers/UserResolver.ts, apps/api/src/__tests__/application/user/*, apps/api/src/__tests__/interface-adapters/resolvers/*
Passes model and base URL through the API, validates custom-provider requirements, exposes status fields, clears saved configuration, and expands use-case coverage.
Resolve configured providers
apps/api/src/constants.ts, apps/api/src/infrastructure/llm/*, apps/api/src/__tests__/infrastructure/llm/*
Adds provider configuration, Anthropic and generic OpenAI-compatible implementations, registry-based construction, and provider-resolution tests.
Configure providers in the account UI
apps/web/src/routes/_authenticated/account.tsx
Adds provider-specific form validation and fields, dynamic provider options, model/base URL status display, GraphQL variables, and reset handling.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related issues

  • Issue 153 — Implements the multi-provider BYOK scope covering provider classes, registry/factory changes, persistence, validation, GraphQL, and frontend updates.

Possibly related PRs

Poem

A rabbit hops through providers bright,
With models tucked away just right.
Custom URLs join the tune,
Anthropic answers by the moon.
Keys clear clean, forms bloom anew—
Thump-thump! Multi-provider magic grew.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: expanding BYOK support to more AI providers.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch worktree-jef-52-multi-provider-byok

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/routes/_authenticated/account.tsx (1)

653-674: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Stale baseUrl value can silently block form submission after switching away from custom.

React Hook Form keeps field values registered by default (no shouldUnregister), so if a user fills in baseUrl while custom is selected, then switches the provider to a named one, the old baseUrl value survives even though its input is now unmounted. On submit, llmApiKeySchema's superRefine raises a baseUrl issue ("Only valid for a custom provider"), but the non-custom branch (Lines 1344-1358) never renders errors.baseUrl, so the form just fails to submit with no visible error — a confusing dead end for the user.

Clear baseUrl (and optionally reset validation state) whenever the provider changes away from custom.

🐛 Proposed fix
   const llmApiKeyProvider = llmApiKeyForm.watch('provider');
   const isCustomLlmProvider = llmApiKeyProvider === CUSTOM_LLM_PROVIDER;
+  useEffect(() => {
+    if (!isCustomLlmProvider) {
+      llmApiKeyForm.setValue('baseUrl', '', { shouldValidate: false });
+    }
+  }, [isCustomLlmProvider]);
   const onSaveLlmApiKey = async (data: LlmApiKeyForm) => {

Also applies to: 1313-1359

🤖 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/_authenticated/account.tsx` around lines 653 - 674,
Update the provider-change handling associated with llmApiKeyForm and
llmApiKeyProvider so switching away from CUSTOM_LLM_PROVIDER clears the
registered baseUrl value and its validation state. Preserve baseUrl when the
provider remains custom, and ensure subsequent submission no longer includes the
stale field or produces the hidden schema error.
🧹 Nitpick comments (2)
apps/api/src/infrastructure/llm/providerRegistry.ts (1)

20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Tighten the registry key type for exhaustiveness.

Record<string, ...> means a new LLM_PROVIDER value can be added without the compiler flagging a missing registry entry; keying on the constant's value union restores that check.

♻️ Proposed change
-export const PROVIDER_REGISTRY: Record<string, LLMProviderRegistryEntry> = {
+type LlmProviderId = (typeof LLM_PROVIDER)[keyof typeof LLM_PROVIDER];
+
+export const PROVIDER_REGISTRY: Record<LlmProviderId, LLMProviderRegistryEntry> = {
🤖 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/api/src/infrastructure/llm/providerRegistry.ts` at line 20, Update
PROVIDER_REGISTRY to use a key type derived from the LLM_PROVIDER constant’s
value union instead of string, so TypeScript enforces an entry for every
provider and rejects unknown keys. Preserve the existing
LLMProviderRegistryEntry value type.
apps/api/src/__tests__/infrastructure/llm/UserLLMProviderFactory.test.ts (1)

113-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

toBeInstanceOf cannot distinguish the OpenAI-compatible providers.

OpenAI, OpenRouter and custom all resolve to OpenAICompatibleLLMProvider, so a mis-wired base URL or model in the registry would still pass. Consider asserting the resolved endpoint/model (e.g. stub fetch, call complete, and check the URL and model in the body), and add a case for CUSTOM without llmBaseUrl/llmModel to cover the throw path in providerRegistry.ts.

🤖 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/api/src/__tests__/infrastructure/llm/UserLLMProviderFactory.test.ts`
around lines 113 - 130, Strengthen the UserLLMProviderFactory custom-provider
tests beyond toBeInstanceOf(OpenAICompatibleLLMProvider): stub fetch, invoke
complete, and assert the stored llmBaseUrl and llmModel are used in the request
URL and body. Add a CUSTOM case with missing llmBaseUrl or llmModel that asserts
the factory throws through the providerRegistry.ts validation path.
🤖 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/api/src/constants.ts`:
- Around line 231-234: Update LLM.GROQ_DEFAULT_MODEL from
llama-3.3-70b-versatile to Groq’s currently recommended replacement, preferably
openai/gpt-oss-120b, so PROVIDER_REGISTRY.GROQ uses a supported fallback when
llmModel is null.

In `@apps/api/src/infrastructure/llm/AnthropicLLMProvider.ts`:
- Around line 43-47: Update the response handling in the AnthropicLLMProvider
method to collect every content block whose type is “text” and concatenate their
text values in order, rather than returning only the first match. Preserve the
empty-string fallback when no text blocks are present.
- Around line 23-36: Update the outbound fetch in AnthropicLLMProvider to
include an AbortSignal timeout so stalled Anthropic requests terminate instead
of waiting indefinitely; use the provider’s existing configuration conventions
for the timeout when available, otherwise define a clear default. Preserve the
current request payload and headers.

In `@apps/api/src/infrastructure/llm/OpenAICompatibleLLMProvider.ts`:
- Line 31: Update the error handling in OpenAICompatibleLLMProvider to avoid
embedding the upstream body in the thrown Error message. Log a truncated version
of body separately using the provider’s existing logger, while keeping the
thrown message limited to safe context such as the response status.

In `@apps/api/src/use-cases/user/SaveLlmApiKeyUseCase.ts`:
- Around line 16-23: Harden isValidUrl before custom baseUrl is persisted:
continue requiring http/https, reject loopback, link-local, private/reserved IPs
and blocked hostnames, and resolve hostnames to validate every resulting address
against those ranges to prevent DNS rebinding. Apply the same validation to the
baseUrl handling around the referenced persistence path, and reject invalid
targets before saving them.

---

Outside diff comments:
In `@apps/web/src/routes/_authenticated/account.tsx`:
- Around line 653-674: Update the provider-change handling associated with
llmApiKeyForm and llmApiKeyProvider so switching away from CUSTOM_LLM_PROVIDER
clears the registered baseUrl value and its validation state. Preserve baseUrl
when the provider remains custom, and ensure subsequent submission no longer
includes the stale field or produces the hidden schema error.

---

Nitpick comments:
In `@apps/api/src/__tests__/infrastructure/llm/UserLLMProviderFactory.test.ts`:
- Around line 113-130: Strengthen the UserLLMProviderFactory custom-provider
tests beyond toBeInstanceOf(OpenAICompatibleLLMProvider): stub fetch, invoke
complete, and assert the stored llmBaseUrl and llmModel are used in the request
URL and body. Add a CUSTOM case with missing llmBaseUrl or llmModel that asserts
the factory throws through the providerRegistry.ts validation path.

In `@apps/api/src/infrastructure/llm/providerRegistry.ts`:
- Line 20: Update PROVIDER_REGISTRY to use a key type derived from the
LLM_PROVIDER constant’s value union instead of string, so TypeScript enforces an
entry for every provider and rejects unknown keys. Preserve the existing
LLMProviderRegistryEntry value type.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 43c6c771-f24c-460f-b3ed-dafe7c05c874

📥 Commits

Reviewing files that changed from the base of the PR and between 973a527 and 1b35881.

📒 Files selected for processing (32)
  • apps/api/drizzle/0002_silly_umar.sql
  • apps/api/drizzle/meta/0002_snapshot.json
  • apps/api/drizzle/meta/_journal.json
  • apps/api/src/__tests__/application/user/ClearLlmApiKeyUseCase.test.ts
  • apps/api/src/__tests__/application/user/GetLlmKeyStatusUseCase.test.ts
  • apps/api/src/__tests__/application/user/SaveLlmApiKeyUseCase.test.ts
  • apps/api/src/__tests__/helpers/createTestDb.ts
  • apps/api/src/__tests__/helpers/mocks.ts
  • apps/api/src/__tests__/infrastructure/llm/AnthropicLLMProvider.test.ts
  • apps/api/src/__tests__/infrastructure/llm/OpenAICompatibleLLMProvider.test.ts
  • apps/api/src/__tests__/infrastructure/llm/OpenRouterLLMProvider.test.ts
  • apps/api/src/__tests__/infrastructure/llm/UserLLMProviderFactory.test.ts
  • apps/api/src/__tests__/infrastructure/llm/providerRegistry.test.ts
  • apps/api/src/__tests__/interface-adapters/resolvers/UserResolver.test.ts
  • apps/api/src/constants.ts
  • apps/api/src/domain/user/User.ts
  • apps/api/src/http/schema/mutations/userMutations.ts
  • apps/api/src/http/schema/types/LlmKeyStatusType.ts
  • apps/api/src/infrastructure/db/repositories/DrizzleUserRepository.ts
  • apps/api/src/infrastructure/db/schema.ts
  • apps/api/src/infrastructure/llm/AnthropicLLMProvider.ts
  • apps/api/src/infrastructure/llm/OpenAICompatibleLLMProvider.ts
  • apps/api/src/infrastructure/llm/UserLLMProviderFactory.ts
  • apps/api/src/infrastructure/llm/providerRegistry.ts
  • apps/api/src/interface-adapters/resolvers/UserResolver.ts
  • apps/api/src/use-cases/ports/IUserRepository.ts
  • apps/api/src/use-cases/user/ClearLlmApiKeyUseCase.ts
  • apps/api/src/use-cases/user/GetLlmKeyStatusUseCase.ts
  • apps/api/src/use-cases/user/IGetLlmKeyStatusUseCase.ts
  • apps/api/src/use-cases/user/ISaveLlmApiKeyUseCase.ts
  • apps/api/src/use-cases/user/SaveLlmApiKeyUseCase.ts
  • apps/web/src/routes/_authenticated/account.tsx
💤 Files with no reviewable changes (1)
  • apps/api/src/tests/infrastructure/llm/OpenRouterLLMProvider.test.ts

Comment thread apps/api/src/constants.ts
Comment on lines +231 to +234
GROQ_API_URL: 'https://api.groq.com/openai/v1/chat/completions',
GROQ_DEFAULT_MODEL: 'llama-3.3-70b-versatile',
XAI_API_URL: 'https://api.x.ai/v1/chat/completions',
XAI_DEFAULT_MODEL: 'grok-2-latest',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Locate constants and LLM default usages =="
git ls-files 'apps/api/src/**/*.ts' | rg '(^|/)constants\.ts$|LLM|llm|api-key' || true

echo
echo "== constants.ts relevant section =="
if [ -f apps/api/src/constants.ts ]; then
  sed -n '180,250p' apps/api/src/constants.ts | nl -ba -v180
fi

echo
echo "== Search default model constants and factory/use-case references =="
rg -n "LLM_|LLM_PROVIDER|GROQ_DEFAULT_MODEL|XAI_DEFAULT_MODEL|gpt|defaultModel|llmModel|model:" apps/api/src -g '*.ts' | head -n 200

echo
echo "== Inspect provider factory/use-case candidates =="
fd -i '.*llm.*|.*api.*|.*key.*' apps/api/src -t f | sed -n '1,120p'

Repository: mankatcheung/job-finder

Length of output: 1142


🌐 Web query:

Groq API deprecations llama-3.3-70b-versatile shutdown date

💡 Result:

The Groq API model llama-3.3-70b-versatile has a scheduled shutdown date of August 16, 2026 [1][2]. According to official Groq documentation, the deprecation of this model, along with llama-3.1-8b-instant, was announced to users via email on June 17, 2026 [1]. Groq recommends that users migrate to alternative models, specifically openai/gpt-oss-120b or qwen/qwen3.6-27b, to replace llama-3.3-70b-versatile [1]. This deprecation applies to free and developer-tier usage; enterprise customers with a committed-spend contract are not affected [1].

Citations:


🌐 Web query:

xAI API current model catalog grok-2-latest Grok 4 Grok 4.5

💡 Result:

As of July 30, 2026, the current flagship model in the xAI API catalog is Grok 4.5, which was released on July 16, 2026 [1][2]. The xAI API catalog features several models, with Grok 4.5 serving as the primary model for coding, agentic tasks, and knowledge work [3][1][2]. Other models currently available or referenced in the official documentation include Grok 4.3 and various versions of the Grok 4.20 reasoning and multi-agent models [4][5]. Regarding your specific queries: - Grok-2-latest: This identifier is not the current flagship alias. The xAI documentation utilizes aliases such as -latest to provide access to the most recent stable versions of models [4][5]. - Grok 4: This model was released on July 9, 2025 [6][7]. It established foundational capabilities in native tool use and real-time search [6]. - Grok 4.5: This is the current latest flagship model released by xAI [1][2]. It supports a 500,000 token context window, includes reasoning effort parameters (low, medium, high), and is integrated into platforms including the xAI API, Grok web/apps, and GitHub Copilot [1][2][8][9]. For the most up-to-date and specific model identifiers and pricing, please refer to the official xAI models documentation [4][5].

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== constants.ts relevant section =="
sed -n '180,260p' apps/api/src/constants.ts

echo
echo "== LLM provider and factory implementations =="
sed -n '1,240p' apps/api/src/use-cases/ports/ILLMProvider.ts
sed -n '1,240p' apps/api/src/use-cases/ports/ILLMProviderFactory.ts
sed -n '1,260p' apps/api/src/infrastructure/llm/UserLLMProviderFactory.ts
sed -n '1,260p' apps/api/src/infrastructure/llm/providerRegistry.ts

echo
echo "== AI service/usage call sites =="
rg -n "GROQ_DEFAULT_MODEL|XAI_DEFAULT_MODEL|LLM_DEFAULT|GROQ_API_URL|XAI_API_URL|model: user\.llmModel|create.*Provider|send|generate|chat" apps/api/src -g '*.ts' | head -n 250

echo
echo "== LLM entity/user schema/model fields =="
rg -n "llmProvider|llmModel|Model|api.*Key|Groq|xAI" apps/api/src -g '*.ts' | head -n 250

Repository: mankatcheung/job-finder

Length of output: 50379


Update the Groq default before merge.

PROVIDER_REGISTRY.GROQ falls back to LLM.GROQ_DEFAULT_MODEL when a user’s llmModel is null, and llama-3.3-70b-versatile is scheduled to be shut down by Groq. Use Groq’s currently recommended alternative, such as openai/gpt-oss-120b, or add an explicit provider default with periodic review coverage.

🤖 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/api/src/constants.ts` around lines 231 - 234, Update
LLM.GROQ_DEFAULT_MODEL from llama-3.3-70b-versatile to Groq’s currently
recommended replacement, preferably openai/gpt-oss-120b, so
PROVIDER_REGISTRY.GROQ uses a supported fallback when llmModel is null.

Comment on lines +23 to +36
const response = await fetch(LLM.ANTHROPIC_API_URL, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
'x-api-key': this.apiKey,
'anthropic-version': LLM.ANTHROPIC_VERSION,
},
body: JSON.stringify({
model: this.model,
max_tokens: maxTokens,
...(system ? { system } : {}),
messages: conversation,
}),
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

No timeout on the outbound Anthropic call.

fetch here has no AbortSignal, so a stalled upstream keeps the request thread and connection held indefinitely. Pass a timeout signal (and consider making it configurable).

🛡️ Proposed fix
     const response = await fetch(LLM.ANTHROPIC_API_URL, {
       method: 'POST',
+      signal: AbortSignal.timeout(LLM.REQUEST_TIMEOUT_MS),
       headers: {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const response = await fetch(LLM.ANTHROPIC_API_URL, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
'x-api-key': this.apiKey,
'anthropic-version': LLM.ANTHROPIC_VERSION,
},
body: JSON.stringify({
model: this.model,
max_tokens: maxTokens,
...(system ? { system } : {}),
messages: conversation,
}),
});
const response = await fetch(LLM.ANTHROPIC_API_URL, {
method: 'POST',
signal: AbortSignal.timeout(LLM.REQUEST_TIMEOUT_MS),
headers: {
'Content-Type': 'application/json',
'x-api-key': this.apiKey,
'anthropic-version': LLM.ANTHROPIC_VERSION,
},
body: JSON.stringify({
model: this.model,
max_tokens: maxTokens,
...(system ? { system } : {}),
messages: conversation,
}),
});
🤖 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/api/src/infrastructure/llm/AnthropicLLMProvider.ts` around lines 23 -
36, Update the outbound fetch in AnthropicLLMProvider to include an AbortSignal
timeout so stalled Anthropic requests terminate instead of waiting indefinitely;
use the provider’s existing configuration conventions for the timeout when
available, otherwise define a clear default. Preserve the current request
payload and headers.

Comment on lines +43 to +47
const json = (await response.json()) as {
content?: Array<{ type: string; text?: string }>;
};

return json.content?.find((block) => block.type === 'text')?.text ?? '';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Only the first text block is returned.

Anthropic may return several text blocks in content; taking just the first silently truncates the completion.

♻️ Proposed fix
-    return json.content?.find((block) => block.type === 'text')?.text ?? '';
+    return (
+      json.content
+        ?.filter((block) => block.type === 'text')
+        .map((block) => block.text ?? '')
+        .join('') ?? ''
+    );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const json = (await response.json()) as {
content?: Array<{ type: string; text?: string }>;
};
return json.content?.find((block) => block.type === 'text')?.text ?? '';
const json = (await response.json()) as {
content?: Array<{ type: string; text?: string }>;
};
return (
json.content
?.filter((block) => block.type === 'text')
.map((block) => block.text ?? '')
.join('') ?? ''
);
🤖 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/api/src/infrastructure/llm/AnthropicLLMProvider.ts` around lines 43 -
47, Update the response handling in the AnthropicLLMProvider method to collect
every content block whose type is “text” and concatenate their text values in
order, rather than returning only the first match. Preserve the empty-string
fallback when no text blocks are present.

if (!response.ok) {
const body = await response.text();
throw new Error(`OpenRouter error ${response.status}: ${body}`);
throw new Error(`LLM provider error ${response.status}: ${body}`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Upstream response body is embedded verbatim in the thrown error.

Provider error bodies can contain request echoes or account details; if this message surfaces in GraphQL errors or logs it becomes a leak vector. Truncate and log the body separately rather than putting it in the message.

🤖 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/api/src/infrastructure/llm/OpenAICompatibleLLMProvider.ts` at line 31,
Update the error handling in OpenAICompatibleLLMProvider to avoid embedding the
upstream body in the thrown Error message. Log a truncated version of body
separately using the provider’s existing logger, while keeping the thrown
message limited to safe context such as the response status.

Comment on lines +16 to +23
function isValidUrl(value: string): boolean {
try {
const url = new URL(value);
return url.protocol === 'http:' || url.protocol === 'https:';
} catch {
return false;
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate and inspect the target file plus nearby provider references without executing repo code.
printf 'Files matching SaveLlmApiKeyUseCase:\n'
fd -a 'SaveLlmApiKeyUseCase\.ts$' . || true

printf '\nOutline:\n'
target="$(fd 'SaveLlmApiKeyUseCase\.ts$' . | head -n 1 || true)"
if [ -n "${target:-}" ]; then
  wc -l "$target"
  ast-grep outline "$target" || true
  printf '\nTarget file with line numbers:\n'
  cat -n "$target"
fi

printf '\nSearch for UserLLMProviderFactory / OpenAICompatibleLLMProvider / useLlmBaseUrl references:\n'
rg -n "UserLLMProviderFactory|OpenAICompatibleLLMProvider|llmBaseUrl|baseUrl|isValidUrl|SaveLlmApiKeyUseCase" -S . --glob '!node_modules' --glob '!dist' --glob '!build' | head -n 200

Repository: mankatcheung/job-finder

Length of output: 18904


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf 'SaveLlmApiKeyUseCase.ts lines 1-85:\n'
cat -n apps/api/src/use-cases/user/SaveLlmApiKeyUseCase.ts | sed -n '1,90p'

printf '\nOpenAICompatibleLLMProvider.ts:\n'
cat -n apps/api/src/infrastructure/llm/OpenAICompatibleLLMProvider.ts

printf '\nUserLLMProviderFactory.ts:\n'
cat -n apps/api/src/infrastructure/llm/UserLLMProviderFactory.ts

printf '\nproviderRegistry.ts lines 60-85:\n'
cat -n apps/api/src/infrastructure/llm/providerRegistry.ts | sed -n '60,85p'

printf '\nRead-only URL validation probe for current isValidUrl behaviour:\n'
node - <<'JS'
function isValidUrl(value) {
  try {
    const url = new URL(value);
    return url.protocol === 'http:' || url.protocol === 'https:';
  } catch {
    return false;
  }
}

const inputs = [
  'https://my-llm.example.com/v1/chat/completions',
  'http://127.0.0.1:8080/v1/chat/completions',
  'http://localhost:8080/v1/chat/completions',
  'http://169.254.169.254/latest/meta-data/token',
  'http://10.internal.example/v1/chat/completions',
  'http://192.168.1.10/v1/chat/completions',
  'http://[::1]/v1/chat/completions',
  'ftp://example.com/foo',
  'not-a-url',
];

for (const input of inputs) {
  console.log(`${input} -> ${isValidUrl(input)}`);
}
JS

printf '\nCheck for outbound fetch configuration / egress proxy / global fetch config:\n'
rg -n "globalThis\.fetch|AbortSignal\.timeout|NO_PROXY|proxy|hostFilter|request\.agent|fetch\(" apps/api/src --glob '*.ts' | head -n 200 || true

Repository: mankatcheung/job-finder

Length of output: 9438


Block internal and DNS-rebinding targets before persisting custom baseUrl.

isValidUrl() only accepts http:/https: URLs, but custom provider baseUrl is persisted and later passed to OpenAICompatibleLLMProvider.complete(), which calls fetch(this.baseUrl). This allows authenticated users to set values such as http://localhost, http://127.0.0.1, http://169.254.169.254/..., or private RFC1918 targets, causing the API server to issue outbound requests on the user’s behalf. Reject loopback, link-local, private IP ranges, blocked hostnames, and resolve hostnames before persisting so DNS rebinding is handled.

Also applies to: 46-50

🤖 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/api/src/use-cases/user/SaveLlmApiKeyUseCase.ts` around lines 16 - 23,
Harden isValidUrl before custom baseUrl is persisted: continue requiring
http/https, reject loopback, link-local, private/reserved IPs and blocked
hostnames, and resolve hostnames to validate every resulting address against
those ranges to prevent DNS rebinding. Apply the same validation to the baseUrl
handling around the referenced persistence path, and reject invalid targets
before saving them.

@mankatcheung
mankatcheung merged commit 72e10bf into main Jul 30, 2026
11 checks passed
@mankatcheung
mankatcheung deleted the worktree-jef-52-multi-provider-byok branch August 18, 2026 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant