Files
msd-core/src/phase-command-router.cts
Tom Boucher 5e0a7b1b56 fix(#4433,#4569,#4126): consolidate the phase-identity seam at name-validity, allocation, and branch-slug (#4640)
* fix(#4433): apply the name-validity guard symmetrically to every milestone-name capture

extractMilestoneHeadingName already refused a punctuation-only captured name
(#4134), but its two sibling capture sites in getMilestoneInfo — the
STATE.md-anchored 🚧-bullet match and the no-STATE.md in-progress 🚧-bullet
fallback — skipped straight to a bare truthiness check, so a malformed bullet
whose only content past the version was punctuation passed through as a real
milestone name.

Extracts the existing inline /[\p{L}\p{N}]/u check into a single shared
hasNameableContent predicate and applies it at all three capture sites, so
the guard is one owner rather than a copy that happened to land at only one
of them.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* test(#4433): pin the name-validity guard at all three milestone-name capture sites

Failing-first coverage for the hasNameableContent extraction: a
punctuation-only 🚧-bullet name must not surface as a real milestone name,
either on the STATE.md-anchored path or the no-STATE.md in-progress
fallback, while a real name (including a digits-only one) still resolves
COMPLETE exactly as before.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(#4569): consolidate decimal-phase-number allocation into one function

cmdPhaseInsert allocated its next decimal sub-phase number by scanning only
on-disk phases/ directories and ### Phase N.M: headings, never the roadmap
summary checklist — so a decimal that existed only as a checklist bullet
(no heading yet, no on-disk directory yet) was invisible, and phase insert
could silently reallocate an already-used number. It also always nested one
level deeper under afterPhase, with no way to request a sibling.

cmdPhaseNextDecimal had its own separate, near-identical two-source scan
(missing the checklist source too) — the exact "duplicate implementations
kept in sync instead of deleted" pattern this issue exists to close.

Extracts scanExistingDecimalPhaseNumbers (directories + headings + checklist
bullets, in one place) and migrates both cmdPhaseInsert and
cmdPhaseNextDecimal onto it — deleting cmdPhaseNextDecimal's own copy rather
than patching it in parallel. Adds an allocation: 'nested' | 'sibling'
argument to cmdPhaseInsert (default 'nested', matching every existing
caller's behavior); a top-level phase with no existing decimal segment falls
back to nested since there is no sibling level to join. No CLI flag wires
'sibling' yet — that is a separate, disclosed follow-up.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* test(#4569): pin decimal-allocation coverage across phase insert and next-decimal

Failing-first coverage for scanExistingDecimalPhaseNumbers: a checklist-only
decimal must not be reallocated by phase insert; a decimal present in
heading, checklist, and on-disk directory simultaneously must count once;
an unrelated phase family's checklist bullet must not cross-pollute; and
phase next-decimal (migrated onto the same shared helper) must see a
checklist-only decimal too, closing the same gap in a second command.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* chore(#4634): extend the phase-id drift guard for name-validity and shell arithmetic

The epic's ratchet requirement: lint-phase-id-drift.cjs must cover the two
new predicates this PR introduces, and must also scan shell inside
gsd-core/workflows/**/*.md and gsd-core/references/**/*.md for
integer-coercing phase-number arithmetic ($((10#...)) and friends), which
neither the canonical TypeScript module nor a source-only lint can reach.

Adds findNameValidityDrift (bans re-deriving /[\p{L}\p{N}]/u outside
hasNameableContent's owner file) and findShellPhaseArithDrift +
scanMarkdownShellArith (bans $((10#...)) in workflow/reference markdown,
sanctioned via <!-- phase-id-owner: --> on the preceding line). scanRepo
keeps its existing, narrower contract (src/**/*.cts only) so the
already-passing "the live repo is clean" test is untouched; a new scanAll
merges both for the CLI's full report.

Running the guard directly against this tree correctly reports the 7
pre-existing #4619 shell sites (workflows/execute-phase.md x4,
workflows/execute-phase/steps/completion-reconciliation.md x2,
references/tdd.md x1) as violations — demonstrating the ratchet works, not
fixing them. #4619 is a live regression tracked and fixed separately; this
PR does not touch those markdown files. A characterization test pins the
current count of 7 so a future change to that number is investigated rather
than silently absorbed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(#4569): wire --sibling through phase insert's CLI so the argument is reachable

cmdPhaseInsert's allocation parameter had no CLI path to 'sibling' — shipped,
untested, unreachable code (code-review finding: a guaranteed surviving
mutant). Adds --sibling to phase insert's argument parsing, threads it
through, and documents the flag in docs/CLI-TOOLS.md.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* test(#4569): exercise --sibling end-to-end through the real CLI

Confirms --sibling joins afterPhase's parent decimal level rather than
nesting, and falls back to nested when afterPhase has no existing decimal
segment (no sibling level to join).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* test(#4634): demonstrate the two new drift detectors end-to-end via a planted violation

The epic asks for the guard to be "demonstrated by watching it go red" on a
reintroduced copy. The two new detectors (name-validity, shell-arith) had
only unit-level fixture tests; mirrors the existing bracket-rule's
planted-violation-in-a-temp-tree test for both, proving they're actually
wired into scanRepo/scanMarkdownShellArith end-to-end, not just correct in
isolation.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* chore(#4634): consolidate the drift guard's own owner-sanction-check logic

Standards review flagged the "walk to nearest preceding non-blank line,
check for a phase-id-owner comment" logic as duplicated across all four
detector functions in a PR whose whole point is eliminating exactly that
pattern. Extracts isSanctionedByPrecedingComment, shared by all four;
behavior-preserving (verified: identical output before/after, same 7 known
#4619 violations, zero token/bracket/name-validity).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* chore(#4634): add Fixed changeset for the name-validity guard and allocation consolidation

pr:0 placeholder — backfilled once the real PR number exists.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(#4126): consolidate branch-name slug substitution into one shared renderer

cmdCommit (commands.cts) and cmdInitExecutePhase (init.cts) each
independently implemented branch-name template substitution, and both
substituted the literal string 'phase' when phase_slug was empty or
undeliverable — producing a non-identifying branch name (gsd/phase-08-phase)
that contradicted the honestly-reported phase_slug: null in the same
payload. Same structural defect as the other three gaps in this epic: two
consumers reimplementing one concept independently instead of sharing an
owner.

Adds renderPhaseBranchName (src/phase-id.cts) as the sole owner: a real slug
substitutes normally; an empty/undeliverable one drops the {slug} token plus
one adjacent separator (collapsing/trimming the result) rather than
substituting a placeholder word, for the shipped default template and any
user-configured shape alike. Both call sites now delegate to it; the old
inline duplicates are deleted, not kept in sync. {project} substitution
stays a separate step in init.cts, unchanged, since it is a config-level
field with its own fallback contract.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* test(#4126): pin renderPhaseBranchName and both migrated call sites

Property-based coverage for the shared renderer's degrade-path invariant
(output, when non-null, never contains {slug} and never starts/ends with a
separator), plus example coverage for real-slug substitution, empty/null/
non-string slug, token position at either edge, a doubled-separator
template, and the only-{slug} -> null case. One regression test each in
commands.test.cjs and init.test.cjs confirms a phase with no derivable slug
no longer produces a branch name ending in the literal '-phase'.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix: route scanExistingDecimalPhaseNumbers through the canonical enumeration owner

Caught by an actual gsd-test run, not a hypothesis: the new decimal-scan
helper (fix(#4569)) enumerated phases/ directories via a raw
fs.readdirSync, which the pre-existing phase-enumeration drift guard
(#3185/#3882) correctly flags as an unsanctioned re-derivation outside its
canonical owner (listAllPhaseDirs / isSentinelPhaseId). Ironic given this
epic's own thesis, and exactly why the guard exists: consolidating one seam
can reintroduce drift in an adjacent one if the new code doesn't route
through what's already there. Migrates the enumeration to listAllPhaseDirs;
identical decimal-detection output for every existing case.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* chore(#4634): extend the drift guard for branch-slug fallback; fix a real regex bug

Adds the fourth detector the epic's ratchet section names ("both
branch-name sites"): bans a `.replace('{slug}', ... || 'phase')` call
outright, sanctioned via renderPhaseBranchName or a dedicated comment.
Wired into scanRepo (no per-file exemption — this is a banned anti-pattern
everywhere, not a grammar with one legitimate owner). Now that #4126's fix
(prior commit) has landed, scanRepo reports zero violations across all four
.cts-scanning rules, restoring the simple "the live repo is clean" assertion
instead of a pinned-known-count characterization.

Also fixes a real bug an actual gsd-test run caught: findNameValidityDrift's
regex didn't tolerate the doubled-backslash template-string form its own
test claimed to cover (0 !== 1) — widened to \{1,2} matching
TOKEN_DRIFT_RE's existing tolerance for the same two forms.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* docs(#4126): document the {slug} degrade behavior; update changeset for the full seam

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix: detectPhaseNumberFromFiles wrongly rejected bare, slug-less phase directories

Caught by an actual gsd-test run on the #4126 regression test, not a
hypothesis: a bare phase directory with no slug remainder (e.g.
.planning/phases/01/) has extractPhaseToken correctly return "01" — which is
simply identical to the directory name in that case, not its no-match
fallback. A stale `token !== phaseDir` check treated that equality as "no
numeric token found" and rejected it regardless, leaving phaseNum null and
silently skipping cmdCommit's phase-branching block entirely (the commit
proceeded on whatever branch was already checked out instead of the
phase branch).

phaseTokenShape.test(normalized) already excludes every genuine non-phase
case on its own: extractPhaseToken's real no-match fallback only fires for a
dirName that doesn't start with a digit or short letter+digit prefix, and
normalizePhaseName's leading-\d+ requirement rejects those regardless. The
equality check was redundant for real rejections and actively wrong for
bare-numeric directories.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* chore: backfill changeset PR number to 4640

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: sim <sim@local>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
2026-09-11 12:42:04 -04:00

319 lines
14 KiB
TypeScript

/**
* Manifest-backed phase subcommand router.
* Keeps gsd-tools.cjs thin while preserving existing command semantics.
*
* Unsupported in this router:
* - scaffold: routed through top-level scaffold command.
*
* CJS-only subcommands: mvp-mode, tdd-applicable (dispatched directly, before hub).
*
* #3788: dispatch is mediated by CommandRoutingHub. The public entry point
* and observable CLI behaviour are unchanged.
*
* ADR-457 build-at-publish: the hand-written bin/lib/phase-command-router.cjs
* collapsed to a TypeScript source of truth. Behaviour is preserved byte-for-behaviour
* from the prior hand-written .cjs; only types are added.
*/
import { PHASE_SUBCOMMANDS } from './command-aliases.cjs';
// ─── CommandRoutingHub (issue #3788, simplified in #175, typed in #176) ───────
// eslint-disable-next-line @typescript-eslint/no-require-imports
import commandRoutingHub = require('./command-routing-hub.cjs');
const { createHub, ERROR_KINDS, makeInvalidArgs } = commandRoutingHub;
// #2620 (ADR-0174 §6): inject the reference DispatchLogger on the live phase
// dispatch path, but only when observability is opt-in enabled; otherwise the
// Hub stays byte-for-byte silent via its no-op fallback.
// eslint-disable-next-line @typescript-eslint/no-require-imports
import observabilityLogger = require('./observability/logger.cjs');
const { createDefaultLogger, isAuditEnabled } = observabilityLogger;
// ─── Types ────────────────────────────────────────────────────────────────────
interface PhaseHandlers {
cmdPhaseMvpMode: (cwd: string, args: string[], raw: boolean) => void;
cmdPhaseTddApplicable: (cwd: string, args: string[], raw: boolean) => void;
cmdPhaseNextDecimal: (cwd: string, arg: string | undefined, raw: boolean) => void;
cmdPhaseAdd: (cwd: string, desc: string, raw: boolean, customId: string | null) => void;
cmdPhaseAddBatch: (cwd: string, descriptions: string[], raw: boolean) => void;
cmdPhaseInsert: (
cwd: string,
pos: string | undefined,
desc: string,
raw: boolean,
allocation?: 'nested' | 'sibling',
) => void;
cmdPhaseRemove: (cwd: string, phaseNum: string, opts: { force: boolean }, raw: boolean) => void;
cmdPhaseComplete: (cwd: string, phaseNum: string | undefined, raw: boolean) => void;
cmdPhaseUatPassed: (cwd: string, phaseNum: string | undefined, raw: boolean, opts?: { policy?: { requireVerification?: boolean } }) => void;
cmdPhaseListPlans: (cwd: string, phaseNum: string | undefined, raw: boolean) => void;
}
interface RoutePhaseCommandOptions {
phase: PhaseHandlers;
args: string[];
cwd: string;
raw: boolean;
error: (message: string) => void;
}
// ─── Implementation ───────────────────────────────────────────────────────────
function routePhaseCommand({ phase, args, cwd, raw, error }: RoutePhaseCommandOptions): void {
// ── Unsupported subcommands ─────────────────────────────────────────────────
// Resolved before dispatch so the error message stays deterministic.
const UNSUPPORTED: Record<string, string> = {
scaffold: 'phase scaffold is routed through the top-level scaffold command.',
};
const subcommand = args[1];
if (subcommand && UNSUPPORTED[subcommand]) {
error(UNSUPPORTED[subcommand]);
return;
}
// ── No subcommand → reject early with helpful error ────────────────────────
// Pre-#3788 code resolved unknown subcommands via routeCjsCommandFamily which
// fell through to error() when no handler matched (including undefined).
// Post-#3788 the hub's manifest check is skipped for falsy subcommand, so we
// must guard here to preserve the deterministic "Available: ..." error message.
if (!subcommand) {
const available = PHASE_SUBCOMMANDS.filter(s => !UNSUPPORTED[s]).join(', ');
error(`Unknown phase subcommand. Available: ${available}`);
return;
}
// ── CJS-only subcommands (dispatched directly, before hub) ─────────────────
// `mvp-mode` has a CJS-native implementation in phase.cmdPhaseMvpMode that
// differs from the SDK query layer (different ROADMAP scan + error codes).
// Dispatch it early to preserve pre-migration observable behaviour (correct
// exit code, correct JSON error reason code, correct ROADMAP scan).
if (subcommand === 'mvp-mode') {
phase.cmdPhaseMvpMode(cwd, args.slice(2), raw);
return;
}
// `tdd-applicable` (#4273): same CJS-native dispatch shape as `mvp-mode`
// immediately above — a precedence cascade over plan/task/config sources
// with its own typed JSON result, not routed through the SDK query layer.
if (subcommand === 'tdd-applicable') {
phase.cmdPhaseTddApplicable(cwd, args.slice(2), raw);
return;
}
// ── Build the CJS registry ──────────────────────────────────────────────────
// Each handler receives a ctx object from the hub and must return a HubResult.
const cjsRegistry = {
phase: {
'next-decimal': (_ctx: Record<string, unknown>): { ok: true; data: null } => {
phase.cmdPhaseNextDecimal(cwd, args[2], raw);
return { ok: true as const, data: null };
},
add: (_ctx: Record<string, unknown>) => {
let customId: string | null = null;
const descArgs: string[] = [];
for (let i = 2; i < args.length; i++) {
const token = args[i];
if (token === '--raw') {
continue;
}
if (token === '--id') {
const id = args[i + 1];
if (!id || id.startsWith('--')) {
return makeInvalidArgs('--id', '--id requires a value');
}
customId = id;
i++;
} else if (token.startsWith('--')) {
return makeInvalidArgs(token, `phase add does not support ${token}`);
} else {
descArgs.push(token);
}
}
phase.cmdPhaseAdd(cwd, descArgs.join(' '), raw, customId);
return { ok: true as const, data: null };
},
'add-batch': (_ctx: Record<string, unknown>) => {
const descFlagIdx = args.indexOf('--descriptions');
let descriptions: string[];
if (descFlagIdx !== -1) {
const rawDescriptions = args[descFlagIdx + 1];
if (!rawDescriptions || rawDescriptions.startsWith('--')) {
return makeInvalidArgs('--descriptions', '--descriptions must be a JSON array');
}
try {
descriptions = JSON.parse(rawDescriptions) as string[];
} catch {
return makeInvalidArgs('--descriptions', '--descriptions must be a JSON array');
}
if (!Array.isArray(descriptions)) {
return makeInvalidArgs('--descriptions', '--descriptions must be a JSON array');
}
} else {
descriptions = args.slice(2).filter(a => a !== '--raw');
}
phase.cmdPhaseAddBatch(cwd, descriptions, raw);
return { ok: true as const, data: null };
},
insert: (_ctx: Record<string, unknown>) => {
if (args.includes('--dry-run')) {
return makeInvalidArgs('--dry-run', 'phase insert does not support --dry-run');
}
// #4569: --sibling opts into joining afterPhase's parent decimal level
// instead of nesting one level deeper. Filtered out like other
// boolean flags (see `remove`'s --force handling above) so it never
// leaks into the free-text description.
const sibling = args.includes('--sibling');
const insertArgs = args.slice(2).filter(token => token !== '--sibling');
phase.cmdPhaseInsert(
cwd,
insertArgs[0],
insertArgs.slice(1).join(' '),
raw,
sibling ? 'sibling' : 'nested',
);
return { ok: true as const, data: null };
},
remove: (_ctx: Record<string, unknown>) => {
const removeArgs = args.slice(2).filter(token => token !== '--raw');
let forceFlag = false;
const positional: string[] = [];
for (const token of removeArgs) {
if (token === '--force') {
forceFlag = true;
continue;
}
if (token.startsWith('--')) {
return makeInvalidArgs(token, `phase remove does not support ${token}`);
}
positional.push(token);
}
if (positional.length !== 1) {
return makeInvalidArgs('<phase-number>', 'phase remove accepts exactly one phase number');
}
phase.cmdPhaseRemove(cwd, positional[0], { force: forceFlag }, raw);
return { ok: true as const, data: null };
},
complete: (_ctx: Record<string, unknown>): { ok: true; data: null } => {
// #2201: accept --phase N as well as the positional form (the state
// family already accepts --phase). An unrecognized flag is a usage
// error, not "Phase --phase not found".
let phaseNum: string | null = null;
for (let i = 2; i < args.length; i++) {
if (args[i] === '--phase') {
phaseNum = args[++i];
if (!phaseNum || phaseNum.startsWith('--'))
return makeInvalidArgs('--phase', '--phase requires a value') as never;
} else if (args[i].startsWith('--phase=')) {
phaseNum = args[i].slice(8);
} else if (args[i] === '--raw') {
continue;
} else if (args[i].startsWith('--')) {
return makeInvalidArgs(args[i], `phase complete does not support ${args[i]}`) as never;
} else {
phaseNum = args[i];
}
}
if (!phaseNum)
return makeInvalidArgs('--phase', 'phase number required (positional or --phase N)') as never;
phase.cmdPhaseComplete(cwd, phaseNum, raw);
return { ok: true as const, data: null };
},
'uat-passed': (_ctx: Record<string, unknown>): { ok: true; data: null } => {
let requireVerification = false;
const positional: string[] = [];
for (const token of args.slice(2)) {
if (token === '--require-verification') {
requireVerification = true;
} else if (token === '--raw') {
// --raw is handled by the outer CLI layer; accepted here silently
} else if (token.startsWith('--')) {
return makeInvalidArgs(token, `phase uat-passed does not support ${token}`) as never;
} else {
positional.push(token);
}
}
phase.cmdPhaseUatPassed(cwd, positional[0], raw, { policy: { requireVerification } });
return { ok: true as const, data: null };
},
// #1437 — list plan files for a phase
'list-plans': (_ctx: Record<string, unknown>): { ok: true; data: null } => {
// #2201: accept --phase N as well as positional.
let phaseNum: string | null = null;
for (let i = 2; i < args.length; i++) {
if (args[i] === '--phase') {
phaseNum = args[++i];
if (!phaseNum || phaseNum.startsWith('--'))
return makeInvalidArgs('--phase', '--phase requires a value') as never;
} else if (args[i].startsWith('--phase=')) {
phaseNum = args[i].slice(8);
} else if (args[i] === '--raw') {
continue;
} else if (args[i].startsWith('--')) {
return makeInvalidArgs(args[i], `phase list-plans does not support ${args[i]}`) as never;
} else {
phaseNum = args[i];
}
}
if (!phaseNum)
return makeInvalidArgs('--phase', 'phase number required (positional or --phase N)') as never;
phase.cmdPhaseListPlans(cwd, phaseNum, raw);
return { ok: true as const, data: null };
},
},
};
// ── Build manifest (available subcommands for UnknownCommand detection) ─────
// `availableSubcommands` is what the error message shows. It excludes
// unsupported commands (already handled above) but does NOT include 'mvp-mode'
// or 'tdd-applicable' (#4273) because both are absent from PHASE_SUBCOMMANDS
// and were not shown in the "Available:" list there either.
//
// `manifestSubcommands` is the full routing set for the hub — it includes
// 'mvp-mode' and 'tdd-applicable' (both routed via a handler even without a
// manifest entry) so the hub's UnknownCommand check passes for them.
const availableSubcommands = PHASE_SUBCOMMANDS.filter(s => !UNSUPPORTED[s]);
const manifestSubcommands = ['mvp-mode', 'tdd-applicable', ...availableSubcommands];
const manifest = { phase: manifestSubcommands };
// ── Construct hub ──────────────────────────────────────────────────────────
// #175: Hub is CJS-only — no mode param, no sdkLoader.
// #2620: wire the reference logger (ADR-0174 §6) only when observability is
// opt-in enabled; otherwise leave it unset so the Hub stays byte-for-byte
// silent via its no-op fallback.
const hub = createHub({ cjsRegistry, manifest, logger: isAuditEnabled() ? createDefaultLogger({ cwd }) : undefined });
// ── Dispatch ────────────────────────────────────────────────────────────────
const result = hub.dispatch({
family: 'phase',
subcommand,
args: args.slice(2),
cwd,
raw,
});
// ── Translate result → CLI output / error (adapter responsibility) ──────────
// CJS handlers call output() themselves (inside phase.cmdPhase*()).
// No further output call is needed here.
if (!result.ok) {
if (result.kind === ERROR_KINDS.UnknownCommand) {
const available = availableSubcommands.join(', ');
error(`Unknown phase subcommand. Available: ${available}`);
return;
}
if (result.kind === ERROR_KINDS.InvalidArgs || result.kind === ERROR_KINDS.HandlerRefusal) {
// #176: typed payload — reason holds the human-readable message
error(result.reason);
return;
}
// HandlerFailure: message field
error(result.message);
return;
}
}
export = {
routePhaseCommand,
};