Merge pull request #3540 from open-gsd/fix/3532-global-defaults-diagnostic
This commit is contained in:
Tom Boucher
5
.changeset/graceful-seals-march.md
Normal file
5
.changeset/graceful-seals-march.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Added
|
||||
pr: 3540
|
||||
---
|
||||
**`~/.gsd/defaults.json` shadowing is now diagnosed instead of silent** — in any project with a `.planning/config.json`, global model-side keys (`model_profile`, `model_overrides`, `models`, `dynamic_routing`, `runtime`, …) were silently ignored for model resolution; a file named `defaults.json` applied to no real project with no signal. GSD now prints a one-time stderr warning naming the shadowed keys. Resolution precedence is unchanged; global `effort` keeps working via effort sync and never warns. (#3532)
|
||||
@@ -1759,6 +1759,22 @@ Save settings as global defaults for future projects:
|
||||
|
||||
When `/gsd-new-project` creates a new `config.json`, it reads global defaults and merges them as the starting configuration. Per-project settings always override globals.
|
||||
|
||||
### What a global file can and cannot set at runtime
|
||||
|
||||
Two different rules apply, and the difference is deliberate ([#3532](https://github.com/open-gsd/gsd-core/issues/3532)):
|
||||
|
||||
- **In a directory with no `.planning/` at all**, `~/.gsd/defaults.json` is the active
|
||||
configuration — model resolution reads it directly.
|
||||
- **In a real project (`.planning/config.json` present, even if empty)**, the global file is
|
||||
**not read for model resolution** — every model-side key it sets (`model_profile`,
|
||||
`model_overrides`, `models`, `dynamic_routing`, `runtime`, and the rest of the resolution
|
||||
set) is inert there. GSD prints a one-time stderr warning naming the shadowed keys when it
|
||||
detects this, instead of failing silently. To apply a global model setting to a project,
|
||||
put it in that project's `.planning/config.json`.
|
||||
- **`effort` is the exception**: the install-time effort channel always merges
|
||||
`~/.gsd/defaults.json` with the project config (that is how `effort sync` works), so a
|
||||
global `effort` block keeps working in projects and does not trigger the warning.
|
||||
|
||||
---
|
||||
|
||||
## Observability
|
||||
|
||||
@@ -317,6 +317,60 @@ function _resetRuntimeWarningCacheForTests(): void {
|
||||
_warnedConfigKeys.clear();
|
||||
_warnedUnknownConfigKeys.clear();
|
||||
_warnedUnusableConfig.clear();
|
||||
_warnedShadowedGlobalKeys.clear();
|
||||
}
|
||||
|
||||
// ─── #3532 (10b): shadowed global-defaults diagnostic ────────────────────────
|
||||
|
||||
// The keys Branch D's `_globalBaseCfg` demonstrably honors from
|
||||
// ~/.gsd/defaults.json when no project config exists. Under a project
|
||||
// .planning/config.json (Branch A — every real project) the global file is
|
||||
// never opened, so each of these set globally is silently inert for resolution.
|
||||
// `effort` is in Branch D's honored set but is EXCLUDED from the shadow warning:
|
||||
// the install-time effort sync (readGsdEffectiveEffortConfig) DOES merge the
|
||||
// global file, so warning on it would be false for the channel users actually
|
||||
// control via `effort sync`. Keep this list in lockstep with `_globalBaseCfg`
|
||||
// below — the per-key canary in tests/config-loader.test.cjs fails first on
|
||||
// drift in either direction.
|
||||
const GLOBAL_DEFAULTS_RESOLUTION_KEYS = [
|
||||
'model_profile', 'commit_docs', 'research', 'plan_checker', 'verifier',
|
||||
'nyquist_validation', 'post_planning_gaps', 'parallelization', 'text_mode',
|
||||
'resolve_model_ids', 'context_window', 'subagent_timeout', 'model_overrides',
|
||||
'models', 'granularity', 'granularities', 'planning', 'dynamic_routing',
|
||||
'effort', 'fast_mode', 'agent_skills', 'response_language', 'runtime',
|
||||
'model_profile_overrides', 'model_policy',
|
||||
];
|
||||
|
||||
// Module-level dedup keyed on the SORTED shadowed-key set: a later call with
|
||||
// the same shadowed set stays quiet, while a config that grows a new shadowed
|
||||
// key re-arms the warning. Stronger than _warnedUnknownConfigKeys (which keys
|
||||
// on insertion order) — same discipline, order-independent key.
|
||||
const _warnedShadowedGlobalKeys = new Set<string>();
|
||||
|
||||
function _warnShadowedGlobalDefaults(globalDefaults: Record<string, unknown>, globalPath: string): void {
|
||||
const shadowed = GLOBAL_DEFAULTS_RESOLUTION_KEYS.filter(k =>
|
||||
k !== 'effort' && Object.prototype.hasOwnProperty.call(globalDefaults, k));
|
||||
// Branch D also honors the nested alias workflow.post_planning_gaps (the
|
||||
// `?? globalDefaults['workflow']?.['post_planning_gaps']` fallback in
|
||||
// _globalBaseCfg) — a global file using only the nested form is equally
|
||||
// shadowed, so it reports under its dotted name.
|
||||
if (!shadowed.includes('post_planning_gaps')) {
|
||||
const wf = globalDefaults['workflow'];
|
||||
if (wf && typeof wf === 'object' && !Array.isArray(wf) &&
|
||||
Object.prototype.hasOwnProperty.call(wf, 'post_planning_gaps')) {
|
||||
shadowed.push('workflow.post_planning_gaps');
|
||||
}
|
||||
}
|
||||
if (shadowed.length === 0) return;
|
||||
const dedupKey = shadowed.slice().sort().join(',');
|
||||
if (_warnedShadowedGlobalKeys.has(dedupKey)) return;
|
||||
_warnedShadowedGlobalKeys.add(dedupKey);
|
||||
try {
|
||||
process.stderr.write(
|
||||
`gsd-tools: warning: ${globalPath} sets ${shadowed.join(', ')} but a project config ` +
|
||||
`takes precedence here — those global keys are ignored for model resolution. (#3532)\n`,
|
||||
);
|
||||
} catch { /* stderr might be closed in some test harnesses */ }
|
||||
}
|
||||
|
||||
// ─── FIX 2: Federated overlay helpers ────────────────────────────────────────
|
||||
@@ -836,6 +890,22 @@ function loadConfigResolved(cwd: string, options: Record<string, unknown> = {}):
|
||||
// Fix 4: empty-string ws ('') resolves the root path → source:'root'.
|
||||
const source: ConfigSource = wsRequested ? 'workstream' : 'root';
|
||||
|
||||
// #3532 (10b): a parsed project config means Branch D never runs, so every
|
||||
// key ~/.gsd/defaults.json sets that Branch D would honor is silently inert
|
||||
// here. Observation only — one deduped stderr warning; precedence is
|
||||
// untouched. Faults in the global file stay silent in this branch (the
|
||||
// project config governs; the nearer file is the actionable one).
|
||||
try {
|
||||
const shadowHome = process.env['GSD_HOME'] || os.homedir();
|
||||
const shadowPath = path.join(shadowHome, '.gsd', 'defaults.json');
|
||||
const shadowRead = _readConfigFile(shadowPath);
|
||||
if (shadowRead.kind === 'ok') {
|
||||
_warnShadowedGlobalDefaults(shadowRead.data, shadowPath);
|
||||
}
|
||||
} catch {
|
||||
// Observation only — never let the diagnostic perturb resolution.
|
||||
}
|
||||
|
||||
// This config parsed — but a DIFFERENT file on the resolution path may not
|
||||
// have. A workstream config that loads cleanly while the root config it
|
||||
// inherits from is corrupt is still a degraded resolution: the root's
|
||||
@@ -965,6 +1035,8 @@ export = {
|
||||
_getNestedConfigDefault,
|
||||
_deepMergeConfig,
|
||||
_warnedUnknownConfigKeys,
|
||||
_warnedShadowedGlobalKeys,
|
||||
GLOBAL_DEFAULTS_RESOLUTION_KEYS,
|
||||
_warnUnknownProfileOverrides,
|
||||
_resetRuntimeWarningCacheForTests,
|
||||
_warnedConfigKeys,
|
||||
|
||||
@@ -740,7 +740,7 @@ const { describe, test, afterEach } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const { createTempProject, cleanup, TOOLS_PATH, TEST_ENV_BASE } = require('./helpers.cjs');
|
||||
const { createTempProject, cleanup, TOOLS_PATH, TEST_ENV_BASE, installSpawnHome } = require('./helpers.cjs');
|
||||
const { runNode } = require('./helpers/process-seam.cjs');
|
||||
const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs');
|
||||
|
||||
@@ -751,7 +751,11 @@ const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs');
|
||||
function runWithStderr(args, cwd, env = {}) {
|
||||
const result = runNode([TOOLS_PATH, ...args], {
|
||||
cwd,
|
||||
env: { ...process.env, ...TEST_ENV_BASE, ...env },
|
||||
// #3532: pin GSD_HOME to an empty sandbox so a developer's real
|
||||
// ~/.gsd/defaults.json cannot leak shadow-key warnings into children that
|
||||
// these suites assert are stderr-clean (TEST_ENV_BASE only BLANKS the
|
||||
// var; an empty string falls through to the real homedir).
|
||||
env: { ...process.env, ...TEST_ENV_BASE, GSD_HOME: installSpawnHome(), ...env },
|
||||
timeoutMs: PROBE_TIMEOUT_MS,
|
||||
});
|
||||
return {
|
||||
@@ -1284,3 +1288,170 @@ describe('#2997: phase_id_convention is not silently dropped on a clean read', (
|
||||
} finally { cleanup(tmpDir); }
|
||||
});
|
||||
});
|
||||
|
||||
// ─── #3532 (10b): shadowed global-defaults diagnostic ─────────────────────────
|
||||
|
||||
// The keys Branch D's _globalBaseCfg demonstrably honors when NO project config
|
||||
// exists. Under a project .planning/config.json (Branch A — every real project)
|
||||
// the global file is never opened, so each of these set globally is silently
|
||||
// inert. `effort` is deliberately absent: the install-time effort sync
|
||||
// (readGsdEffectiveEffortConfig) DOES merge the global file, so warning on it
|
||||
// would be false for the channel users control via effort sync.
|
||||
const GLOBAL_KEYS_SHADOWED_UNDER_PROJECT = [
|
||||
'model_profile', 'commit_docs', 'research', 'plan_checker', 'verifier',
|
||||
'nyquist_validation', 'post_planning_gaps', 'parallelization', 'text_mode',
|
||||
'resolve_model_ids', 'context_window', 'subagent_timeout', 'model_overrides',
|
||||
'models', 'granularity', 'granularities', 'planning', 'dynamic_routing',
|
||||
'fast_mode', 'agent_skills', 'response_language', 'runtime',
|
||||
'model_profile_overrides', 'model_policy',
|
||||
];
|
||||
|
||||
describe('#3532 shadowed global-defaults warning', () => {
|
||||
let tmpDir;
|
||||
let gsdHome;
|
||||
let stderrLines;
|
||||
let originalStderrWrite;
|
||||
let originalGsdHome;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = makeTempProject('gsd-3532-shadow-');
|
||||
gsdHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3532-home-'));
|
||||
stderrLines = [];
|
||||
originalStderrWrite = process.stderr.write.bind(process.stderr);
|
||||
process.stderr.write = (chunk) => { stderrLines.push(String(chunk)); return true; };
|
||||
originalGsdHome = process.env.GSD_HOME;
|
||||
process.env.GSD_HOME = gsdHome;
|
||||
if (_resetRuntimeWarningCacheForTests) _resetRuntimeWarningCacheForTests();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
process.stderr.write = originalStderrWrite;
|
||||
if (originalGsdHome === undefined) delete process.env.GSD_HOME;
|
||||
else process.env.GSD_HOME = originalGsdHome;
|
||||
if (tmpDir) cleanup(tmpDir);
|
||||
if (gsdHome) cleanup(gsdHome);
|
||||
tmpDir = gsdHome = null;
|
||||
});
|
||||
|
||||
function writeGlobalDefaults(obj) {
|
||||
fs.mkdirSync(path.join(gsdHome, '.gsd'), { recursive: true });
|
||||
fs.writeFileSync(
|
||||
path.join(gsdHome, '.gsd', 'defaults.json'),
|
||||
JSON.stringify(obj, null, 2),
|
||||
);
|
||||
}
|
||||
|
||||
test('project config + global model keys -> one warning naming both keys', () => {
|
||||
writeConfig(tmpDir, { model_profile: 'balanced' });
|
||||
writeGlobalDefaults({ model_overrides: { 'gsd-executor': 'haiku' }, model_profile: 'quality' });
|
||||
loadConfigResolved(tmpDir);
|
||||
const warnings = stderrLines.filter(l => l.includes('model_overrides') && l.includes('model_profile'));
|
||||
assert.equal(warnings.length, 1, `expected exactly one shadowed-keys warning, got: ${stderrLines.join('')}`);
|
||||
});
|
||||
|
||||
test('second loadConfig call does not repeat the warning', () => {
|
||||
writeConfig(tmpDir, { model_profile: 'balanced' });
|
||||
writeGlobalDefaults({ model_profile: 'quality' });
|
||||
loadConfigResolved(tmpDir);
|
||||
loadConfigResolved(tmpDir);
|
||||
const warnings = stderrLines.filter(l => l.includes('model_profile') && l.includes('defaults.json'));
|
||||
assert.ok(warnings.length <= 1, `warning emitted more than once: ${warnings.length}`);
|
||||
});
|
||||
|
||||
test('global effort keys do not warn (honored by the install-time effort sync)', () => {
|
||||
writeConfig(tmpDir, { model_profile: 'balanced' });
|
||||
writeGlobalDefaults({ effort: { default: 'low' } });
|
||||
loadConfigResolved(tmpDir);
|
||||
assert.equal(stderrLines.filter(l => l.includes('defaults.json')).length, 0,
|
||||
`effort must not trigger the shadow warning: ${stderrLines.join('')}`);
|
||||
});
|
||||
|
||||
test('absent global defaults never warn', () => {
|
||||
writeConfig(tmpDir, { model_profile: 'balanced' });
|
||||
loadConfigResolved(tmpDir);
|
||||
assert.equal(stderrLines.length, 0, `unexpected warnings: ${stderrLines.join('')}`);
|
||||
});
|
||||
|
||||
test('bare dir without .planning honors global defaults without warning (Branch D)', () => {
|
||||
const bare = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3532-bare-'));
|
||||
try {
|
||||
writeGlobalDefaults({ model_profile: 'quality' });
|
||||
const resolution = loadConfigResolved(bare);
|
||||
assert.equal(resolution.source, 'global-defaults');
|
||||
assert.equal(resolution.config['model_profile'], 'quality');
|
||||
assert.equal(stderrLines.length, 0, `Branch D must not warn: ${stderrLines.join('')}`);
|
||||
} finally {
|
||||
cleanup(bare);
|
||||
}
|
||||
});
|
||||
|
||||
test('unparseable global defaults skip the shadow warning', () => {
|
||||
writeConfig(tmpDir, { model_profile: 'balanced' });
|
||||
fs.mkdirSync(path.join(gsdHome, '.gsd'), { recursive: true });
|
||||
fs.writeFileSync(path.join(gsdHome, '.gsd', 'defaults.json'), '{not json');
|
||||
loadConfigResolved(tmpDir);
|
||||
assert.equal(stderrLines.filter(l => l.includes('shadowed')).length, 0);
|
||||
});
|
||||
|
||||
test('present-but-empty project config still shadows', () => {
|
||||
writeConfig(tmpDir, {});
|
||||
writeGlobalDefaults({ model_profile: 'quality' });
|
||||
loadConfigResolved(tmpDir);
|
||||
assert.ok(stderrLines.some(l => l.includes('model_profile')),
|
||||
`empty project config must still warn: ${stderrLines.join('')}`);
|
||||
});
|
||||
|
||||
test('non-resolution global keys do not warn from this check', () => {
|
||||
writeConfig(tmpDir, { model_profile: 'balanced' });
|
||||
writeGlobalDefaults({ __gsd_3532_arbitrary__: true });
|
||||
loadConfigResolved(tmpDir);
|
||||
assert.equal(stderrLines.filter(l => l.includes('__gsd_3532_arbitrary__') && l.includes('shadowed')).length, 0);
|
||||
});
|
||||
|
||||
// Typed-IR parity canary (CONTRIBUTING: assert the exported dedup Set, not
|
||||
// stderr prose — #2674 precedent). Every key Branch D honors must register
|
||||
// as shadowed when set globally under a project config.
|
||||
for (const key of GLOBAL_KEYS_SHADOWED_UNDER_PROJECT) {
|
||||
test(`canary: global "${key}" alone warns under a project config`, () => {
|
||||
writeConfig(tmpDir, { model_profile: 'balanced' });
|
||||
writeGlobalDefaults({ [key]: true });
|
||||
loadConfigResolved(tmpDir);
|
||||
const registered = [...configLoader._warnedShadowedGlobalKeys].some(set => set.split(',').includes(key));
|
||||
assert.ok(registered, `global "${key}" must register as shadowed`);
|
||||
});
|
||||
}
|
||||
|
||||
// The nested alias Branch D honors (workflow.post_planning_gaps fallback in
|
||||
// _globalBaseCfg) is equally shadowed and reports under its dotted name.
|
||||
test('canary: nested workflow.post_planning_gaps warns under a project config', () => {
|
||||
writeConfig(tmpDir, { model_profile: 'balanced' });
|
||||
writeGlobalDefaults({ workflow: { post_planning_gaps: 'extended' } });
|
||||
loadConfigResolved(tmpDir);
|
||||
const registered = [...configLoader._warnedShadowedGlobalKeys].some(set => set.split(',').includes('workflow.post_planning_gaps'));
|
||||
assert.ok(registered, 'nested workflow.post_planning_gaps must register as shadowed');
|
||||
});
|
||||
|
||||
// List parity, both directions: the implementation's exported list (minus
|
||||
// effort) must equal this file's expected list — a key _globalBaseCfg grows
|
||||
// without updating GLOBAL_DEFAULTS_RESOLUTION_KEYS goes silently unwarned,
|
||||
// and a key the export grows without _globalBaseCfg reading makes the
|
||||
// warning lie.
|
||||
test('GLOBAL_DEFAULTS_RESOLUTION_KEYS parity with the expected shadow set', () => {
|
||||
const exported = configLoader.GLOBAL_DEFAULTS_RESOLUTION_KEYS.filter(k => k !== 'effort').sort();
|
||||
const expected = GLOBAL_KEYS_SHADOWED_UNDER_PROJECT.slice().sort();
|
||||
assert.deepEqual(exported, expected,
|
||||
`resolution-key list drifted: exported=${JSON.stringify(exported)} expected=${JSON.stringify(expected)}`);
|
||||
});
|
||||
|
||||
test('_resetRuntimeWarningCacheForTests clears the shadowed-key dedup set', () => {
|
||||
writeConfig(tmpDir, { model_profile: 'balanced' });
|
||||
writeGlobalDefaults({ model_profile: 'quality' });
|
||||
loadConfigResolved(tmpDir);
|
||||
assert.ok(
|
||||
configLoader._warnedShadowedGlobalKeys && configLoader._warnedShadowedGlobalKeys.size > 0,
|
||||
'precondition: a shadowed key must populate the dedup set',
|
||||
);
|
||||
_resetRuntimeWarningCacheForTests();
|
||||
assert.equal(configLoader._warnedShadowedGlobalKeys.size, 0);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user