Skip to content

fix(nexus): exclude models with unconfigured provider credentials from routed selection - #1632

Merged
krishagel merged 2 commits into
devfrom
claude/focused-hugle-a79a6f
Aug 11, 2026
Merged

fix(nexus): exclude models with unconfigured provider credentials from routed selection#1632
krishagel merged 2 commits into
devfrom
claude/focused-hugle-a79a6f

Conversation

@krishagel

Copy link
Copy Markdown
Member

Problem

nexus-chat-pii-passthrough.functional.spec.ts failed nondeterministically in full local E2E walls with "Failed to process chat request" (diagnosed 2026-08-10). In AUTO mode the Nexus model router's candidate lists can contain OpenAI/Google/Azure/Latimer models. On machines where that provider's key is not configured, the router still selected the model; provider creation then threw ("OpenAI API key not configured") deep in the streaming path and POST /api/nexus/chat 500'd. The router's fallbackModelId cannot catch this — routing has already returned by the time the provider factory throws. The nova-micro classifier's per-run variance made it intermittent.

Fix

Router excludes models whose provider credentials are not configured (chosen over engaging the fallback on missing-key errors, which would need a retry around the whole stream-setup path, and over pinning a family in the spec, which would mask the same 500 for real local AUTO-mode users).

  • lib/ai/provider-credentials.ts (new): getConfiguredChatProviders() probes OpenAI/Google/Azure/Latimer credentials via the settings manager (DB-first, env fallback, existing 5-min cache). amazon-bedrock always counts as configured — it authenticates through the ambient AWS credential chain (ECS/Lambda IAM roles), which cannot be probed. Fails open on probe errors so a settings outage cannot take chat down.
  • lib/nexus/model-router/router.ts: routeNexusRequest() filters the model pool to configured providers before all routed selection (text, image-specialist, and required-tools paths), logging excluded providers. The client's explicitly selected model is looked up in the unfiltered list on purpose: explicit selection keeps its honest provider-configuration error instead of being silently rerouted. Router-off and shadow-mode legacy execution unchanged.
  • docs/features/nexus-model-routing.md: request-flow step 5 documents the filter and the explicit-selection carve-out.

Verification

  • New unit tests: 5 for the credential probe (bedrock-only default, full set, whitespace keys, Azure key+resourceName pairing, fail-open) and 4 router tests (AUTO skips unconfigured candidate; router-off keeps explicit model; Advanced family fails clearly; image intent respects the filter). Existing 19 router tests unchanged.
  • bunx jest lib/nexus lib/ai: 35 suites / 298 tests pass. Full lint + typecheck clean.
  • Authenticated E2E: the previously flaky spec passed 4/4 with --repeat-each=4 and retries disabled; pre-push wall 319 passed / 0 failed.
  • Environment probe on the dev machine confirms getConfiguredChatProviders() returns only amazon-bedrock (no provider keys in .env.local, empty settings rows), so local AUTO routing is now deterministic instead of classifier-dependent.

…m routed selection

Root cause of the nondeterministic nexus-chat-pii-passthrough.functional
E2E failures (diagnosed 2026-08-10): in AUTO mode the Nexus model router's
configured candidate lists can contain OpenAI (or Google/Azure/Latimer)
models. On machines where that provider's API key is not configured
(local dev: .env.local has no provider keys and the settings table is
empty), the router still selected the model, provider creation then threw
"OpenAI API key not configured" deep in the streaming path, and
POST /api/nexus/chat 500'd with "Failed to process chat request". The
router's fallbackModelId cannot catch this because the error occurs after
routing, at stream time. The failure was nondeterministic because the
nova-micro classifier's tier/intent output varies per run, changing which
candidate wins.

Fix (option "router excludes unconfigured families", chosen over
fallback-on-missing-key — which would need a retry wrapped around the
entire stream-setup path — and over pinning a family in the spec, which
would mask the same 500 for real local AUTO-mode users):

- lib/ai/provider-credentials.ts (new): getConfiguredChatProviders()
  probes OpenAI/Google/Azure/Latimer credentials through the settings
  manager (database-first, environment fallback, existing 5-minute
  cache). amazon-bedrock is always treated as configured because it can
  authenticate through the ambient AWS credential chain (ECS/Lambda IAM
  roles), which cannot be probed here. Fails open on probe errors so a
  settings outage cannot take chat down.

- lib/nexus/model-router/router.ts: routeNexusRequest() filters the model
  pool to configured providers before any routed selection (text, image
  specialist, and required-tools enforcement paths all inherit the
  filter), logging the excluded providers. The fallback model — the
  client's explicitly selected model — is deliberately looked up in the
  UNFILTERED list, so an explicit selection of an unconfigured provider
  keeps its honest configuration error instead of being silently
  rerouted; router-off mode and shadow-mode legacy execution are
  unchanged.

Tests:
- lib/ai/__tests__/provider-credentials.test.ts (new): bedrock-only
  default, full-set when all keys present, whitespace keys rejected,
  Azure requires key AND resource name, fail-open on probe error.
