fix(config): respect user model selection in web mode instead of reverting to plugin defaults (fixes #2351)#2670
Conversation
…rting to plugin defaults (fixes code-yeongyu#2351)
|
I have read the CLA Document and I hereby sign the CLA |
![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: 9737b6c2b0
ℹ️ 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".
| result[targetGeneratedKey] = { | ||
| ...generatedRecord, | ||
| model: runtimeModel, | ||
| ...(typeof incomingConfig.variant === "string" ? { variant: incomingConfig.variant } : {}), | ||
| ...(typeof incomingConfig.thinking === "string" |
There was a problem hiding this comment.
When web mode only persists agent.<builtin>.model, this helper mutates the already-generated builtin config instead of regenerating it. For primary agents, the prompt and provider-specific settings are derived from the model (createSisyphusAgent switches between GPT reasoningEffort and Claude thinking, and Atlas picks a different prompt family based on the model), so changing only model here can leave a GPT model paired with the old Claude prompt/settings or vice versa. That means the new “respect the selected model” path still produces the wrong agent behavior for any model switch that crosses those prompt/settings boundaries.
Useful? React with 👍 / 👎.
| ...generatedAgentConfig, | ||
| ...filterDisabledAgents(filteredUserAgents), | ||
| ...filterDisabledAgents(filteredProjectAgents), | ||
| ...filterDisabledAgents(filteredPluginAgents), |
There was a problem hiding this comment.
This still breaks the display-name case that the new test covers. applyRuntimeSelectedModels updates the generated builtin for keys like "Sisyphus (Ultraworker)", but filteredConfigAgents is merged immediately afterwards and still contains that display-name entry because the earlier filter only excludes raw builtin keys. Once remapAgentKeysToDisplayNames() runs, that later stub overwrites the fully generated builtin config, so the final agent can be reduced to just the runtime fields from config.agent and lose its prompt/mode.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Confidence score: 3/5
- There is a concrete regression risk in
src/plugin-handlers/agent-config-handler.ts: patching onlymodel/variant/thinkingover an existing builtin config may preserve stale provider-specific fields when switching model families (for example, Claude to GPT). - Given the medium severity (6/10) and fairly solid confidence (7/10), this is more than a cosmetic issue and could cause incorrect runtime behavior from mismatched config state.
- Pay close attention to
src/plugin-handlers/agent-config-handler.ts- ensure provider-specific settings are fully refreshed/cleared when model family changes.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/plugin-handlers/agent-config-handler.ts">
<violation number="1" location="src/plugin-handlers/agent-config-handler.ts:62">
P2: Patching only `model`/`variant`/`thinking` on top of the already-generated builtin config can leave stale provider-specific settings when the user switches across model families (e.g., Claude → GPT). The generated record's prompt, `reasoningEffort`, `thinking` block config, and other model-dependent fields were derived from the *original* model and remain unchanged. Consider regenerating the full builtin agent config with the new model (calling the appropriate `create*Agent` function) rather than shallow-patching the model string onto the old config.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| if (generatedModel === runtimeModel) continue; | ||
|
|
||
| result[targetGeneratedKey] = { |
There was a problem hiding this comment.
P2: Patching only model/variant/thinking on top of the already-generated builtin config can leave stale provider-specific settings when the user switches across model families (e.g., Claude → GPT). The generated record's prompt, reasoningEffort, thinking block config, and other model-dependent fields were derived from the original model and remain unchanged. Consider regenerating the full builtin agent config with the new model (calling the appropriate create*Agent function) rather than shallow-patching the model string onto the old config.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/plugin-handlers/agent-config-handler.ts, line 62:
<comment>Patching only `model`/`variant`/`thinking` on top of the already-generated builtin config can leave stale provider-specific settings when the user switches across model families (e.g., Claude → GPT). The generated record's prompt, `reasoningEffort`, `thinking` block config, and other model-dependent fields were derived from the *original* model and remain unchanged. Consider regenerating the full builtin agent config with the new model (calling the appropriate `create*Agent` function) rather than shallow-patching the model string onto the old config.</comment>
<file context>
@@ -27,6 +27,51 @@ type AgentConfigRecord = Record<string, Record<string, unknown> | undefined> & {
+
+ if (generatedModel === runtimeModel) continue;
+
+ result[targetGeneratedKey] = {
+ ...generatedRecord,
+ model: runtimeModel,
</file context>
|
Re: P2 identified by cubic — stale provider-specific fields when switching model families. Acknowledged as a known limitation. The current fix targets the specific issue reported in #2351: user-selected model reverting to plugin defaults after each message in web mode. Full config regeneration on model family switch (e.g., Claude -> GPT) would require detecting the model family change and calling the appropriate For now, the shallow patch preserves the user's runtime model selection without introducing cross-cutting changes to the agent config pipeline. |
|
Superseded by #2788 which fixes both root causes. |
2 participants
Summary
Problem
When oh-my-opencode is enabled with agent-specific models, the web UI model selection reverts to the plugin-configured default after each message. The config merge flow dropped managed builtin agent entries from the incoming runtime config, so rebuilt agent configs overwrote a user-selected model.
Fix
Updated managed agent config merging to preserve runtime-selected model overrides for builtin agents when the incoming runtime model differs from the plugin-generated default. This keeps explicit user model choices in web mode while still allowing normal plugin-managed agent generation.
Changes
src/plugin-handlers/agent-config-handler.tsmodel/variant/thinkingvalues for managed builtin agents during agent config rebuildsrc/plugin-handlers/agent-config-handler.test.tsFixes #2351
Summary by cubic
Keep the user's chosen model in web mode for builtin agents instead of reverting to plugin defaults after each message. Fixes #2351 by preserving runtime overrides for
model,variant, andthinkingduring agent config rebuild.Written for commit 9737b6c. Summary will update on new commits.