fix(delegate-task): honor user model override in category-resolver cold cache (fixes #2712)#2892
Conversation
|
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: 5d5eb46f19
ℹ️ 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".
| const userModelOverride = explicitCategoryModel ?? overrideModel | ||
| if (userModelOverride) { |
There was a problem hiding this comment.
This new block never executes for real overrides: resolveModelForDelegateTask() returns { model: userModel } before it can return { skipped: true }, so when explicitCategoryModel ?? overrideModel is non-empty, control cannot reach this "skipped" branch. That means the intended cold-cache override fix is effectively a no-op for valid user overrides, and the behavior you intended to change remains unchanged.
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 1 file
Confidence score: 3/5
- There is a concrete medium-severity risk (6/10, high confidence 9/10) in
src/tools/delegate-task/category-resolver.ts, so this is not a no-risk merge. - The cold-cache override path can accept an invalid
provider/modelformat, which may leaveactualModelinconsistent whilecategoryModelis undefined instead of failing fast. - Pay close attention to
src/tools/delegate-task/category-resolver.ts- cold-cache override should validateprovider/modelinput to prevent inconsistent model resolution.
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/tools/delegate-task/category-resolver.ts">
<violation number="1" location="src/tools/delegate-task/category-resolver.ts:139">
P2: Cold-cache override path does not validate invalid `provider/model` format, allowing inconsistent `actualModel` + undefined `categoryModel` to pass without error.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const userModelOverride = explicitCategoryModel ?? overrideModel | ||
| if (userModelOverride) { | ||
| actualModel = userModelOverride | ||
| const parsedModel = parseModelString(actualModel) |
There was a problem hiding this comment.
P2: Cold-cache override path does not validate invalid provider/model format, allowing inconsistent actualModel + undefined categoryModel to pass without error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tools/delegate-task/category-resolver.ts, line 139:
<comment>Cold-cache override path does not validate invalid `provider/model` format, allowing inconsistent `actualModel` + undefined `categoryModel` to pass without error.</comment>
<file context>
@@ -133,6 +133,16 @@ Available categories: ${allCategoryNames}`,
+ const userModelOverride = explicitCategoryModel ?? overrideModel
+ if (userModelOverride) {
+ actualModel = userModelOverride
+ const parsedModel = parseModelString(actualModel)
+ const variantToUse = userCategories?.[args.category!]?.variant ?? resolved.config.variant
+ categoryModel = parsedModel
</file context>
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
Correct fix for the category-resolver cold-cache gap. The partial fix in subagent-resolver.ts left category-resolver.ts unhandled — when resolution.skipped === true, the user model override should still apply. The logic mirrors what subagent-resolver already does. LGTM.
2 participants
Summary
Problem
The partial fix in commit a8ec927 addressed the cold-cache model override issue in
subagent-resolver.tsbut not incategory-resolver.ts. When usingcategoryparameter (e.g.,task(category="quick")), the model resolution goes throughcategory-resolver.tswhich still ignores the user override whenresolveModelForDelegateTask()returns{ skipped: true }(cold cache).This causes:
categoryModel = undefinedFix
Added cold-cache user override handling in
category-resolver.tsthat mirrors the existing fix insubagent-resolver.ts. When resolution is skipped (cold cache) but the user has explicitly configured a model (explicitCategoryModeloroverrideModel), the user override is applied directly instead of falling through.Changes
src/tools/delegate-task/category-resolver.ts{ skipped: true }Fixes #2712
Summary by cubic
Honor user model overrides in
category-resolverwhen the provider cache is cold. If resolution is skipped, we apply the explicit or override model (with the right variant), matchingsubagent-resolverand preventing wrong model selection on first run (fixes #2712).Written for commit 5d5eb46. Summary will update on new commits.