Merge branch 'next' into fix/2028-phase-complete-milestone-end-and-workstream-guard

tests/config.test.cjs

@@ -747,6 +747,131 @@ describe('config-set — no silent coercion (#1581)', () => {
   });
 });
 
+// ─── config-set <key> null — unset/clear (#2046) ─────────────────────────────
+
+describe('config-set <key> null — unset/clear (#2046)', () => {
+  let tmpDir;
+
+  beforeEach(() => { tmpDir = createTempProject(); });
+  afterEach(() => { cleanup(tmpDir); });
+
+  test('non-secret routing key: config-set <key> null removes the key (not the string "null")', () => {
+    const setResult = runGsdTools('config-set review.models.gemini foo', tmpDir);
+    assert.ok(setResult.success, `Command failed: ${setResult.error}`);
+    assert.strictEqual(readConfig(tmpDir).review.models.gemini, 'foo');
+
+    const unsetResult = runGsdTools('config-set review.models.gemini null', tmpDir);
+    assert.ok(unsetResult.success, `Command failed: ${unsetResult.error}`);
+
+    const config = readConfig(tmpDir);
+    assert.ok(
+      !config.review || !config.review.models || !Object.prototype.hasOwnProperty.call(config.review.models, 'gemini'),
+      'review.models.gemini must be absent on disk after unset, not the string "null"'
+    );
+
+    const getResult = runGsdTools('config-get review.models.gemini', tmpDir);
+    assert.ok(
+      !getResult.output || !getResult.output.trim() || getResult.output.trim() === 'undefined',
+      `config-get should return empty/undefined after unset, got: ${getResult.output}`
+    );
+    assert.notStrictEqual(getResult.output && getResult.output.trim(), 'null');
+  });
+
+  test('secret key: config-set brave_search null removes the key (never persists "null")', () => {
+    const setResult = runGsdTools('config-set brave_search sk-test-1234', tmpDir);
+    assert.ok(setResult.success, `Command failed: ${setResult.error}`);
+    assert.strictEqual(readConfig(tmpDir).brave_search, 'sk-test-1234');
+
+    const unsetResult = runGsdTools('config-set brave_search null', tmpDir);
+    assert.ok(unsetResult.success, `Command failed: ${unsetResult.error}`);
+
+    const configPath = path.join(tmpDir, '.planning', 'config.json');
+    const rawText = fs.readFileSync(configPath, 'utf-8');
+    const config = JSON.parse(rawText);
+    assert.ok(
+      !Object.prototype.hasOwnProperty.call(config, 'brave_search'),
+      'brave_search must be absent on disk after unset'
+    );
+    assert.doesNotMatch(rawText, /"brave_search":\s*"null"/,
+      'raw config.json must never contain brave_search mapped to the literal string "null"');
+  });
+
+  test('typed-key bypass: config-set context null clears an enum-typed key without validator rejection', () => {
+    const setResult = runGsdTools('config-set context dev', tmpDir);
+    assert.ok(setResult.success, `Command failed: ${setResult.error}`);
+    assert.strictEqual(readConfig(tmpDir).context, 'dev');
+
+    const unsetResult = runGsdTools('config-set context null', tmpDir);
+    assert.ok(unsetResult.success, `config-set context null must succeed (bypass enum validator), got error: ${unsetResult.error}`);
+
+    const config = readConfig(tmpDir);
+    assert.ok(!Object.prototype.hasOwnProperty.call(config, 'context'),
+      'context key must be removed after unset');
+  });
+
+  test('idempotent: config-set <never-set key> null succeeds with no crash and no key written', () => {
+    const result = runGsdTools('config-set git.base_branch null', tmpDir);
+    assert.ok(result.success, `Command failed: ${result.error}`);
+
+    const config = readConfig(tmpDir);
+    assert.ok(
+      !config.git || !Object.prototype.hasOwnProperty.call(config.git, 'base_branch'),
+      'git.base_branch must not be present after unsetting a key that was never set'
+    );
+  });
+
+  test('literal-"null" guard: no config-set <key> null ever persists the literal string "null" on disk', () => {
+    runGsdTools('config-set review.models.gemini foo', tmpDir);
+    runGsdTools('config-set review.models.gemini null', tmpDir);
+    runGsdTools('config-set brave_search sk-test-1234', tmpDir);
+    runGsdTools('config-set brave_search null', tmpDir);
+    runGsdTools('config-set context dev', tmpDir);
+    runGsdTools('config-set context null', tmpDir);
+
+    const configPath = path.join(tmpDir, '.planning', 'config.json');
+    const rawText = fs.readFileSync(configPath, 'utf-8');
+    assert.doesNotMatch(rawText, /:\s*"null"/,
+      `config.json must never contain a key mapped to the literal string "null": ${rawText}`);
+  });
+
+  test('prototype-pollution guard: unsetting a __proto__ leaf is rejected, no pollution (alert #26 parity)', () => {
+    // Create the intermediate so the unset walk reaches the leaf guard (a bare
+    // dynamic-prefix key passes the schema gate, so this exercises the
+    // _unsetNestedValue guard, not the schema gate — mirrors the set-path test).
+    const seed = runGsdTools('config-set agent_skills.sonnet-coder true', tmpDir);
+    assert.ok(seed.success, `seed failed: ${seed.error}`);
+
+    const result = runGsdTools('config-set agent_skills.__proto__ null', tmpDir);
+    assert.strictEqual(result.success, false, `Expected rejection, got: ${result.output}`);
+    assert.ok(
+      result.error.includes('prototype pollution guard'),
+      `Expected pollution-guard error, got: ${result.error}`,
+    );
+    // No prototype pollution occurred via the unset path.
+    assert.strictEqual(({}).polluted, undefined);
+    assert.strictEqual(Object.prototype.hasOwnProperty.call(Object.prototype, 'polluted'), false);
+  });
+
+  test('deep (4-segment) nested key: config-set <deep> null removes only the leaf', () => {
+    const set = runGsdTools('config-set review.reviewer_instances.myinst.model some-model', tmpDir);
+    assert.ok(set.success, `deep set failed: ${set.error}`);
+    assert.strictEqual(readConfig(tmpDir).review.reviewer_instances.myinst.model, 'some-model');
+
+    const unset = runGsdTools('config-set review.reviewer_instances.myinst.model null', tmpDir);
+    assert.ok(unset.success, `deep unset failed: ${unset.error}`);
+
+    const config = readConfig(tmpDir);
+    assert.ok(
+      !config.review.reviewer_instances.myinst ||
+        !Object.prototype.hasOwnProperty.call(config.review.reviewer_instances.myinst, 'model'),
+      'the leaf model key must be removed',
+    );
+    // Parent objects along the path are preserved (unset removes only the leaf).
+    assert.ok(config.review && config.review.reviewer_instances,
+      'parent objects on the path must remain after leaf unset');
+  });
+});
+
 // ─── config-set (research_before_questions and discuss_mode) ──────────────────
 
 describe('config-set research_before_questions and discuss_mode', () => {

tests/core-utils.test.cjs

@@ -533,6 +533,21 @@ describe('extractCanonicalPlanId', () => {
     assert.ok(typeof result === 'string');
     assert.ok(result.length > 0);
   });
+
+  test('rejects single-digit slug word (#2043)', () => {
+    // "46-6-rs-pipeline-orchestrator" is not a valid phase-token filename shape
+    // (the "6" is a slug word, not a sub-phase segment) — must not collapse to
+    // the pre-fix, buggy "46-6".
+    assert.notStrictEqual(
+      coreUtils.extractCanonicalPlanId('46-6-rs-pipeline-orchestrator'),
+      '46-6',
+    );
+    // Legit multi-segment phase-token filenames are still extracted correctly,
+    // including a single-digit letter-suffix phase id ("3A") — the "3A" token is
+    // found and paired with its zero-padded plan index, not left unpaired.
+    assert.strictEqual(coreUtils.extractCanonicalPlanId('01-02-PLAN.md'), '01-02');
+    assert.strictEqual(coreUtils.extractCanonicalPlanId('3A-01-feature-PLAN.md'), '3A-01');
+  });
 });
 
 // ─── countMatchedSummaries (#1988) ───────────────────────────────────────────

