refactor(llm): route llm.default reads through resolveCallSiteConfig#30258
Conversation
credence-the-bot
Bot
force-pushed
the
credence/llm-default-resolver-routing
branch
from
9ff3106 to
571326b
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
|
||
| const fastModeEnabled = isAssistantFeatureFlagEnabled("fast-mode", config); | ||
| const resolvedSpeed = speedOverride ?? config.llm.default.speed; | ||
| const resolvedSpeed = speedOverride ?? resolvedMainAgent.speed; |
There was a problem hiding this comment.
🔴 Incomplete migration: thinking and effort still read from raw llm.default while sibling fields use resolved config
The PR migrates streamThinking (line 438), speed (line 490), and contextWindow (line 497) to read from resolvedMainAgent (i.e. resolveCallSiteConfig("mainAgent", config.llm)), but the agentLoopConfig at assistant/src/daemon/conversation.ts:502-503 still reads thinking and effort from the raw llmDefault (config.llm.default). When a user has an active profile or callSites.mainAgent override that changes thinking.enabled or effort, those overrides are respected for streamThinking and speed but silently ignored for the agent loop's actual thinking/effort wire config. For example, a profile that sets thinking: { enabled: false } would cause this.streamThinking to be false (resolved) but agentLoopConfig.thinking.enabled to remain true (raw default), leading to the model still receiving thinking instructions.
Prompt for agents
In assistant/src/daemon/conversation.ts, after line 437 introduces `const resolvedMainAgent = resolveCallSiteConfig("mainAgent", config.llm)`, lines 502-503 still read `thinking` and `effort` from `llmDefault` (which is `config.llm.default`). These should be migrated to use `resolvedMainAgent` for consistency with `streamThinking`, `speed`, and `contextWindow` which already use `resolvedMainAgent`. Specifically, change `thinking: llmDefault.thinking` to `thinking: resolvedMainAgent.thinking` and `effort: llmDefault.effort` to `effort: resolvedMainAgent.effort` in the `agentLoopConfig` object literal around line 501-503. The `llmDefault` variable on line 491 can then be removed if it has no other usages.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (!baseProvider) { | ||
| throw new Error( | ||
| `Subagent: default provider '${appConfig.llm.default.provider}' is not registered`, | ||
| `Subagent: default provider '${resolveCallSiteConfig("subagentSpawn", appConfig.llm).provider}' is not registered`, |
There was a problem hiding this comment.
🟡 Subagent error message resolves provider name from wrong call site
The error message at assistant/src/subagent/manager.ts:194 uses resolveCallSiteConfig("subagentSpawn", appConfig.llm).provider to report which provider failed, but the actual provider resolution is done by resolveDefaultProvider(appConfig) which internally uses resolveCallSiteConfig("mainAgent", ...) (assistant/src/providers/connection-resolution.ts:149). If subagentSpawn has a call-site override pointing to a different provider, the error message will report the wrong provider name, making debugging harder.
| `Subagent: default provider '${resolveCallSiteConfig("subagentSpawn", appConfig.llm).provider}' is not registered`, | |
| `Subagent: default provider '${resolveCallSiteConfig("mainAgent", appConfig.llm).provider}' is not registered`, |
Was this helpful? React with 👍 or 👎 to provide feedback.
credence-the-bot
Bot
force-pushed
the
credence/llm-default-resolver-routing
branch
from
571326b to
532559e
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ff3106b50
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const resolved = resolveCallSiteConfig("mainAgent", config.llm); | ||
| const connectionName = resolved.provider_connection; |
There was a problem hiding this comment.
Changing resolveDefaultProvider to read resolveCallSiteConfig("mainAgent", ...) makes every satellite that bootstraps a base provider (e.g. createApprovalCopyGenerator / createApprovalConversationGenerator in assistant/src/daemon/approval-generators.ts, SubagentManager in assistant/src/subagent/manager.ts) depend on the active main-agent profile even when their own call-site profile has a valid provider_connection. In the scenario where the active main-agent profile is temporarily unresolvable (missing credential/transient auth) but a satellite call site is valid, these paths now bail out early before wrapWithCallSiteRouting can reroute, so the feature fails unnecessarily.
Useful? React with 👍 / 👎.
credence-the-bot
Bot
force-pushed
the
credence/llm-default-resolver-routing
branch
from
532559e to
7254070
Compare
|
Thanks bots — fixed both Devin findings in the new commit:
On Codex P2 ( This coupling is intentional per the plan and accepted as the design trade-off: |
~30 call sites read `config.llm.default.{provider, model, contextWindow,
speed, thinking, maxTokens, ...}` directly instead of going through
`resolveCallSiteConfig`. With managed + user profiles, the active profile
can differ from `llm.default` and those reads return stale/wrong values.
This routes the reads through the resolver so they pick up the active
profile's effective config.
Interface change: widen `ProvidersConfig.llm` from `{ default: {...} }`
to `LLMConfig`. All production callers pass `getConfig()` which already
satisfies the wider type. Five test files needed manual updates to the
local `ProvidersConfig` construction (spread from `LLMSchema.parse({})`).
Tier 1 (dispatch path):
- providers/connection-resolution.ts
- providers/registry.ts (also widens ProvidersConfig.llm)
- daemon/conversation.ts (3 sites)
- daemon/conversation-agent-loop.ts
- daemon/conversation-store.ts
- subagent/manager.ts
Tier 2 (playground / context-window routes):
- runtime/routes/playground/{state,inject-failures,reset-circuit}.ts
Tier 3 (display / diagnostic):
- daemon/handlers/config-model.ts
- daemon/conversation-slash.ts
- runtime/routes/debug-routes.ts
- usage/attribution.ts
- workspace/provider-commit-message-generator.ts
- memory/embedding-backend.ts
Tests updated to construct full LLMConfig:
- __tests__/provider-managed-proxy-integration.test.ts
- __tests__/inference-no-mode-boot-e2e.test.ts
- __tests__/provider-registry-ollama.test.ts
- __tests__/secret-routes-managed-proxy.test.ts
- providers/__tests__/satellite-connection-routing.test.ts
Out of scope (separate follow-up):
- `setModel` in handlers/config-model.ts still writes to `llm.default` via
`setLlmDefaultField`. With profiles, this should probably write to the
active profile instead. Needs a design call.
credence-the-bot
Bot
force-pushed
the
credence/llm-default-resolver-routing
branch
from
7254070 to
883125c
Compare
1 participant
What
Routes ~25 call sites that read
config.llm.default.*throughresolveCallSiteConfigso they respect the active inference profile, not always the literalllm.defaultvalue.Why
With managed + user profiles (Phase 1.2 #30232), the active profile can differ from
llm.default. Direct reads ofllm.default.{provider, model, contextWindow, ...}return stale / wrong values when a profile is active.How
Widen
ProvidersConfig.llmfrom{ default: {...} }toLLMConfig. All production callers passgetConfig()which already hasllm: LLMConfig. Five test files that constructProvidersConfigmanually were updated to spreadLLMSchema.parse({}).Tier 1 — Dispatch path (behavioral bugs fixed)
providers/connection-resolution.tsproviders/registry.ts(also widensProvidersConfig.llm)daemon/conversation.ts(3 sites)daemon/conversation-agent-loop.tsdaemon/conversation-store.tssubagent/manager.tsTier 2 — Playground / context-window routes
runtime/routes/playground/{state,inject-failures,reset-circuit}.tsTier 3 — Display / diagnostic
daemon/handlers/config-model.tsdaemon/conversation-slash.tsruntime/routes/debug-routes.tsusage/attribution.tsworkspace/provider-commit-message-generator.tsmemory/embedding-backend.tsOut of scope (separate follow-up)
setModelinhandlers/config-model.tsstill writes tollm.defaultviasetLlmDefaultField. Only the read side (no-op detection + provider-change detection) is fixed here. Whether the write should target the active profile is a UX call (managed-profile edge case, null-profile case) — design doc + follow-up PR pending.Also out of scope:
providers/registry.tsollama-init guard now uses the resolved mainAgent provider (strict improvement over the previousllm.default.provider === "ollama"check). Fully scanning all profiles for ollama usage is a bigger change deferred separately.CI status — pre-existing main breakage
Type Check + Test on Assistant Checks are red, but the failures are pre-existing on
mainfrom the OAuth CLI IPC refactor cascade (#30251, #30253, #30255). Same failures reproduce against bareorigin/main(3bdf0ed885) — see run 25649496257.This PR does not introduce new failures:
inference-no-mode-boot-e2e,provider-commit-message-generator,provider-registry-ollama,provider-managed-proxy-integration,secret-routes-managed-proxy,satellite-connection-routing).cli/commands/oauth/*,inference-send,routes,credentials-cli,credential-security-invariants,mcp-auth-routes,usage-cli,guard-tests) — none touched here. Each fails identically against bare main.oauth-cli.test.ts:440 requirePlatformClient) is identical on bare main.Recommend admin-merging once main goes green, or admin-merging now with the standard red-CI acceptance.