From 9ed8c7d5744759d72216969d4ebb7343c6ecb524 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 11 Jun 2026 00:58:21 -0400 Subject: [PATCH] =?UTF-8?q?feat(#1026):=20=C2=A75.6/ui-phase=20cutover=20?= =?UTF-8?q?=E2=80=94=20first=20gate=20dispatch=20(plan:pre=20step=20+=20bl?= =?UTF-8?q?ocking=20gate)=20(#1028)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace plan-phase.md §5.6 (a 6-branch inline UI gate) with a capability-driven loop.render-hooks plan:pre dispatch — the FIRST gate dispatch in any workflow. A step (ui-phase, when:workflow.ui_phase) + a new blocking gate (when:workflow.ui_safety_gate). New ui.plan-gate check verb returns {frontend, hasUiSpec, block}; the dispatch runs it unconditionally then fires the active step (pipeline) or halts on the active blocking gate (manual). The gate-handling (run check.query; halt if blocking+block) is the reusable phase-6 template for blocking-gate cutovers. Config semantics fixed per #1022 + maintainer call: ui_phase gates plan-time UI-SPEC generation, ui_safety_gate gates the planning block. Common case + all ui_phase=false cases are equivalence-preserving; the one intended change is {ui_phase:true, ui_safety_gate:false} now auto-generating in pipelines. Review found it broken twice (non-generic dispatch, phase-lookup divergence, then the step-only check nested in a gate loop) — fixed; final Codex pass verified all 8 (ui_phase,ui_safety_gate)x{pipeline,manual} cases correct. gsd-ui-phase skill + autonomous §3a.5 untouched (§3a.5 deferred). getRoadmapPhaseWithFallback mirrors cmdRoadmapGetPhase for lookup parity. Closes #1026 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 --- capabilities/ui/capability.json | 1 + gsd-core/bin/lib/capability-registry.cjs | 22 +- gsd-core/workflows/plan-phase.md | 77 ++--- src/check-command-router.cts | 143 +++++++- src/roadmap.cts | 42 +++ ...06-ui-safety-gate-false-positives.test.cjs | 104 +++--- tests/capability-registry.test.cjs | 12 + tests/check-ui-plan-gate.test.cjs | 318 ++++++++++++++++++ tests/loop-hook-firing-spike.test.cjs | 81 +++-- tests/plan-phase-ui-redirect.test.cjs | 297 ++++++++++++++-- 10 files changed, 972 insertions(+), 125 deletions(-) create mode 100644 tests/check-ui-plan-gate.test.cjs diff --git a/capabilities/ui/capability.json b/capabilities/ui/capability.json index dbd4c2745..bc013952e 100644 --- a/capabilities/ui/capability.json +++ b/capabilities/ui/capability.json @@ -16,6 +16,7 @@ ], "contributions": [], "gates": [ + { "point": "plan:pre", "check": { "query": "ui.plan-gate" }, "when": "workflow.ui_safety_gate", "blocking": true, "onError": "halt" }, { "point": "execute:wave:post", "check": { "query": "ui.safety-gate" }, "when": "workflow.ui_safety_gate", "blocking": true, "onError": "halt" } ] } diff --git a/gsd-core/bin/lib/capability-registry.cjs b/gsd-core/bin/lib/capability-registry.cjs index d1d0222b5..49329b265 100644 --- a/gsd-core/bin/lib/capability-registry.cjs +++ b/gsd-core/bin/lib/capability-registry.cjs @@ -157,6 +157,15 @@ const capabilities = { ], "contributions": [], "gates": [ + { + "point": "plan:pre", + "check": { + "query": "ui.plan-gate" + }, + "when": "workflow.ui_safety_gate", + "blocking": true, + "onError": "halt" + }, { "point": "execute:wave:post", "check": { @@ -211,7 +220,18 @@ const byLoopPoint = { } ], "contributions": [], - "gates": [] + "gates": [ + { + "capId": "ui", + "point": "plan:pre", + "check": { + "query": "ui.plan-gate" + }, + "when": "workflow.ui_safety_gate", + "blocking": true, + "onError": "halt" + } + ] }, "plan:post": { "steps": [], diff --git a/gsd-core/workflows/plan-phase.md b/gsd-core/workflows/plan-phase.md index 6b0c5aa7a..5e47daec3 100644 --- a/gsd-core/workflows/plan-phase.md +++ b/gsd-core/workflows/plan-phase.md @@ -648,60 +648,63 @@ Continue to step 5.6. Security config is passed to the planner in step 8. ## 5.6. UI Design Contract Gate -> Skip if `workflow.ui_phase` is explicitly `false` AND `workflow.ui_safety_gate` is explicitly `false` in `.planning/config.json`. If keys are absent, treat as enabled. +> Capability-driven dispatch. Resolves active `plan:pre` hooks via the capability registry; each hook's `when` condition (`workflow.ui_phase` for step hooks, `workflow.ui_safety_gate` for gate hooks) is evaluated by the registry — no inline config-get needed. +> +> **Config semantics (cutover fix):** `workflow.ui_phase` gates UI-SPEC *generation* (step); `workflow.ui_safety_gate` gates the *planning block* (gate). Both-on = identical to OLD §5.6. Intended change: `{ui_phase:true, ui_safety_gate:false}` now auto-generates in pipelines but does NOT block manual planning (each key controls exactly what its description says). ```bash -UI_PHASE_CFG=$(gsd_run query config-get workflow.ui_phase 2>/dev/null || echo "true") -UI_GATE_CFG=$(gsd_run query config-get workflow.ui_safety_gate 2>/dev/null || echo "true") +HOOKS_JSON=$(gsd_run loop render-hooks plan:pre --raw) ``` -**If both are `false`:** Skip to step 6. +Read the `activeHooks` array directly from `HOOKS_JSON` (in-context — do NOT invoke a shell pipeline). -Check if phase has frontend indicators: +**Branch 1 — both toggles off (`activeHooks` is empty or absent):** Skip to step 6. + +Run whenever **any** `plan:pre` UI hook is active — including the step-only case (`workflow.ui_safety_gate` off). (`check.query` = `"ui.plan-gate"`; router normalizes dots→hyphens.) ```bash -PHASE_SECTION=$(gsd_run query roadmap.get-phase "${PHASE}" 2>/dev/null) -# Shell-free word-boundary gate (#3718): Node.js helper — no locale env-var dependency. -# Reads via stdin to avoid OS ARG_MAX limits on large phase text. -# Resolve the helper against the GSD install dir via RUNTIME_DIR (#448) — NOT the consuming -# project's git root — falling back to git toplevel / $HOME/.claude. Exit codes mirror grep (0=UI,1=none). -_GSD_RT="${RUNTIME_DIR:-$(git rev-parse --show-toplevel 2>/dev/null || pwd)}" -UI_GATE_JS=$(for _c in "$_GSD_RT/gsd-core/bin/lib/ui-safety-gate.cjs" "$_GSD_RT/bin/lib/ui-safety-gate.cjs" "$_GSD_RT/.claude/bin/lib/ui-safety-gate.cjs" "$HOME/.claude/gsd-core/bin/lib/ui-safety-gate.cjs" "$HOME/.claude/bin/lib/ui-safety-gate.cjs"; do [ -f "$_c" ] && { echo "$_c"; break; }; done) -if [ -n "$UI_GATE_JS" ]; then printf '%s' "$PHASE_SECTION" | node "$UI_GATE_JS" >/dev/null 2>&1; HAS_UI=$?; else echo "WARN: ui-safety-gate.cjs not found via RUNTIME_DIR/\$HOME (#448) — assuming UI present" >&2; HAS_UI=0; fi +GATE=$(gsd_run check ui-plan-gate "${PHASE}" --raw) ``` -**If `HAS_UI` is 0 (frontend indicators found):** +Read `frontend`, `hasUiSpec`, and `block` from `GATE`. -Check for existing UI-SPEC: -```bash -UI_SPEC_FILE=$(ls "${PHASE_DIR}"/*-UI-SPEC.md 2>/dev/null | head -1) -``` +**Branch 2 — no frontend indicators (`frontend` is `false`):** Skip silently to step 5.7. -**If UI-SPEC.md found:** Set `UI_SPEC_PATH=$UI_SPEC_FILE`. Display: `Using UI design contract: ${UI_SPEC_PATH}` +**Branch 3 — UI-SPEC already exists (`hasUiSpec` is `true`):** -**If UI-SPEC.md missing AND `--skip-ui` flag is present in $ARGUMENTS:** Skip silently to step 6. - -**If UI-SPEC.md missing AND `UI_GATE_CFG` is `true`:** - -Read ephemeral chain flag (same field as `check.auto-mode` → `auto_chain_active`): -```bash -AUTO_CHAIN=$(gsd_run query check auto-mode --pick auto_chain_active 2>/dev/null || echo "false") -``` - -**If `AUTO_CHAIN` is `true` (running inside a `--chain` or `--auto` pipeline):** - -Auto-generate UI-SPEC without prompting: -``` -Skill(skill="gsd-ui-phase", args="${PHASE} --auto ${GSD_WS}") -``` -After `gsd-ui-phase` returns, re-read: ```bash UI_SPEC_FILE=$(ls "${PHASE_DIR}"/*-UI-SPEC.md 2>/dev/null | head -1) UI_SPEC_PATH="${UI_SPEC_FILE}" ``` + +Display: `Using UI design contract: ${UI_SPEC_PATH}`. Continue to step 6. + +**Branch 4 — `--skip-ui` in `$ARGUMENTS`:** Skip silently to step 6. + +**Branches 5 & 6 — frontend detected, UI-SPEC missing, no `--skip-ui`.** + +Read the ephemeral auto-chain flag: + +```bash +AUTO_CHAIN=$(gsd_run query check auto-mode --pick auto_chain_active 2>/dev/null || echo "false") +``` + +**Branch 5 — `AUTO_CHAIN` is `true` (pipeline / `--auto`):** Fire each active **step** hook — runs independently of whether a gate is active (covers `{ui_phase:true, ui_safety_gate:false}`). For each entry in `activeHooks` (in array order) where `kind == "step"` and `ref.skill` is set: + +``` +Skill(skill="gsd-${ref.skill}", args="${PHASE} --auto ${GSD_WS}") +``` + +(prepend `gsd-` to `ref.skill` — `ui-phase` → `gsd-ui-phase`.) After all step hooks return, re-read: + +```bash +UI_SPEC_FILE=$(ls "${PHASE_DIR}"/*-UI-SPEC.md 2>/dev/null | head -1) +UI_SPEC_PATH="${UI_SPEC_FILE}" +``` + Continue to step 6. -**If `AUTO_CHAIN` is `false` (manual invocation):** +**Branch 6 — `AUTO_CHAIN` is `false` (manual): generic gate handling.** For each entry in `activeHooks` where `kind == "gate"` and `blocking` is `true`: if `block:true` (from `GATE`), output the block below and **EXIT the plan-phase workflow**. If no active blocking gate (e.g. `workflow.ui_safety_gate` is off), continue to step 6 — no block. Output this markdown directly (not as a code block): @@ -716,8 +719,6 @@ Also available: **Exit the plan-phase workflow. Do not continue.** -**If `HAS_UI` is 1 (no frontend indicators):** Skip silently to step 5.7. - ## 5.7. Schema Push Detection Gate > Detects schema-relevant files in the phase scope and injects a mandatory `[BLOCKING]` schema push task into the plan. Prevents false-positive verification where build/types pass because TypeScript types come from config, not the live database. diff --git a/src/check-command-router.cts b/src/check-command-router.cts index d7b8c7e86..67f4f6aba 100644 --- a/src/check-command-router.cts +++ b/src/check-command-router.cts @@ -14,6 +14,10 @@ import core = require('./core.cjs'); const { output, error, ERROR_REASON } = core; import { parseDecisions } from './decisions.cjs'; import type { Decision } from './decisions.cjs'; +import { checkUiPresence } from './ui-safety-gate.cjs'; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import roadmapModule = require('./roadmap.cjs'); +const { getRoadmapPhaseWithFallback } = roadmapModule; // ─── Helpers ────────────────────────────────────────────────────────────────── @@ -335,6 +339,129 @@ function cmdDecisionCoverageVerify(projectDir: string, args: string[], raw: bool }, raw, undefined); } +// ─── ui-plan-gate ───────────────────────────────────────────────────────────── + +/** + * ui-plan-gate: given a phase number, checks whether the phase has frontend + * indicators and whether a *-UI-SPEC.md already exists in the phase directory. + * + * Returns JSON: { frontend: boolean, hasUiSpec: boolean, block: boolean } + * block = frontend && !hasUiSpec (gate fires when UI work is detected but no spec exists) + * + * Invocable as: gsd_run check ui-plan-gate + * + * Uses checkUiPresence from ui-safety-gate.cjs — does NOT reimplement frontend detection. + * Uses getRoadmapPhaseInternal + findPhaseInternal from core.cjs for phase data. + */ +function findUiSpecInDir(phaseDir: string): string { + if (!phaseDir || !fs.existsSync(phaseDir)) return ''; + try { + const files = fs.readdirSync(phaseDir); + const found = files.find((f) => /-UI-SPEC\.md$/.test(f)); + return found ? path.join(phaseDir, found) : ''; + } catch { + return ''; + } +} + +/** + * Pure logic for ui-plan-gate — exposed for direct behavioral testing. + * + * Given a projectDir and phase number: + * (a) Reads the phase section from ROADMAP.md via getRoadmapPhaseWithFallback — + * same two-pass lookup (current milestone → full roadmap) as `roadmap.get-phase` + * (cmdRoadmapGetPhase). Cross-milestone / older frontend phases resolve correctly. + * If ROADMAP.md is missing, phaseSection is '' (ROADMAP.md not present = project + * has no roadmap = cannot be frontend). If the phase truly can't be found after + * both passes, phaseSection is '' and phaseLookupFailed is set so callers can + * surface the miss — we do NOT silently degrade to frontend:false if the roadmap + * exists but the phase header is absent. + * (b) Runs checkUiPresence (frontend detection) — no reimplementation. + * (c) Resolves the phase directory via core.findPhaseInternal; checks for *-UI-SPEC.md. + * + * Returns: { frontend, hasUiSpec, block, uiSpecPath, phaseLookupFailed } + * block = frontend && !hasUiSpec + * phaseLookupFailed = ROADMAP.md present but phase header not found (surfaced for + * onError:halt gates so a missing phase doesn't silently bypass) + */ +function computeUiPlanGate(projectDir: string, phase: string): { + frontend: boolean; + hasUiSpec: boolean; + block: boolean; + uiSpecPath: string | null; + phaseLookupFailed?: boolean; +} { + // (a) Read the phase section text using the same two-pass lookup as roadmap.get-phase. + // getRoadmapPhaseWithFallback: current-milestone first, then stripShippedMilestones + // fallback — mirrors cmdRoadmapGetPhase exactly. + let phaseSection = ''; + let phaseLookupFailed: boolean | undefined; + try { + const section = getRoadmapPhaseWithFallback(projectDir, phase); + if (section === null) { + // Distinguish: ROADMAP.md missing (no-roadmap project) vs phase not found in ROADMAP. + // core.planningDir(cwd) resolves the .planning/ root for workstream-aware paths. + const planDir: string = typeof (core as unknown as Record)['planningDir'] === 'function' + ? (core as unknown as Record string>)['planningDir'](projectDir) + : path.join(projectDir, '.planning'); + const roadmapPath = path.join(planDir, 'ROADMAP.md'); + if (fs.existsSync(roadmapPath)) { + // ROADMAP.md exists but phase was not found → surface the miss + phaseLookupFailed = true; + } + // phaseSection stays '' + } else { + phaseSection = section; + } + } catch { /* roadmap read failure → treat as empty (non-frontend) */ } + + // (b) Run checkUiPresence (frontend detection) — reuse existing helper; no reimplementation + const presenceResult = checkUiPresence(phaseSection); + const frontend = presenceResult.hasUI; + + // (c) Resolve phase directory via findPhaseInternal and check for *-UI-SPEC.md + const coreModule = core as unknown as Record; + let phaseDir = ''; + try { + const findPhase = coreModule['findPhaseInternal'] as ((cwd: string, phase: string) => Record | string | null) | undefined; + if (typeof findPhase === 'function') { + const result = findPhase(projectDir, phase); + if (result && typeof result === 'object') { + // findPhaseInternal returns { directory: '', ... } + // directory is relative to cwd — resolve it to absolute. + const relDir = typeof result['directory'] === 'string' ? result['directory'] : ''; + if (relDir) { + phaseDir = path.resolve(projectDir, relDir); + } + } else if (typeof result === 'string') { + phaseDir = result; + } + } + } catch { /* phase dir lookup failure → hasUiSpec=false */ } + + const uiSpecPath = findUiSpecInDir(phaseDir); + const hasUiSpec = uiSpecPath !== ''; + + // block = frontend phase with no UI-SPEC + const block = frontend && !hasUiSpec; + + const result: { frontend: boolean; hasUiSpec: boolean; block: boolean; uiSpecPath: string | null; phaseLookupFailed?: boolean } = { + frontend, hasUiSpec, block, uiSpecPath: hasUiSpec ? uiSpecPath : null, + }; + if (phaseLookupFailed) result.phaseLookupFailed = true; + return result; +} + +function cmdUiPlanGate(projectDir: string, args: string[], raw: boolean): void { + // args[0] = 'check', args[1] = 'ui-plan-gate', args[2] = phase + const phase = args[2] || ''; + if (!phase) { + error('ui-plan-gate requires a phase argument: check ui-plan-gate ', ERROR_REASON.SDK_MISSING_ARG); + return; + } + output(computeUiPlanGate(projectDir, phase), raw, undefined); +} + interface RouteCheckCommandOptions { args: string[]; cwd: string; @@ -342,7 +469,14 @@ interface RouteCheckCommandOptions { } function routeCheckCommand({ args, cwd, raw }: RouteCheckCommandOptions): void { - const subcommand = args[1]; + // Normalize dots to hyphens in the subcommand so both forms are accepted. + // This makes `check.query = "ui.plan-gate"` (dotted form in capability.json gates) + // directly runnable as `gsd_run check ui.plan-gate` — the dot is normalized to + // `ui-plan-gate` before routing. The generic gate-dispatch in §5.6 reads + // `check.query` from the active gate hook and runs `gsd_run check ${hook.check.query}`, + // so the declared query must be dispatchable exactly as declared. + const rawSubcommand = args[1]; + const subcommand = typeof rawSubcommand === 'string' ? rawSubcommand.replace(/\./g, '-') : rawSubcommand; if (subcommand === 'auto-mode') { cmdAutoMode(cwd, raw); return; @@ -355,11 +489,16 @@ function routeCheckCommand({ args, cwd, raw }: RouteCheckCommandOptions): void { cmdDecisionCoverageVerify(cwd, args, raw); return; } - error('Unknown check subcommand. Available: auto-mode, decision-coverage-plan, decision-coverage-verify', ERROR_REASON.SDK_UNKNOWN_COMMAND); + if (subcommand === 'ui-plan-gate') { + cmdUiPlanGate(cwd, args, raw); + return; + } + error('Unknown check subcommand. Available: auto-mode, decision-coverage-plan, decision-coverage-verify, ui-plan-gate', ERROR_REASON.SDK_UNKNOWN_COMMAND); } export = { routeCheckCommand, decisionMentioned, extractPlanDesignatedSections, + computeUiPlanGate, }; diff --git a/src/roadmap.cts b/src/roadmap.cts index b5335fa62..58a1f013c 100644 --- a/src/roadmap.cts +++ b/src/roadmap.cts @@ -175,6 +175,47 @@ function searchPhaseInContent(content: string, escapedPhase: string, phaseNum: s }; } +// ─── getRoadmapPhaseWithFallback ────────────────────────────────────────────── + +/** + * Two-pass phase lookup that mirrors cmdRoadmapGetPhase's resolution strategy. + * + * Pass 1: current-milestone slice (extractCurrentMilestone). + * Pass 2: full roadmap content (stripShippedMilestones) — covers cross-milestone + * and older frontend phases that are no longer in the current milestone slice. + * + * Returns the phase section string if found, null if ROADMAP.md is missing, + * or throws if ROADMAP.md read fails. + * + * Used by check-command-router (computeUiPlanGate) so ui-plan-gate uses the SAME + * phase resolution as `roadmap.get-phase` — not a milestone-only subset. + */ +function getRoadmapPhaseWithFallback(cwd: string, phaseNum: string): string | null { + const roadmapPath = planningPaths(cwd).roadmap; + if (!fs.existsSync(roadmapPath)) return null; + + const rawContent = fs.readFileSync(roadmapPath, 'utf-8'); + const milestoneContent = extractCurrentMilestone(rawContent, cwd); + const fullContent = stripShippedMilestones(rawContent); + + const exactSource = phaseMarkdownRegexSourceExact(phaseNum); + if (exactSource) { + const exactMilestone = searchPhaseInContent(milestoneContent, exactSource, phaseNum); + if (exactMilestone && !exactMilestone.error) return exactMilestone.section ?? null; + const exactFull = searchPhaseInContent(fullContent, exactSource, phaseNum); + if (exactFull && !exactFull.error) return exactFull.section ?? null; + } + + const escapedPhase = phaseMarkdownRegexSource(phaseNum); + const milestoneResult = searchPhaseInContent(milestoneContent, escapedPhase, phaseNum); + const result = (milestoneResult && !milestoneResult.error) + ? milestoneResult + : searchPhaseInContent(fullContent, escapedPhase, phaseNum) || milestoneResult; + + if (!result || result.error) return null; + return result.section ?? null; +} + // ─── cmdRoadmapGetPhase ─────────────────────────────────────────────────────── function cmdRoadmapGetPhase(cwd: string, phaseNum: string, raw: boolean): void { @@ -710,6 +751,7 @@ function cmdRoadmapAnnotateDependencies(cwd: string, phaseNum: string | null | u export = { cmdRoadmapGetPhase, + getRoadmapPhaseWithFallback, cmdRoadmapAnalyze, cmdRoadmapUpdatePlanProgress, cmdRoadmapAnnotateDependencies, diff --git a/tests/bug-3706-ui-safety-gate-false-positives.test.cjs b/tests/bug-3706-ui-safety-gate-false-positives.test.cjs index a354bcdec..627b8c54e 100644 --- a/tests/bug-3706-ui-safety-gate-false-positives.test.cjs +++ b/tests/bug-3706-ui-safety-gate-false-positives.test.cjs @@ -66,49 +66,71 @@ describe('Workflow .md structural guard (#3718)', () => { // The UI safety gate invocation is embedded in workflow prose-as-code. // These structural tests guard against regression where someone re-introduces // the shell-based locale-prefix invocation that silently breaks on Windows PowerShell. - for (const [label, filePath] of [ - ['plan-phase.md', PLAN_PHASE_PATH], - ['autonomous.md', AUTONOMOUS_PATH], - ]) { - test(`${label} must invoke ui-safety-gate.cjs via stdin, anchored to the GSD install dir`, () => { - const content = fs.readFileSync(filePath, 'utf-8'); - assert.ok( - content.includes('ui-safety-gate.cjs'), - `${label}: must reference ui-safety-gate.cjs for cross-shell portability (#3718)` - ); - // Scope structural assertions to the gate invocation region so we test the - // gate's OWN resolution, not §1's unrelated RUNTIME_DIR usage for gsd-tools. - const gi = content.indexOf('ui-safety-gate.cjs'); - const region = content.slice(Math.max(0, gi - 800), gi + 200); + // plan-phase.md (post-#1026): §5.6 now delegates UI gate evaluation to + // `gsd_run check ui-plan-gate` (a CLI command that internally calls checkUiPresence + // from ui-safety-gate.cjs). The direct shell invocation of ui-safety-gate.cjs was + // intentionally removed from the workflow — cross-shell portability is now provided + // by the CLI command layer, not inline shell code. + test('plan-phase.md must invoke check ui-plan-gate (capability-driven UI gate — #1026)', () => { + const content = fs.readFileSync(PLAN_PHASE_PATH, 'utf-8'); + assert.ok( + content.includes('check ui-plan-gate'), + 'plan-phase.md: §5.6 must delegate to `check ui-plan-gate` (capability-driven, #1026)' + ); + assert.ok( + !content.includes('LC_ALL=C grep'), + 'plan-phase.md: must NOT contain LC_ALL=C grep — that silently fails on Windows PowerShell (#3718)' + ); + // Must NOT reintroduce the old shell-based path-search loop for ui-safety-gate.cjs + assert.ok( + !content.includes('UI_GATE_JS=$(for _c in'), + 'plan-phase.md: must NOT contain the old shell-based ui-safety-gate.cjs path-search (old §5.6 pattern)' + ); + }); - // #448: the helper ships inside the GSD package, so it must be resolved - // against the GSD install dir (RUNTIME_DIR), NOT the consuming project's - // git root — otherwise the node call fails and the gate silently no-ops. - assert.ok( - region.includes('RUNTIME_DIR'), - `${label}: UI gate must resolve ui-safety-gate.cjs against RUNTIME_DIR (the GSD install dir), not the consuming project's git root (#448)` - ); - assert.ok( - !region.includes('GSD_REPO_ROOT'), - `${label}: UI gate must NOT anchor the helper to GSD_REPO_ROOT (the consuming project's git root) — that silently no-ops in installed repos (#448)` - ); - // Retain a git rev-parse --show-toplevel fallback when RUNTIME_DIR is unset (#3718). - assert.ok( - region.includes('git rev-parse --show-toplevel'), - `${label}: must retain a git rev-parse --show-toplevel fallback for the install-dir resolution` - ); - // Confirm stdin pipe usage (printf or echo piped to node) - assert.ok( - content.includes('printf') || content.includes('echo'), - `${label}: must pipe phase section via stdin (printf/echo | node) to avoid ARG_MAX` - ); - assert.ok( - !content.includes('LC_ALL=C grep'), - `${label}: must NOT contain LC_ALL=C grep — that silently fails on Windows PowerShell (#3718)` - ); - }); - } + // autonomous.md: still directly invokes ui-safety-gate.cjs via node stdin. + // This test is unchanged — autonomous.md §3a.5 has not been cut over to the + // capability-driven pattern yet (deferred per #1026 implementation notes). + test('autonomous.md must invoke ui-safety-gate.cjs via stdin, anchored to the GSD install dir', () => { + const label = 'autonomous.md'; + const content = fs.readFileSync(AUTONOMOUS_PATH, 'utf-8'); + assert.ok( + content.includes('ui-safety-gate.cjs'), + `${label}: must reference ui-safety-gate.cjs for cross-shell portability (#3718)` + ); + + // Scope structural assertions to the gate invocation region so we test the + // gate's OWN resolution, not §1's unrelated RUNTIME_DIR usage for gsd-tools. + const gi = content.indexOf('ui-safety-gate.cjs'); + const region = content.slice(Math.max(0, gi - 800), gi + 200); + + // #448: the helper ships inside the GSD package, so it must be resolved + // against the GSD install dir (RUNTIME_DIR), NOT the consuming project's + // git root — otherwise the node call fails and the gate silently no-ops. + assert.ok( + region.includes('RUNTIME_DIR'), + `${label}: UI gate must resolve ui-safety-gate.cjs against RUNTIME_DIR (the GSD install dir), not the consuming project's git root (#448)` + ); + assert.ok( + !region.includes('GSD_REPO_ROOT'), + `${label}: UI gate must NOT anchor the helper to GSD_REPO_ROOT (the consuming project's git root) — that silently no-ops in installed repos (#448)` + ); + // Retain a git rev-parse --show-toplevel fallback when RUNTIME_DIR is unset (#3718). + assert.ok( + region.includes('git rev-parse --show-toplevel'), + `${label}: must retain a git rev-parse --show-toplevel fallback for the install-dir resolution` + ); + // Confirm stdin pipe usage (printf or echo piped to node) + assert.ok( + content.includes('printf') || content.includes('echo'), + `${label}: must pipe phase section via stdin (printf/echo | node) to avoid ARG_MAX` + ); + assert.ok( + !content.includes('LC_ALL=C grep'), + `${label}: must NOT contain LC_ALL=C grep — that silently fails on Windows PowerShell (#3718)` + ); + }); }); // ── Cross-shell spawn test (#3718) ──────────────────────────────────────────── diff --git a/tests/capability-registry.test.cjs b/tests/capability-registry.test.cjs index 09e54a362..c10c85ad9 100644 --- a/tests/capability-registry.test.cjs +++ b/tests/capability-registry.test.cjs @@ -100,6 +100,18 @@ describe('UI pilot capability', () => { assert.ok(uiPhaseStep, 'plan:pre.steps should contain the ui-phase step'); assert.strictEqual(uiPhaseStep.capId, 'ui'); + // byLoopPoint['plan:pre'].gates contains the new ui.plan-gate (#1026) + const planPreGates = registry.byLoopPoint['plan:pre'].gates; + assert.ok(Array.isArray(planPreGates), 'plan:pre.gates should be an array'); + const uiPlanGate = planPreGates.find( + (g) => g.check && g.check.query === 'ui.plan-gate', + ); + assert.ok(uiPlanGate, 'plan:pre.gates should contain the ui.plan-gate (#1026)'); + assert.strictEqual(uiPlanGate.capId, 'ui'); + assert.strictEqual(uiPlanGate.blocking, true); + assert.strictEqual(uiPlanGate.when, 'workflow.ui_safety_gate'); + assert.strictEqual(uiPlanGate.onError, 'halt'); + // byLoopPoint['execute:wave:post'].gates contains the UI safety gate const execWavePostGates = registry.byLoopPoint['execute:wave:post'].gates; assert.ok(Array.isArray(execWavePostGates), 'execute:wave:post.gates should be an array'); diff --git a/tests/check-ui-plan-gate.test.cjs b/tests/check-ui-plan-gate.test.cjs new file mode 100644 index 000000000..76f99fe75 --- /dev/null +++ b/tests/check-ui-plan-gate.test.cjs @@ -0,0 +1,318 @@ +'use strict'; + +/** + * Behavioral tests for the `check ui-plan-gate` subcommand (#1026). + * + * Tests the `computeUiPlanGate` pure function exported from check-command-router.cjs. + * Uses in-memory tmpdir fixtures — no real CLI subprocess needed. + * + * Return shape: { frontend: bool, hasUiSpec: bool, block: bool, uiSpecPath: string|null } + * Invariant: block = frontend && !hasUiSpec + * + * Per RULESET.TESTS.boundary-coverage: exercises all three branches: + * (a) frontend+no-spec → block:true + * (b) frontend+spec → block:false + * (c) non-frontend → block:false + * + * Per RULESET.TESTS.coderabbit-fix-prefer: calls the exported function and asserts typed fields. + */ + +const { describe, test, before, after } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const { cleanup } = require('./helpers.cjs'); +const { computeUiPlanGate } = require('../gsd-core/bin/lib/check-command-router.cjs'); + +// ─── Helpers ────────────────────────────────────────────────────────────────── + +/** + * Create a minimal project dir with: + * .planning/ROADMAP.md — one phase section with `phaseSection` body + * .planning/phases/01-test-phase/ — phase directory + * (optionally) a *-UI-SPEC.md inside the phase dir + */ +function makeProject({ phaseSection = '', hasUiSpec = false } = {}) { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ui-plan-gate-test-')); + const planningDir = path.join(tmpDir, '.planning'); + const phasesDir = path.join(planningDir, 'phases'); + const phaseDir = path.join(phasesDir, '01-test-phase'); + + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(planningDir, 'config.json'), JSON.stringify({}), 'utf8'); + + // Minimal ROADMAP.md with one phase section + const roadmapContent = [ + '# Project Roadmap', + '', + '## Phase 1: Test Phase', + '', + phaseSection, + '', + ].join('\n'); + fs.writeFileSync(path.join(planningDir, 'ROADMAP.md'), roadmapContent, 'utf8'); + + if (hasUiSpec) { + fs.writeFileSync(path.join(phaseDir, '01-UI-SPEC.md'), '# UI Design Contract\n', 'utf8'); + } + + return { tmpDir, phaseDir }; +} + +// ─── Tests ───────────────────────────────────────────────────────────────────── + +describe('computeUiPlanGate — ui.plan-gate check logic (#1026)', () => { + let frontendNoSpec, frontendWithSpec, nonFrontend; + + before(() => { + // Branch (a): frontend + no UI-SPEC → block:true + frontendNoSpec = makeProject({ + phaseSection: 'Build the user interface and dashboard components for the frontend.', + hasUiSpec: false, + }); + // Branch (b): frontend + UI-SPEC exists → block:false + frontendWithSpec = makeProject({ + phaseSection: 'Build the frontend dashboard with React components and UI forms.', + hasUiSpec: true, + }); + // Branch (c): no frontend indicators → block:false + nonFrontend = makeProject({ + phaseSection: 'Add a REST API endpoint and database migration for the user table.', + hasUiSpec: false, + }); + }); + + after(() => { + for (const { tmpDir } of [frontendNoSpec, frontendWithSpec, nonFrontend]) { + try { cleanup(tmpDir); } catch { /* ignore */ } + } + }); + + describe('return shape', () => { + test('result has required keys: frontend, hasUiSpec, block, uiSpecPath', () => { + const result = computeUiPlanGate(nonFrontend.tmpDir, '1'); + assert.ok(typeof result === 'object' && result !== null, 'result must be an object'); + assert.ok(typeof result.frontend === 'boolean', 'frontend must be boolean'); + assert.ok(typeof result.hasUiSpec === 'boolean', 'hasUiSpec must be boolean'); + assert.ok(typeof result.block === 'boolean', 'block must be boolean'); + assert.ok('uiSpecPath' in result, 'uiSpecPath key must be present'); + }); + + test('block invariant: block === frontend && !hasUiSpec for all scenarios', () => { + for (const [label, { tmpDir }] of [ + ['frontendNoSpec', frontendNoSpec], + ['frontendWithSpec', frontendWithSpec], + ['nonFrontend', nonFrontend], + ]) { + const r = computeUiPlanGate(tmpDir, '1'); + assert.strictEqual( + r.block, + r.frontend && !r.hasUiSpec, + `${label}: block invariant violated — frontend=${r.frontend} hasUiSpec=${r.hasUiSpec} block=${r.block}`, + ); + } + }); + }); + + describe('branch (a) — frontend + no UI-SPEC → block:true', () => { + test('detects frontend indicators in phase section', () => { + const r = computeUiPlanGate(frontendNoSpec.tmpDir, '1'); + assert.strictEqual(r.frontend, true, 'should detect frontend indicators'); + }); + + test('hasUiSpec is false when no *-UI-SPEC.md exists', () => { + const r = computeUiPlanGate(frontendNoSpec.tmpDir, '1'); + assert.strictEqual(r.hasUiSpec, false, 'hasUiSpec must be false'); + }); + + test('block is true when frontend + no UI-SPEC', () => { + const r = computeUiPlanGate(frontendNoSpec.tmpDir, '1'); + assert.strictEqual(r.block, true, 'block must be true'); + }); + + test('uiSpecPath is null when no UI-SPEC', () => { + const r = computeUiPlanGate(frontendNoSpec.tmpDir, '1'); + assert.strictEqual(r.uiSpecPath, null, 'uiSpecPath must be null'); + }); + }); + + describe('branch (b) — frontend + UI-SPEC exists → block:false', () => { + test('detects frontend indicators in phase section', () => { + const r = computeUiPlanGate(frontendWithSpec.tmpDir, '1'); + assert.strictEqual(r.frontend, true, 'should detect frontend indicators'); + }); + + test('hasUiSpec is true when *-UI-SPEC.md exists', () => { + const r = computeUiPlanGate(frontendWithSpec.tmpDir, '1'); + assert.strictEqual(r.hasUiSpec, true, 'hasUiSpec must be true'); + }); + + test('block is false when UI-SPEC exists', () => { + const r = computeUiPlanGate(frontendWithSpec.tmpDir, '1'); + assert.strictEqual(r.block, false, 'block must be false when spec exists'); + }); + + test('uiSpecPath is a non-empty string ending in -UI-SPEC.md', () => { + const r = computeUiPlanGate(frontendWithSpec.tmpDir, '1'); + assert.ok(typeof r.uiSpecPath === 'string' && r.uiSpecPath.length > 0, + 'uiSpecPath must be a non-empty string'); + assert.ok(r.uiSpecPath.endsWith('-UI-SPEC.md'), 'uiSpecPath must end with -UI-SPEC.md'); + }); + }); + + describe('branch (c) — non-frontend phase → block:false', () => { + test('frontend is false for non-UI phase section', () => { + const r = computeUiPlanGate(nonFrontend.tmpDir, '1'); + assert.strictEqual(r.frontend, false, 'should NOT detect frontend indicators'); + }); + + test('block is false for non-frontend phases', () => { + const r = computeUiPlanGate(nonFrontend.tmpDir, '1'); + assert.strictEqual(r.block, false, 'block must be false'); + }); + }); + + describe('uses checkUiPresence word-boundary rules — no detection reimplementation', () => { + test('"microfrontend" (compound word) does NOT trigger frontend:true', () => { + const proj = makeProject({ + phaseSection: 'Refactor the microfrontend architecture for better code reuse.', + hasUiSpec: false, + }); + try { + const r = computeUiPlanGate(proj.tmpDir, '1'); + assert.strictEqual(r.frontend, false, + '"microfrontend" compound word must NOT trigger frontend (word-boundary rule from checkUiPresence)'); + } finally { + try { cleanup(proj.tmpDir); } catch { /* ignore */ } + } + }); + + test('"micro-frontend" (hyphenated) triggers frontend:true', () => { + const proj = makeProject({ + phaseSection: 'Refactor the micro-frontend architecture for better code reuse.', + hasUiSpec: false, + }); + try { + const r = computeUiPlanGate(proj.tmpDir, '1'); + assert.strictEqual(r.frontend, true, + '"micro-frontend" must trigger frontend detection (word-boundary rule from checkUiPresence)'); + } finally { + try { cleanup(proj.tmpDir); } catch { /* ignore */ } + } + }); + }); + + describe('graceful degradation', () => { + test('non-existent project dir returns frontend:false, block:false (no crash)', () => { + const r = computeUiPlanGate('/tmp/nonexistent-gsd-test-dir-xyz', '1'); + assert.strictEqual(typeof r.frontend, 'boolean', 'frontend must be boolean'); + assert.strictEqual(r.frontend, false, 'missing roadmap → no frontend indicators'); + assert.strictEqual(r.block, false, 'missing roadmap → block false'); + }); + + test('missing ROADMAP.md returns frontend:false gracefully (no phaseLookupFailed)', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ui-gate-nomap-')); + try { + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '01-phase'), { recursive: true }); + const r = computeUiPlanGate(tmpDir, '1'); + assert.strictEqual(r.frontend, false, 'no ROADMAP → no frontend indicators'); + assert.strictEqual(r.block, false, 'no ROADMAP → no block'); + // phaseLookupFailed must NOT be set when ROADMAP.md is absent (no-roadmap project) + assert.ok( + !r.phaseLookupFailed, + 'phaseLookupFailed must NOT be set when ROADMAP.md is absent (no-roadmap project is not a lookup failure)' + ); + } finally { + try { cleanup(tmpDir); } catch { /* ignore */ } + } + }); + + test('ROADMAP.md present but phase not found → phaseLookupFailed:true (not silent false)', () => { + // This verifies FIX 2: when ROADMAP.md exists but the phase header is absent, + // we surface phaseLookupFailed rather than silently degrading to frontend:false, + // so an onError:halt gate cannot be silently bypassed by a typo in the phase number. + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ui-gate-noPhase-')); + try { + const planningDir = path.join(tmpDir, '.planning'); + const phasesDir = path.join(planningDir, 'phases'); + fs.mkdirSync(path.join(phasesDir, '01-test-phase'), { recursive: true }); + // ROADMAP.md exists but has no Phase 99 header + fs.writeFileSync(path.join(planningDir, 'ROADMAP.md'), [ + '# Project Roadmap', + '', + '## Phase 1: Test Phase', + '', + 'Build the frontend dashboard with React components.', + '', + ].join('\n'), 'utf8'); + // Phase 99 is not in the roadmap + const r = computeUiPlanGate(tmpDir, '99'); + assert.strictEqual(r.phaseLookupFailed, true, + 'phaseLookupFailed must be true when ROADMAP.md exists but phase is not found'); + // frontend should be false because section is empty + assert.strictEqual(r.frontend, false, 'empty section → no frontend indicators'); + } finally { + try { cleanup(tmpDir); } catch { /* ignore */ } + } + }); + }); + + describe('full-roadmap fallback (FIX 2 — mirrors roadmap.get-phase two-pass lookup)', () => { + test('phase in non-current milestone section is found via full-roadmap fallback', () => { + // Simulates a project where STATE.md declares milestone v1.0, but Phase 1 is + // in the v0.9 section (an older milestone, NOT in a
block). + // extractCurrentMilestone(content, cwd) returns only the v1.0 section → misses Phase 1. + // stripShippedMilestones(content) returns the FULL roadmap (strips only
) → finds Phase 1. + // computeUiPlanGate must find it via the stripShippedMilestones fallback, matching + // what `gsd_run query roadmap.get-phase` does. + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ui-gate-milestone-')); + try { + const planningDir = path.join(tmpDir, '.planning'); + const phasesDir = path.join(planningDir, 'phases'); + fs.mkdirSync(path.join(phasesDir, '01-test-phase'), { recursive: true }); + + // STATE.md declares current milestone = v1.0 + fs.writeFileSync(path.join(planningDir, 'STATE.md'), [ + '---', + 'milestone: v1.0', + '---', + '', + 'State content.', + ].join('\n'), 'utf8'); + + // ROADMAP.md: Phase 1 is in v0.9 (NOT
), Phase 2 is in v1.0. + // With STATE.md pointing to v1.0, extractCurrentMilestone returns the v1.0 section only. + const roadmap = [ + '# Project Roadmap', + '', + '## v0.9 — Previous Milestone', + '', + '### Phase 1: Frontend Dashboard', + '', + 'Build the user interface and dashboard components for the frontend.', + '', + '## v1.0 — Current Milestone', + '', + '### Phase 2: API Layer', + '', + 'Add REST API endpoints.', + '', + ].join('\n'); + fs.writeFileSync(path.join(planningDir, 'ROADMAP.md'), roadmap, 'utf8'); + + // Phase 1 is a frontend phase in a non-current milestone — must still be detected + const r = computeUiPlanGate(tmpDir, '1'); + assert.strictEqual(r.frontend, true, + 'frontend:true must be detected for phase in non-current milestone (full-roadmap fallback)'); + assert.ok( + !r.phaseLookupFailed, + 'phaseLookupFailed must be false when phase is found via full-roadmap fallback' + ); + } finally { + try { cleanup(tmpDir); } catch { /* ignore */ } + } + }); + }); +}); diff --git a/tests/loop-hook-firing-spike.test.cjs b/tests/loop-hook-firing-spike.test.cjs index 6276ea881..6768a9de8 100644 --- a/tests/loop-hook-firing-spike.test.cjs +++ b/tests/loop-hook-firing-spike.test.cjs @@ -110,40 +110,81 @@ describe('spike #1018 — off means off (structural proof)', () => { "onError must be 'skip' as declared in the registry"); }); - // ── Case 2: UI off ───────────────────────────────────────────────────────── + // ── Case 2: STEP-only-off (ui_phase=false, ui_safety_gate=true) ────────────── // - // config { workflow: { ui_phase: false } } → step filtered out. - // hostConsume must report activeCount=0, skillsToInvoke=[], and - // rendered must equal the base/no-active form — the OFF output IS the base. + // (#1026) plan:pre now has TWO hooks — a step (when: workflow.ui_phase) and a + // gate (when: workflow.ui_safety_gate). They are INDEPENDENT toggles by design. + // Turning off ui_phase suppresses the step but the gate (ui_safety_gate=true) + // still fires → rendered is NOT the zero-hooks base (it contains the gate block). + // + // This case proves the step surface is a pure function of step-kind hooks: + // activeCount=0, skillsToInvoke=[] — "step off means step off". + // It also asserts the gate IS present in activeHooks and rendered differs from + // the empty base, documenting that the two toggles are genuinely independent. - test('UI off: config {workflow:{ui_phase:false}} → activeCount=0, skillsToInvoke=[], rendered equals zero-hooks base', () => { - const offConfig = { workflow: { ui_phase: false } }; + test('STEP-only-off: config {workflow:{ui_phase:false, ui_safety_gate:true}} → step absent, gate present, rendered ≠ base', () => { + const stepOffConfig = { workflow: { ui_phase: false, ui_safety_gate: true } }; const resolved = resolveLoopHooks({ point: 'plan:pre', registry: realRegistry, - config: offConfig, + config: stepOffConfig, }); const envelope = { activeHooks: resolved.activeHooks, rendered: renderLoopHooks(resolved) }; const consumed = hostConsume(envelope); - const base = makeBaseEnvelope('plan:pre'); - const baseConsumed = hostConsume(base); - + // Step-level aggregate: step is off assert.strictEqual(consumed.activeCount, 0, 'Expected 0 active steps when ui_phase=false'); assert.deepEqual(consumed.skillsToInvoke, [], 'skillsToInvoke must be [] when ui_phase=false'); - // STRUCTURAL ASSERTION (the spike's point): - // The off-state host output is identical to the host's zero-hooks base. - // The aggregate is a pure function of activeHooks — nothing in host source - // was mutated. This proves "off means off" by construction. - assert.strictEqual(consumed.rendered, baseConsumed.rendered, - 'OFF rendered output must be byte-identical to the zero-hooks base rendered output (pure function of activeHooks)'); - assert.strictEqual(consumed.activeCount, baseConsumed.activeCount, - 'OFF activeCount must equal base activeCount'); - assert.deepEqual(consumed.skillsToInvoke, baseConsumed.skillsToInvoke, - 'OFF skillsToInvoke must equal base skillsToInvoke'); + // Gate is still active: activeHooks is NOT empty (contains the gate hook) + const gateHooks = resolved.activeHooks.filter(h => h.kind === 'gate'); + assert.ok(gateHooks.length > 0, + 'Gate hook must still be present in activeHooks when ui_safety_gate=true'); + + // Rendered is NOT the zero-hooks base because the gate block is present + const base = makeBaseEnvelope('plan:pre'); + assert.notStrictEqual(consumed.rendered, base.rendered, + 'rendered must NOT equal the zero-hooks base when the gate is still active (ui_safety_gate=true)'); + + // Rendered must NOT include a step block for ui-phase + assert.ok( + !consumed.rendered.includes('### Step') || !consumed.rendered.includes('ui-phase'), + 'OFF rendered must not include an active step block for ui-phase', + ); + }); + + // ── Case 2b: ALL-OFF (ui_phase=false, ui_safety_gate=false) ───────────────── + // + // Both toggles off → activeHooks is genuinely empty → rendered is byte-identical + // to the zero-hooks base produced by the empty-registry helper. + // + // THIS is the clean structural "off means off → base output" proof. + // It must exist as a concrete, computable assertion — not be elided because a + // partial-off case happens to have a gate. The empty base is computed, not + // hand-coded, so if renderLoopHooks ever changes its empty format this still holds. + + test('ALL-OFF: config {workflow:{ui_phase:false, ui_safety_gate:false}} → activeHooks empty AND rendered === base', () => { + const allOffConfig = { workflow: { ui_phase: false, ui_safety_gate: false } }; + const resolved = resolveLoopHooks({ + point: 'plan:pre', + registry: realRegistry, + config: allOffConfig, + }); + const envelope = { activeHooks: resolved.activeHooks, rendered: renderLoopHooks(resolved) }; + + // STRUCTURAL ASSERTION (the spike's core proof — restored): + // When every hook at this point is toggled off, activeHooks must be empty + // and rendered must be byte-identical to the computed zero-hooks base. + // This proves the host aggregate is a pure function of activeHooks — no + // capability leaks through when all its controlling config keys are false. + assert.strictEqual(resolved.activeHooks.length, 0, + 'ALL-OFF: activeHooks must be empty when both ui_phase and ui_safety_gate are false'); + + const base = makeBaseEnvelope('plan:pre'); + assert.strictEqual(envelope.rendered, base.rendered, + 'ALL-OFF: rendered must be byte-identical to the zero-hooks base when activeHooks is empty'); }); // ── Case 3: Synthetic multi-hook ────────────────────────────────────────── diff --git a/tests/plan-phase-ui-redirect.test.cjs b/tests/plan-phase-ui-redirect.test.cjs index 2689ec674..a0b0740a8 100644 --- a/tests/plan-phase-ui-redirect.test.cjs +++ b/tests/plan-phase-ui-redirect.test.cjs @@ -10,7 +10,7 @@ const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); -describe('plan-phase UI-SPEC missing behavior', () => { +describe('plan-phase §5.6 UI Design Contract Gate', () => { const workflowPath = path.join( __dirname, '..', @@ -19,38 +19,289 @@ describe('plan-phase UI-SPEC missing behavior', () => { 'plan-phase.md' ); + // Load once test('workflow file exists', () => { assert.ok(fs.existsSync(workflowPath), `Expected workflow file at ${workflowPath}`); }); - test('does NOT contain hard-blocking exit redirect to /gsd-ui-phase', () => { - const text = fs.readFileSync(workflowPath, 'utf8'); - // The hard redirect pattern: AskUserQuestion option exits with "Run /gsd-ui-phase... Exit workflow." - // This is the pattern from line ~503 in the original file + // ── Capability-driven dispatch ───────────────────────────────────────────── + + test('§5.6 dispatches loop render-hooks plan:pre (capability-driven)', () => { + const section = extractSection56(workflowPath); + assert.ok( + section.includes('loop render-hooks plan:pre'), + '§5.6 must dispatch `loop render-hooks plan:pre` to resolve active capability hooks' + ); + }); + + test('§5.6 invokes gsd_run check ui-plan-gate UNCONDITIONALLY (not inside a gate loop)', () => { + const section = extractSection56(workflowPath); + // The fix for {ui_phase:true, ui_safety_gate:false}: the check must be run + // UNCONDITIONALLY — established before any per-hook branching — so the step's + // frontend/UI-SPEC precondition is available even when no gate hook is active. + // The literal `check ui-plan-gate` must appear in the section. + assert.ok( + section.includes('check ui-plan-gate'), + '§5.6 must run `gsd_run check ui-plan-gate` unconditionally (not gated inside a kind==gate loop)' + ); + // The "including the step-only case" wording proves the unconditional intent. + assert.ok( + section.includes('step-only case'), + '§5.6 must document "step-only case" to prove the check runs independently of gate presence' + ); + }); + + test('§5.6 establishes GATE= before any branch-5 step dispatch (check precedes step)', () => { + const section = extractSection56(workflowPath); + // GATE= must be assigned before Branch 5 fires step hooks. + // Both must be present; GATE= must appear earlier in the section text. + const gateIdx = section.indexOf('GATE=$(gsd_run check ui-plan-gate'); + const branch5Idx = section.indexOf('Branch 5'); + assert.ok(gateIdx !== -1, '§5.6 must contain `GATE=$(gsd_run check ui-plan-gate` assignment'); + assert.ok(branch5Idx !== -1, '§5.6 must contain Branch 5'); + assert.ok( + gateIdx < branch5Idx, + `§5.6 GATE= check must appear BEFORE Branch 5 step dispatch (gateIdx=${gateIdx} branch5Idx=${branch5Idx})` + ); + }); + + test('§5.6 invokes gsd_run check with gate check.query (generic dispatch — ui.plan-gate or ui-plan-gate)', () => { + const section = extractSection56(workflowPath); + // The dispatch MUST use the `check` verb and reference the gate query. + // OLD: hardcoded `check ui-plan-gate` inside gate loop + // NEW: unconditional `check ui-plan-gate` — literal command; check.query = "ui.plan-gate" + // Both forms must be accepted: the section references either the literal query or the + // generic pattern. The check router normalizes dots to hyphens so both are equivalent. + assert.ok( + section.includes('check ui-plan-gate') || + section.includes('check ui.plan-gate') || + (section.includes('check') && section.includes('check.query')), + '§5.6 must invoke `gsd_run check` with the gate\'s check.query (generic dispatch — not hardcoded for a specific check)' + ); + }); + + test('§5.6 reads frontend, hasUiSpec, block from gate result', () => { + const section = extractSection56(workflowPath); + assert.ok(section.includes('frontend'), '§5.6 must read `frontend` from gate result'); + assert.ok(section.includes('hasUiSpec'), '§5.6 must read `hasUiSpec` from gate result'); + assert.ok(section.includes('block'), '§5.6 must read `block` from gate result'); + }); + + test('§5.6 does NOT inline config-get workflow.ui_phase (toggle owned by registry)', () => { + const section = extractSection56(workflowPath); + assert.ok( + !section.includes('config-get workflow.ui_phase'), + '§5.6 must NOT inline `config-get workflow.ui_phase` — toggle is resolved by render-hooks' + ); + }); + + test('§5.6 does NOT inline config-get workflow.ui_safety_gate (toggle owned by registry)', () => { + const section = extractSection56(workflowPath); + assert.ok( + !section.includes('config-get workflow.ui_safety_gate'), + '§5.6 must NOT inline `config-get workflow.ui_safety_gate` — toggle is resolved by render-hooks' + ); + }); + + // ── 6-branch equivalence ─────────────────────────────────────────────────── + + test('Branch 1: activeHooks empty → skip to step 6 (NOT §5.7)', () => { + const section = extractSection56(workflowPath); + // Branch 1 (both toggles off) MUST skip to step 6 — NOT §5.7. + // OLD §5.6 branch-1 target was "step 6". §5.7 (Schema Push) is NOT skipped + // when both UI toggles are off — schema detection is independent. + assert.ok( + section.includes('activeHooks'), + '§5.6 Branch 1 must reference activeHooks' + ); + // The skip target must be step 6 + assert.ok( + section.match(/activeHooks[^.]*?step 6/s) || + section.match(/empty[^.]*?step 6/s) || + section.match(/absent[^.]*?step 6/s) || + /Branch 1[^.]*?step 6/s.test(section), + '§5.6 Branch 1 (both-off / empty activeHooks) must skip to step 6, NOT §5.7' + ); + }); + + test('Branch 2: no frontend indicators → skip silently to §5.7', () => { + const section = extractSection56(workflowPath); + assert.ok( + section.includes('frontend') && section.includes('5.7'), + '§5.6 Branch 2 must route non-frontend phases to §5.7' + ); + }); + + test('Branch 3: hasUiSpec true → sets UI_SPEC_PATH, displays using-contract message', () => { + const section = extractSection56(workflowPath); + assert.ok( + section.includes('hasUiSpec') && section.includes('UI_SPEC_PATH') && section.includes('Using UI design contract'), + '§5.6 Branch 3 must set UI_SPEC_PATH and display the using-contract message' + ); + }); + + test('Branch 4: --skip-ui in $ARGUMENTS → skip to step 6', () => { + const content = fs.readFileSync(workflowPath, 'utf8'); + assert.ok( + content.includes('--skip-ui'), + '§5.6 must include --skip-ui bypass option (Branch 4)' + ); + }); + + test('Branch 5: AUTO_CHAIN=true → step hooks dispatched via gsd-${ref.skill}', () => { + const section = extractSection56(workflowPath); + assert.ok( + section.includes('AUTO_CHAIN') && section.includes('gsd-${ref.skill}'), + '§5.6 Branch 5 must dispatch step hooks via gsd-${ref.skill} in pipeline mode' + ); + }); + + test('Branch 5: pipeline dispatch uses kind=="step" and ref.skill filter', () => { + const section = extractSection56(workflowPath); + assert.ok( + section.includes('kind') && section.includes('ref.skill'), + '§5.6 Branch 5 must filter activeHooks by kind=="step" and ref.skill' + ); + }); + + test('Branch 5: step hooks fire independently of whether a gate is active ({ui_phase:T,ui_safety_gate:F} regression guard)', () => { + const section = extractSection56(workflowPath); + // KEY REGRESSION GUARD: the step must fire even when ui_safety_gate=false (no gate hook). + // The new wording "independently of whether a gate is active" makes this explicit. + assert.ok( + section.includes('independently of whether a gate is active'), + '§5.6 Branch 5 must state step hooks fire "independently of whether a gate is active" — guards {ui_phase:T,ui_safety_gate:F} regression' + ); + }); + + test('Branch 6: AUTO_CHAIN=false → generic gate handling (kind==gate, blocking, block→exit; no active gate→continue)', () => { + const section = extractSection56(workflowPath); + assert.ok( + section.includes('AUTO_CHAIN'), + '§5.6 Branch 6 must read AUTO_CHAIN flag' + ); + // Branch 6 is the generic gate-handling branch: + // - iterates activeHooks where kind=="gate" and blocking:true + // - only EXITs when block:true is set on that gate + // - if no active blocking gate is present (e.g. ui_safety_gate off), continues to step 6 + assert.ok( + section.includes('kind == "gate"') || section.includes('kind=="gate"'), + '§5.6 Branch 6 must filter by kind=="gate" (generic gate-handling)' + ); + assert.ok( + /blocking.*true/i.test(section), + '§5.6 Branch 6 must check `blocking` is true for the gate' + ); + // Must halt on block:true — both "Exit the plan-phase workflow" AND "Do not continue" are required + assert.ok( + section.includes('Exit the plan-phase workflow'), + '§5.6 Branch 6 must say "Exit the plan-phase workflow"' + ); + assert.ok( + section.includes('Do not continue'), + '§5.6 Branch 6 must say "Do not continue"' + ); + // If no active blocking gate → continue (no block when ui_safety_gate is off) + assert.ok( + section.includes('no active blocking gate') || section.includes('no block'), + '§5.6 Branch 6 must document: no active blocking gate → continue to step 6 (no block)' + ); + }); + + test('Branch 6: recommendation block contains EXACT warning heading + /gsd:ui-phase + --skip-ui', () => { + const section = extractSection56(workflowPath); + // Assert the EXACT halt recommendation block text, not substring-OR + assert.ok( + section.includes('⚠ UI-SPEC.md missing for Phase'), + '§5.6 must include the "⚠ UI-SPEC.md missing for Phase" warning heading' + ); + assert.ok( + section.includes('Recommended next step'), + '§5.6 must include the "Recommended next step" label in the recommendation block' + ); + assert.ok( + section.includes('/gsd:ui-phase'), + '§5.6 must include /gsd:ui-phase recommendation in the block' + ); + assert.ok( + section.includes('--skip-ui'), + '§5.6 must include --skip-ui as bypass option in the recommendation block' + ); + // The "Also available" line must be present (shows the skip-ui option) + assert.ok( + section.includes('Also available'), + '§5.6 recommendation block must include the "Also available" section' + ); + }); + + // ── Generic gate-dispatch contract ──────────────────────────────────────── + + test('§5.6 documents gate check.query binding (check.query="ui.plan-gate" → check ui-plan-gate)', () => { + const section = extractSection56(workflowPath); + // The section must document WHERE "ui-plan-gate" comes from — it is the gate hook's + // check.query value "ui.plan-gate" normalized to a hyphen form by the check router. + // The literal `check ui-plan-gate` command is acceptable (and now required by bug-3706), + // but the section must also document the check.query binding so the contract is auditable. + assert.ok( + section.includes('check.query') || section.includes('check ui-plan-gate'), + '§5.6 must reference check.query or the literal check ui-plan-gate command (gate binding contract)' + ); + }); + + test('§5.6 documents that check router normalizes dots to hyphens (ui.plan-gate → ui-plan-gate)', () => { + const section = extractSection56(workflowPath); + // The dot-normalization rule must be documented so the declared check.query is runnable + assert.ok( + section.includes('ui.plan-gate') || section.includes('normalizes dots') || section.includes('dots to hyphens'), + '§5.6 must document that the check router normalizes dots to hyphens for the declared check.query' + ); + }); + + test('§5.6 partial-off case: ui_phase=true, ui_safety_gate=false → step fires, gate does NOT block', () => { + const section = extractSection56(workflowPath); + // The intended behavior change: ui_phase gates the step, ui_safety_gate gates the block. + // When only ui_safety_gate is false, the step fires but the gate doesn't halt. + // Document this in the section. + assert.ok( + section.includes('workflow.ui_phase') || section.includes('ui_phase'), + '§5.6 must reference workflow.ui_phase (step hook config key)' + ); + assert.ok( + section.includes('workflow.ui_safety_gate') || section.includes('ui_safety_gate'), + '§5.6 must reference workflow.ui_safety_gate (gate hook config key)' + ); + }); + + // ── Legacy pattern that must NOT appear ──────────────────────────────────── + + test('does NOT contain hard-blocking exit redirect to /gsd-ui-phase (old pattern)', () => { + const content = fs.readFileSync(workflowPath, 'utf8'); const hardExitPattern = /Generate UI-SPEC first.*Exit workflow/s; assert.ok( - !hardExitPattern.test(text), - 'plan-phase.md must NOT contain a hard "Generate UI-SPEC first → Exit workflow" redirect. ' + - 'It should offer a primary recommendation with --skip-ui bypass option instead.' + !hardExitPattern.test(content), + 'plan-phase.md must NOT contain a hard "Generate UI-SPEC first → Exit workflow" redirect' ); }); - test('contains --skip-ui bypass option when UI-SPEC.md is missing', () => { - const text = fs.readFileSync(workflowPath, 'utf8'); + test('does NOT inline the shell-based ui-safety-gate.cjs path-search block (old §5.6)', () => { + const section = extractSection56(workflowPath); + // The old §5.6 resolved the gate helper via a for-loop over path candidates assert.ok( - text.includes('--skip-ui'), - 'plan-phase.md must include --skip-ui as a bypass option when UI-SPEC.md is missing' - ); - }); - - test('contains a primary recommendation block for missing UI-SPEC', () => { - const text = fs.readFileSync(workflowPath, 'utf8'); - const hasRecommendationPattern = - text.includes('Recommended next step') && - text.includes('gsd-ui-phase'); - assert.ok( - hasRecommendationPattern, - 'plan-phase.md must include a "Recommended next step" recommendation for /gsd-ui-phase when UI-SPEC.md is missing' + !section.includes('UI_GATE_JS=$(for _c in'), + '§5.6 must NOT contain the old shell-based ui-safety-gate.cjs path-search (now delegated to check ui-plan-gate)' ); }); }); + +/** + * Extract the text of §5.6 through (but not including) §5.7. + * Scoped to avoid false positives from other parts of the file. + */ +function extractSection56(workflowPath) { + const content = fs.readFileSync(workflowPath, 'utf8'); + const start = content.indexOf('## 5.6.'); + assert.ok(start !== -1, '§5.6 heading must be present in plan-phase.md'); + const end = content.indexOf('## 5.7.', start); + assert.ok(end !== -1, '§5.7 heading must follow §5.6 in plan-phase.md'); + return content.slice(start, end); +}