* fix(#2617): project verification next_command onto the runtime's command surface `src/verification.cts` stored and synthesized hard-coded `/gsd:…` command strings with no runtime context, and `phase complete` relayed that raw field straight into its verification-blocked error. On a Codex project the suggested next step was `/gsd:execute-phase`, a surface Codex does not install — it installs `$gsd-execute-phase`. The colon form is wrong twice over: `runtime-slash.cts` documents that "the colon form is never emitted", so EVERY runtime — not just Codex — was being handed a deprecated shape. Fixed at the one routing seam rather than per caller: - The routing table now stores BARE command names (`execute-phase`), never a prefixed literal. A prefixed literal in the table is what leaked. - A single `projectNextCommand(bare, runtime, tail)` helper runs every return path through `formatGsdSlash`, preserving the argument tail (`01 --gaps`) untouched. An empty command stays empty, so "no next step" never becomes a bare prefix. - `readVerificationStatus` accepts `opts.runtime`; `cmdVerificationStatus` and `phase complete` pass `resolveRuntime(cwd)`. The default is `claude`, which yields the canonical `/gsd-` hyphen form. All four routed states are covered: missing, unknown, gaps_found, stale. `init.cts` keeps its own projector deliberately. It already formats correctly, and its command CONTENT differs from the router's on purpose (it appends the phase number to `execute-phase`, and routes `human_needed` to `verify-work`). Consolidating them would silently change `init`'s user-visible output, which this issue did not ask for — so the divergence is left intact and the new tests instead pin the property that matters on both surfaces: no raw colon form escapes. Failing-first record: `origin/next:src/verification.cts` carried the four `/gsd:` literals (lines 101, 108, 382, 392), and 11 existing assertions in tests/verification-status.test.cjs asserted the colon form. Those 11 are corrected in this commit — they passed before the fix and fail after it, which is precisely the regression this closes. Tests are folded into the module's primary suite rather than added as a third file (`lint-test-file-count` caps the `verification` module at two, and consolidating is its documented remedy — growing the allowlist is not). The `phase complete` assertion reads `res.error`, not `res.stderr`: `runGsdTools` exposes a clean non-zero exit's stderr as `error`, and reading the wrong field yields '' and makes the whole check vacuous — which is how this user-visible path stayed untested. Closes #2617 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * test(#2617): scope the new hooks to their describes; cover gaps_found through the CLI Two findings from the orthogonal review of the first commit, both in the tests this change added. 1. The folded block's `beforeEach`/`afterEach` were declared at MODULE scope. node:test applies module-scope hooks to every test in the file, so hooks added for the #2617 suites also wrapped the ~40 pre-existing tests in verification-status.test.cjs — making an unrelated block a single point of failure for them (currently benign, but a throwing hook would have failed suites it has nothing to do with). They now install inside their own describes via a small `useProjectionPhaseDir()` helper, with a comment recording why. 2. The live-CLI `phase complete` test exercised only the `missing` state, so a regression in any other routed branch would have shown up in the router's return object but not in the text a user actually reads. Added a `gaps_found` case per runtime, asserting the projected `plan-phase <N> --gaps` reaches the blocked-completion error. Whole file verified green: 48 tests, 48 pass — the ~40 pre-existing ones included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * test(#2617): correct the last colon-form assertion in phase.test.cjs The remote run surfaced one more stale assertion outside tests/verification-status.test.cjs: the `phase complete` canonical-gate suite matched the blocked-completion message against `/\/gsd:verify-work 0?1/`. That project fixture configures no runtime, so it takes the `claude` default, which now yields the canonical `/gsd-verify-work 01` hyphen form. The colon form this asserted is exactly the deprecated shape #2617 removes — `runtime-slash.cts` documents that "the colon form is never emitted". Like the eleven corrected in the first commit, this assertion passed before the fix and fails after it, which is the regression record rather than a test being loosened: the surrounding assertions (failure reason, `stale` wording, and that neither ROADMAP.md nor STATE.md was mutated) are untouched. Verified against the real CLI: the emitted message is now "Phase 1 verification is incomplete: Verification is stale. Re-run verify-work before transition. Next: /gsd-verify-work 01". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * fix(#2617): collapse the two verification projectors into one seam The orthogonal review found that `init.cts` carried a second, independently maintained `verificationNextCommand()` that had drifted from the router's table in CONTENT, not just formatting: state router (before) init.cts missing execute-phase execute-phase <N> unknown execute-phase execute-phase <N> human_needed "" (no command) verify-work <N> The `human_needed` row is the sharp one: two GSD surfaces disagreed about whether a next command existed at all, and the router's own next_action told the user to "re-run the verify step until status is passed" while naming no command to run. init's answers were the useful ones, so the router adopts them and init now delegates to it — satisfying the issue's "keep one verification-routing seam" direction. `verificationNextCommand()` is deleted. Appending the phase number surfaced a trap the old bare commands hid. `extractPhaseToken` also returns project-code forms (`PROJ-07`), which are indistinguishable by shape from an ordinary directory name — `gsd-651-parent` yields `gsd-651` — so deriving the argument blindly emits `execute-phase gsd-651`. The number is therefore appended only when it is unambiguously numeric, or when the caller supplies it explicitly. `init` does supply it: its `phaseDir` is unresolved in several branches, where the router could not derive one at all. dir `01-example` -> $gsd-execute-phase 01, $gsd-verify-work 01 dir `gsd-651-parent` -> $gsd-execute-phase, $gsd-verify-work Suites verified green against the built lib: verification-status 50/50, phase 268/268, init 143/143, init-manager 40/40. `npm run lint:ci` clean. User-visible change beyond the reported bug, as agreed: `query verification.status` and `phase complete` now append the phase number for missing/unknown, and emit `verify-work <N>` for human_needed where they previously emitted nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * chore(#2617): backfill changeset PR number (#2700) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
30
src/init.cts
30
src/init.cts
@@ -165,22 +165,6 @@ interface PhaseCompletionProjection {
|
||||
verification_next_command: string;
|
||||
}
|
||||
|
||||
function verificationNextCommand(
|
||||
status: string,
|
||||
phaseNumber: string,
|
||||
slashRuntime: string,
|
||||
): string {
|
||||
if (status === 'gaps_found') {
|
||||
return `${formatGsdSlash('plan-phase', slashRuntime) as string} ${phaseNumber} --gaps`;
|
||||
}
|
||||
if (status === 'human_needed' || status === 'stale') {
|
||||
return `${formatGsdSlash('verify-work', slashRuntime) as string} ${phaseNumber}`;
|
||||
}
|
||||
if (status === 'missing' || status === 'unknown') {
|
||||
return `${formatGsdSlash('execute-phase', slashRuntime) as string} ${phaseNumber}`;
|
||||
}
|
||||
return '';
|
||||
}
|
||||
|
||||
function projectCompletionStatus(
|
||||
implementationComplete: boolean,
|
||||
@@ -201,8 +185,14 @@ function buildPhaseCompletionProjection(
|
||||
): PhaseCompletionProjection {
|
||||
const implementationComplete = planCount > 0 && summaryCount >= planCount;
|
||||
const phaseFullDir = phaseDir ? path.join(cwd, phaseDir) : '';
|
||||
// #2617: ONE verification-routing seam. init used to re-derive next_command
|
||||
// from the status with its own projector, which had drifted from the router's
|
||||
// table — it appended the phase number and answered `human_needed`; the table
|
||||
// did neither. The router now owns both the content and the runtime
|
||||
// projection, and init passes the phase number it already knows (its phaseDir
|
||||
// is unresolved in some branches, where the router could not derive one).
|
||||
const verificationStatus = implementationComplete
|
||||
? readVerificationStatus(phaseFullDir)
|
||||
? readVerificationStatus(phaseFullDir, { runtime: slashRuntime, phaseNumber })
|
||||
: { status: 'not_required', next_action: '', next_command: '' };
|
||||
const projectedVerificationStatus = verificationStatus.status;
|
||||
const projectedVerificationAction = verificationStatus.next_action;
|
||||
@@ -216,11 +206,7 @@ function buildPhaseCompletionProjection(
|
||||
phase_complete: phaseComplete,
|
||||
completion_status: projectCompletionStatus(implementationComplete, verificationPassed),
|
||||
verification_next_action: projectedVerificationAction,
|
||||
verification_next_command: verificationNextCommand(
|
||||
projectedVerificationStatus,
|
||||
phaseNumber,
|
||||
slashRuntime,
|
||||
),
|
||||
verification_next_command: verificationStatus.next_command,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -1760,7 +1760,10 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
|
||||
let isLastPhase = true;
|
||||
|
||||
const verificationBlocked = withPlanningLock(cwd, () => {
|
||||
const verificationStatus = readVerificationStatus(phaseFullDir);
|
||||
// #2617: pass the project's runtime so the blocked-completion error below
|
||||
// suggests the command surface this runtime actually installs
|
||||
// ($gsd-… on Codex) rather than a hard-coded Claude-style string.
|
||||
const verificationStatus = readVerificationStatus(phaseFullDir, { runtime: resolveRuntime(cwd) });
|
||||
if (verificationStatus.status !== 'passed') {
|
||||
return verificationStatus;
|
||||
}
|
||||
|
||||
@@ -38,6 +38,7 @@ import frontmatterMod = require('./frontmatter.cjs');
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports -- plan-scan.cjs is an export= CommonJS module
|
||||
import scanPhasePlans = require('./plan-scan.cjs');
|
||||
import { execGit } from './shell-command-projection.cjs';
|
||||
import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs';
|
||||
|
||||
const { output, error } = io;
|
||||
const { extractPhaseToken } = phaseId;
|
||||
@@ -70,6 +71,12 @@ interface VerificationRoute {
|
||||
*
|
||||
* For 'gaps_found', next_command is built at call time in readVerificationStatus
|
||||
* by substituting the phase number — it is NOT stored as a function in the table.
|
||||
*
|
||||
* #2617: `next_command` here holds a BARE command name (`execute-phase`), never a
|
||||
* prefixed one. Every return path projects it through `formatGsdSlash` with the
|
||||
* caller's runtime, so Codex sees `$gsd-execute-phase` and slash-hyphen runtimes
|
||||
* see `/gsd-execute-phase`. Storing a prefixed literal is what leaked the
|
||||
* hard-coded (and deprecated) `/gsd:` colon form to every runtime.
|
||||
*/
|
||||
const VERIFICATION_ROUTING_TABLE: Record<string, VerificationRoute> = {
|
||||
passed: {
|
||||
@@ -86,7 +93,12 @@ const VERIFICATION_ROUTING_TABLE: Record<string, VerificationRoute> = {
|
||||
human_needed: {
|
||||
status: 'human_needed',
|
||||
next_action: "Human verification required. Complete the manual tests in the phase's *-UAT.md, then re-run the verify step until status is passed.",
|
||||
next_command: '',
|
||||
// #2617: was '' — next_action told the user to "re-run the verify step" but
|
||||
// named no command, while init.cts's parallel projector emitted
|
||||
// `verify-work <N>` for this same state. The two surfaces disagreed on
|
||||
// whether a next command existed at all; init's answer was the useful one,
|
||||
// and init now delegates here rather than re-deriving it.
|
||||
next_command: 'verify-work',
|
||||
},
|
||||
stale: {
|
||||
status: 'stale',
|
||||
@@ -98,17 +110,31 @@ const VERIFICATION_ROUTING_TABLE: Record<string, VerificationRoute> = {
|
||||
missing: {
|
||||
status: 'missing',
|
||||
next_action: 'No verification report found — the verify step never completed. Re-run execute-phase.',
|
||||
next_command: '/gsd:execute-phase',
|
||||
next_command: 'execute-phase',
|
||||
},
|
||||
// INTERNAL SENTINEL: constructed when the file has a status value not in
|
||||
// VERIFIER_STATUSES. Never emitted by the verifier.
|
||||
unknown: {
|
||||
status: 'unknown',
|
||||
next_action: '', // filled in dynamically with the raw value
|
||||
next_command: '/gsd:execute-phase',
|
||||
next_command: 'execute-phase',
|
||||
},
|
||||
};
|
||||
|
||||
/**
|
||||
* Project a BARE command name (plus optional argument tail) into the surface the
|
||||
* given runtime actually installs (#2617).
|
||||
*
|
||||
* `formatGsdSlash` owns the per-runtime shape (`$gsd-<cmd>` for shell-var
|
||||
* runtimes like Codex, `/gsd-<cmd>` otherwise) and is idempotent, so passing an
|
||||
* already-prefixed string is safe. An empty command stays empty — "no next
|
||||
* command" must not become a bare prefix.
|
||||
*/
|
||||
function projectNextCommand(bare: string, runtime: string, tail = ''): string {
|
||||
if (!bare) return '';
|
||||
return `${formatGsdSlash(bare, runtime) as string}${tail}`;
|
||||
}
|
||||
|
||||
// ─── Helpers ─────────────────────────────────────────────────────────────────
|
||||
|
||||
interface FsLike {
|
||||
@@ -235,12 +261,12 @@ function defaultPhaseCleanCommitTimesMs(
|
||||
* Used for two early-return paths: no *-VERIFICATION.md file found, and
|
||||
* file present but no parseable frontmatter status.
|
||||
*/
|
||||
function missingResult(): VerificationStatusResult {
|
||||
function missingResult(runtime: string, phaseArg: string): VerificationStatusResult {
|
||||
const route = VERIFICATION_ROUTING_TABLE['missing'];
|
||||
return {
|
||||
status: route.status,
|
||||
next_action: route.next_action,
|
||||
next_command: route.next_command,
|
||||
next_command: projectNextCommand(route.next_command, runtime, phaseArg),
|
||||
};
|
||||
}
|
||||
|
||||
@@ -250,6 +276,20 @@ interface ReadVerificationStatusOptions {
|
||||
fs?: FsLike;
|
||||
/** Injectable per-phase clean-commit-time resolver for the staleness clock (#2348). */
|
||||
phaseCleanCommitTimesMs?: PhaseCleanCommitTimesFn;
|
||||
/**
|
||||
* Runtime whose command surface `next_command` is projected into (#2617).
|
||||
* Callers that have a cwd should pass `resolveRuntime(cwd)`. Defaults to
|
||||
* `'claude'`, which yields the canonical `/gsd-<cmd>` hyphen form — never the
|
||||
* deprecated `/gsd:` colon form this field used to hard-code.
|
||||
*/
|
||||
runtime?: string;
|
||||
/**
|
||||
* Phase number appended to the routed command (#2617). Defaults to the token
|
||||
* parsed from `phaseDir`, but only when that token is unambiguously numeric.
|
||||
* Callers that already know the number pass it explicitly — `init` reaches
|
||||
* this with `phaseDir` unresolved in some branches.
|
||||
*/
|
||||
phaseNumber?: string;
|
||||
}
|
||||
|
||||
interface VerificationStatusResult {
|
||||
@@ -321,6 +361,8 @@ function findStaleVerificationSummary(
|
||||
*
|
||||
* @param phaseDir - Absolute path to the phase directory.
|
||||
* @param opts - Options. `opts.fs` allows test injection (defaults to node:fs).
|
||||
* `opts.runtime` selects the command surface `next_command` is
|
||||
* projected into (#2617).
|
||||
*/
|
||||
function readVerificationStatus(
|
||||
phaseDir: string,
|
||||
@@ -329,11 +371,20 @@ function readVerificationStatus(
|
||||
const fsImpl: FsLike = opts.fs ?? fs;
|
||||
const phaseCleanCommitTimesMs: PhaseCleanCommitTimesFn =
|
||||
opts.phaseCleanCommitTimesMs ?? defaultPhaseCleanCommitTimesMs;
|
||||
const runtime = opts.runtime ?? 'claude';
|
||||
|
||||
// Phase token for the gaps_found command
|
||||
const baseName = path.basename(phaseDir);
|
||||
const phaseToken = extractPhaseToken(baseName);
|
||||
const phaseNumber = phaseToken.length > 0 ? phaseToken : baseName;
|
||||
const derivedPhaseNumber = phaseToken.length > 0 ? phaseToken : baseName;
|
||||
// #2617: the phase number becomes a COMMAND ARGUMENT, so it is appended only
|
||||
// when it is unambiguously one. extractPhaseToken also returns project-code
|
||||
// forms (`PROJ-07`), which are indistinguishable by shape from an ordinary
|
||||
// directory name — `gsd-651-parent` yields `gsd-651` — and emitting
|
||||
// `execute-phase gsd-651` is worse than emitting no argument at all. Callers
|
||||
// that already know the number (init) pass it explicitly and always get it.
|
||||
const phaseArgSource = opts.phaseNumber ?? (/^\d+(\.\d+)*$/.test(derivedPhaseNumber) ? derivedPhaseNumber : '');
|
||||
const phaseArg = phaseArgSource ? ` ${phaseArgSource}` : '';
|
||||
|
||||
// 1. Find *-VERIFICATION.md
|
||||
let verificationFile: string | null = null;
|
||||
@@ -347,7 +398,7 @@ function readVerificationStatus(
|
||||
}
|
||||
|
||||
if (!verificationFile) {
|
||||
return missingResult();
|
||||
return missingResult(runtime, phaseArg);
|
||||
}
|
||||
|
||||
// 2. Read and parse frontmatter using the shared parser.
|
||||
@@ -369,7 +420,7 @@ function readVerificationStatus(
|
||||
}
|
||||
|
||||
if (!rawStatus) {
|
||||
return missingResult();
|
||||
return missingResult(runtime, phaseArg);
|
||||
}
|
||||
|
||||
// gaps_found takes priority over stale — gap closure is the correct next
|
||||
@@ -379,7 +430,7 @@ function readVerificationStatus(
|
||||
return {
|
||||
status: entry.status,
|
||||
next_action: entry.next_action,
|
||||
next_command: `/gsd:plan-phase ${phaseNumber} --gaps`,
|
||||
next_command: projectNextCommand('plan-phase', runtime, `${phaseArg} --gaps`),
|
||||
};
|
||||
}
|
||||
|
||||
@@ -389,7 +440,7 @@ function readVerificationStatus(
|
||||
return {
|
||||
status: entry.status,
|
||||
next_action: entry.next_action,
|
||||
next_command: `/gsd:verify-work ${phaseNumber}`,
|
||||
next_command: projectNextCommand('verify-work', runtime, phaseArg),
|
||||
};
|
||||
}
|
||||
|
||||
@@ -406,7 +457,7 @@ function readVerificationStatus(
|
||||
return {
|
||||
status: entry.status,
|
||||
next_action: entry.next_action,
|
||||
next_command: entry.next_command,
|
||||
next_command: projectNextCommand(entry.next_command, runtime, phaseArg),
|
||||
};
|
||||
}
|
||||
|
||||
@@ -415,7 +466,7 @@ function readVerificationStatus(
|
||||
return {
|
||||
status: unknownRoute.status,
|
||||
next_action: `Unexpected verification status '${rawStatus}'. Re-run execute-phase verification.`,
|
||||
next_command: unknownRoute.next_command,
|
||||
next_command: projectNextCommand(unknownRoute.next_command, runtime, phaseArg),
|
||||
};
|
||||
}
|
||||
|
||||
@@ -433,7 +484,7 @@ function cmdVerificationStatus(cwd: string, phaseDirArg: string | undefined, raw
|
||||
return;
|
||||
}
|
||||
const phaseDir = path.resolve(cwd, phaseDirArg);
|
||||
const result = readVerificationStatus(phaseDir);
|
||||
const result = readVerificationStatus(phaseDir, { runtime: resolveRuntime(cwd) });
|
||||
output(result, raw);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user