- lib/nexus/model-router/__tests__/router.test.ts: AUTO routing skips an
  unconfigured-provider candidate; router-off keeps the explicit model
  without its provider key; Advanced family with unconfigured provider
  fails with the clear family error; image intent does not route through
  an unconfigured provider. Existing 19 router tests unchanged (the new
  credential mock defaults to all-configured).

Verification:
- bunx jest lib/nexus lib/ai: 35 suites, 298 tests pass.
- bun run lint and bun run typecheck: clean over the entire codebase.
- Authenticated E2E: nexus-chat-pii-passthrough.functional.spec.ts passed
  4/4 with --repeat-each=4 and retries disabled via scripts/test/e2e-local.sh.
- Environment probe on the dev machine confirms getConfiguredChatProviders()
  returns only amazon-bedrock, so local AUTO routing is now deterministic
  (Bedrock-only) instead of classifier-dependent.

docs/features/nexus-model-routing.md: request-flow step 5 now documents
the credential filter and the explicit-selection carve-out.
Copilot AI lite review requested due to automatic review settings August 11, 2026 02:14

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d934a768aa

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

config,
mode,
models,
models: routableModels,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve the explicit fallback after routed candidates are exhausted

When active Standard/Auto routing has no accessible model from a configured provider, passing only routableModels means selectRoutedTextModel cannot find the explicitly selected fallback because it searches for that fallback inside its models argument. The request therefore throws No accessible Nexus model is available before provider creation instead of retaining the selected model and surfacing its provider-configuration error as the new comment and routing documentation specify. Keep the unconfigured fallback out of normal candidate selection, but allow it as the final non-routed fallback when no configured alternative exists.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Behavior locked in with tests in 60c8140, but keeping the fail-fast rather than executing the unconfigured explicit fallback: both outcomes are errors, and running a model whose provider creation must throw reproduces the exact stream-time missing-key 500 this PR removes — a misleading mid-stream crash instead of an honest routing error. The docs carve-out applies to the paths that execute the explicit model directly (router-off without required tools, shadow retention), which are unchanged.

@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review: #1632 — exclude models with unconfigured provider credentials from routed selection

A well-scoped fix with a clear root-cause writeup and solid test coverage for the core cases (AUTO skip, router-off explicit-selection carve-out, Advanced-family clear error, image intent). A few notes:

Code quality / correctness

  • lib/ai/provider-credentials.ts is clean, well-documented, and consistent with the existing Settings DB-first/env-fallback/cache pattern. Failing open on a probe error is the right call (a settings outage should not take down chat), and that path is covered by a test.

  • Edge case worth a second look: in routeNexusRequest (lib/nexus/model-router/router.ts:437-450), routableModels (filtered by configured provider) is what gets passed into buildRouterOffResult / routeWithConfiguredRouter, while fallback itself is resolved from the unfiltered models list. That split is intentional and documented for the common "router off, no required tools" path, so the client explicit pick keeps its honest provider error.

    However, when mode === "off" and requiredTools.length > 0 (for example attachments forcing searchNexusAttachments), buildRouterOffResult calls selectModel({ models: routableModels, ..., fallbackModelId }). If the client explicitly-selected model provider is not configured, the last-resort "use the original fallback if it meets requirements" branch in selectRoutedTextModel (lib/ai/model-router/core.ts:173-178) can no longer find that model since it has been filtered out of args.models, so it falls through to a generic "No accessible Nexus model is available" error instead of the previous behavior. Arguably more correct (never route to a broken provider), but it is a behavior change in a combination the new tests do not cover — the existing "enforces attachment tools when legacy routing is off" test (router.test.ts:168) always mocks the full 5-provider set. Worth an explicit test for router-off plus required-tools plus an unconfigured fallback provider so this path is locked in on purpose.

Maintainability (nitpick)

  • There are now three near-identical hardcoded provider-id lists: ALL_CHAT_PROVIDERS (lib/ai/provider-credentials.ts:6), EXECUTABLE_PROVIDERS (lib/nexus/model-router/router.ts:33, pre-existing), and the equivalent set in lib/ai/model-router/core.ts:24, plus VALID_PROVIDERS in lib/constants/providers.ts (which also includes google-vertex, not handled by any of these). Not a bug today since the values are in sync, but a new provider added to one and not the others would silently break either credential filtering or execution eligibility. Could be worth consolidating into a single source of truth in a follow-up.

Performance

  • No concern. getConfiguredChatProviders() reuses the existing 5-minute settings cache, so the added call on every routeNexusRequest is a cache hit in steady state, same cost profile as existing Settings.* usage elsewhere in the codebase.

Security

  • No issues. This is a read-only credential presence probe (checks for non-blank values), does not log secret values, and fails open rather than closed in a way that is explicitly justified (availability over over-filtering) — consistent with repo conventions around PII/secret-safe logging.

