diff --git a/.changeset/2043-phase-token-single-digit-slug.md b/.changeset/2043-phase-token-single-digit-slug.md new file mode 100644 index 000000000..ce7b100ac --- /dev/null +++ b/.changeset/2043-phase-token-single-digit-slug.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2059 +--- +**Phase directories whose slug begins with a single digit now resolve correctly.** A phase like `46-6-rs-pipeline-orchestrator` (roadmap name "6 Rs Pipeline Orchestrator") had its phase token over-collected as `46-6` instead of `46`, so `gsd-tools` phase-by-number lookups resolved `phase_dir=null` / `has_context=false` (breaking `init.plan-phase`, `init.phase-op`, and downstream execute/verify/ship). Numeric phase-token components must now be zero-padded (≥2 digits), so a single-digit slug word is no longer absorbed into the token. Fixed consistently across every same-class implementation — `extractPhaseToken`, `PHASE_TOKEN_FROM_DIR_RE` and `canonicalPlanStem` (health checks / plan pairing), `isDirInMilestone`'s numeric matcher (milestone filtering), and `extractCanonicalPlanId` — so the health-check and milestone-filter subsystems are fixed alongside phase resolution. diff --git a/.changeset/2046-config-set-null-unset.md b/.changeset/2046-config-set-null-unset.md new file mode 100644 index 000000000..1272e73da --- /dev/null +++ b/.changeset/2046-config-set-null-unset.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2058 +--- +**`gsd-tools config-set null` now clears (removes) the key instead of persisting the literal string `"null"`.** The documented "Clear" action previously fell through the value parser and stored `"null"` — a truthy value — so "cleared" keys stayed set and `config-get` returned `"null"`; for secret keys (`brave_search`/`firecrawl`/`exa_search`) a masked success line hid a truthy value on disk that integrations could pass along as a real credential. `config-set null` now deletes the key (short-circuiting the typed per-key validators so clearing an enum/boolean/number key removes it rather than being rejected), making the "Clear" flows in `settings-integrations.md` / `settings-advanced.md` actually clear. diff --git a/src/config.cts b/src/config.cts index ed42da9dd..0e54cabaa 100644 --- a/src/config.cts +++ b/src/config.cts @@ -38,6 +38,14 @@ interface SetConfigValueResult { previousValue: unknown; } +interface UnsetConfigValueResult { + updated: boolean; + unset: true; + key: string; + value: null; + previousValue: unknown; +} + interface WorkstreamContext { configPath?: string; [key: string]: unknown; @@ -452,6 +460,87 @@ function _setNestedValue( return previousValue; } +/** + * Deletes a value from the config object, allowing nested values via dot + * notation (e.g., "review.models.gemini"). Mirrors `_setNestedValue`'s + * prototype-pollution guard on every path segment (including intermediates). + * + * Unlike `_setNestedValue`, this NEVER creates missing intermediate objects — + * if any segment along the path is missing (or not a plain, non-array + * object), the key doesn't exist and we return early without mutating + * `config` at all. + * + * Does not prune now-empty parent objects after deletion (matches the + * conservative, structure-preserving behaviour callers expect from a bare + * unset). + * + * Returns { previousValue, existed } — existed is false when the leaf key + * (or an intermediate segment) was never present. + * Calls error() (process.exit(1)) on prototype-pollution attempts. + */ +function _unsetNestedValue( + config: Record, + keyPath: string, +): { previousValue: unknown; existed: boolean } { + const keys = keyPath.split('.'); + let current: Record = config; + for (let i = 0; i < keys.length - 1; i++) { + const key = keys[i]; + if (key === '__proto__' || key === 'prototype' || key === 'constructor') { + error('Invalid config key (prototype pollution guard): ' + keyPath, ERROR_REASON.CONFIG_PARSE_FAILED); + } + const existingChild = current[key]; + if (existingChild === undefined || existingChild === null || typeof existingChild !== 'object' || Array.isArray(existingChild)) { + // Path doesn't exist — nothing to unset, and we must not create it. + return { previousValue: undefined, existed: false }; + } + current = existingChild as Record; + } + const lastKey = keys[keys.length - 1]; + if (lastKey === '__proto__' || lastKey === 'prototype' || lastKey === 'constructor') { + error('Invalid config key (prototype pollution guard): ' + keyPath, ERROR_REASON.CONFIG_PARSE_FAILED); + } + const existed = Object.prototype.hasOwnProperty.call(current, lastKey); + const previousValue = current[lastKey]; + if (existed) { + delete current[lastKey]; + } + return { previousValue, existed }; +} + +/** + * Deletes a key from the config file, allowing nested values via dot + * notation. Mirrors `setConfigValue`'s load/lock/write cycle. + * + * Does not call `output()`, so can be used as one step in a command without triggering `exit(0)` in + * the happy path. But note that `error()` will still `exit(1)` out of the process. + */ +function unsetConfigValue(cwd: string, keyPath: string): UnsetConfigValueResult { + const configPath = path.join(planningDir(cwd), 'config.json'); + + return withPlanningLock(cwd, () => { + // Load existing config or start with empty object + let config: Record = {}; + try { + if (fs.existsSync(configPath)) { + config = JSON.parse(fs.readFileSync(configPath, 'utf-8')) as Record; + } + } catch (err) { + error('Failed to read config.json: ' + (err as Error).message, ERROR_REASON.CONFIG_PARSE_FAILED); + } + + const { previousValue, existed } = _unsetNestedValue(config, keyPath); + + // Write back + try { + platformWriteSync(configPath, JSON.stringify(config, null, 2)); + return { updated: existed, unset: true, key: keyPath, value: null, previousValue }; + } catch (err) { + error('Failed to write config.json: ' + (err as Error).message); + } + }) as UnsetConfigValueResult; +} + /** * Sets a value in the config file, allowing nested values via dot notation (e.g., * "workflow.research"). @@ -586,6 +675,7 @@ function cmdConfigSet(cwd: string, keyPath: string | undefined, value: string | let parsedValue: unknown = val; if (val === 'true') parsedValue = true; else if (val === 'false') parsedValue = false; + else if (val === 'null') parsedValue = null; // #1581: Number.isFinite (not !isNaN) so 'Infinity'/'-Infinity' are NOT // coerced to non-finite numbers that JSON.stringify later renders as `null` // (disk=null while the CLI echoed 'Infinity'). They fall through to the @@ -596,6 +686,25 @@ function cmdConfigSet(cwd: string, keyPath: string | undefined, value: string | try { parsedValue = JSON.parse(val); } catch { /* keep as string */ } } + // #2046: a bare `null` unsets (deletes) the key — the documented "Clear" action. + // Short-circuits before every typed per-key validator so clearing a typed key + // (enum/boolean/number) removes it rather than being rejected. Deleting (not + // persisting JSON null) is the correct "clear": a persisted null is still a + // present, truthy-adjacent value that consumers must special-case — worst for + // secret keys where a leftover value can be passed as a real credential. + if (parsedValue === null) { + const unsetResult = unsetConfigValue(cwd, kp); + if (isSecretKey(kp)) { + const maskedPrev = unsetResult.previousValue === undefined + ? undefined + : maskSecret(unsetResult.previousValue as Parameters[0]); + output({ ...unsetResult, value: null, previousValue: maskedPrev, masked: true }, raw, `${kp} unset`); + return; + } + output(unsetResult, raw, `${kp} unset`); + return; + } + // #1581: project_code is an identifier string — never number-coerce it. A // leading-zero code like '007' must persist verbatim (not collapse to 7). if (kp === 'project_code') { diff --git a/src/core-utils.cts b/src/core-utils.cts index 9c02a6d1c..ec6f18f5c 100644 --- a/src/core-utils.cts +++ b/src/core-utils.cts @@ -178,7 +178,10 @@ function timeAgo(date: Date): string { function extractCanonicalPlanId(filename: string): string { const base = filename.replace(/-PLAN\.md$/i, '').replace(/-SUMMARY\.md$/i, '').replace(/\.md$/i, ''); const parts = base.split('-').filter(Boolean); - const tokenRe = /^\d+[A-Z]?(?:\.\d+)*$/i; + // #2043: a phase/plan token component is either a zero-padded number (≥2 digits) + // or a single-digit-plus-letter id ("3A"); a *bare* single digit is a slug word, + // so "46-6-rs-…" is not paired into a "46-6" id while "3A-01" stays intact. + const tokenRe = /^(?:\d{2,}[A-Z]?|\d[A-Z])(?:\.\d+)*$/i; const phaseIdx = parts.findIndex(p => tokenRe.test(p)); if (phaseIdx >= 0 && phaseIdx + 1 < parts.length && tokenRe.test(parts[phaseIdx + 1])) { return `${parts[phaseIdx]}-${parts[phaseIdx + 1]}`; diff --git a/src/phase-id.cts b/src/phase-id.cts index a401e3168..07d0799e6 100644 --- a/src/phase-id.cts +++ b/src/phase-id.cts @@ -205,9 +205,29 @@ function extractPhaseToken(dirName: string): string { const segments = rest.split('-'); const tokenSegments: string[] = []; + // #2043: distinguish a real (zero-padded, ≥2-digit) phase/sub-phase segment + // from a single-digit slug word. A pure-numeric leading segment ("46") only + // continues with ≥2-digit segments, so "46-6-rs-…" yields "46" (the "6" is the + // slug's first word), not "46-6". Milestone-prefixed ids like "M1-2" reach here + // with "M1-" already stripped as a project-code prefix (see + // PROJECT_CODE_PREFIX_CAPTURE_RE_I), so "2" is the leading segment and the same + // pure-numeric rule applies (M1-46-6-rs → "M1-46"). The firstLetterPrefixed + // carve-out covers letter+digit leading segments that survive prefix stripping + // because of punctuation (e.g. "P0.3-2"), whose single-digit continuation is + // intentionally preserved (unchanged from prior behaviour). + let firstLetterPrefixed = false; for (let i = 0; i < segments.length; i++) { const seg = segments[i]; - if (/^\d/.test(seg) || (i === 0 && /^[A-Za-z]{1,3}\d/.test(seg))) { + if (i === 0) { + if (/^\d/.test(seg)) { + tokenSegments.push(seg); + } else if (/^[A-Za-z]{1,3}\d/.test(seg)) { + tokenSegments.push(seg); + firstLetterPrefixed = true; + } else { + break; + } + } else if (/^\d{2,}/.test(seg) || (firstLetterPrefixed && /^\d/.test(seg))) { tokenSegments.push(seg); } else { break; diff --git a/src/phase.cts b/src/phase.cts index 526f64df3..51d18284b 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -104,7 +104,10 @@ function extractCanonicalPlanId(filename: string): string { .replace(/-SUMMARY\.md$/i, '') .replace(/\.md$/i, ''); const parts = base.split('-').filter(Boolean); - const tokenRe = /^\d+[A-Z]?(?:\.\d+)*$/i; + // #2043: a phase/plan token component is either a zero-padded number (≥2 digits) + // or a single-digit-plus-letter id ("3A"); a *bare* single digit is a slug word, + // so "46-6-rs-…" is not paired into a "46-6" id while "3A-01" stays intact. + const tokenRe = /^(?:\d{2,}[A-Z]?|\d[A-Z])(?:\.\d+)*$/i; const phaseIdx = parts.findIndex((p) => tokenRe.test(p)); if (phaseIdx >= 0 && phaseIdx + 1 < parts.length && tokenRe.test(parts[phaseIdx + 1])) { return `${parts[phaseIdx]}-${parts[phaseIdx + 1]}`; diff --git a/src/roadmap-parser.cts b/src/roadmap-parser.cts index d4523c98b..1dd7c1c99 100644 --- a/src/roadmap-parser.cts +++ b/src/roadmap-parser.cts @@ -469,8 +469,12 @@ function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, p } const roadmapUsesHyphenedIds = [...normalized].some(n => n.includes('-')); + // #2043: milestone-prefixed sub-phase components must be zero-padded (≥2 digits) + // — "-\d{2,}" instead of "-0*\d+" — so a single-digit slug word after the phase + // number (e.g. dir "46-6-rs-…") captures "46" and is not silently excluded from + // the milestone as a bogus "46-6" id. const numericRe = roadmapUsesHyphenedIds - ? /^0*(\d+(?:-0*\d+)*[A-Za-z]?(?:\.\d+)*)/ + ? /^0*(\d+(?:-\d{2,})*[A-Za-z]?(?:\.\d+)*)/ : /^0*(\d+[A-Za-z]?(?:\.\d+)*)/; function isDirInMilestone(dirName: string): boolean { diff --git a/src/validate.cts b/src/validate.cts index c1167a4d5..5ed32d96c 100644 --- a/src/validate.cts +++ b/src/validate.cts @@ -44,16 +44,25 @@ export const phaseDirNameRe = new RegExp( ); // Extracts the full phase token from a directory name, including milestone-prefixed // multi-segment tokens like "02-01" from "02-01-setup" or "GSD-02-01-setup". -// Greedily captures all leading all-digit segments before the first letter-start segment. +// #2043: a *continuation* sub-phase segment must be zero-padded (≥2 digits), so a +// single-digit slug word after a phase number (e.g. "46-6-rs-…", slug "6 Rs …") is +// NOT absorbed — it captures "46", not "46-6". The first component stays "\d+" +// (with the "[A-Z]?" suffix) so single-digit letter-suffixed phase ids ("1A") and +// milestone-prefixed single-digit sub-phases ("M1-2" → prefix "M1-" stripped, then +// "2") still match. The trailing boundary "(?:-|$)" (was "(?:-[a-z]|$)") lets a slug +// that starts with a digit terminate the token. export const PHASE_TOKEN_FROM_DIR_RE = new RegExp( - `^${OPTIONAL_PROJECT_CODE_PREFIX_SOURCE}(\\d+(?:-\\d+)*[A-Z]?(?:\\.\\d+)*)(?:-[a-z]|$)`, + `^${OPTIONAL_PROJECT_CODE_PREFIX_SOURCE}(\\d+(?:-\\d{2,})*[A-Z]?(?:\\.\\d+)*)(?:-|$)`, 'i', ); export const MILESTONE_ARCHIVE_DIR_RE = /^v\d+.*-phases$/i; // ── Issue #26: I001 canonicalization ──────────────────────────────────────── export function canonicalPlanStem(stem: string): string { - const m = stem.match(/^(\d+[A-Z]?(?:\.\d+)*-\d+)/i); + // #2043: the plan component (after the phase number) must be zero-padded + // (≥2 digits), so a digit-leading slug word (e.g. "46-6-rs-…") is not mistaken + // for a "46-6" phase/plan pair. + const m = stem.match(/^(\d+[A-Z]?(?:\.\d+)*-\d{2,})/i); return m ? m[1] : stem; } diff --git a/tests/config.test.cjs b/tests/config.test.cjs index 13a25ecf0..96d67e10e 100644 --- a/tests/config.test.cjs +++ b/tests/config.test.cjs @@ -747,6 +747,131 @@ describe('config-set — no silent coercion (#1581)', () => { }); }); +// ─── config-set null — unset/clear (#2046) ───────────────────────────── + +describe('config-set null — unset/clear (#2046)', () => { + let tmpDir; + + beforeEach(() => { tmpDir = createTempProject(); }); + afterEach(() => { cleanup(tmpDir); }); + + test('non-secret routing key: config-set 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 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 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 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', () => { diff --git a/tests/core-utils.test.cjs b/tests/core-utils.test.cjs index 09e78c9e9..c994e0cdf 100644 --- a/tests/core-utils.test.cjs +++ b/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) ─────────────────────────────────────────── diff --git a/tests/health-validation.test.cjs b/tests/health-validation.test.cjs index 957d3c7d9..94a7841f6 100644 --- a/tests/health-validation.test.cjs +++ b/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'); + }); }); }); } diff --git a/tests/phase-id.test.cjs b/tests/phase-id.test.cjs index 957723c8e..94746235b 100644 --- a/tests/phase-id.test.cjs +++ b/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 ──────────────────────────────────────────────────────── diff --git a/tests/review-model-config.test.cjs b/tests/review-model-config.test.cjs index 3777108e7..4ae6c0d3b 100644 --- a/tests/review-model-config.test.cjs +++ b/tests/review-model-config.test.cjs @@ -4,11 +4,14 @@ * Verifies the review.models. dynamic config key pattern: * - isValidConfigKey accepts review.models. * - validateKnownConfigKeyPath suggests review.models. 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 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. config key', () => { @@ -110,28 +113,49 @@ describe('review.models. 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 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'); }); }); diff --git a/tests/roadmap-parser.test.cjs b/tests/roadmap-parser.test.cjs index 0f9280df9..f86b8fcc4 100644 --- a/tests/roadmap-parser.test.cjs +++ b/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',