From d617735bedeb9853627118af0e89aebe03dc5bfe Mon Sep 17 00:00:00 2001 From: radioflyer28 <9313101+radioflyer28@users.noreply.github.com> Date: Wed, 10 Jun 2026 11:41:01 -0400 Subject: [PATCH] fix(codex): avoid partial model effort pinning (#842) Co-authored-by: Andrew Kriz Co-authored-by: Tom Boucher --- ...38-codex-agent-model-effort-consistency.md | 5 +++ bin/install.js | 15 ++++++-- tests/codex-config.test.cjs | 29 ++++++++++++++++ ...443-effort-install-wiring.install.test.cjs | 34 +++++++++++++------ 4 files changed, 69 insertions(+), 14 deletions(-) create mode 100644 .changeset/838-codex-agent-model-effort-consistency.md diff --git a/.changeset/838-codex-agent-model-effort-consistency.md b/.changeset/838-codex-agent-model-effort-consistency.md new file mode 100644 index 000000000..d9d962f62 --- /dev/null +++ b/.changeset/838-codex-agent-model-effort-consistency.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 842 +--- +Codex agent TOML generation no longer pins `model_reasoning_effort` when the agent is intentionally inheriting the active Codex chat model. GSD still emits both `model` and `model_reasoning_effort` when a per-agent model override or `runtime: "codex"` resolver pins the model, avoiding the confusing partial state where the model followed Codex UI selection while effort followed GSD catalog defaults. (#838) diff --git a/bin/install.js b/bin/install.js index ea212a84f..a335fab69 100755 --- a/bin/install.js +++ b/bin/install.js @@ -3620,14 +3620,17 @@ function generateCodexAgentToml(agentName, agentContent, modelOverrides = null, // Task() model parameters). See #2256. // Precedence: per-agent model_overrides > runtime-aware tier resolution (#2517). const modelOverride = modelOverrides?.[resolvedName] || modelOverrides?.[agentName]; + let hasPinnedModel = false; if (modelOverride) { lines.push(`model = ${JSON.stringify(modelOverride)}`); + hasPinnedModel = true; } else if (runtimeResolver) { // #2517 — runtime-aware tier resolution. Embeds Codex-native model + reasoning_effort // from RUNTIME_PROFILE_MAP / model_profile_overrides for the configured tier. const entry = runtimeResolver.resolve(resolvedName) || runtimeResolver.resolve(agentName); if (entry?.model) { lines.push(`model = ${JSON.stringify(entry.model)}`); + hasPinnedModel = true; // model is resolved here; reasoning_effort from catalog tier is REPLACED by the // unified effort resolver below (#443). Do NOT emit entry.reasoning_effort here. } @@ -3638,9 +3641,15 @@ function generateCodexAgentToml(agentName, agentContent, modelOverrides = null, // from the same effort.agent_overrides / effort.routing_tier_defaults / effort.default // config source. Codex does not support 'max' → clamped to 'xhigh' by // gsdRenderEffortForRuntime('codex', ...). - const _universalEffortCodex = resolveInstallTimeEffort(effortCfg, resolvedName !== agentName ? resolvedName : agentName); - const _renderedEffortCodex = _getGsdEffortCatalog().renderEffortForRuntime('codex', _universalEffortCodex).value; - lines.push(`model_reasoning_effort = ${JSON.stringify(_renderedEffortCodex)}`); + // #838 — Do not pin effort when Codex is intentionally inheriting the parent + // chat model. A TOML with no `model` but a static `model_reasoning_effort` + // creates confusing partial routing: model follows the Codex UI while effort + // follows GSD. Keep those knobs coupled unless GSD also pins the model. + if (hasPinnedModel) { + const _universalEffortCodex = resolveInstallTimeEffort(effortCfg, resolvedName !== agentName ? resolvedName : agentName); + const _renderedEffortCodex = _getGsdEffortCatalog().renderEffortForRuntime('codex', _universalEffortCodex).value; + lines.push(`model_reasoning_effort = ${JSON.stringify(_renderedEffortCodex)}`); + } // #774 — Emit service_tier and model_verbosity for light-tier agents. // Light-tier agents (routingTier: "light" in model-catalog.json) are haiku-equivalent diff --git a/tests/codex-config.test.cjs b/tests/codex-config.test.cjs index 3e1e955fe..e80390411 100644 --- a/tests/codex-config.test.cjs +++ b/tests/codex-config.test.cjs @@ -389,6 +389,35 @@ tools: Read, Grep, Glob assert.ok(!result.includes('model ='), 'model field must be absent when no override'); }); + test('does not emit reasoning effort when Codex model is inherited (#838)', () => { + const result = generateCodexAgentToml('gsd-executor', sampleAgent, null); + assert.ok(!result.includes('model ='), 'model field must be absent when Codex should inherit'); + assert.ok( + !result.includes('model_reasoning_effort ='), + 'reasoning effort must stay absent when the model is inherited' + ); + }); + + test('emits reasoning effort when model override pins Codex model (#838)', () => { + const overrides = { 'gsd-executor': 'gpt-5.3-codex' }; + const result = generateCodexAgentToml('gsd-executor', sampleAgent, overrides); + assert.ok(result.includes('model = "gpt-5.3-codex"'), 'model override must pin model'); + assert.ok( + result.includes('model_reasoning_effort ='), + 'reasoning effort is safe to emit when GSD also pins model' + ); + }); + + test('emits reasoning effort when runtime resolver pins Codex model (#838)', () => { + const runtimeResolver = { resolve: () => ({ model: 'gpt-5.5' }) }; + const result = generateCodexAgentToml('gsd-executor', sampleAgent, null, runtimeResolver); + assert.ok(result.includes('model = "gpt-5.5"'), 'runtime resolver must pin model'); + assert.ok( + result.includes('model_reasoning_effort ='), + 'reasoning effort is safe to emit when runtime resolver pins model' + ); + }); + test('does not emit model field when modelOverrides has no entry for this agent (#2256)', () => { const overrides = { 'gsd-planner': 'gpt-5.4' }; const result = generateCodexAgentToml('gsd-executor', sampleAgent, overrides); diff --git a/tests/feat-443-effort-install-wiring.install.test.cjs b/tests/feat-443-effort-install-wiring.install.test.cjs index 4eb9d025f..1dfac84ef 100644 --- a/tests/feat-443-effort-install-wiring.install.test.cjs +++ b/tests/feat-443-effort-install-wiring.install.test.cjs @@ -9,10 +9,10 @@ * Verifies: * 1. Claude global install injects `effort:` into agent .md frontmatter. * 2. Gemini global install does NOT inject `effort:` (Gemini-safe .md). - * 3. Codex global install emits `model_reasoning_effort` in .toml via the - * unified resolver (not the old catalog-static path). + * 3. Codex inherited-model installs omit `model_reasoning_effort` so model + * and effort are not partially pinned (#838). * 4. Config-driven proof: effort.agent_overrides wins over tier defaults - * for both Claude .md and Codex .toml. + * for Claude .md and for Codex .toml when runtime:"codex" pins a model. * 5. Source agents/gsd-planner.md has NO effort: key (injection is * install-only, source stays Gemini-safe). */ @@ -189,9 +189,9 @@ describe('#443 Gemini install: effort: absent (Gemini-safe)', () => { }); }); -// ─── describe 3: Codex install emits model_reasoning_effort in .toml ───────── +// ─── describe 3: Codex inherited-model install omits model_reasoning_effort ── -describe('#443 Codex install: model_reasoning_effort in .toml (unified resolver)', () => { +describe('#838 Codex install: inherited model omits model_reasoning_effort', () => { let tmpDir; let codexHome; @@ -205,13 +205,15 @@ describe('#443 Codex install: model_reasoning_effort in .toml (unified resolver) cleanup(tmpDir); }); - test('gsd-planner.toml contains model_reasoning_effort = "xhigh" (heavy tier)', () => { + test('gsd-planner.toml omits both model and model_reasoning_effort when model is inherited', () => { runGlobalInstall('codex', codexHome); const tomlContent = fs.readFileSync( path.join(codexHome, 'agents', 'gsd-planner.toml'), 'utf8' ); - assert.match(tomlContent, /^model_reasoning_effort\s*=\s*"xhigh"$/m, - `gsd-planner.toml should have model_reasoning_effort = "xhigh"\nActual:\n${tomlContent.slice(0, 500)}`); + assert.doesNotMatch(tomlContent, /^model\s*=/m, + `gsd-planner.toml should omit model when inheriting Codex chat model\nActual:\n${tomlContent.slice(0, 500)}`); + assert.doesNotMatch(tomlContent, /^model_reasoning_effort\s*=/m, + `gsd-planner.toml should omit model_reasoning_effort when model is inherited\nActual:\n${tomlContent.slice(0, 500)}`); }); }); @@ -241,8 +243,11 @@ describe('#443 Config-driven: effort.agent_overrides drives install-time effort' fs.mkdirSync(codexHome, { recursive: true }); fs.mkdirSync(path.join(projectDir, '.planning'), { recursive: true }); - // Write a project config with effort.agent_overrides overriding gsd-planner to 'low' + // Write a project config with effort.agent_overrides overriding gsd-planner to 'low'. + // runtime:"codex" pins a Codex-native model, so emitting model_reasoning_effort + // remains valid under the #838 model/effort coupling rule. const config = { + runtime: 'codex', effort: { agent_overrides: { 'gsd-planner': 'low', @@ -273,6 +278,8 @@ describe('#443 Config-driven: effort.agent_overrides drives install-time effort' const tomlContent = fs.readFileSync( path.join(codexHome, 'agents', 'gsd-planner.toml'), 'utf8' ); + assert.match(tomlContent, /^model\s*=\s*"gpt-5.5"$/m, + `gsd-planner.toml should pin Codex model when runtime:"codex" is configured\nActual:\n${tomlContent.slice(0, 500)}`); assert.match(tomlContent, /^model_reasoning_effort\s*=\s*"low"$/m, `gsd-planner.toml should have model_reasoning_effort = "low" from config override\nActual:\n${tomlContent.slice(0, 500)}`); }); @@ -281,6 +288,7 @@ describe('#443 Config-driven: effort.agent_overrides drives install-time effort' const projectDir = path.dirname(codexHome); // Overwrite config with max override const config = { + runtime: 'codex', effort: { agent_overrides: { 'gsd-planner': 'max', @@ -296,6 +304,8 @@ describe('#443 Config-driven: effort.agent_overrides drives install-time effort' const tomlContent = fs.readFileSync( path.join(codexHome, 'agents', 'gsd-planner.toml'), 'utf8' ); + assert.match(tomlContent, /^model\s*=\s*"gpt-5.5"$/m, + `gsd-planner.toml should pin Codex model when runtime:"codex" is configured\nActual:\n${tomlContent.slice(0, 500)}`); // Codex does not support 'max' → clamped to 'xhigh' assert.match(tomlContent, /^model_reasoning_effort\s*=\s*"xhigh"$/m, `gsd-planner.toml should clamp max → xhigh for Codex\nActual:\n${tomlContent.slice(0, 500)}`); @@ -372,13 +382,15 @@ describe('#443 resolveInstallTimeEffort: invalid tokens fall through to valid ef // "medium" is valid, so it should appear (or tier default if medium is invalid, but medium is valid) }); - test('effort.default="ultra" (invalid) -> Codex .toml model_reasoning_effort is VALID', () => { + test('effort.default="ultra" (invalid) + runtime:"codex" -> Codex .toml model_reasoning_effort is VALID', () => { // BUG before fix: "ultra" written into .toml verbatim - writeProjectConfig({ effort: { default: 'ultra' } }); + writeProjectConfig({ runtime: 'codex', effort: { default: 'ultra' } }); runGlobalInstall('codex', codexHome); const tomlContent = fs.readFileSync( path.join(codexHome, 'agents', 'gsd-planner.toml'), 'utf8' ); + assert.match(tomlContent, /^model\s*=\s*"gpt-5.5"$/m, + `gsd-planner.toml should pin Codex model when runtime:"codex" is configured\nActual:\n${tomlContent.slice(0, 500)}`); const match = tomlContent.match(/^model_reasoning_effort\s*=\s*"([^"]+)"/m); assert.ok(match, `model_reasoning_effort must be present in .toml\nActual:\n${tomlContent.slice(0, 500)}`); assert.ok(VALID_EFFORTS.includes(match[1]),