diff --git a/get-shit-done/bin/lib/config-schema.cjs b/get-shit-done/bin/lib/config-schema.cjs index 75387268e..7768c4ade 100644 --- a/get-shit-done/bin/lib/config-schema.cjs +++ b/get-shit-done/bin/lib/config-schema.cjs @@ -47,6 +47,7 @@ const VALID_CONFIG_KEYS = new Set([ 'workflow.inline_plan_threshold', 'hooks.context_warnings', 'hooks.workflow_guard', + 'workflow.context_coverage_gate', 'statusline.show_last_command', 'workflow.ui_review', 'workflow.max_discuss_passes', diff --git a/sdk/src/query/config-mutation.test.ts b/sdk/src/query/config-mutation.test.ts index e84aaf633..5f25542e3 100644 --- a/sdk/src/query/config-mutation.test.ts +++ b/sdk/src/query/config-mutation.test.ts @@ -86,6 +86,42 @@ describe('isValidConfigKey', () => { expect(r2.valid).toBe(false); expect(r2.suggestion).toBe('workflow.nyquist_validation'); }); + + // #2653 — SDK/CJS config-schema drift regression. + // Every key accepted by the CJS config-set must also be accepted by + // the SDK config-set. We exercise every entry in the shared schema + // so drift fails this test the moment it is introduced. + it('#2653 — accepts every key in shared VALID_CONFIG_KEYS', async () => { + const { isValidConfigKey } = await import('./config-mutation.js'); + const { VALID_CONFIG_KEYS } = await import('./config-schema.js'); + const rejected: string[] = []; + for (const key of VALID_CONFIG_KEYS) { + const { valid } = isValidConfigKey(key); + if (!valid) rejected.push(key); + } + expect(rejected).toEqual([]); + }); + + it('#2653 — accepts sample dynamic keys from every DYNAMIC_KEY_PATTERN', async () => { + const { isValidConfigKey } = await import('./config-mutation.js'); + const samples = [ + 'agent_skills.gsd-planner', + 'review.models.claude', + 'features.some_feature', + 'claude_md_assembly.blocks.intro', + 'model_profile_overrides.codex.opus', + 'model_profile_overrides.codex.sonnet', + 'model_profile_overrides.my-runtime.haiku', + ]; + for (const key of samples) { + expect(isValidConfigKey(key).valid, `expected ${key} to be accepted`).toBe(true); + } + }); + + it('#2653 — accepts planning.sub_repos (CJS/docs key, previously rejected by SDK)', async () => { + const { isValidConfigKey } = await import('./config-mutation.js'); + expect(isValidConfigKey('planning.sub_repos').valid).toBe(true); + }); }); // ─── parseConfigValue ────────────────────────────────────────────────────── diff --git a/sdk/src/query/config-mutation.ts b/sdk/src/query/config-mutation.ts index 5892a7915..010f2de70 100644 --- a/sdk/src/query/config-mutation.ts +++ b/sdk/src/query/config-mutation.ts @@ -23,6 +23,7 @@ import { homedir } from 'node:os'; import { join } from 'node:path'; import { GSDError, ErrorClassification } from '../errors.js'; import { VALID_PROFILES, getAgentToModelMapForProfile } from './config-query.js'; +import { VALID_CONFIG_KEYS, DYNAMIC_KEY_PATTERNS } from './config-schema.js'; import { planningPaths } from './helpers.js'; import { acquireStateLock, releaseStateLock } from './state-mutation.js'; import type { QueryHandler } from './utils.js'; @@ -45,44 +46,8 @@ async function atomicWriteConfig(configPath: string, config: Record = { 'hooks.research_questions': 'workflow.research_before_questions', 'workflow.research_questions': 'workflow.research_before_questions', 'workflow.codereview': 'workflow.code_review', + 'workflow.review_command': 'workflow.code_review_command', 'workflow.review': 'workflow.code_review', 'workflow.code_review_level': 'workflow.code_review_depth', 'workflow.review_depth': 'workflow.code_review_depth', + 'review.model': 'review.models.', + 'sub_repos': 'planning.sub_repos', + 'plan_checker': 'workflow.plan_check', }; // ─── isValidConfigKey ───────────────────────────────────────────────────── @@ -117,11 +86,10 @@ const CONFIG_KEY_SUGGESTIONS: Record = { export function isValidConfigKey(keyPath: string): { valid: boolean; suggestion?: string } { if (VALID_CONFIG_KEYS.has(keyPath)) return { valid: true }; - // Dynamic patterns: agent_skills. - if (/^agent_skills\.[a-zA-Z0-9_-]+$/.test(keyPath)) return { valid: true }; - - // Dynamic patterns: features. - if (/^features\.[a-zA-Z0-9_]+$/.test(keyPath)) return { valid: true }; + // Dynamic patterns — all sourced from shared config-schema (#2653). + // Covers agent_skills.*, review.models.*, features.*, + // claude_md_assembly.blocks.*, and model_profile_overrides.*.. + if (DYNAMIC_KEY_PATTERNS.some((p) => p.test(keyPath))) return { valid: true }; // D9: Check curated suggestions before LCP fallback if (CONFIG_KEY_SUGGESTIONS[keyPath]) { diff --git a/sdk/src/query/config-schema.ts b/sdk/src/query/config-schema.ts new file mode 100644 index 000000000..ab2b3e6ac --- /dev/null +++ b/sdk/src/query/config-schema.ts @@ -0,0 +1,117 @@ +/** + * SDK-side mirror of get-shit-done/bin/lib/config-schema.cjs. + * + * Single source of truth for valid config key paths accepted by + * `config-set`. MUST stay in sync with the CJS schema — enforced + * by tests/config-schema-sdk-parity.test.cjs (CI drift guard). + * + * If you add/remove a key here, make the identical change in + * get-shit-done/bin/lib/config-schema.cjs (and vice versa). The + * parity test asserts the two allowlists are set-equal and that + * DYNAMIC_KEY_PATTERN_SOURCES produce identical regex source strings. + * + * See #2653 — CJS/SDK drift caused config-set to reject documented + * keys. #2479 added CJS↔docs parity; #2653 adds CJS↔SDK parity. + */ + +/** Exact-match config key paths accepted by config-set. */ +export const VALID_CONFIG_KEYS: ReadonlySet = new Set([ + 'mode', 'granularity', 'parallelization', 'commit_docs', 'model_profile', + 'search_gitignored', 'brave_search', 'firecrawl', 'exa_search', + 'workflow.research', 'workflow.plan_check', 'workflow.verifier', + 'workflow.nyquist_validation', 'workflow.ai_integration_phase', 'workflow.ui_phase', 'workflow.ui_safety_gate', + 'workflow.auto_advance', 'workflow.node_repair', 'workflow.node_repair_budget', + 'workflow.tdd_mode', + 'workflow.text_mode', + 'workflow.research_before_questions', + 'workflow.discuss_mode', + 'workflow.skip_discuss', + 'workflow.auto_prune_state', + 'workflow.use_worktrees', + 'workflow.code_review', + 'workflow.code_review_depth', + 'workflow.code_review_command', + 'workflow.pattern_mapper', + 'workflow.plan_bounce', + 'workflow.plan_bounce_script', + 'workflow.plan_bounce_passes', + 'workflow.plan_chunked', + 'workflow.post_planning_gaps', + 'workflow.security_enforcement', + 'workflow.security_asvs_level', + 'workflow.security_block_on', + 'workflow.drift_threshold', + 'workflow.drift_action', + 'git.branching_strategy', 'git.base_branch', 'git.phase_branch_template', 'git.milestone_branch_template', 'git.quick_branch_template', + 'planning.commit_docs', 'planning.search_gitignored', 'planning.sub_repos', + 'workflow.cross_ai_execution', 'workflow.cross_ai_command', 'workflow.cross_ai_timeout', + 'workflow.subagent_timeout', + 'workflow.inline_plan_threshold', + 'hooks.context_warnings', + 'hooks.workflow_guard', + 'workflow.context_coverage_gate', + 'statusline.show_last_command', + 'workflow.ui_review', + 'workflow.max_discuss_passes', + 'features.thinking_partner', + 'context', + 'features.global_learnings', + 'learnings.max_inject', + 'project_code', 'phase_naming', + 'manager.flags.discuss', 'manager.flags.plan', 'manager.flags.execute', + 'response_language', + 'context_window', + 'intel.enabled', + 'graphify.enabled', + 'graphify.build_timeout', + 'claude_md_path', + 'claude_md_assembly.mode', + // #2517 — runtime-aware model profiles + 'runtime', +]); + +/** + * Dynamic-pattern validators — keys matching these regexes are also accepted. + * Each entry's `source` MUST equal the corresponding CJS regex `.source` + * (the parity test enforces this). + */ +export interface DynamicKeyPattern { + readonly test: (k: string) => boolean; + readonly description: string; + readonly source: string; +} + +export const DYNAMIC_KEY_PATTERNS: readonly DynamicKeyPattern[] = [ + { + source: '^agent_skills\\.[a-zA-Z0-9_-]+$', + description: 'agent_skills.', + test: (k) => /^agent_skills\.[a-zA-Z0-9_-]+$/.test(k), + }, + { + source: '^review\\.models\\.[a-zA-Z0-9_-]+$', + description: 'review.models.', + test: (k) => /^review\.models\.[a-zA-Z0-9_-]+$/.test(k), + }, + { + source: '^features\\.[a-zA-Z0-9_]+$', + description: 'features.', + test: (k) => /^features\.[a-zA-Z0-9_]+$/.test(k), + }, + { + source: '^claude_md_assembly\\.blocks\\.[a-zA-Z0-9_]+$', + description: 'claude_md_assembly.blocks.
', + test: (k) => /^claude_md_assembly\.blocks\.[a-zA-Z0-9_]+$/.test(k), + }, + // #2517 — runtime-aware model profile overrides: model_profile_overrides.. + { + source: '^model_profile_overrides\\.[a-zA-Z0-9_-]+\\.(opus|sonnet|haiku)$', + description: 'model_profile_overrides..', + test: (k) => /^model_profile_overrides\.[a-zA-Z0-9_-]+\.(opus|sonnet|haiku)$/.test(k), + }, +]; + +/** Returns true if keyPath is a valid config key (exact or dynamic pattern). */ +export function isValidConfigKeyPath(keyPath: string): boolean { + if (VALID_CONFIG_KEYS.has(keyPath)) return true; + return DYNAMIC_KEY_PATTERNS.some((p) => p.test(keyPath)); +} diff --git a/tests/bug-2492-context-coverage-gate.test.cjs b/tests/bug-2492-context-coverage-gate.test.cjs index d06fcaf8f..35a612be8 100644 --- a/tests/bug-2492-context-coverage-gate.test.cjs +++ b/tests/bug-2492-context-coverage-gate.test.cjs @@ -18,6 +18,8 @@ const PLAN_PHASE = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'pla const VERIFY_PHASE = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'verify-phase.md'); const CONFIG_TS = path.join(__dirname, '..', 'sdk', 'src', 'config.ts'); const CONFIG_MUTATION_TS = path.join(__dirname, '..', 'sdk', 'src', 'query', 'config-mutation.ts'); +// #2653 — allowlist moved to shared schema module. +const CONFIG_SCHEMA_TS = path.join(__dirname, '..', 'sdk', 'src', 'query', 'config-schema.ts'); const CONFIG_GATES_TS = path.join(__dirname, '..', 'sdk', 'src', 'query', 'config-gates.ts'); const QUERY_INDEX_TS = path.join(__dirname, '..', 'sdk', 'src', 'query', 'index.ts'); @@ -144,8 +146,9 @@ describe('SDK wiring for #2492 gates', () => { ); }); - test('config-mutation.ts VALID_CONFIG_KEYS allows workflow.context_coverage_gate', () => { - const c = fs.readFileSync(CONFIG_MUTATION_TS, 'utf-8'); + test('config-schema.ts VALID_CONFIG_KEYS allows workflow.context_coverage_gate', () => { + // #2653 — allowlist moved out of config-mutation.ts into shared config-schema.ts. + const c = fs.readFileSync(CONFIG_SCHEMA_TS, 'utf-8'); assert.ok( c.includes("'workflow.context_coverage_gate'"), 'workflow.context_coverage_gate must be in VALID_CONFIG_KEYS', diff --git a/tests/config-schema-sdk-parity.test.cjs b/tests/config-schema-sdk-parity.test.cjs new file mode 100644 index 000000000..62aa98cfd --- /dev/null +++ b/tests/config-schema-sdk-parity.test.cjs @@ -0,0 +1,96 @@ +'use strict'; + +/** + * CJS↔SDK config-schema parity (#2653). + * + * The SDK has its own config-set handler at sdk/src/query/config-mutation.ts, + * which validates keys against sdk/src/query/config-schema.ts. That allowlist + * MUST match the CJS allowlist at get-shit-done/bin/lib/config-schema.cjs or + * SDK users are told "Unknown config key" for documented keys (regression + * that #2653 fixes). + * + * This test parses the TS file as text (to avoid requiring a TS toolchain + * in the node:test runner) and asserts: + * 1. Every key in CJS VALID_CONFIG_KEYS appears in the SDK literal set. + * 2. Every dynamic pattern source in CJS has an identical counterpart + * in the SDK file. + * 3. The reverse direction — SDK has no keys/patterns the CJS side lacks. + */ + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const ROOT = path.resolve(__dirname, '..'); +const { VALID_CONFIG_KEYS: CJS_KEYS, DYNAMIC_KEY_PATTERNS: CJS_PATTERNS } = + require('../get-shit-done/bin/lib/config-schema.cjs'); + +const SDK_SCHEMA_PATH = path.join(ROOT, 'sdk', 'src', 'query', 'config-schema.ts'); +const SDK_SRC = fs.readFileSync(SDK_SCHEMA_PATH, 'utf8'); + +function extractSdkKeys(src) { + const start = src.indexOf('VALID_CONFIG_KEYS'); + assert.ok(start > -1, 'SDK config-schema.ts must export VALID_CONFIG_KEYS'); + const setOpen = src.indexOf('new Set([', start); + const setClose = src.indexOf('])', setOpen); + assert.ok(setOpen > -1 && setClose > -1, 'VALID_CONFIG_KEYS must be a new Set([...]) literal'); + const body = src.slice(setOpen + 'new Set(['.length, setClose); + const keys = new Set(); + for (const match of body.matchAll(/'([^']+)'/g)) keys.add(match[1]); + return keys; +} + +function extractSdkPatternSources(src) { + const sources = []; + for (const match of src.matchAll(/source:\s*'([^']+)'/g)) { + // TS source file stores escape sequences; convert \\ -> \ so the + // extracted value matches RegExp.source from the CJS side. + sources.push(match[1].replace(/\\\\/g, '\\')); + } + return sources; +} + +test('#2653 — SDK VALID_CONFIG_KEYS matches CJS VALID_CONFIG_KEYS', () => { + const sdkKeys = extractSdkKeys(SDK_SRC); + const missingInSdk = [...CJS_KEYS].filter((k) => !sdkKeys.has(k)); + const extraInSdk = [...sdkKeys].filter((k) => !CJS_KEYS.has(k)); + assert.deepStrictEqual( + missingInSdk, + [], + 'CJS keys missing from sdk/src/query/config-schema.ts:\n' + + missingInSdk.map((k) => ' ' + k).join('\n'), + ); + assert.deepStrictEqual( + extraInSdk, + [], + 'SDK keys missing from get-shit-done/bin/lib/config-schema.cjs:\n' + + extraInSdk.map((k) => ' ' + k).join('\n'), + ); +}); + +test('#2653 — SDK DYNAMIC_KEY_PATTERNS sources match CJS regex .source', () => { + const sdkSources = new Set(extractSdkPatternSources(SDK_SRC)); + const cjsSources = CJS_PATTERNS.map((p) => { + // Reconstruct each CJS pattern's .source by probing with a known string + // that identifies the regex. CJS stores a `test` arrow only, so derive + // `.source` by running against sentinel inputs — instead, inspect function + // text as a fallback cross-check. + const fnSrc = p.test.toString(); + const regexMatch = fnSrc.match(/\/(\^[^/]+\$)\//); + assert.ok(regexMatch, 'CJS dynamic pattern test function must embed a literal regex: ' + fnSrc); + return regexMatch[1]; + }); + for (const src of cjsSources) { + assert.ok( + sdkSources.has(src), + `CJS dynamic pattern ${src} not mirrored in SDK config-schema.ts (sources: ${[...sdkSources].join(', ')})`, + ); + } + for (const src of sdkSources) { + assert.ok( + cjsSources.includes(src), + `SDK dynamic pattern ${src} not mirrored in CJS config-schema.cjs`, + ); + } +});