tests/health-validation.test.cjs

@@ -1132,6 +1132,23 @@ describe('Drift item W006-archived — MILESTONE_ARCHIVE_DIR_RE and PHASE_TOKEN_
     assert.strictEqual(re.exec('APP1-64-auth')?.[1], '64');
     assert.strictEqual(re.exec('APP_1-64-auth')?.[1], '64');
   });
+
+  test('PHASE_TOKEN_FROM_DIR_RE rejects a single-digit slug word after a phase number (#2043)', () => {
+    const gen = require('../gsd-core/bin/lib/validate.cjs');
+    const re = gen.PHASE_TOKEN_FROM_DIR_RE;
+    // Roadmap phase name "6 Rs Pipeline Orchestrator" slugifies to
+    // "6-rs-pipeline-orchestrator"; the resulting dir "46-6-rs-pipeline-orchestrator"
+    // must extract phase token "46", not "46-6" (was the pre-fix, buggy behavior).
+    assert.strictEqual(re.exec('46-6-rs-pipeline-orchestrator')?.[1], '46');
+    // Legit multi-segment (zero-padded) milestone-prefixed tokens are preserved.
+    assert.strictEqual(re.exec('02-01-setup')?.[1], '02-01');
+    // Single-digit letter-suffix phase ids ("1A"/"01A") and milestone-prefixed
+    // single-digit sub-phases ("M1-2" → "2") must still match (the fix tightens
+    // only the continuation, not the first component).
+    assert.strictEqual(re.exec('1A-foo')?.[1], '1A');
+    assert.strictEqual(re.exec('01A-foo')?.[1], '01A');
+    assert.strictEqual(re.exec('M1-2-setup')?.[1], '2');
+  });
 });
 
 // ── Drift Item I001: canonicalPlanStem ────────────────────────────────────────
