* 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 <test@example.com>
(cherry picked from commit 00c859fae5)
This commit is contained in:
Tom Boucher
5
.changeset/graceful-lynx-sing.md
Normal file
5
.changeset/graceful-lynx-sing.md
Normal file
@@ -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)
|
||||
102
bin/install.js
102
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=<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: <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
|
||||
|
||||
@@ -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 <answer_validation> 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 <answer_validation> 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."
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
43
tests/issue-2834-codex-install-model-ordering.test.cjs
Normal file
43
tests/issue-2834-codex-install-model-ordering.test.cjs
Normal file
@@ -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');
|
||||
});
|
||||
Reference in New Issue
Block a user