fix(#2653): eliminate SDK↔CJS config-schema drift (#2670)

The SDK's config-set kept its own hand-maintained allowlist (28-key drift vs. get-shit-done/bin/lib/config-schema.cjs), so documented keys accepted by the CJS config-set — planning.sub_repos, workflow.code_review_command, workflow.security_*, review.models.*, model_profile_overrides.*, etc. — were rejected with "Unknown config key" when routed through the SDK. Changes: - New sdk/src/query/config-schema.ts mirrors the CJS schema exactly (exact-match keys + dynamic regex sources). - config-mutation.ts imports VALID_CONFIG_KEYS / DYNAMIC_KEY_PATTERNS from the shared module instead of rolling its own set and regex branches. - Drop hand-coded agent_skills.* / features.* regex branches — now schema-driven so claude_md_assembly.blocks.*, review.models.*, and model_profile_overrides.<runtime>.<tier> are also accepted. - Add tests/config-schema-sdk-parity.test.cjs (node:test) as the CI drift guard: asserts CJS VALID_CONFIG_KEYS set-equals the literal set parsed from config-schema.ts, and that every CJS dynamic pattern source has an identical counterpart in the SDK. Parallel to the CJS↔docs parity added in #2479. - Vitest #2653 specs iterate every CJS key through the SDK validator, spot-check each dynamic pattern, and lock in planning.sub_repos. - While here: add workflow.context_coverage_gate to the CJS schema (already in docs and SDK; CJS previously rejected it) and sync the missing curated typo-suggestions (review.model, sub_repos, plan_checker, workflow.review_command) into the SDK. Fixes #2653.
2026-04-24 18:05:16 -04:00
parent e973ff4cb6
commit 387c8a1f9c
6 changed files with 266 additions and 45 deletions
--- 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',
--- 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 ──────────────────────────────────────────────────────
--- 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<string, unkn
 }

 // ─── VALID_CONFIG_KEYS ────────────────────────────────────────────────────
-
-/**
- * Allowlist of valid config key paths.
- *
- * Ported from config.cjs lines 14-37.
- * Dynamic patterns (agent_skills.*, features.*) are handled
- * separately in isValidConfigKey.
- */
-const VALID_CONFIG_KEYS = 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.ui_phase', 'workflow.ui_safety_gate',
-  'workflow.auto_advance', 'workflow.node_repair', 'workflow.node_repair_budget',
-  'workflow.text_mode',
-  'workflow.research_before_questions',
-  'workflow.discuss_mode',
-  'workflow.skip_discuss',
-  'workflow.ui_review',
-  'workflow.max_discuss_passes',
-  'workflow.use_worktrees',
-  'workflow.code_review',
-  'workflow.code_review_depth',
-  'git.branching_strategy', 'git.base_branch', 'git.phase_branch_template',
-  'git.milestone_branch_template', 'git.quick_branch_template',
-  'planning.commit_docs', 'planning.search_gitignored',
-  'workflow.subagent_timeout',
-  'workflow.context_coverage_gate',
-  'hooks.context_warnings',
-  'hooks.workflow_guard',
-  'features.thinking_partner',
-  'features.global_learnings',
-  'learnings.max_inject',
-  'context',
-  'project_code', 'phase_naming',
-  'manager.flags.discuss', 'manager.flags.plan', 'manager.flags.execute',
-  'response_language',
-]);
+// Imported from ./config-schema.js — single source of truth, kept in sync
+// with get-shit-done/bin/lib/config-schema.cjs by a CI parity test (#2653).

 // ─── CONFIG_KEY_SUGGESTIONS (D9 — match CJS config.cjs:57-67) ────────────

@@ -97,9 +62,13 @@ const CONFIG_KEY_SUGGESTIONS: Record<string, string> = {
  '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.<cli-name>',
+  'sub_repos': 'planning.sub_repos',
+  'plan_checker': 'workflow.plan_check',
 };

 // ─── isValidConfigKey ─────────────────────────────────────────────────────
@@ -117,11 +86,10 @@ const CONFIG_KEY_SUGGESTIONS: Record<string, string> = {
 export function isValidConfigKey(keyPath: string): { valid: boolean; suggestion?: string } {
  if (VALID_CONFIG_KEYS.has(keyPath)) return { valid: true };

-  // Dynamic patterns: agent_skills.<agent-type>
-  if (/^agent_skills\.[a-zA-Z0-9_-]+$/.test(keyPath)) return { valid: true };
-
-  // Dynamic patterns: features.<feature_name>
-  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.*.<tier>.
+  if (DYNAMIC_KEY_PATTERNS.some((p) => p.test(keyPath))) return { valid: true };

  // D9: Check curated suggestions before LCP fallback
  if (CONFIG_KEY_SUGGESTIONS[keyPath]) {
--- a/sdk/src/query/config-schema.ts
+++ 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<string> = 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.<agent-type>',
+    test: (k) => /^agent_skills\.[a-zA-Z0-9_-]+$/.test(k),
+  },
+  {
+    source: '^review\\.models\\.[a-zA-Z0-9_-]+$',
+    description: 'review.models.<cli-name>',
+    test: (k) => /^review\.models\.[a-zA-Z0-9_-]+$/.test(k),
+  },
+  {
+    source: '^features\\.[a-zA-Z0-9_]+$',
+    description: 'features.<feature_name>',
+    test: (k) => /^features\.[a-zA-Z0-9_]+$/.test(k),
+  },
+  {
+    source: '^claude_md_assembly\\.blocks\\.[a-zA-Z0-9_]+$',
+    description: 'claude_md_assembly.blocks.<section>',
+    test: (k) => /^claude_md_assembly\.blocks\.[a-zA-Z0-9_]+$/.test(k),
+  },
+  // #2517 — runtime-aware model profile overrides: model_profile_overrides.<runtime>.<tier>
+  {
+    source: '^model_profile_overrides\\.[a-zA-Z0-9_-]+\\.(opus|sonnet|haiku)$',
+    description: 'model_profile_overrides.<runtime>.<opus|sonnet|haiku>',
+    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));
+}
--- 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',
--- a/tests/config-schema-sdk-parity.test.cjs
+++ 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`,
+    );
+  }
+});