@@ -1186,6 +1203,18 @@ describe('Drift item I001 — canonicalPlanStem: long PLAN stem matches short SU
     assert.strictEqual(gen.canonicalPlanStem('3A-01-feature'), '3A-01');
     assert.strictEqual(gen.canonicalPlanStem('no-match'), 'no-match');
   });
+
+  test('canonicalPlanStem rejects a single-digit slug word after a phase number (#2043)', () => {
+    const gen = require('../gsd-core/bin/lib/validate.cjs');
+    // "46-6-rs-pipeline-orchestrator" is not a valid PLAN stem shape (the "6"
+    // is a slug word, not a sub-phase segment), so it must return the input
+    // unchanged rather than the pre-fix, buggy "46-6".
+    assert.notStrictEqual(gen.canonicalPlanStem('46-6-rs-pipeline-orchestrator'), '46-6');
+    // Legit multi-segment stems are still canonicalized correctly, including a
+    // single-digit letter-suffix phase id ("3A") whose plan component is zero-padded.
+    assert.strictEqual(gen.canonicalPlanStem('68-01-scaffolding'), '68-01');
+    assert.strictEqual(gen.canonicalPlanStem('3A-01-feature'), '3A-01');
+  });
 });
   });
 }

tests/phase-id.test.cjs

