From b7ff14fe5198a33fc46b6f14c72cc9311f109a35 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 25 Apr 2026 12:08:59 -0400 Subject: [PATCH] fix(#2687): derive KNOWN_TOP_LEVEL from DYNAMIC_KEY_PATTERNS to eliminate read-side drift (#2706) Co-authored-by: Claude Sonnet 4.6 --- get-shit-done/bin/lib/config-schema.cjs | 10 +- get-shit-done/bin/lib/core.cjs | 11 +- ...g-2687-config-read-warning-parity.test.cjs | 131 ++++++++++++++++++ 3 files changed, 143 insertions(+), 9 deletions(-) create mode 100644 tests/bug-2687-config-read-warning-parity.test.cjs diff --git a/get-shit-done/bin/lib/config-schema.cjs b/get-shit-done/bin/lib/config-schema.cjs index 7768c4ade..85e79d7d5 100644 --- a/get-shit-done/bin/lib/config-schema.cjs +++ b/get-shit-done/bin/lib/config-schema.cjs @@ -73,13 +73,13 @@ const VALID_CONFIG_KEYS = new Set([ * Each entry has a `test` function and a human-readable `description`. */ const DYNAMIC_KEY_PATTERNS = [ - { test: (k) => /^agent_skills\.[a-zA-Z0-9_-]+$/.test(k), description: 'agent_skills.' }, - { test: (k) => /^review\.models\.[a-zA-Z0-9_-]+$/.test(k), description: 'review.models.' }, - { test: (k) => /^features\.[a-zA-Z0-9_]+$/.test(k), description: 'features.' }, - { test: (k) => /^claude_md_assembly\.blocks\.[a-zA-Z0-9_]+$/.test(k), description: 'claude_md_assembly.blocks.
' }, + { topLevel: 'agent_skills', test: (k) => /^agent_skills\.[a-zA-Z0-9_-]+$/.test(k), description: 'agent_skills.' }, + { topLevel: 'review', test: (k) => /^review\.models\.[a-zA-Z0-9_-]+$/.test(k), description: 'review.models.' }, + { topLevel: 'features', test: (k) => /^features\.[a-zA-Z0-9_]+$/.test(k), description: 'features.' }, + { topLevel: 'claude_md_assembly', test: (k) => /^claude_md_assembly\.blocks\.[a-zA-Z0-9_]+$/.test(k), description: 'claude_md_assembly.blocks.
' }, // #2517 — runtime-aware model profile overrides: model_profile_overrides.. // is a free string (so users can map non-built-in runtimes); is enum-restricted. - { test: (k) => /^model_profile_overrides\.[a-zA-Z0-9_-]+\.(opus|sonnet|haiku)$/.test(k), + { topLevel: 'model_profile_overrides', test: (k) => /^model_profile_overrides\.[a-zA-Z0-9_-]+\.(opus|sonnet|haiku)$/.test(k), description: 'model_profile_overrides..' }, ]; diff --git a/get-shit-done/bin/lib/core.cjs b/get-shit-done/bin/lib/core.cjs index 28c22a2a2..32f97c0fe 100644 --- a/get-shit-done/bin/lib/core.cjs +++ b/get-shit-done/bin/lib/core.cjs @@ -336,14 +336,17 @@ function loadConfig(cwd) { // Derived from config-set's VALID_CONFIG_KEYS (canonical source) plus internal-only // keys that loadConfig handles but config-set doesn't expose. This avoids maintaining // a hardcoded duplicate that drifts when new config keys are added. - const { VALID_CONFIG_KEYS } = require('./config.cjs'); + // DYNAMIC_KEY_PATTERNS supplies topLevel for each pattern so adding a new + // dynamic-pattern namespace to config-schema.cjs automatically updates this set + // — no more drift between the read side and the write side (#2687). + const { VALID_CONFIG_KEYS, DYNAMIC_KEY_PATTERNS } = require('./config-schema.cjs'); const KNOWN_TOP_LEVEL = new Set([ // Extract top-level key names from dot-notation paths (e.g., 'workflow.research' → 'workflow') ...[...VALID_CONFIG_KEYS].map(k => k.split('.')[0]), - // Section containers that hold nested sub-keys - 'git', 'workflow', 'planning', 'hooks', 'features', + // Dynamic-pattern top-level containers (e.g. review, model_profile_overrides) + ...DYNAMIC_KEY_PATTERNS.map(p => p.topLevel), // Internal keys loadConfig reads but config-set doesn't expose - 'model_overrides', 'agent_skills', 'context_window', 'resolve_model_ids', 'claude_md_path', + 'model_overrides', 'context_window', 'resolve_model_ids', 'claude_md_path', // Deprecated keys (still accepted for migration, not in config-set) 'depth', 'multiRepo', ]); diff --git a/tests/bug-2687-config-read-warning-parity.test.cjs b/tests/bug-2687-config-read-warning-parity.test.cjs new file mode 100644 index 000000000..d1e3fbd23 --- /dev/null +++ b/tests/bug-2687-config-read-warning-parity.test.cjs @@ -0,0 +1,131 @@ +'use strict'; + +/** + * Regression test for #2687 — loadConfig must not emit "unknown config key" + * warnings for keys that are registered in DYNAMIC_KEY_PATTERNS (e.g. review, + * model_profile_overrides, claude_md_assembly). These keys were absent from + * the hand-maintained KNOWN_TOP_LEVEL set in core.cjs, causing false-positive + * warnings on every read. + * + * We trigger loadConfig via `resolve-model` (which calls loadConfig internally). + * We use spawnSync to capture stderr from a process that exits 0 (warnings are + * written to stderr but don't cause a non-zero exit, so runGsdTools' error field + * is empty for successful commands). + */ + +const { describe, test, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); +const { createTempProject, cleanup, TOOLS_PATH } = require('./helpers.cjs'); + +const TEST_ENV_BASE = { + GSD_SESSION_KEY: '', + CODEX_THREAD_ID: '', + CLAUDE_SESSION_ID: '', + CLAUDE_CODE_SSE_PORT: '', + OPENCODE_SESSION_ID: '', + GEMINI_SESSION_ID: '', + CURSOR_SESSION_ID: '', + WINDSURF_SESSION_ID: '', + TERM_SESSION_ID: '', + WT_SESSION: '', + TMUX_PANE: '', + ZELLIJ_SESSION_NAME: '', + TTY: '', + SSH_TTY: '', +}; + +/** + * Run gsd-tools and return { stdout, stderr, status }. + * Captures stderr even when the process exits 0 (unlike runGsdTools which only + * surfaces stderr via result.error on non-zero exit). + */ +function runWithStderr(args, cwd) { + const result = spawnSync(process.execPath, [TOOLS_PATH, ...args], { + cwd, + encoding: 'utf-8', + env: { ...process.env, ...TEST_ENV_BASE }, + }); + return { + stdout: result.stdout || '', + stderr: result.stderr || '', + status: result.status, + }; +} + +describe('bug-2687 — no warning for dynamic-pattern containers in loadConfig', () => { + let tmpDir; + + afterEach(() => { + if (tmpDir) cleanup(tmpDir); + tmpDir = null; + }); + + test('review — loadConfig emits no warning when config.json contains review key', () => { + tmpDir = createTempProject('gsd-2687-review-'); + const configPath = path.join(tmpDir, '.planning', 'config.json'); + fs.writeFileSync( + configPath, + JSON.stringify({ review: { models: { 'test-cli': 'test-command' } } }, null, 2), + 'utf-8' + ); + + // resolve-model calls loadConfig internally, triggering the KNOWN_TOP_LEVEL check + const result = runWithStderr(['resolve-model', 'planner'], tmpDir); + + assert.ok( + !result.stderr.includes('unknown config key'), + `loadConfig must not warn about "review" — got stderr: ${result.stderr}` + ); + assert.ok( + !result.stderr.includes('warning'), + `loadConfig must not warn about "review" — got stderr: ${result.stderr}` + ); + }); + + test('model_profile_overrides — loadConfig emits no warning when config.json contains model_profile_overrides key', () => { + tmpDir = createTempProject('gsd-2687-mpo-'); + const configPath = path.join(tmpDir, '.planning', 'config.json'); + fs.writeFileSync( + configPath, + JSON.stringify({ model_profile_overrides: { codex: { sonnet: 'claude-sonnet-4' } } }, null, 2), + 'utf-8' + ); + + // resolve-model calls loadConfig internally, triggering the KNOWN_TOP_LEVEL check + const result = runWithStderr(['resolve-model', 'planner'], tmpDir); + + assert.ok( + !result.stderr.includes('unknown config key'), + `loadConfig must not warn about "model_profile_overrides" — got stderr: ${result.stderr}` + ); + assert.ok( + !result.stderr.includes('warning'), + `loadConfig must not warn about "model_profile_overrides" — got stderr: ${result.stderr}` + ); + }); + + test('claude_md_assembly — loadConfig emits no warning when config.json contains claude_md_assembly key', () => { + tmpDir = createTempProject('gsd-2687-cma-'); + const configPath = path.join(tmpDir, '.planning', 'config.json'); + fs.writeFileSync( + configPath, + JSON.stringify({ claude_md_assembly: { mode: 'custom', blocks: { identity: true } } }, null, 2), + 'utf-8' + ); + + // resolve-model calls loadConfig internally, triggering the KNOWN_TOP_LEVEL check + const result = runWithStderr(['resolve-model', 'planner'], tmpDir); + + assert.ok( + !result.stderr.includes('unknown config key'), + `loadConfig must not warn about "claude_md_assembly" — got stderr: ${result.stderr}` + ); + assert.ok( + !result.stderr.includes('warning'), + `loadConfig must not warn about "claude_md_assembly" — got stderr: ${result.stderr}` + ); + }); +});