From 2ac65920961050c5cfd9c5344c7d54a8d0d8ed2c Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 10 Jun 2026 22:40:58 -0400 Subject: [PATCH] =?UTF-8?q?feat(#1023):=20first=20phase-6=20cutover=20?= =?UTF-8?q?=E2=80=94=20ui-review=20(verify:post)=20inline=20=E2=86=92=20lo?= =?UTF-8?q?op.render-hooks=20dispatch=20(#1024)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the inlined ui-review invocation in autonomous.md §3d.5 with a loop.render-hooks verify:post dispatch — the first workflow to consume render-hooks and fire a skill from it (closes the #1018 live-execution residual as real wiring). Capability-driven, equivalence-preserving for the current registry (only ui-review at verify:post, default on): fires gsd-ui-review under the same precondition (UI-SPEC exists via consumes-gate + workflow.ui_review). Gate findings (real pattern issues, fixed so every future cutover inherits them): - bug-2643 static "Skill() references a real skill" check vs templated Skill(skill="gsd-${ref.skill}") dispatch → skip ${...}-templated names. - Coverage moved, not lost: gen-capability-registry now validates steps[].ref.skill in skills + ref.agent in agents + rejects gsd- double-prefix. - Tightened §3d.5 tests; markdown clarity (consumes rule, LLM-native JSON read, UI-REVIEW.md score hint). gsd-ui-review skill + §3a.5/ui-phase untouched. §5.6/ui-phase cutover deferred (#1022 step-can-halt-vs-gate model question). Closes #1023 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 --- gsd-core/workflows/autonomous.md | 24 +-- scripts/gen-capability-registry.cjs | 42 ++++- tests/autonomous-ui-steps.test.cjs | 79 +++++++-- .../bug-2643-skill-frontmatter-name.test.cjs | 24 ++- tests/capability-registry.test.cjs | 163 ++++++++++++++++++ 5 files changed, 300 insertions(+), 32 deletions(-) diff --git a/gsd-core/workflows/autonomous.md b/gsd-core/workflows/autonomous.md index be9cca710..991e4c89c 100644 --- a/gsd-core/workflows/autonomous.md +++ b/gsd-core/workflows/autonomous.md @@ -575,33 +575,27 @@ On **"Stop autonomous mode"**: Go to handle_blocker with "User stopped — gaps > Run after any successful execution routing (passed, human_needed accepted, or gaps deferred/accepted) — before proceeding to the iterate step. -Check if this phase had a UI-SPEC (created in step 3a.5 or pre-existing): +Resolve the active post-verification hooks and the UI-SPEC gate: ```bash UI_SPEC_FILE=$(ls "${PHASE_DIR}"/*-UI-SPEC.md 2>/dev/null | head -1) +HOOKS_JSON=$(gsd_run loop render-hooks verify:post --raw) ``` -Check if UI review is enabled: +Read the `activeHooks` array directly from the `HOOKS_JSON` value already in context (do not invoke a shell `jq` pipeline — parse as the JSON object it is). **If `activeHooks` is empty or absent:** skip silently to the iterate step. -```bash -UI_REVIEW_CFG=$(gsd_run query config-get workflow.ui_review 2>/dev/null || echo "true") -``` +For each entry in `activeHooks` in array order where `kind == "step"` and `ref.skill` is set: -**If `UI_SPEC_FILE` is not empty AND `UI_REVIEW_CFG` is not `false`:** - -Display: +- **Honor `consumes`:** if the hook's `consumes` array includes `"UI-SPEC.md"` and `UI_SPEC_FILE` is empty (no `*-UI-SPEC.md` exists in `PHASE_DIR`) → skip that hook (`onError: skip`). Hooks that do not declare `"UI-SPEC.md"` in their `consumes` proceed normally regardless of `UI_SPEC_FILE`. +- Invoke: ``` -Phase ${PHASE_NUM}: Frontend phase with UI-SPEC — running UI review audit... +Skill(skill="gsd-${ref.skill}", args="${PHASE_NUM}") ``` -``` -Skill(skill="gsd-ui-review", args="${PHASE_NUM}") -``` +(i.e. prepend `gsd-` to `ref.skill` — so `ui-review` → `gsd-ui-review`.) -Display the review result summary (score from UI-REVIEW.md if produced). Continue to iterate step regardless of score — UI review is advisory, not blocking. - -**If `UI_SPEC_FILE` is empty OR `UI_REVIEW_CFG` is `false`:** Skip silently to iterate step. +Display the review result summary and score from UI-REVIEW.md if produced. Continue to iterate step regardless of result — hooks at this point are advisory, not blocking. diff --git a/scripts/gen-capability-registry.cjs b/scripts/gen-capability-registry.cjs index 17ee3adbc..d7b191f81 100644 --- a/scripts/gen-capability-registry.cjs +++ b/scripts/gen-capability-registry.cjs @@ -418,11 +418,16 @@ function validateFeatureBody(cap) { } } + // Build the declared skill/agent sets for ref membership checks (used in validateStep). + // Only build these if the arrays are valid (already validated above). + const declaredSkills = Array.isArray(cap.skills) ? new Set(cap.skills.filter((s) => typeof s === 'string')) : null; + const declaredAgents = Array.isArray(cap.agents) ? new Set(cap.agents.filter((a) => typeof a === 'string')) : null; + if (!Array.isArray(cap.steps)) { errors.push('steps must be an array'); } else { for (let i = 0; i < cap.steps.length; i++) { - errors.push(...validateStep(cap.steps[i], 'steps[' + i + ']')); + errors.push(...validateStep(cap.steps[i], 'steps[' + i + ']', declaredSkills, declaredAgents)); } } @@ -491,7 +496,18 @@ function validateRuntimeBody(cap) { return errors; } -function validateStep(step, prefix) { +/** + * Validate a single step entry. + * + * @param {object} step The step to validate. + * @param {string} prefix Path prefix for error messages (e.g. "steps[0]"). + * @param {Set|null} declaredSkills Set of skill stems declared in this capability's skills array, + * or null if the skills array was not valid (skip membership check). + * @param {Set|null} declaredAgents Set of agent names declared in this capability's agents array, + * or null if the agents array was not valid (skip membership check). + * @returns {string[]} + */ +function validateStep(step, prefix, declaredSkills, declaredAgents) { const errors = []; if (!VALID_LOOP_POINTS.has(step.point)) { @@ -511,9 +527,31 @@ function validateStep(step, prefix) { } if (hasSkill && typeof step.ref.skill !== 'string') { errors.push(prefix + '.ref.skill must be a string'); + } else if (hasSkill && typeof step.ref.skill === 'string' && step.ref.skill.startsWith('gsd-')) { + // Double-prefix guard: ref.skill is an unprefixed stem (e.g. "ui-review"). + // Workflow dispatch prepends "gsd-" at runtime → "gsd-ui-review". + // A stem that already starts with "gsd-" would produce "gsd-gsd-..." at dispatch. + errors.push( + prefix + '.ref.skill "' + step.ref.skill + '" must not start with "gsd-" ' + + '(it is an unprefixed stem; the workflow prepends "gsd-" at dispatch — ' + + 'starting with "gsd-" would produce "gsd-' + step.ref.skill + '")', + ); + } else if (hasSkill && typeof step.ref.skill === 'string' && declaredSkills !== null && !declaredSkills.has(step.ref.skill)) { + // Membership check: ref.skill must be declared in this capability's skills array. + // This catches typos and ensures every dispatched skill is owned by this capability. + errors.push( + prefix + '.ref.skill "' + step.ref.skill + '" is not declared in this capability\'s skills: [' + + [...declaredSkills].join(', ') + ']', + ); } if (hasAgent && typeof step.ref.agent !== 'string') { errors.push(prefix + '.ref.agent must be a string'); + } else if (hasAgent && typeof step.ref.agent === 'string' && declaredAgents !== null && !declaredAgents.has(step.ref.agent)) { + // Membership check: ref.agent must be declared in this capability's agents array. + errors.push( + prefix + '.ref.agent "' + step.ref.agent + '" is not declared in this capability\'s agents: [' + + [...declaredAgents].join(', ') + ']', + ); } } diff --git a/tests/autonomous-ui-steps.test.cjs b/tests/autonomous-ui-steps.test.cjs index 28205874b..b8ab59fb0 100644 --- a/tests/autonomous-ui-steps.test.cjs +++ b/tests/autonomous-ui-steps.test.cjs @@ -83,33 +83,86 @@ describe('autonomous workflow ui-phase and ui-review integration (#1375)', () => ); }); - test('UI review step checks for UI-SPEC existence before running', () => { - // The UI review should only run if a UI-SPEC was created/exists + test('UI review step dispatches loop render-hooks verify:post', () => { + // Phase 6 cutover: §3d.5 now dispatches render-hooks verify:post instead of + // inlining a direct skill="gsd-ui-review" call. + const reviewSection = content.slice(content.indexOf('3d.5')); + assert.ok( + reviewSection.includes('loop render-hooks verify:post'), + 'UI review step should dispatch loop render-hooks verify:post to resolve active hooks' + ); + }); + + test('UI review step constructs skill via gsd- prefix dispatch and is non-blocking', () => { + // Phase 6 cutover: §3d.5 dispatches loop render-hooks verify:post, invokes skills via + // gsd-${ref.skill} prefix, gates on UI_SPEC_FILE for consumes:UI-SPEC.md hooks, + // and is explicitly advisory/non-blocking. + // All four properties must be present within the §3d.5 section itself. + const sectionStart = content.indexOf('3d.5'); + // Bound to the closing tag of the execute_phase step + const sectionEnd = content.indexOf('', sectionStart); + assert.ok(sectionStart !== -1, '§3d.5 heading must be present in autonomous.md'); + assert.ok(sectionEnd !== -1, ' must follow §3d.5'); + const reviewSection = content.slice(sectionStart, sectionEnd); + + assert.ok( + reviewSection.includes('loop render-hooks verify:post'), + '§3d.5 must dispatch `loop render-hooks verify:post` to resolve active capability hooks' + ); + assert.ok( + reviewSection.includes('gsd-${ref.skill}'), + '§3d.5 must construct skill name via `gsd-${ref.skill}` prefix (capability-driven dispatch)' + ); + assert.ok( + reviewSection.includes('UI_SPEC_FILE'), + '§3d.5 must gate on UI_SPEC_FILE for hooks that consume UI-SPEC.md' + ); + assert.ok( + reviewSection.includes('advisory') || reviewSection.includes('non-blocking') || reviewSection.includes('regardless of result'), + '§3d.5 must be explicitly advisory/non-blocking' + ); + }); + + test('UI review step gates on UI-SPEC file via consumes check', () => { + // The consumes:[UI-SPEC.md] gate is still enforced; UI_SPEC_FILE is still defined + // and used as the precondition for hooks that consume UI-SPEC.md. const reviewSection = content.slice(content.indexOf('3d.5')); assert.ok( reviewSection.includes('UI_SPEC_FILE'), - 'UI review step should check for UI-SPEC file existence' + 'UI review step should still gate on UI_SPEC_FILE for hooks that consume UI-SPEC.md' + ); + assert.ok( + reviewSection.includes('consumes'), + 'UI review step should reference hook consumes array for the UI-SPEC gate' ); }); - test('UI review step respects workflow.ui_review config toggle', () => { - assert.ok( - content.includes('workflow.ui_review'), - 'should respect workflow.ui_review config toggle' - ); - }); + test('UI review step respects workflow.ui_review config toggle (resolved via render-hooks)', () => { + // Phase 6 cutover: workflow.ui_review is no longer inlined as `config-get workflow.ui_review`. + // Instead, §3d.5 calls `loop render-hooks verify:post` which internally honours the + // `when: workflow.ui_review` field declared in the capability registry. + // The §3d.5 section must use render-hooks (not a literal `config-get workflow.ui_review`) + // so the toggle is resolved by the capability system, not duplicated inline. + const sectionStart = content.indexOf('3d.5'); + const sectionEnd = content.indexOf('', sectionStart); + assert.ok(sectionStart !== -1, '§3d.5 heading must be present in autonomous.md'); + assert.ok(sectionEnd !== -1, ' must follow §3d.5'); + const reviewSection = content.slice(sectionStart, sectionEnd); - test('UI review step invokes gsd:ui-review skill', () => { assert.ok( - content.includes('skill="gsd-ui-review"'), - 'should invoke gsd-ui-review via Skill()' + reviewSection.includes('render-hooks'), + '§3d.5 must resolve the workflow.ui_review toggle via render-hooks (not inline config-get)' + ); + assert.ok( + !reviewSection.includes('config-get workflow.ui_review'), + '§3d.5 must NOT inline `config-get workflow.ui_review` — the toggle is owned by the capability registry' ); }); test('UI review is advisory (non-blocking)', () => { const reviewSection = content.slice(content.indexOf('3d.5')); assert.ok( - reviewSection.includes('advisory') || reviewSection.includes('non-blocking') || reviewSection.includes('regardless of score'), + reviewSection.includes('advisory') || reviewSection.includes('non-blocking') || reviewSection.includes('regardless of result'), 'UI review should be advisory and not block phase progression' ); }); diff --git a/tests/bug-2643-skill-frontmatter-name.test.cjs b/tests/bug-2643-skill-frontmatter-name.test.cjs index 9fe4362a0..0ed5cbb6f 100644 --- a/tests/bug-2643-skill-frontmatter-name.test.cjs +++ b/tests/bug-2643-skill-frontmatter-name.test.cjs @@ -155,13 +155,27 @@ describe('skill frontmatter name parity (#2643 / #2808)', () => { test('every workflow Skill(skill="gsd-") resolves to an emitted skill name', () => { const workflowFiles = collectFiles(WORKFLOWS_DIR); const referenced = new Set(); + const templatedSkipped = []; for (const f of workflowFiles) { const src = fs.readFileSync(f, 'utf-8'); - for (const n of extractSkillNamesHyphen(src)) referenced.add(n); + for (const n of extractSkillNamesHyphen(src)) { + // Skip template expressions (e.g. `gsd-${ref.skill}`): these are + // capability-dispatched — the skill stem is resolved at runtime from + // the `loop render-hooks` registry output (ADR-857 phase 6), so there + // is no single literal skill file to validate against here. + // The capability registry's own validateStep gate (gen-capability-registry.cjs) + // is responsible for ensuring each `steps[].ref.skill` corresponds to a + // real skill declared in the capability's `skills` array. + if (n.includes('${')) { + templatedSkipped.push(path.basename(f) + ': ' + n); + } else { + referenced.add(n); + } + } } assert.ok( referenced.size > 0, - `expected at least one Skill(skill="gsd-") reference in workflows under ${WORKFLOWS_DIR}` + `expected at least one literal Skill(skill="gsd-") reference in workflows under ${WORKFLOWS_DIR}` ); const emitted = new Set(); @@ -182,5 +196,11 @@ describe('skill frontmatter name parity (#2643 / #2808)', () => { [], 'workflow refs not emitted as skill names: ' + missing.join(', '), ); + // Informational: report how many templated dispatches were intentionally skipped. + // (Templated names are validated by the capability registry, not statically here.) + if (templatedSkipped.length > 0) { + // Not a failure — just a note for test output transparency. + // Use a diagnostic comment: node:test does not have a skip-within-test API. + } }); }); diff --git a/tests/capability-registry.test.cjs b/tests/capability-registry.test.cjs index 4f391be4d..09e54a362 100644 --- a/tests/capability-registry.test.cjs +++ b/tests/capability-registry.test.cjs @@ -727,6 +727,169 @@ describe('Fix #4: step.ref must be exclusive skill XOR agent', () => { }); }); +describe('double-prefix guard: step.ref.skill must not start with "gsd-"', () => { + // ref.skill is an unprefixed stem (e.g. "ui-review"). Workflow dispatch prepends + // "gsd-" at runtime. A stem already starting with "gsd-" would produce "gsd-gsd-..." + // at dispatch time, silently invoking a non-existent skill. + + test('ref.skill starting with "gsd-" is rejected', () => { + const cap = { + ...UI_CAP, + steps: [ + { + point: 'verify:post', + ref: { skill: 'gsd-ui-review' }, // wrong: stem must NOT have gsd- prefix + produces: ['UI-REVIEW.md'], + consumes: ['UI-SPEC.md'], + when: 'workflow.ui_review', + onError: 'skip', + }, + ], + }; + const errors = validateCapability(cap, 'ui'); + assert.ok(errors.length > 0, 'Expected an error for gsd-prefixed ref.skill'); + assert.ok( + errors.some((e) => e.includes('gsd-') && (e.includes('double') || e.includes('unprefixed') || e.includes('must not start'))), + 'Error should mention the double-prefix problem, got: ' + JSON.stringify(errors), + ); + }); + + test('ref.skill without "gsd-" prefix is accepted (stem only)', () => { + const cap = { + ...UI_CAP, + steps: [ + { + point: 'verify:post', + ref: { skill: 'ui-review' }, // correct: unprefixed stem + produces: ['UI-REVIEW.md'], + consumes: ['UI-SPEC.md'], + when: 'workflow.ui_review', + onError: 'skip', + }, + ], + }; + const prefixErrors = validateCapability(cap, 'ui').filter((e) => e.includes('gsd-') && e.includes('stem')); + assert.deepEqual(prefixErrors, [], 'No prefix errors expected for unprefixed stem, got: ' + JSON.stringify(prefixErrors)); + }); + + test('real UI capability.json uses unprefixed ref.skill values', () => { + // Verify the live capability uses unprefixed stems and therefore passes the new guard. + const errors = validateCapability(UI_CAP, 'ui'); + const prefixErrors = errors.filter((e) => e.includes('must not start with')); + assert.deepEqual(prefixErrors, [], 'Live UI capability.json should not trigger the double-prefix guard: ' + JSON.stringify(prefixErrors)); + }); +}); + +// ─── Fix: ref.skill/ref.agent membership in declared skills/agents ──────────── + +describe('ref membership check: step.ref.skill must be in cap.skills', () => { + // A capability declares skills: ["ui-phase", "ui-review"]. + // A step with ref.skill "typo-skill" (not in skills) must be rejected. + + test('step.ref.skill NOT in cap.skills is rejected', () => { + const cap = { + ...UI_CAP, + steps: [ + { + point: 'plan:pre', + ref: { skill: 'typo-skill' }, // not in skills: ["ui-phase", "ui-review"] + produces: ['UI-SPEC.md'], + consumes: ['CONTEXT.md'], + when: 'workflow.ui_phase', + onError: 'skip', + }, + ], + }; + const errors = validateCapability(cap, 'ui'); + assert.ok(errors.length > 0, 'Expected errors for undeclared ref.skill'); + assert.ok( + errors.some((e) => e.includes('typo-skill') && e.includes('not declared')), + 'Error should mention "typo-skill" and "not declared", got: ' + JSON.stringify(errors), + ); + }); + + test('step.ref.skill IN cap.skills is accepted', () => { + const cap = { + ...UI_CAP, + steps: [ + { + point: 'plan:pre', + ref: { skill: 'ui-phase' }, // declared in skills: ["ui-phase", "ui-review"] + produces: ['UI-SPEC.md'], + consumes: ['CONTEXT.md'], + when: 'workflow.ui_phase', + onError: 'skip', + }, + ], + }; + const errors = validateCapability(cap, 'ui'); + const membershipErrors = errors.filter((e) => e.includes('not declared') && e.includes('ui-phase')); + assert.deepEqual( + membershipErrors, [], + 'No membership errors expected for declared ref.skill, got: ' + JSON.stringify(membershipErrors), + ); + }); + + test('real UI capability passes: ui-phase and ui-review are both in skills', () => { + // Regression guard: the real UI capability must not trigger the new membership check. + const errors = validateCapability(UI_CAP, 'ui'); + const membershipErrors = errors.filter((e) => e.includes('not declared')); + assert.deepEqual( + membershipErrors, [], + 'Real UI capability should pass membership check for all ref.skill values, got: ' + JSON.stringify(membershipErrors), + ); + }); +}); + +describe('ref membership check: step.ref.agent must be in cap.agents', () => { + // A capability declares agents: ["gsd-ui-checker", "gsd-ui-auditor"]. + // A step with ref.agent "gsd-unknown-agent" (not in agents) must be rejected. + + test('step.ref.agent NOT in cap.agents is rejected', () => { + const cap = { + ...UI_CAP, + steps: [ + { + point: 'plan:pre', + ref: { agent: 'gsd-unknown-agent' }, // not in agents: ["gsd-ui-checker", "gsd-ui-auditor"] + produces: ['UI-SPEC.md'], + consumes: ['CONTEXT.md'], + when: 'workflow.ui_phase', + onError: 'skip', + }, + ], + }; + const errors = validateCapability(cap, 'ui'); + assert.ok(errors.length > 0, 'Expected errors for undeclared ref.agent'); + assert.ok( + errors.some((e) => e.includes('gsd-unknown-agent') && e.includes('not declared')), + 'Error should mention "gsd-unknown-agent" and "not declared", got: ' + JSON.stringify(errors), + ); + }); + + test('step.ref.agent IN cap.agents is accepted', () => { + const cap = { + ...UI_CAP, + steps: [ + { + point: 'plan:pre', + ref: { agent: 'gsd-ui-checker' }, // declared in agents + produces: ['UI-SPEC.md'], + consumes: ['CONTEXT.md'], + when: 'workflow.ui_phase', + onError: 'skip', + }, + ], + }; + const errors = validateCapability(cap, 'ui'); + const membershipErrors = errors.filter((e) => e.includes('not declared') && e.includes('gsd-ui-checker')); + assert.deepEqual( + membershipErrors, [], + 'No membership errors expected for declared ref.agent, got: ' + JSON.stringify(membershipErrors), + ); + }); +}); + describe('Fix: 3-node requires cycle (A→B→C→A) is detected', () => { test('three-node requires cycle is reported as an error', () => { const capA = {