@@ -217,6 +217,27 @@ describe('extractPhaseToken', () => {
   test('stops at first non-numeric-starting segment', () => {
     assert.strictEqual(phaseId.extractPhaseToken('01-02-name-03'), '01-02');
   });
+
+  test('rejects a single-digit slug word after a phase number (#2043)', () => {
+    // A phase dir like "46-6-rs-pipeline-orchestrator" (roadmap phase name
+    // "6 Rs Pipeline Orchestrator" → slug "6-rs-...") must yield token "46",
+    // not "46-6" — the "6" is the slug's first word, not a sub-phase segment.
+    assert.strictEqual(phaseId.extractPhaseToken('46-6-rs-pipeline-orchestrator'), '46');
+    assert.strictEqual(phaseId.extractPhaseToken('68-6-rs'), '68');
+    // Legit cases are unaffected: a real zero-padded milestone-sub-phase pair
+    // stays intact, and a single-digit sub-phase after a letter-prefixed
+    // milestone id (e.g. "M1-2") is still valid.
+    assert.strictEqual(phaseId.extractPhaseToken('01-02-some-name'), '01-02');
+    assert.strictEqual(phaseId.extractPhaseToken('M1-2-brain'), 'M1-2');
+    // Milestone-prefixed convention: "M1-" strips as a project-code prefix, so
+    // the same rule fixes the slug-collision there too — a phase 46 named
+    // "6 Rs …" under milestone M1 yields "M1-46", not "M1-46-6". Phase 6 under
+    // M1 ("M1-6-rs") correctly stays "M1-6" (the 6 is the phase number).
+    assert.strictEqual(phaseId.extractPhaseToken('M1-46-6-rs-pipeline-orchestrator'), 'M1-46');
+    assert.strictEqual(phaseId.extractPhaseToken('M1-6-rs-pipeline'), 'M1-6');
+    // Single-digit + letter-suffix phase id ("1A") is a real token, not a slug word.
+    assert.strictEqual(phaseId.extractPhaseToken('1A-brain'), '1A');
+  });
 });
 
 // ─── phaseTokenMatches ────────────────────────────────────────────────────────

tests/review-model-config.test.cjs

@@ -4,11 +4,14 @@
  * Verifies the review.models.<cli> dynamic config key pattern:
  *   - isValidConfigKey accepts review.models.<cli-name>
  *   - validateKnownConfigKeyPath suggests review.models.<cli-name> for review.model
