From dc736805325b93f97f30883e19ab41bda00b4cc0 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 31 Jul 2026 00:47:55 -0400 Subject: [PATCH] fix(#2834): write defaults.json before agent TOML generation on clean Codex install (#2900) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2834): write defaults.json before agent TOML generation on clean Codex install Extracted writeNonClaudeDefaults(runtime) and called it BEFORE installCodexConfig so the runtime-aware model resolver has resolve_model_ids=omit + runtime=codex in ~/.gsd/defaults.json before agent TOMLs are generated. Pre-fix, a clean first Codex install generated TOMLs with no model fields (the resolver didn't know the runtime); a second run fixed it. The original inline defaults-write block (which ran AFTER agent generation) is replaced by the earlier function call (idempotent). * chore(#2834): changeset fragment * fix+test(#2834): acknowledge codex TOML drift (emitted-attribution) + fix test comment window The emitted-attribution gate flags 19 codex agent TOMLs that now carry model-routing fields (the fix's correct effect) but can't link them to a .md or src/ change (the fix is in bin/install.js ordering). Acknowledge the drift in emitted-drift-ack.json. Fix the test's comment-detection window (300 chars to capture the #2834 rationale). * fix(#2834): ack remaining 15 codex TOML drift paths * chore(#2834): backfill changeset PR number (2900) * chore(#2834): ack code-review.md growth from concurrent merge (rebase pickup) * fix(#2834): remove stale code-review.md ack (emitted-attribution failure) CI failed: 'differential attribution over the real tree' — the code-review.md ack added in 6e0b4b3b8 ('ack code-review.md growth from concurrent merge') is STALE: this PR's diff does not touch code-review.md (only bin/install.js + tests + changeset), so the ack explains growth that isn't here. The base already absorbed the concurrent code-review.md growth; the ack is inert here and the gate flags it as stale. Remove it. --------- Co-authored-by: Test (cherry picked from commit 00c859fae5c9be13f13719c5fd4aea86fe71591d) --- .changeset/graceful-lynx-sing.md | 5 + bin/install.js | 102 ++++++++-------- tests/emitted-drift-ack.json | 113 ++++++++++++++++-- ...2834-codex-install-model-ordering.test.cjs | 43 +++++++ 4 files changed, 204 insertions(+), 59 deletions(-) create mode 100644 .changeset/graceful-lynx-sing.md create mode 100644 tests/issue-2834-codex-install-model-ordering.test.cjs diff --git a/.changeset/graceful-lynx-sing.md b/.changeset/graceful-lynx-sing.md new file mode 100644 index 000000000..ea340f3db --- /dev/null +++ b/.changeset/graceful-lynx-sing.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2900 +--- +**A clean Codex install now applies balanced model settings to agent TOMLs on the first run** — `~/.gsd/defaults.json` (`resolve_model_ids` + `runtime`) is now written before agent TOML generation, so the runtime-aware model resolver knows the target runtime during the first pass. Previously a second install was required. (#2834) diff --git a/bin/install.js b/bin/install.js index d942836cb..bc2ad0085 100755 --- a/bin/install.js +++ b/bin/install.js @@ -6983,6 +6983,47 @@ function writeCopilotHookConfig(targetDir) { * Generate config.toml and per-agent .toml files for Codex. * Reads agent .md files from source, extracts metadata, writes .toml configs. */ + +/** + * #2834: Write ~/.gsd/defaults.json for non-Claude runtimes — sets + * resolve_model_ids="omit" (so resolveModelInternal() returns '' instead of + * Claude aliases the runtime can't resolve) and runtime= (so + * resolveRuntime() resolves correctly out of the box). MUST be called BEFORE + * installCodexConfig (or any other step that reads defaults.json at generation + * time), so a clean first install produces correctly-model-routed agent TOMLs. + * No-op for Claude runtimes (Claude is the resolveRuntime fallback + has native + * model aliases). Preserves an explicit `true` opt-in and existing values. + */ +function writeNonClaudeDefaults(runtime) { + if (_hostBehaviors(runtime).nativeModelAliases || process.env.GSD_TEST_MODE) return; + const gsdDir = path.join(os.homedir(), '.gsd'); + const defaultsPath = path.join(gsdDir, 'defaults.json'); + try { + fs.mkdirSync(gsdDir, { recursive: true }); + let defaults = {}; + try { defaults = JSON.parse(fs.readFileSync(defaultsPath, 'utf8')); } catch { /* new file */ } + if (defaults === null || typeof defaults !== 'object' || Array.isArray(defaults)) { + defaults = {}; + } + // Three-valued domain: false/absent → aliases; true → full IDs; "omit" → ''. + const existing = defaults.resolve_model_ids; + const shouldDefaultToOmit = existing !== true && existing !== 'omit'; + if (shouldDefaultToOmit) { + defaults.resolve_model_ids = 'omit'; + fs.writeFileSync(defaultsPath, JSON.stringify(defaults, null, 2) + '\n'); + console.log(` ${green}✓${reset} Set resolve_model_ids: "omit" in ~/.gsd/defaults.json`); + } + // #2395: persist runtime for non-Claude runtimes. + if (defaults.runtime === undefined || defaults.runtime === null || defaults.runtime === '') { + defaults.runtime = runtime; + fs.writeFileSync(defaultsPath, JSON.stringify(defaults, null, 2) + '\n'); + console.log(` ${green}✓${reset} Set runtime: "${runtime}" in ~/.gsd/defaults.json`); + } + } catch (e) { + console.log(` ${yellow}⚠${reset} Could not write ~/.gsd/defaults.json: ${e.message}`); + } +} + function installCodexConfig(targetDir, agentsSrc, sandboxTier = 'codex-agent-sandbox') { // ADR-1239 Phase B write-confinement: every Codex config write stays under targetDir. const configPath = assertDestWithinConfigHome(targetDir, 'config.toml'); @@ -11586,6 +11627,10 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { let agentCount = 0; if (!isMinimalMode(_effectiveInstallMode)) { + // #2834: write ~/.gsd/defaults.json (resolve_model_ids + runtime) BEFORE generating + // agent TOMLs — installCodexConfig reads defaults.json at generation time, so on a + // clean first install the runtime-aware model resolver must already know the runtime. + writeNonClaudeDefaults(runtime); try { // Generate Codex config.toml and per-agent .toml files. agentCount = installCodexConfig(targetDir, agentsSrc, plan.sandboxTier); @@ -12361,58 +12406,11 @@ function finishInstall(settingsPath, settings, statuslineCommand, shouldInstallS configureAntigravityMcpConfig(isGlobal, configDir); } - // For non-Claude runtimes, DEFAULT resolve_model_ids to "omit" in ~/.gsd/defaults.json - // when it is absent or falsy, so resolveModelInternal() returns '' instead of Claude - // aliases (opus/sonnet/haiku) the runtime can't resolve. An explicit `true` opt-in - // (resolveModelInternal returns full materialized model IDs) MUST be preserved — - // rewriting it to "omit" would make generated agent manifests inherit the active - // chat model instead of pinning the resolved model. See #1156 (default-to-omit - // intent) and #1569 (preserve explicit true). Guard matches the #130-class pattern - // on configureOpencodePermissions above. - if (!_hostBehaviors(runtime).nativeModelAliases && !process.env.GSD_TEST_MODE) { - const gsdDir = path.join(os.homedir(), '.gsd'); - const defaultsPath = path.join(gsdDir, 'defaults.json'); - try { - fs.mkdirSync(gsdDir, { recursive: true }); - let defaults = {}; - try { defaults = JSON.parse(fs.readFileSync(defaultsPath, 'utf8')); } catch { /* new file */ } - // Recover a malformed (valid-JSON-but-non-object) defaults.json to a fresh object so - // the write below succeeds and the file is no longer broken. Without this, `null` / - // `[]` / a number / a string bypass the parse catch and either throw a TypeError on - // property access (swallowed by the outer try/catch, leaving the file broken) or get - // a property set that won't round-trip through JSON.stringify. (#1657) - if (defaults === null || typeof defaults !== 'object' || Array.isArray(defaults)) { - defaults = {}; - } - // Three-valued domain: false/absent → aliases; true → full IDs; "omit" → ''. - // Honor ONLY an explicit canonical `true` opt-in (full model IDs) and an existing - // "omit"; default everything else — absent, falsy, OR any non-canonical value — to - // "omit", the safe non-Claude default. Allowlist-based so malformed values - // (0, "", "yes", {}, …) don't leak Claude aliases the runtime can't resolve (#1569). - const existing = defaults.resolve_model_ids; - const shouldDefaultToOmit = existing !== true && existing !== 'omit'; - if (shouldDefaultToOmit) { - defaults.resolve_model_ids = 'omit'; - fs.writeFileSync(defaultsPath, JSON.stringify(defaults, null, 2) + '\n'); - console.log(` ${green}✓${reset} Set resolve_model_ids: "omit" in ~/.gsd/defaults.json`); - } - - // #2395: also persist `runtime: ` for non-Claude runtimes, so - // resolveRuntime() (precedence: GSD_RUNTIME env > config.runtime > 'claude') - // resolves to the install's actual runtime identity out of the box — without - // this, agent_runtime and every runtime-branded slash hint falls through to - // the hard-coded 'claude' default. Mirrors the resolve_model_ids write above: - // honor an explicit pre-existing value (any string), only default-populating - // when absent. Claude is the resolveRuntime() fallback, so it needs no write. - if (defaults.runtime === undefined || defaults.runtime === null || defaults.runtime === '') { - defaults.runtime = runtime; - fs.writeFileSync(defaultsPath, JSON.stringify(defaults, null, 2) + '\n'); - console.log(` ${green}✓${reset} Set runtime: "${runtime}" in ~/.gsd/defaults.json`); - } - } catch (e) { - console.log(` ${yellow}⚠${reset} Could not write ~/.gsd/defaults.json: ${e.message}`); - } - } + // #2834: defaults.json (resolve_model_ids + runtime) is now written BEFORE + // installCodexConfig via writeNonClaudeDefaults(runtime) — extracted into a + // function so it can run at the right point in the flow (before agent TOML + // generation reads it). This call is idempotent (preserves existing values). + writeNonClaudeDefaults(runtime); // program + command are now single-source lookups (ADR-1239 Phase B / #1679): // program is the runtime display label; command is the per-host /gsd-new-project diff --git a/tests/emitted-drift-ack.json b/tests/emitted-drift-ack.json index ede87875f..5ca42ea3a 100644 --- a/tests/emitted-drift-ack.json +++ b/tests/emitted-drift-ack.json @@ -2,19 +2,118 @@ "version": 1, "paths": { "plan-phase.md": { - "reason": "#2770: decision-coverage gate recomputes CONTEXT_PATH in-block + guards the empty-glob case (handler now fails closed on empty arg). Net growth kept under the ADR-857 size cap by condensing adjacent §13a prose/JSON; the residual +89 bytes are the irreducible glob+guard logic." + "reason": "#2770: decision-coverage gate recomputes CONTEXT_PATH in-block + guards the empty-glob case (handler now fails closed on empty arg). Net growth kept under the ADR-857 size cap by condensing adjacent \u00a713a prose/JSON; the residual +89 bytes are the irreducible glob+guard logic." }, "discuss-phase-assumptions.md": { - "reason": "#2772: re-synced to the parent canonical block (had drifted — lost the 'Other' empty-text branch) + fixed the auto_advance→confirm_creation circularity (end the workflow). discuss-phase.md is net -11 (condensed); auto.md shrank -4650 (removed a dead MAX_PASSES resolver shim)." + "reason": "#2772: re-synced to the parent canonical block (had drifted \u2014 lost the 'Other' empty-text branch) + fixed the auto_advance\u2192confirm_creation circularity (end the workflow). discuss-phase.md is net -11 (condensed); auto.md shrank -4650 (removed a dead MAX_PASSES resolver shim)." }, "autonomous.md": { - "reason": "#2800 — the hand-enumerated reviewer-flag list is replaced by a loop over `gsd_run review-lane flags`, and the whole CONVERGENCE_ARGS construction is relocated below the runtime-launcher preamble because it now calls gsd_run and each bash fence is its own shell. Growth is the explanatory comments carrying that constraint plus the loop body, against nine deleted flag literals. Deliberate: the byte delta is the cost of the flags no longer being hand-maintained in three places that had drifted apart." + "reason": "#2800 \u2014 the hand-enumerated reviewer-flag list is replaced by a loop over `gsd_run review-lane flags`, and the whole CONVERGENCE_ARGS construction is relocated below the runtime-launcher preamble because it now calls gsd_run and each bash fence is its own shell. Growth is the explanatory comments carrying that constraint plus the loop body, against nine deleted flag literals. Deliberate: the byte delta is the cost of the flags no longer being hand-maintained in three places that had drifted apart." }, "next.md": { - "reason": "#2800 — same derived-flag-loop change as autonomous.md. next.md needed no relocation (its launcher preamble already precedes the loop), so the growth here is only the loop plus the comment recording that --all and --text stay literal because they are convergence controls, not reviewer lanes." + "reason": "#2800 \u2014 same derived-flag-loop change as autonomous.md. next.md needed no relocation (its launcher preamble already precedes the loop), so the growth here is only the loop plus the comment recording that --all and --text stay literal because they are convergence controls, not reviewer lanes." }, - "code-review.md": { - "reason": "#2667: the fallow structural pre-pass failure WARNING now names the failure KIND (timeout / spawn failure / crash / not-found) via a case statement, so a Windows .cmd spawn failure (CVE-2024-27980) is not mistaken for an absent binary. Growth is the case statement + the explanatory prose." + "agents/gsd-advisor-researcher.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-ai-researcher.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-assumptions-analyzer.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-code-fixer.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-code-reviewer.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-codebase-mapper.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-debug-session-manager.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-debugger.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-doc-classifier.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-doc-synthesizer.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-doc-verifier.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-doc-writer.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-domain-researcher.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-eval-auditor.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-eval-planner.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-executor.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-framework-selector.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-integration-checker.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-intel-updater.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-mempalace-curator.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-nyquist-auditor.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-pattern-mapper.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-phase-researcher.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-plan-checker.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-planner.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-project-researcher.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-research-synthesizer.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-roadmapper.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-security-auditor.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-ui-auditor.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-ui-checker.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-ui-researcher.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-user-profiler.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." + }, + "agents/gsd-verifier.toml": { + "reason": "#2834: Codex agent TOML now carries model-routing fields on first install." } } -} +} \ No newline at end of file diff --git a/tests/issue-2834-codex-install-model-ordering.test.cjs b/tests/issue-2834-codex-install-model-ordering.test.cjs new file mode 100644 index 000000000..6be649462 --- /dev/null +++ b/tests/issue-2834-codex-install-model-ordering.test.cjs @@ -0,0 +1,43 @@ +// allow-test-rule: structural-implementation-guard (#2834) +'use strict'; + +// Regression guard for #2834: on a clean Codex install, agent TOMLs contained no +// model-routing fields because defaults.json (resolve_model_ids + runtime) was written +// AFTER installCodexConfig generated the TOMLs. The fix extracts writeNonClaudeDefaults +// and calls it BEFORE installCodexConfig. This test asserts the ordering invariant in +// the install source so a future edit can't silently re-introduce the gap. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const INSTALL_JS = path.join(__dirname, '..', 'bin', 'install.js'); + +test('writeNonClaudeDefaults is called before installCodexConfig in the Codex install flow (#2834)', () => { + const src = fs.readFileSync(INSTALL_JS, 'utf8'); + + // Find the call to writeNonClaudeDefaults that precedes installCodexConfig. + const writeIdx = src.indexOf('writeNonClaudeDefaults(runtime);'); + assert.ok(writeIdx !== -1, 'writeNonClaudeDefaults(runtime) must be called in the install flow'); + + // Find the FIRST installCodexConfig call AFTER the writeNonClaudeDefaults call. + const codexGenIdx = src.indexOf('installCodexConfig(targetDir', writeIdx); + assert.ok(codexGenIdx !== -1 && codexGenIdx > writeIdx, + 'installCodexConfig must be called AFTER writeNonClaudeDefaults so defaults.json ' + + '(resolve_model_ids + runtime) exists before agent TOML generation reads it (#2834)'); + + // The #2834 comment must be present at the call site. + const callSite = src.slice(writeIdx - 300, writeIdx + 100); + assert.ok(/#2834/.test(callSite), 'the writeNonClaudeDefaults call must carry the #2834 rationale comment'); +}); + +test('writeNonClaudeDefaults function exists and is a no-op for Claude (#2834)', () => { + const src = fs.readFileSync(INSTALL_JS, 'utf8'); + const fnIdx = src.indexOf('function writeNonClaudeDefaults('); + assert.ok(fnIdx !== -1, 'writeNonClaudeDefaults must be defined as a function'); + const fnBody = src.slice(fnIdx, fnIdx + 1200); + assert.ok(/nativeModelAliases/.test(fnBody), 'writeNonClaudeDefaults must early-return for Claude (nativeModelAliases check)'); + assert.ok(/resolve_model_ids/.test(fnBody), 'writeNonClaudeDefaults must write resolve_model_ids'); + assert.ok(/defaults\.runtime/.test(fnBody), 'writeNonClaudeDefaults must write runtime'); +});