Fix Codex lock compiler: move --model flag after exec subcommand#33841
Conversation
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes Codex CLI invocation generation so the optional --model flag is emitted after the exec subcommand (so Codex treats it as a subcommand flag rather than a global flag and doesn’t ignore it).
Changes:
- Reordered Codex command construction (harness and non-harness) to
codex exec …followed by the conditional model expansion. - Updated tests and golden snapshots to match the new command shape.
- Updated the changeset text describing the fix.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/codex_engine.go |
Reorders generated Codex CLI command so exec precedes the conditional --model expansion. |
pkg/workflow/model_env_vars_test.go |
Updates expected model expansion formatting; adds a regression test for exec vs --model ordering. |
pkg/workflow/engine_includes_test.go |
Updates lock-file assertion to match codex exec${...} pattern. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden |
Updates snapshot to reflect corrected Codex command ordering. |
.github/workflows/pr-code-quality-reviewer.lock.yml |
Lock file regenerated; notably drops shared/reporting.md runtime-import. |
.changeset/patch-fix-codex-model-flag.md |
Updates release note describing the Codex model-flag positioning fix. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 3
| // Find the model shell expansion pattern in the generated command | ||
| modelPattern := "${" + constants.EnvVarModelAgentCodex + ":+" | ||
| modelIdx := strings.Index(stepsContent, modelPattern) | ||
| if modelIdx == -1 { | ||
| t.Fatalf("Model expansion pattern '%s' not found in steps:\n%s", modelPattern, stepsContent) | ||
| } | ||
|
|
||
| // Find "exec" in the generated command (look for the last occurrence before the model pattern | ||
| // to handle multi-line step content) | ||
| execIdx := strings.LastIndex(stepsContent[:modelIdx], "exec") | ||
| if execIdx == -1 { | ||
| t.Errorf("'exec' subcommand must appear before the model flag '%s' in the generated command.\n"+ | ||
| "This indicates the model flag is placed before 'exec', causing Codex to ignore it.\n"+ | ||
| "Got:\n%s", modelPattern, stepsContent) |
| --- | ||
|
|
||
| Fixed the Codex engine to pass the model via `--model` instead of the deprecated TOML override so engine model selection works with newer Codex CLI versions. | ||
| Fixed the Codex engine to emit `codex exec --model "$VAR"` instead of `codex "$VAR"exec` so the model flag is correctly positioned after the `exec` subcommand. Previously the model parameter was placed before `exec`, causing Codex CLI to ignore it and silently fall back to the default model. |
| cat "${RUNNER_TEMP}/gh-aw/prompts/github_mcp_tools_with_safeoutputs_prompt.md" | ||
| if [ "$GITHUB_EVENT_NAME" = "issue_comment" ] && [ -n "$GH_AW_IS_PR_COMMENT" ] || [ "$GITHUB_EVENT_NAME" = "pull_request_review_comment" ] || [ "$GITHUB_EVENT_NAME" = "pull_request_review" ]; then | ||
| cat "${RUNNER_TEMP}/gh-aw/prompts/pr_context_prompt.md" | ||
| fi | ||
| cat << 'GH_AW_PROMPT_03f9bc9676534c1b_EOF' | ||
| cat << 'GH_AW_PROMPT_4f5501639950bddf_EOF' | ||
| </system> | ||
| {{#runtime-import .github/workflows/shared/reporting.md}} | ||
| {{#runtime-import .github/workflows/shared/otlp.md}} | ||
| {{#runtime-import .github/workflows/shared/github-guard-policy.md}} | ||
| {{#runtime-import .github/workflows/shared/pr-code-review-config.md}} | ||
| {{#runtime-import .github/workflows/shared/noop-reminder.md}} | ||
| {{#runtime-import .github/workflows/pr-code-quality-reviewer.md}} |
|
``
|
|
@copilot address review feedback and merge main |
…k-compiler-flag-position Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…geset, revert unintended lock file, merge main Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Done. Three changes addressed from the review:
Also merged main (with conflict resolved in the golden snapshot — main had a pre-existing inconsistency in that file, so I regenerated it from the current source). |
|
@copilot lint go |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. The |
The Codex lock compiler was emitting the
--modelflag before theexecsubcommand. Codex CLI treats flags in that position as global options and silently ignores them, causing all custom model selections to fall back to the default.Before (broken):
After (fixed):
Changes
codex_engine.go: Reorder both format strings (harness and non-harness paths) soexecprecedesmodelParam; adjust shell expansion from trailing-space to leading-space form (${VAR:+ --model "$VAR"}) so spacing is correct in both the set and unset cases.model_env_vars_test.go: Update expected pattern to match new format; add regression testTestCodexModelFlagPositionAfterExecthat assertsexecappears before the model expansion in the generated command.engine_includes_test.go: Update assertion from"codex ${"→"codex exec${".testdata/TestWasmGolden_AllEngines/codex.golden: Update snapshot..changeset/patch-fix-codex-model-flag.md: Update description to accurately describe the position fix.pr-sous-chef: requested branch update from run https://github.com/github/gh-aw/actions/runs/26248288802