- *   - End-to-end round-trip via config-set / config-get for both model IDs and null
+ *   - End-to-end round-trip via config-set / config-get for model IDs and the
+ *     null "Clear" action (#2046 — config-set <key> null unsets the key)
  */
 
 const { test, describe, beforeEach, afterEach } = require('node:test');
 const assert = require('node:assert/strict');
+const fs = require('node:fs');
+const path = require('node:path');
 const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs');
 
 describe('review.models.<cli> config key', () => {
@@ -110,28 +113,49 @@ describe('review.models.<cli> config key', () => {
     );
   });
 
-  test('round-trip: config-set null then config-get returns "null"', () => {
-    // The issue spec documents null as the "fall back to CLI default" sentinel.
-    // cmdConfigSet does not parse 'null' as JSON null — it stores the literal
-    // string 'null'. config-get --raw returns the string 'null', and the
-    // workflow's `[ "$VAR" != "null" ]` guard handles this.
+  test('round-trip: config-set null UNSETS the model key (#2046 — the "Clear" action)', () => {
+    // #2046: `config-set <key> null` now DELETES the key (the documented "Clear"
+    // action) instead of persisting the literal string "null". A previously-set
+    // model override is removed cleanly; config-get then reports key-not-found.
+    // The review workflow's guard (`[ -n "$VAR" ] && [ "$VAR" != "null" ]`,
+    // review.md:259) treats the resulting empty read as "no override → use the
+    // reviewer's default", exactly as it treated the old "null" sentinel.
     const setResult = runGsdTools(
+      ['config-set', 'review.models.gemini', 'gemini-3.1-pro-preview'],
+      tmpDir,
+      { HOME: tmpDir, USERPROFILE: tmpDir }
+    );
+    assert.ok(setResult.success, `config-set failed: ${setResult.error}`);
+
+    const clearResult = runGsdTools(
       ['config-set', 'review.models.gemini', 'null'],
       tmpDir,
       { HOME: tmpDir, USERPROFILE: tmpDir }
     );
-    assert.ok(setResult.success, `config-set null failed: ${setResult.error}`);
+    assert.ok(clearResult.success, `config-set null failed: ${clearResult.error}`);
 
+    // The key is gone from disk — not persisted as the string "null".
+    const configPath = path.join(tmpDir, '.planning', 'config.json');
+    const rawText = fs.readFileSync(configPath, 'utf-8');
+    const config = JSON.parse(rawText);
+    assert.ok(
+      !config.review || !config.review.models ||
+        !Object.prototype.hasOwnProperty.call(config.review.models, 'gemini'),
+      `review.models.gemini must be absent after clear, got: ${rawText}`
+    );
+    assert.doesNotMatch(rawText, /"gemini":\s*"null"/,
+      'must never persist review.models.gemini as the literal string "null"');
+
+    // config-get on the removed key reports not-found (the review workflow reads
+    // it as `... 2>/dev/null || echo ""` → empty → the `[ -n "$VAR" ]` guard falls
+    // back to the reviewer default).
     const getResult = runGsdTools(
       ['config-get', 'review.models.gemini', '--raw'],
       tmpDir,
       { HOME: tmpDir, USERPROFILE: tmpDir }
     );
-    assert.ok(getResult.success, `config-get failed: ${getResult.error}`);
-    assert.strictEqual(
-      getResult.output,
-      'null',
-      'config-get should return the literal string "null" so the workflow guard can match it'
-    );
+    assert.ok(!getResult.success, 'config-get on a cleared key should report not-found');
+    assert.notStrictEqual(getResult.output && getResult.output.trim(), 'null',
+      'config-get must not emit the literal string "null" for a cleared key');
   });
 });

tests/roadmap-parser.test.cjs

@@ -428,6 +428,34 @@ describe('roadmap-parser: getMilestonePhaseFilter', () => {
     assert.strictEqual(filter('02-03-other'), false, '02-03 not in milestone');
   });
 
+  test('single-digit slug word after a phase number is not wrongly excluded (#2043)', () => {
+    // The roadmap uses milestone-prefixed hyphenated phase IDs (e.g. "2-01"),
+    // which switches getMilestonePhaseFilter's dir-matching regex into
+    // hyphenated mode. Phase 46's roadmap name "6 Rs Pipeline Orchestrator"
+    // slugifies to a dir starting with a single-digit word ("46-6-rs-…").
+    // Before #2043, the hyphenated-mode regex over-collected that single
+    // digit into the phase token ("46-6"), which never matched the roadmap's
+    // "46" phase number, so the dir was wrongly excluded from the milestone.
+    writeState(tmpDir, { milestone: 'v1.0' });
+    writeRoadmap(tmpDir, [
+      '## v1.0: Current',
+      '### Phase 2-01: Alpha',
+      '**Goal:** first alpha phase',
+      '',
+      '### Phase 46: 6 Rs Pipeline Orchestrator',
+      '**Goal:** orchestrate the rs',
+    ].join('\n'));
+
+    const filter = getMilestonePhaseFilter(tmpDir);
+    assert.strictEqual(
+      filter('46-6-rs-pipeline-orchestrator'),
+      true,
+      '46-6-rs-pipeline-orchestrator (phase 46, single-digit slug word "6") must match Phase 46',
+    );
+    // Legit milestone-prefixed dir still matches as before.
+    assert.strictEqual(filter('02-01-alpha'), true, '02-01-alpha matches Phase 2-01');
+  });
+
   test('versionOverride uses specified version slice', () => {
     writeRoadmap(tmpDir, [
       '## v1.0: Old',