From 4631923982d783cca657bcdeb15d7ad6d5769662 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 24 Jun 2026 13:21:19 -0400 Subject: [PATCH] fix(#1569): preserve explicit resolve_model_ids in non-Claude installs (#1653) * fix(#1569): preserve explicit resolve_model_ids in non-Claude installs The non-Claude finishInstall step keyed its resolve_model_ids:"omit" write on !== "omit", so an explicit true opt-in (resolveModelInternal returns full model IDs) was silently clobbered on every install/upgrade across all 14 non-Claude runtimes, making generated agent manifests inherit the active chat model instead of pinning the resolved model. Now only absent/falsy is defaulted to "omit"; an explicit true (and an existing "omit") is preserved. Regression test parameterizes across codex/opencode/gemini and covers the absent/false/idempotent/ claude/malformed boundaries. * chore(#1569): backfill changeset pr ref to 1653 * fix(#1569): default non-canonical resolve_model_ids values to omit (codex review) Adversarial review (codex, gpt-5.5/high) flagged that the original allowlist-by- enumeration condition (undefined/null/false -> omit) preserved malformed values (0, "", "yes", {}) instead of defaulting them to omit, letting them leak Claude aliases a non-Claude runtime cannot resolve. Switch to an allowlist condition (existing !== true && existing !== 'omit') so only an explicit canonical true opt-in and an existing omit are preserved; everything else defaults to the safe non-Claude omit. Adds a parameterized test over [0, "", "yes", {}]. --- .changeset/sharp-eagles-wake.md | 5 + bin/install.js | 21 ++- ...-install-defaults-test-mode-guard.test.cjs | 139 ++++++++++++++++++ 3 files changed, 160 insertions(+), 5 deletions(-) create mode 100644 .changeset/sharp-eagles-wake.md diff --git a/.changeset/sharp-eagles-wake.md b/.changeset/sharp-eagles-wake.md new file mode 100644 index 000000000..4deb758d2 --- /dev/null +++ b/.changeset/sharp-eagles-wake.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1653 +--- +**Non-Claude installs no longer rewrite an explicit `resolve_model_ids: true` to "omit"** — Codex, OpenCode, Gemini, and the other non-Claude runtimes were silently clobbering the deliberate opt-in to full materialized model IDs on every install/upgrade, so generated agent manifests inherited the active chat model instead of pinning the resolved model. The finish step now only defaults `resolve_model_ids` to "omit" when it is absent or falsy; an explicit `true` is preserved. (#1569) diff --git a/bin/install.js b/bin/install.js index f3da4b1da..37b893f8c 100755 --- a/bin/install.js +++ b/bin/install.js @@ -11307,10 +11307,14 @@ function finishInstall(settingsPath, settings, statuslineCommand, shouldInstallS configureKiloPermissions(isGlobal, configDir); } - // For non-Claude runtimes, set resolve_model_ids: "omit" in ~/.gsd/defaults.json - // so resolveModelInternal() returns '' instead of Claude aliases (opus/sonnet/haiku) - // that the runtime can't resolve. Users can still use model_overrides for explicit IDs. - // See #1156. Guard matches the #130-class pattern on configureOpencodePermissions above. + // 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 (runtime !== 'claude' && !process.env.GSD_TEST_MODE) { const gsdDir = path.join(os.homedir(), '.gsd'); const defaultsPath = path.join(gsdDir, 'defaults.json'); @@ -11318,7 +11322,14 @@ function finishInstall(settingsPath, settings, statuslineCommand, shouldInstallS fs.mkdirSync(gsdDir, { recursive: true }); let defaults = {}; try { defaults = JSON.parse(fs.readFileSync(defaultsPath, 'utf8')); } catch { /* new file */ } - if (defaults.resolve_model_ids !== 'omit') { + // 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`); diff --git a/tests/bug-410-install-defaults-test-mode-guard.test.cjs b/tests/bug-410-install-defaults-test-mode-guard.test.cjs index eff6c61b4..d0e73da92 100644 --- a/tests/bug-410-install-defaults-test-mode-guard.test.cjs +++ b/tests/bug-410-install-defaults-test-mode-guard.test.cjs @@ -115,3 +115,142 @@ describe('Bug #410: finishInstall non-Claude runtime + GSD_TEST_MODE side-effect } }); }); + +// Bug #1569 folded here (sibling on the SAME finishInstall resolve_model_ids block): +// the #1156 default-to-"omit" step keyed its write on `!== "omit"`, so an explicit +// `resolve_model_ids: true` opt-in (resolveModelInternal returns full materialized +// model IDs) was silently clobbered across all 14 non-Claude runtimes. The fix +// preserves `true` and only defaults absent/falsy → "omit". Reuses the #410 harness. + +describe('Bug #1569: non-Claude finishInstall preserves explicit resolve_model_ids:true', () => { + function seedDefaults(obj) { + fs.mkdirSync(GSD_DIR, { recursive: true }); + fs.writeFileSync(DEFAULTS_PATH, JSON.stringify(obj, null, 2) + '\n', 'utf8'); + } + + function withUserPath(fn) { + const saved = process.env.GSD_TEST_MODE; + delete process.env.GSD_TEST_MODE; + try { + return fn(); + } finally { + process.env.GSD_TEST_MODE = saved; + } + } + + test('explicit resolve_model_ids:true survives a codex global install (the reported case)', () => { + withUserPath(() => { + seedDefaults({ runtime: 'codex', model_profile: 'balanced', resolve_model_ids: true }); + callFinishInstallForRuntime('codex'); + const after = JSON.parse(fs.readFileSync(DEFAULTS_PATH, 'utf8')); + assert.equal( + after.resolve_model_ids, + true, + 'explicit resolve_model_ids:true must be preserved across a codex install, not clobbered to "omit"', + ); + }); + }); + + // The clobber guard is runtime-agnostic (`runtime !== 'claude'`); parameterize + // across a representative slice of non-Claude runtimes. + for (const runtime of ['codex', 'opencode', 'gemini']) { + test(`explicit resolve_model_ids:true survives a ${runtime} global install`, () => { + withUserPath(() => { + seedDefaults({ runtime, resolve_model_ids: true }); + callFinishInstallForRuntime(runtime); + const after = JSON.parse(fs.readFileSync(DEFAULTS_PATH, 'utf8')); + assert.equal( + after.resolve_model_ids, + true, + `explicit resolve_model_ids:true must be preserved for ${runtime}`, + ); + }); + }); + } + + test('absent resolve_model_ids still defaults to "omit" (preserves #1156 intent)', () => { + withUserPath(() => { + seedDefaults({ runtime: 'codex' }); + callFinishInstallForRuntime('codex'); + const after = JSON.parse(fs.readFileSync(DEFAULTS_PATH, 'utf8')); + assert.equal( + after.resolve_model_ids, + 'omit', + 'absent resolve_model_ids must still default to "omit" for non-Claude runtimes', + ); + }); + }); + + test('explicit resolve_model_ids:false still defaults to "omit"', () => { + withUserPath(() => { + seedDefaults({ runtime: 'codex', resolve_model_ids: false }); + callFinishInstallForRuntime('codex'); + const after = JSON.parse(fs.readFileSync(DEFAULTS_PATH, 'utf8')); + assert.equal(after.resolve_model_ids, 'omit', 'false must still be normalized to "omit"'); + }); + }); + + test('non-canonical resolve_model_ids values (0, "", "yes", {}) default to "omit" — no Claude alias leak (#1569 codex review)', () => { + // The domain is true/false/"omit"/absent. Any OTHER value is malformed; the safe + // non-Claude default is "omit" (don't leak Claude aliases the runtime can't resolve). + withUserPath(() => { + for (const bad of [0, '', 'yes', {}]) { + seedDefaults({ runtime: 'codex', resolve_model_ids: bad }); + callFinishInstallForRuntime('codex'); + const after = JSON.parse(fs.readFileSync(DEFAULTS_PATH, 'utf8')); + assert.equal( + after.resolve_model_ids, + 'omit', + `non-canonical resolve_model_ids:${JSON.stringify(bad)} must default to "omit", not pass through`, + ); + } + }); + }); + + test('already-"omit" is left unchanged (idempotent, no rewrite churn)', () => { + withUserPath(() => { + seedDefaults({ runtime: 'codex', resolve_model_ids: 'omit' }); + const beforeMtime = fs.statSync(DEFAULTS_PATH).mtimeMs; + // fs mtime resolution can be coarse; wait briefly so an accidental rewrite is detectable. + const start = Date.now(); + while (Date.now() - start < 20) { /* spin briefly */ } + callFinishInstallForRuntime('codex'); + const after = JSON.parse(fs.readFileSync(DEFAULTS_PATH, 'utf8')); + const afterMtime = fs.statSync(DEFAULTS_PATH).mtimeMs; + assert.equal(after.resolve_model_ids, 'omit'); + assert.equal( + afterMtime, + beforeMtime, + 'defaults.json must not be rewritten when resolve_model_ids is already "omit" (idempotent)', + ); + }); + }); + + test('claude runtime never touches resolve_model_ids (cross-runtime parity)', () => { + withUserPath(() => { + seedDefaults({ runtime: 'claude', resolve_model_ids: true }); + callFinishInstallForRuntime('claude'); + const after = JSON.parse(fs.readFileSync(DEFAULTS_PATH, 'utf8')); + assert.equal( + after.resolve_model_ids, + true, + 'claude install must never rewrite resolve_model_ids', + ); + }); + }); + + test('malformed defaults.json does not crash — still defaults to "omit"', () => { + withUserPath(() => { + fs.mkdirSync(GSD_DIR, { recursive: true }); + fs.writeFileSync(DEFAULTS_PATH, '{ not valid json }', 'utf8'); + // Must not throw. + callFinishInstallForRuntime('codex'); + const after = JSON.parse(fs.readFileSync(DEFAULTS_PATH, 'utf8')); + assert.equal( + after.resolve_model_ids, + 'omit', + 'malformed defaults.json must be recovered to a valid state with resolve_model_ids:omit', + ); + }); + }); +});