fix(source-control): override the built-in model when a recipe sets --model#8740
fix(source-control): override the built-in model when a recipe sets --model#8740xianjianlf2 wants to merge 1 commit into
Conversation
…-model Action recipe CLI arguments were appended verbatim after Orca's generated Codex command, so a recipe `--model` produced a duplicate `--model` flag and Codex aborted with "the argument '--model <MODEL>' cannot be used multiple times". Treat `--model`/`-m` as a singleton: a recipe-supplied model now replaces Orca's generated model in place instead of adding a second flag. Closes stablyai#8722
📝 WalkthroughWalkthroughUpdated command-plan argument merging so recipe-provided 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ca163263-b5ce-43ff-97a2-bd5fe7aa3432
📒 Files selected for processing (2)
src/shared/commit-message-plan.test.tssrc/shared/commit-message-plan.ts
| // Why: `--model`/`-m` is a singleton flag — Codex rejects a repeated one with | ||
| // "cannot be used multiple times". When a recipe's CLI arguments set the model, | ||
| // it must override Orca's generated model in place instead of appending a second | ||
| // occurrence. | ||
| const MODEL_FLAGS = new Set(['--model', '-m']) | ||
|
|
||
| function readModelOverride( | ||
| agentArgs: string[] | ||
| ): { value: string; index: number; consumed: number } | null { | ||
| for (let i = 0; i < agentArgs.length; i++) { | ||
| const token = agentArgs[i] | ||
| const equalsMatch = /^(?:--model|-m)=(.*)$/s.exec(token) | ||
| if (equalsMatch) { | ||
| return { value: equalsMatch[1], index: i, consumed: 1 } | ||
| } | ||
| if (MODEL_FLAGS.has(token) && i + 1 < agentArgs.length && !agentArgs[i + 1].startsWith('-')) { | ||
| return { value: agentArgs[i + 1], index: i, consumed: 2 } | ||
| } | ||
| } | ||
| return null | ||
| } | ||
|
|
||
| function applyModelFlagOverride( | ||
| baseArgs: string[], | ||
| agentArgs: string[] | ||
| ): { baseArgs: string[]; agentArgs: string[] } { | ||
| const override = readModelOverride(agentArgs) | ||
| if (!override) { | ||
| return { baseArgs, agentArgs } | ||
| } | ||
| const baseIndex = baseArgs.findIndex( | ||
| (token, i) => | ||
| MODEL_FLAGS.has(token) && i + 1 < baseArgs.length && !baseArgs[i + 1].startsWith('-') | ||
| ) | ||
| if (baseIndex === -1) { | ||
| return { baseArgs, agentArgs } | ||
| } | ||
| const nextBase = [...baseArgs] | ||
| nextBase[baseIndex + 1] = override.value | ||
| const nextAgent = [...agentArgs] | ||
| nextAgent.splice(override.index, override.consumed) | ||
| return { baseArgs: nextBase, agentArgs: nextAgent } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Only the first --model/-m occurrence in agentArgs is consumed.
readModelOverride returns on the first match; if a recipe's agentArgs string happens to contain the model flag twice (e.g., assembled from two argument fragments), the second occurrence is never spliced out and gets appended verbatim by the final merge step in insertAdditionalAgentArgs. That reproduces the exact duplicate---model failure this PR is fixing, just for a slightly different input shape.
🐛 Proposed fix: consume every model-flag occurrence, keeping the last as the effective override
function applyModelFlagOverride(
baseArgs: string[],
agentArgs: string[]
): { baseArgs: string[]; agentArgs: string[] } {
- const override = readModelOverride(agentArgs)
+ let remainingAgentArgs = agentArgs
+ let override: { value: string; index: number; consumed: number } | null = null
+ let found = readModelOverride(remainingAgentArgs)
+ while (found) {
+ override = found
+ remainingAgentArgs = [
+ ...remainingAgentArgs.slice(0, found.index),
+ ...remainingAgentArgs.slice(found.index + found.consumed)
+ ]
+ found = readModelOverride(remainingAgentArgs)
+ }
if (!override) {
- return { baseArgs, agentArgs }
+ return { baseArgs, agentArgs: remainingAgentArgs }
}
const baseIndex = baseArgs.findIndex(
(token, i) =>
MODEL_FLAGS.has(token) && i + 1 < baseArgs.length && !baseArgs[i + 1].startsWith('-')
)
if (baseIndex === -1) {
- return { baseArgs, agentArgs }
+ return { baseArgs, agentArgs: remainingAgentArgs }
}
const nextBase = [...baseArgs]
nextBase[baseIndex + 1] = override.value
- const nextAgent = [...agentArgs]
- nextAgent.splice(override.index, override.consumed)
- return { baseArgs: nextBase, agentArgs: nextAgent }
+ return { baseArgs: nextBase, agentArgs: remainingAgentArgs }
}🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 82-82: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
|
Thank you for jumping on #8722, @xianjianlf2. I compared this after first opening independent PR #8773. I selected #8773 as the integration path because it is on current |
2 participants
What & why
Action Recipe "CLI arguments" were appended verbatim after Orca's generated agent command. Every agent spec's
buildArgsalready emits--model <model>, so a recipe that also sets--modelproduced a duplicate--modelflag and Codex aborted:This made per-recipe model overrides impossible (#8722).
Fix
Treat
--model/-mas a singleton ininsertAdditionalAgentArgs(src/shared/commit-message-plan.ts): when a recipe supplies a model, it replaces Orca's generated model value in place (preserving argument position, so a trailing-c model_reasoning_effort=…stays intact) instead of adding a second flag. Repeatable flags like-care left untouched and still append. When the base args contain no--model(e.g. custom-command agents), behavior is unchanged. This implements the issue author's recommended solution #1.Tests
src/shared/commit-message-plan.test.ts:--modeloverride (Codex + stdin agents, and the-malias) and a repeatable--c-flag non-override case (proves only singletons are deduplicated).--modeloutput.vitest run … src/shared/commit-message-plan.test.ts→ 16 passed. TDD red→green verified (the duplicate---modelreproduction fails before the fix, passes after).Cross-platform / SSH / git providers
Pure argv construction — no platform, path, SSH, or git-provider assumptions. Provider-neutral across all agent specs.
AI coding agent review summary
Change is localized to commit-message command assembly. Verified TDD red→green, confirmed repeatable flags (
-c) still append while only--model/-mis deduplicated, and that agents without a base--modelare unaffected.X: @mark86202384100
Closes #8722