From 76b7d73039f85feca081a9637d40bcd56763a90b Mon Sep 17 00:00:00 2001 From: Rezolv Date: Fri, 31 Jul 2026 10:20:17 -0400 Subject: [PATCH] fix(#2733): route gate-passed spec-phase paths into the probe steps (#2779) * fix(#2733): route gate-passed spec-phase paths into the probe steps All four gate-passed transitions in spec-phase.md said "Jump to Step 6", textually bypassing the mandatory Step 5.5 edge-completeness and Step 5.6 prohibition-completeness probes. Steps 5.5/5.6 were spliced between Step 5 and Step 6 by two later feature commits and the pre-existing jumps were never re-pointed, so no jump instruction in the file reached Step 5.5 at all and both probes were unreachable dead prose. Re-point the four gate-passed jumps (lines 129, 162, 168, 170) to Step 5.5. Control then flows 5.5 -> 5.6 -> 6 as the probes' own preconditions prescribe. The max-rounds "write anyway" bypasses and the probes' own "proceed to Step 6" exits are deliberately unchanged. Add tests/spec-phase-probe-reachability.test.cjs, which derives the mandatory probe steps from the file's own headings rather than hardcoding 5.5/5.6, so a future spliced-in probe step is covered without editing the test. It also locks the two coupled constraints: the max-rounds bypass must not be redirected into a probe, and each probe must keep its own onward exit. The existing probe contract tests are untouched and still pass; both scope from the "## Step 5.5"/"## Step 5.6" heading onward and were structurally incapable of observing the upstream jump text. * chore(changeset): Fixed fragment for #2779 (spec-phase probe reachability) * fix(#2733): route Step 5.5's own soft gate into Step 5.6 Round-1 review blocker. The four upstream gate-passed jumps were re-pointed to Step 5.5, but Step 5.5's own terminal soft gate at :305 still read "proceed to Step 6" - so the COMMON path (all applicable edges resolved) skipped the prohibition-completeness probe outright. Same defect class as the four this PR already fixed, on the success path of the very step being fixed: the SPEC shipped with an empty Prohibitions section instead of an empty Edge Coverage one. Its sibling at :393 is byte-identical yet correct, because Step 6 genuinely follows Step 5.6. Position, not phrasing, is the discriminator. The guard could not see it: the transition matcher keyed only on the literal "Jump to Step", and :305 says "proceed to Step". Widened it to a verb alternation (jump/proceed/continue/go/return/skip + "to Step N", case-insensitive) and renamed it TRANSITION_RE to match what it now models. This makes the file's own docstring promise - that a future spliced-in probe is covered without editing the test - true for a step whose exit is worded differently. Verified no false positives: the two pre-existing "continue to Step 3/4" transitions are upstream of both probes but target pre-probe steps, and the max-rounds bypass block contains no step transitions at all. Fail-first verified before fixing :305 - with the widened matcher against the unfixed workflow the guard fails naming exactly "spec-phase.md:305 jumps to Step 6, skipping mandatory Step 5.6", 4 pass / 1 fail; after the fix, 5/5. The two sibling probe contract tests stay 16/16. Also from review: - STEP_HEADING_RE gains an explicit \r? before $. Without it, on a CRLF checkout `.` stops before the \r and the unanchored $ fails to match, yielding ZERO steps and vacuously passing every assertion in the file. Not live today (.gitattributes forces eol=lf) but this repo has a recurring CRLF-regex bug class, so the guard no longer leans on it. - allow-test-rule category corrected to source-text-is-the-product; the previous runtime-contract-is-the-product is not one of the six recognized categories (CONTRIBUTING.md:609-619). - changeset body given the documented bold-lead-in form. - emitted-drift ack reason updated: +8 -> +10 bytes across five transitions (31987 -> 31997), DEFAULT tier, cap 40960. --------- Co-authored-by: CI Rebase Check Co-authored-by: Tom Boucher --- .changeset/agile-seals-wake.md | 5 + gsd-core/workflows/spec-phase.md | 10 +- tests/emitted-drift-ack.json | 5 +- tests/spec-phase-probe-reachability.test.cjs | 229 +++++++++++++++++++ 4 files changed, 243 insertions(+), 6 deletions(-) create mode 100644 .changeset/agile-seals-wake.md create mode 100644 tests/spec-phase-probe-reachability.test.cjs diff --git a/.changeset/agile-seals-wake.md b/.changeset/agile-seals-wake.md new file mode 100644 index 000000000..ab6a097e3 --- /dev/null +++ b/.changeset/agile-seals-wake.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2779 +--- +**`/gsd-spec-phase` now actually runs its edge-completeness and prohibition-completeness probes** — every gate-passed path reaches Step 5.5, and Step 5.5 now falls through to Step 5.6 instead of jumping past it. Previously all four gate-passed transitions went straight to SPEC generation and Step 5.5's own "all edges resolved" gate skipped the prohibition probe, so a SPEC could ship with an empty Edge Coverage section, an empty Prohibitions section, or both — and a weaker model following the prose literally would never notice. Since the probes are what carry must-NOT constraints and data-shape edges into `must_haves`, the plan and the verifier inherited the gap too. (#2733) diff --git a/gsd-core/workflows/spec-phase.md b/gsd-core/workflows/spec-phase.md index f25731292..c26688c42 100644 --- a/gsd-core/workflows/spec-phase.md +++ b/gsd-core/workflows/spec-phase.md @@ -126,7 +126,7 @@ Acceptance Criteria:[score 0.0–1.0] Ambiguity: [score] ([calculate]) ``` -**If `--auto` and initial ambiguity already ≤ 0.20 with all minimums met:** Skip interview — derive SPEC.md directly from roadmap + requirements. Log: `[auto] Phase requirements are already sufficiently clear — generating SPEC.md from existing context.` Jump to Step 6. +**If `--auto` and initial ambiguity already ≤ 0.20 with all minimums met:** Skip interview — derive SPEC.md directly from roadmap + requirements. Log: `[auto] Phase requirements are already sufficiently clear — generating SPEC.md from existing context.` Jump to Step 5.5. **Otherwise:** Continue to Step 4. @@ -159,15 +159,15 @@ After round [N]: If gate passes (ambiguity ≤ 0.20 AND all minimums met): -**If `--auto`:** Jump to Step 6. +**If `--auto`:** Jump to Step 5.5. **Otherwise:** AskUserQuestion: - header: "Spec Gate Passed" - question: "Ambiguity is [score] — requirements are clear enough to write SPEC.md. Proceed?" - options: - - "Yes — write SPEC.md" → Jump to Step 6 + - "Yes — write SPEC.md" → Jump to Step 5.5 - "One more round" → Continue interview - - "Done talking — write it" → Jump to Step 6 + - "Done talking — write it" → Jump to Step 5.5 **If max rounds reached (6) and gate not passed:** @@ -302,7 +302,7 @@ For each Requirement gathered so far: **Dismiss (reason)**, or **Defer**). A manual-review nudge, not a hard block. **Soft gate (after resolving):** -- All applicable edges resolved → proceed to Step 6. +- All applicable edges resolved → proceed to Step 5.6. - Any `unresolved` → AskUserQuestion: - header: "Edge Coverage" - question: "[N] edge(s) are unresolved: [list]. What do you want to do?" diff --git a/tests/emitted-drift-ack.json b/tests/emitted-drift-ack.json index 1284ce27d..a05e13c8b 100644 --- a/tests/emitted-drift-ack.json +++ b/tests/emitted-drift-ack.json @@ -34,6 +34,9 @@ "agents/gsd-ui-researcher.toml": "#2834: Codex agent TOML now carries model-routing fields on first install.", "agents/gsd-user-profiler.toml": "#2834: Codex agent TOML now carries model-routing fields on first install.", "agents/gsd-verifier.toml": "#2834: Codex agent TOML now carries model-routing fields on first install.", - "gsd-code-fixer.md": "#2825: setup_worktree now reads workflow.use_worktrees and gates git worktree add on it (skipping worktree creation when the user opted out), the cleanup tail is gated to a no-op in that mode, and the spec adds three safety guardrails — honor the opt-out, never rm -rf a possible Windows reparse point/junction (the delete-the-target path that wiped real node_modules), and record where verification ran. Growth is the gated bash branch + the three guardrail paragraphs." + "gsd-code-fixer.md": "#2825: setup_worktree now reads workflow.use_worktrees and gates git worktree add on it (skipping worktree creation when the user opted out), the cleanup tail is gated to a no-op in that mode, and the spec adds three safety guardrails — honor the opt-out, never rm -rf a possible Windows reparse point/junction (the delete-the-target path that wiped real node_modules), and record where verification ran. Growth is the gated bash branch + the three guardrail paragraphs.", + "spec-phase.md": { + "reason": "#2733: five transitions in gsd-core/workflows/spec-phase.md were re-pointed so control reaches the mandatory Step 5.5 edge-completeness and Step 5.6 prohibition-completeness probes, which no path could reach before. Four upstream gate-passed jumps went from 'Jump to Step 6' to 'Jump to Step 5.5', and Step 5.5's own terminal soft gate at :305 went from 'proceed to Step 6' to 'proceed to Step 5.6' so the common all-edges-resolved path stops skipping the prohibition probe. The +10 bytes is exactly those five targets growing by 2 bytes each ('Step 6' -> 'Step 5.5' / 'Step 5.6'); it is the literal fix, not incidental prose growth, and cannot be avoided without leaving a probe unreachable. Verified: 31987 -> 31997 bytes, DEFAULT tier, cap 40960." + } } } diff --git a/tests/spec-phase-probe-reachability.test.cjs b/tests/spec-phase-probe-reachability.test.cjs new file mode 100644 index 000000000..cb11bfb84 --- /dev/null +++ b/tests/spec-phase-probe-reachability.test.cjs @@ -0,0 +1,229 @@ +// allow-test-rule: source-text-is-the-product (see #2733) — spec-phase.md IS the deployed +// workflow the runtime executes, so its control flow is the product under assertion, not an +// implementation detail behind a typed seam. There is no structured artifact to assert on: the +// jump instructions ARE prose the model follows. +// +// Why this file exists (#2733): Step 5.5 (edge-completeness) and Step 5.6 (prohibition- +// completeness) were spliced between Step 5 and Step 6 by two later feature commits, but the +// four pre-existing gate-passed "Jump to Step 6" instructions were never re-pointed — so every +// gate-passed path textually routed around both mandatory probes and NO jump in the file +// reached Step 5.5 at all. +// +// A FIFTH occurrence survived the first pass and was caught in review: Step 5.5's own terminal +// soft gate (":305") read "proceed to Step 6", so the COMMON path — all edges resolved — skipped +// the prohibition probe outright. It was invisible to this guard because the transition matcher +// keyed only on "Jump to Step"; see TRANSITION_RE. Its sibling at ":393" is byte-identical yet +// CORRECT, because Step 6 genuinely follows Step 5.6 — position is the discriminator, not text. +// +// The two existing probe contract tests (edge-probe-spec-phase-contract.test.cjs, +// prohibition-probe.spec-phase-contract.test.cjs) are structurally blind to this: both slice +// the file from the "## Step 5.5" / "## Step 5.6" heading onward, so no assertion in either can +// observe the upstream jump text. They assert the probes' CONTENTS; this file asserts their +// REACHABILITY. Neither is modified by this fix. + +'use strict'; + +process.env.GSD_TEST_MODE = '1'; + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const SPEC_PHASE_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'spec-phase.md'); + +/** + * `## Step 5.5: Edge-Completeness Probe` → captures the id and the title. + * + * The explicit `\r?` matters: `.` excludes line terminators, so on a CRLF checkout `(.*)` + * stops before the `\r` and an unanchored `$` then fails to match, silently yielding ZERO + * steps and vacuously passing every assertion below. `.gitattributes:2` forces `eol=lf` today, + * but this repo has a recurring CRLF-regex bug class, so the guard does not lean on it. + */ +const STEP_HEADING_RE = /^## Step ([0-9]+(?:\.[0-9]+)*)\s*:?\s*(.*?)\r?$/; + +/** + * A transition instruction the executing model is told to follow. + * + * This must cover EVERY phrasing the workflow uses to move control, not just `Jump to Step`. + * The original `Jump to Step`-only form was blind to Step 5.5's own soft gate at :305 + * (`proceed to Step 6`), which is the same probe-skipping defect class this file guards — + * so the guard could not see the very case it claimed to hold. The verb alternation is what + * makes the docstring's promise ("a future spliced-in probe is covered without editing this + * test") true for a step whose exit is worded differently. + * + * Position, not phrasing, is what separates a correct transition from a violation: :305 and + * :393 are byte-identical `proceed to Step 6` lines and only differ in which step encloses + * them. That discrimination lives in the `jumpIsUpstreamOfProbe` guard below, not here. + */ +const TRANSITION_RE = /(?:jump|proceed|continue|go|return|skip)\s+to\s+Step\s+([0-9]+(?:\.[0-9]+)*)/i; + +/** + * The max-rounds bypass: reached ONLY when the ambiguity gate never passed. Both probes are + * scoped to requirements that already cleared the gate ("you probe edges of clear + * requirements, not vague ones"), so these paths legitimately go straight to SPEC generation + * and must NOT be redirected into a probe. + */ +const MAX_ROUNDS_MARKER = 'If max rounds reached'; + +function readSpecPhase() { + return fs.readFileSync(SPEC_PHASE_PATH, 'utf8'); +} + +/** Segment-wise semver-ish compare so 5.10 sorts after 5.9 (a float compare would not). */ +function compareStepIds(a, b) { + const pa = a.split('.').map(Number); + const pb = b.split('.').map(Number); + for (let i = 0; i < Math.max(pa.length, pb.length); i += 1) { + const da = pa[i] ?? 0; + const db = pb[i] ?? 0; + if (da !== db) return da < db ? -1 : 1; + } + return 0; +} + +/** Every `## Step N` heading, in document order, with the line it sits on. */ +function collectSteps(lines) { + const steps = []; + lines.forEach((line, idx) => { + const m = line.match(STEP_HEADING_RE); + if (m) steps.push({ id: m[1], title: m[2].trim(), line: idx + 1 }); + }); + return steps; +} + +/** + * The MANDATORY probe steps, DERIVED from the file rather than hardcoded — any step whose + * heading names it a probe. Deriving them means a future Step 5.7 probe is covered by this + * assertion the moment it is spliced in, without anyone remembering to edit this test. That + * is precisely the failure mode #2733 was. + */ +function collectProbeSteps(steps) { + return steps.filter(s => /probe/i.test(s.title)); +} + +/** Every control-transition instruction (any phrasing), with the line it sits on. */ +function collectJumps(lines) { + const jumps = []; + lines.forEach((line, idx) => { + const m = line.match(TRANSITION_RE); + if (m) jumps.push({ target: m[1], line: idx + 1, text: line.trim() }); + }); + return jumps; +} + +/** Line range of the max-rounds bypass block: its marker through the next `## ` heading. */ +function maxRoundsRange(lines) { + const start = lines.findIndex(l => l.includes(MAX_ROUNDS_MARKER)); + if (start === -1) return null; + let end = lines.length; + for (let i = start + 1; i < lines.length; i += 1) { + if (lines[i].startsWith('## ')) { end = i; break; } + } + return { start: start + 1, end }; +} + +test('#2733: the probe steps exist and are DERIVABLE as mandatory from their headings', () => { + const steps = collectSteps(readSpecPhase().split('\n')); + const probes = collectProbeSteps(steps); + + assert.ok( + probes.length >= 2, + 'spec-phase.md must define at least the edge-completeness and prohibition-completeness ' + + `probe steps; found ${probes.length}: ${probes.map(p => p.id).join(', ')}` + ); +}); + +test('#2733: no jump instruction may skip a mandatory probe step', () => { + const lines = readSpecPhase().split('\n'); + const steps = collectSteps(lines); + const probes = collectProbeSteps(steps); + const jumps = collectJumps(lines); + const bypass = maxRoundsRange(lines); + + assert.ok(jumps.length > 0, 'spec-phase.md must contain jump instructions to assert on'); + + const violations = []; + for (const jump of jumps) { + // The max-rounds bypass never passed the gate, so the probes do not apply to it. + if (bypass && jump.line >= bypass.start && jump.line <= bypass.end) continue; + + for (const probe of probes) { + const jumpIsUpstreamOfProbe = jump.line < probe.line; + const targetIsPastProbe = compareStepIds(jump.target, probe.id) > 0; + if (jumpIsUpstreamOfProbe && targetIsPastProbe) { + violations.push( + `spec-phase.md:${jump.line} jumps to Step ${jump.target}, skipping mandatory ` + + `Step ${probe.id} (${probe.title}) — "${jump.text}"` + ); + } + } + } + + assert.deepEqual( + violations, + [], + 'Every gate-passed transition must route INTO the first mandatory probe, not past it. ' + + `Found ${violations.length} probe-skipping jump(s):\n ${violations.join('\n ')}` + ); +}); + +test('#2733: at least one jump routes into the first mandatory probe', () => { + const lines = readSpecPhase().split('\n'); + const probes = collectProbeSteps(collectSteps(lines)); + const jumps = collectJumps(lines); + + assert.ok(probes.length > 0, 'expected at least one probe step'); + const first = probes[0]; + const into = jumps.filter(j => j.target === first.id); + + assert.ok( + into.length > 0, + `Step ${first.id} (${first.title}) is defined but no jump instruction reaches it — ` + + 'it is unreachable dead prose. Jump targets found: ' + + `${[...new Set(jumps.map(j => j.target))].join(', ')}` + ); +}); + +test('#2733 coupled: the max-rounds bypass is NOT redirected into a probe', () => { + const lines = readSpecPhase().split('\n'); + const probes = collectProbeSteps(collectSteps(lines)); + const bypass = maxRoundsRange(lines); + + assert.ok(bypass, `expected a "${MAX_ROUNDS_MARKER}" block in spec-phase.md`); + + const block = lines.slice(bypass.start - 1, bypass.end); + const probeIds = new Set(probes.map(p => p.id)); + const redirected = block + .map((line, i) => ({ line: bypass.start + i, text: line.trim(), m: line.match(TRANSITION_RE) })) + .filter(e => e.m && probeIds.has(e.m[1])); + + assert.deepEqual( + redirected.map(e => `spec-phase.md:${e.line} — "${e.text}"`), + [], + 'The max-rounds "write anyway" paths never passed the ambiguity gate, so the probes do ' + + 'not apply to them; they must continue straight to SPEC generation.' + ); + + assert.ok( + block.some(l => l.includes('Write SPEC.md')), + 'The max-rounds bypass must still route to SPEC generation.' + ); +}); + +test('#2733 coupled: each probe step keeps its own exit to SPEC generation', () => { + const lines = readSpecPhase().split('\n'); + const steps = collectSteps(lines); + const probes = collectProbeSteps(steps); + + for (const probe of probes) { + const next = steps.find(s => s.line > probe.line); + const block = lines.slice(probe.line - 1, next ? next.line - 1 : lines.length).join('\n'); + assert.match( + block, + /proceed to Step [0-9]/i, + `Step ${probe.id} (${probe.title}) must keep its own onward exit — the probes' soft ` + + 'gates are what carry control to SPEC generation once resolved.' + ); + } +});