Test coverage

  • Good: 5 unit tests for the probe (default/full/whitespace/Azure pairing/fail-open) plus 4 router tests (AUTO skip, router-off carve-out, Advanced-family error, image intent). Per the PR description, bunx jest lib/nexus lib/ai reports green.
  • Gap noted above: the router-off plus required-tools plus unconfigured-fallback-provider combination is not exercised by a test.

Docs

  • docs/features/nexus-model-routing.md step 5 update accurately reflects the new filtering behavior and the explicit-selection carve-out.

Overall: solid, well-reasoned fix for a real nondeterministic-failure root cause. The one item worth resolving before or after merge is confirming, with a test, that the router-off plus required-tools path degrades the way you intend when the fallback model own provider is not configured.

… exhaustion behavior

Answers the PR #1632 review asks (claude-review + Codex P2) on the
credential filter's edge cases without changing behavior:

- Router off + required tools + explicitly selected model on an
  unconfigured provider: routing selects a configured, tool-capable
  model (required_tools_enforced) instead of executing a model whose
  provider creation would throw at stream time.
- No configured provider has an accessible model: routing fails fast
  with the clear 'No accessible Nexus model is available' error BEFORE
  provider creation. The Codex P2 suggestion to execute the unconfigured
  explicit model as a final fallback is intentionally declined — both
  outcomes are errors, but executing it reproduces the exact stream-time
  missing-key 500 this filter exists to prevent, with a misleading
  mid-stream failure instead of an honest routing error.
@krishagel

Copy link
Copy Markdown
Member Author

Review follow-up in 60c8140: added the two tests covering the edge both reviews flagged — (1) router-off + required tools + unconfigured explicit provider re-routes to a configured tool-capable model, (2) full exhaustion fails fast with the clear routing error before provider creation. Declined executing the unconfigured explicit model as a last resort (rationale in the inline reply): it would reproduce the stream-time missing-key 500 this PR eliminates.

@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review

Solid, well-scoped fix — reviewed the diff, the CLAUDE.md-documented silent-failure patterns, and cross-checked the new probe against lib/ai/provider-factory.ts's actual per-provider requirements.

Correctness

  • getConfiguredChatProviders()'s checks match provider-factory.ts exactly for every provider: OpenAI/Google/Latimer key presence, and Azure's key + resourceName pair (verified createAzureModel also only requires those two, not endpoint — so the probe isn't over- or under-strict). Bedrock's "always configured" carve-out is correctly justified (ambient IAM credential chain can't be probed).
  • The unfiltered fallback lookup (router.ts:429-433) before the credential filter is applied is a good call — it preserves the explicit-selection semantics described in the PR body and is directly exercised by the "router-off keeps explicit model" test.
  • Filtering modelsroutableModels once at the top of routeNexusRequest and threading it through both buildRouterOffResult and routeWithConfiguredRouter means the image-specialist path (selectModel's intent === "image" branch) and the required-tools path both inherit the filter for free, rather than needing separate filtering logic. Confirmed via selectRoutedTextModel's final fallback step in lib/ai/model-router/core.ts — since it does args.models.find(...) against the already-filtered array, an unconfigured fallback model correctly cannot be resurrected as a last-resort pick in the routed path.
  • Fail-open behavior on probe errors is correct per the stated goal (a settings outage must not take chat down) and is unit-tested.

Minor / non-blocking

  • EXECUTABLE_PROVIDERS is now defined in both lib/ai/model-router/core.ts and lib/nexus/model-router/router.ts (pre-existing duplication, not introduced here) — could be worth consolidating into one shared constant at some point, but out of scope for this fix.
  • The new log.info on partial exclusion logs excludedProviders but not which specific model IDs were dropped; probably fine for this log's purpose (provider-level signal), but if debugging routing decisions later becomes common, model-level detail could help. Not a blocker.
  • Shadow mode with requiredTools.length === 0 still executes the original unfiltered fallback model directly (selectedRuntimeModel, router.ts:266-274), bypassing the new filter entirely for that one path. That matches existing shadow-mode semantics (it never executes the proposed route, only records it) and isn't a regression, but worth confirming this is intentional given shadow mode's purpose is presumably to validate what active mode would do — an unconfigured provider in the shadow-proposed model won't be caught until active mode is flipped on.

Test coverage

Good breadth — credential probe (5 cases covering whitespace keys, Azure pairing, fail-open) plus 5 router-level tests covering AUTO exclusion, router-off passthrough, Advanced-family failure, required-tools re-routing, and the image-specialist path. The "fails fast before provider creation" test directly encodes the regression this PR prevents.

Security

No concerns — this only reads already-server-side settings to build an allow-list for routing; no new attack surface, no secrets logged (only provider names).

Nothing blocking. Nice root-cause fix — addresses the actual gap (router's fallbackModelId can't catch a provider-creation throw that happens after routing returns) rather than papering over the flaky test.

@krishagel
krishagel merged commit 2b5ffd6 into dev Aug 11, 2026
7 checks passed
@krishagel
krishagel deleted the claude/focused-hugle-a79a6f branch August 11, 2026 03:01
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.

2 participants