diff --git a/.changeset/wired-discuss-loop-step.md b/.changeset/wired-discuss-loop-step.md new file mode 100644 index 000000000..42b812890 --- /dev/null +++ b/.changeset/wired-discuss-loop-step.md @@ -0,0 +1,6 @@ +--- +type: Fixed +pr: 1199 +--- + +**Wire the discuss loop step for capability hooks** — capabilities can now register `discuss:pre`/`discuss:post` hooks (e.g. discuss-time context recall and CONTEXT capture); previously `discuss` was contract-declared but structurally unwireable. Also collapses the host-loop file set to a single source of truth and adds an authoring-time guard rejecting hooks at unwired extension points. (#1199) diff --git a/docs/INVENTORY-MANIFEST.json b/docs/INVENTORY-MANIFEST.json index 67722ca60..b6eb15ae9 100644 --- a/docs/INVENTORY-MANIFEST.json +++ b/docs/INVENTORY-MANIFEST.json @@ -216,6 +216,7 @@ "git-integration.md", "git-planning-commit.md", "ios-scaffold.md", + "loop-hook-dispatch.md", "mandatory-initial-read.md", "model-profile-resolution.md", "model-profiles.md", diff --git a/docs/INVENTORY.md b/docs/INVENTORY.md index 328c47fe0..0e0e64227 100644 --- a/docs/INVENTORY.md +++ b/docs/INVENTORY.md @@ -302,6 +302,7 @@ Full roster at `gsd-core/references/*.md`. References are shared knowledge docum | `domain-probes.md` | Domain-specific probing questions for discuss-phase. | | `edge-probe.md` | Spec-phase edge-completeness probe — 8-category edge taxonomy, shape classification, and the `requirements → checks → verifier` resolution model (Step 5.5). | | `gate-prompts.md` | Gate/checkpoint prompt templates. | +| `loop-hook-dispatch.md` | Generic dispatch contract for consuming `gsd_run loop render-hooks --raw` output in any host-loop workflow — envelope shape, per-kind dispatch rules (contribution/step/gate), and liveness banner. | | `scout-codebase.md` | Phase-type→codebase-map selection table for discuss-phase scout step (extracted via #2551). | | `revision-loop.md` | Plan revision iteration patterns. | | `universal-anti-patterns.md` | Universal anti-patterns to detect and avoid. | diff --git a/gsd-core/references/loop-hook-dispatch.md b/gsd-core/references/loop-hook-dispatch.md new file mode 100644 index 000000000..66eb9c43a --- /dev/null +++ b/gsd-core/references/loop-hook-dispatch.md @@ -0,0 +1,61 @@ +# Loop Hook Dispatch Contract + +Generic reference for consuming the `--raw` JSON output of `gsd_run loop render-hooks ` +in any host-loop workflow. This document is point-agnostic — it applies to every loop +extension point (discuss:pre, discuss:post, plan:pre, plan:post, execute:pre, execute:wave:pre, +execute:wave:post, execute:post, verify:pre, verify:post, ship:pre, ship:post). + +## Envelope shape + +```json +{ + "point": "discuss:pre", + "activeHooks": [ + { "kind": "contribution", "into": "orchestrator", "fragment": { "inline": "..." } }, + { "kind": "step", "ref": { "skill": "my-skill" } }, + { "kind": "gate", "check": { "query": "..." }, "blocking": true, "onError": "skip" } + ], + "rendered": "..." +} +``` + +`activeHooks` is an array of enabled hook entries for the named point. It is empty (or absent) +when no capability has registered an active hook at this point — treat that as a no-op. + +## Dispatch rules by `kind` + +### `contribution` + +Inject `fragment.inline` verbatim into the context for the role named in `into` +(e.g. `orchestrator`, `planner`). Do not paraphrase — the text is the product. + +### `step` + +Dispatch the referenced unit: + +- `ref.skill` present → dispatch via the Skill tool with skill id `gsd-`. +- `ref.agent` present → dispatch via the Agent tool with `subagent_type` = `ref.agent`. + Before dispatching an agent, print the canonical liveness banner so users know silence + is expected and do not kill a healthy agent: + + ``` + ◆ Spawning ... (runs in a subagent — no output until it returns; expected, not a freeze) + ``` + +Wait for the result before continuing to the next hook or the next step. + +### `gate` + +Evaluate `check` (one of `query`, `predicate`, or `agentVerdict`). Then honor `blocking`: + +- `blocking: true` → if the check returns `block: true`, surface `check.message` to the user + and stop the current step. Do not continue. +- `blocking: false` → advisory only; surface the message but continue regardless of outcome. + +Honor `onError` if the check itself errors: `skip` means treat as non-blocking and continue; +`fail` means surface the error and stop. + +## Empty / absent `activeHooks` + +If `activeHooks` is absent, null, or an empty array, skip silently and continue to the next +step in the workflow. No output to the user is needed. diff --git a/gsd-core/workflows/discuss-phase.md b/gsd-core/workflows/discuss-phase.md index 4ef55ecc0..66eeeeaef 100644 --- a/gsd-core/workflows/discuss-phase.md +++ b/gsd-core/workflows/discuss-phase.md @@ -293,6 +293,13 @@ Read `@~/.claude/gsd-core/references/scout-codebase.md` — it contains the phas 3. Build internal `` per the reference's output schema + +```bash +DISCUSS_PRE_HOOKS_JSON=$(gsd_run loop render-hooks discuss:pre --raw) +``` +Apply each entry in `activeHooks` per @~/.claude/gsd-core/references/loop-hook-dispatch.md. Empty list → continue to `analyze_phase`. + + Analyze the phase to identify gray areas. Use both `prior_decisions` and `codebase_context` to ground the analysis. @@ -405,6 +412,13 @@ The template documents variable substitutions and conditional sections. Substitu Write the file. + +```bash +DISCUSS_POST_HOOKS_JSON=$(gsd_run loop render-hooks discuss:post --raw) +``` +Apply each entry in `activeHooks` per @~/.claude/gsd-core/references/loop-hook-dispatch.md. Empty list → continue to `confirm_creation`. + + Present summary and next steps: diff --git a/scripts/gen-capability-registry.cjs b/scripts/gen-capability-registry.cjs index d68d28a82..aaee3d66a 100644 --- a/scripts/gen-capability-registry.cjs +++ b/scripts/gen-capability-registry.cjs @@ -34,6 +34,9 @@ const SCHEMA_VERSION = '1'; // registry generator and the loop-host-contract generator share one source of truth. const { LOOP_HOST_CONTRACT } = require('../gsd-core/bin/lib/loop-host-contract.cjs'); +// Wired-points helper — tells us which points actually have render-hooks call sites. +const { getWiredLoopPoints } = require('./gen-loop-host-contract.cjs'); + // Canonical point order — explicit constant (do NOT rely on Set insertion order). // Used for point-ordering semantics in consumes-satisfiability validation and topo-sort. const POINT_ORDER = [ @@ -1961,6 +1964,54 @@ function runConfigFormatParityGate(capMap) { } } +// ─── Gen-time wired guard ───────────────────────────────────────────────────── + +/** + * Validate that every hook point declared by a capability has a corresponding + * `loop render-hooks ` call site in one of the host-loop workflow files. + * + * Only valid loop points (in VALID_LOOP_POINTS) are checked here. Invalid points + * are already caught by validateStep/validateContribution/validateGate — do not + * double-report. + * + * @param {object} cap Validated capability object. + * @param {Set} wiredSet Set of points that have call sites in host workflows. + * @returns {string[]} Array of error strings; empty means all points are wired. + */ +function validateHooksWired(cap, wiredSet) { + const errors = []; + const capId = cap.id || '(unknown)'; + + function checkPoint(point, groupName, idx) { + // Only flag valid points that are unwired — invalid points are schema-validator's job. + if (!VALID_LOOP_POINTS.has(point)) return; + if (!wiredSet.has(point)) { + errors.push( + 'capability "' + capId + '" ' + groupName + '[' + idx + '].point "' + point + + '" is declared but not wired in any host-loop workflow ' + + '(no `loop render-hooks ' + point + '` call site). ' + + 'Wire the call site in the host workflow ' + + '(see scripts/gen-loop-host-contract.cjs STEP_WORKFLOWS) or remove the hook.', + ); + } + } + + for (let i = 0; i < (cap.steps || []).length; i++) { + const hook = cap.steps[i]; + if (hook.point !== undefined) checkPoint(hook.point, 'steps', i); + } + for (let i = 0; i < (cap.contributions || []).length; i++) { + const hook = cap.contributions[i]; + if (hook.point !== undefined) checkPoint(hook.point, 'contributions', i); + } + for (let i = 0; i < (cap.gates || []).length; i++) { + const hook = cap.gates[i]; + if (hook.point !== undefined) checkPoint(hook.point, 'gates', i); + } + + return errors; +} + // ─── Registry builder ───────────────────────────────────────────────────────── /** @@ -1982,6 +2033,10 @@ function loadAndValidate(centralKeys, capabilitiesDir) { return { capMap, errors }; } + // Compute wired points ONCE before iterating capabilities so the filesystem + // scan is not repeated per-capability. ROOT is the repo root (defined at top of file). + const wiredSet = getWiredLoopPoints(ROOT); + const folderEntries = fs.readdirSync(resolvedCapDir, { withFileTypes: true }) .filter((e) => e.isDirectory()) .map((e) => e.name) @@ -2013,6 +2068,13 @@ function loadAndValidate(centralKeys, capabilitiesDir) { continue; } + // Gen-time wired guard: reject hooks that declare a valid point with no call site. + const wiredErrors = validateHooksWired(cap, wiredSet); + if (wiredErrors.length > 0) { + for (const e of wiredErrors) errors.push(folderId + '/capability.json: ' + e); + continue; + } + const fragmentErrors = materializeHookFragments(cap, path.dirname(capPath)); if (fragmentErrors.length > 0) { for (const e of fragmentErrors) errors.push(folderId + '/capability.json: ' + e); @@ -2499,6 +2561,7 @@ module.exports = { POINT_TO_CONTRACT, HOST_ARTIFACT_EARLIEST_POINT_IDX, SCHEMA_VERSION, + validateHooksWired, // ADR-857 phase 4a: derived views + gates deriveCapabilityClusters, deriveProfileMembership, diff --git a/scripts/gen-loop-host-contract.cjs b/scripts/gen-loop-host-contract.cjs index 5838e0d0d..9d7bed19c 100644 --- a/scripts/gen-loop-host-contract.cjs +++ b/scripts/gen-loop-host-contract.cjs @@ -449,6 +449,58 @@ function main() { } } +// ─── Derived single-source-of-truth exports ─────────────────────────────────── + +/** + * Repo-relative paths to every host-loop workflow file, derived from STEP_WORKFLOWS. + * This is the ONLY canonical enumeration of host-loop files — all consumers (tests, + * registry generator, conformance gate) must derive from this rather than maintaining + * a separate hardcoded list. + */ +const HOST_LOOP_FILES = STEP_WORKFLOWS.map((w) => 'gsd-core/workflows/' + w.file); + +/** + * Pure function: scan a text string for `loop render-hooks ` call sites. + * Returns a Set of matched point strings. + * + * @param {string} text Content of a workflow file (or any text). + * @returns {Set} + */ +function scanWiredPoints(text) { + const re = /loop render-hooks\s+([a-z:]+)/g; + const result = new Set(); + let m; + while ((m = re.exec(text)) !== null) { + result.add(m[1]); + } + return result; +} + +/** + * Read every host-loop workflow file and return the union of all wired loop points + * (i.e. points that have a `loop render-hooks ` call site). + * + * @param {string} [repoRoot] Path to the repository root. Defaults to ROOT. + * @returns {Set} + */ +function getWiredLoopPoints(repoRoot) { + const resolvedRoot = repoRoot !== undefined ? repoRoot : ROOT; + const result = new Set(); + for (const relPath of HOST_LOOP_FILES) { + const absPath = path.join(resolvedRoot, relPath); + let content; + try { + content = fs.readFileSync(absPath, 'utf8'); + } catch (err) { + throw new Error('getWiredLoopPoints: cannot read host-loop file ' + absPath + ': ' + err.message); + } + for (const point of scanWiredPoints(content)) { + result.add(point); + } + } + return result; +} + // ─── Exports (for tests) ───────────────────────────────────────────────────── module.exports = { @@ -459,9 +511,12 @@ module.exports = { serializeContract, normalizeLineEndings, STEP_WORKFLOWS, + HOST_LOOP_FILES, CANONICAL_POINTS, EXPECTED_POINTS_BY_STEP, ROLE_TO_AGENT, + scanWiredPoints, + getWiredLoopPoints, }; // ─── CLI entry point ────────────────────────────────────────────────────────── diff --git a/tests/capability-registry.test.cjs b/tests/capability-registry.test.cjs index 91101e5ed..70846abf3 100644 --- a/tests/capability-registry.test.cjs +++ b/tests/capability-registry.test.cjs @@ -54,8 +54,22 @@ const { validateRuntimeCompat, validateRuntimeBody, loadCentralConfigKeys, + validateHooksWired, + POINT_ORDER, } = require('../scripts/gen-capability-registry.cjs'); +const { + STEP_WORKFLOWS, + HOST_LOOP_FILES, + scanWiredPoints, + getWiredLoopPoints, + CANONICAL_POINTS, +} = require('../scripts/gen-loop-host-contract.cjs'); + +const { LOOP_HOST_CONTRACT } = require('../gsd-core/bin/lib/loop-host-contract.cjs'); + +const fc = require('fast-check'); + const ROOT = path.resolve(__dirname, '..'); // ─── UI pilot fixture (from capabilities/ui/capability.json) ───────────────── @@ -4383,8 +4397,10 @@ describe('duplicate-producer invariant — same artifact same point (Issue #1123 }); test('PASSING: same artifact at DIFFERENT points does not trigger duplicate-producer error', () => { - // cap-diff-a produces SAME.md at plan:pre; cap-diff-b produces SAME.md at execute:pre + // cap-diff-a produces SAME.md at plan:pre; cap-diff-b produces SAME.md at execute:post // Different pointIdx → must not throw the duplicate-producer error. + // NOTE: both points must be wired (have render-hooks call sites in host workflows) + // to pass the validateHooksWired gen-time guard added in #1196. const capDiffA = makeFeatureCap({ id: 'diff-a', skills: ['diff-a-skill'], @@ -4409,7 +4425,7 @@ describe('duplicate-producer invariant — same artifact same point (Issue #1123 config: {}, steps: [ { - point: 'execute:pre', + point: 'execute:post', ref: { skill: 'diff-b-skill' }, produces: ['SAME.md'], consumes: [], @@ -4429,3 +4445,288 @@ describe('duplicate-producer invariant — same artifact same point (Issue #1123 ); }); }); + +// ─── #1196 — discuss loop wiring + wired-point guard ───────────────────────── + +describe('#1196 — discuss loop wiring + wired-point guard', () => { + // ─── Defect 1 — discuss is now wireable ───────────────────────────────────── + + describe('Defect 1: discuss is a wireable loop host', () => { + test('HOST_LOOP_FILES includes discuss-phase.md', () => { + assert.ok( + Array.isArray(HOST_LOOP_FILES), + 'HOST_LOOP_FILES must be an array', + ); + assert.ok( + HOST_LOOP_FILES.includes('gsd-core/workflows/discuss-phase.md'), + `HOST_LOOP_FILES must include 'gsd-core/workflows/discuss-phase.md'. Got: ${JSON.stringify(HOST_LOOP_FILES)}`, + ); + }); + + test('getWiredLoopPoints(ROOT) contains discuss:pre', () => { + const wired = getWiredLoopPoints(ROOT); + assert.ok( + wired instanceof Set, + 'getWiredLoopPoints must return a Set', + ); + assert.ok( + wired.has('discuss:pre'), + `getWiredLoopPoints(ROOT) must contain 'discuss:pre'. Got: ${JSON.stringify([...wired])}`, + ); + }); + + test('getWiredLoopPoints(ROOT) contains discuss:post', () => { + const wired = getWiredLoopPoints(ROOT); + assert.ok( + wired.has('discuss:post'), + `getWiredLoopPoints(ROOT) must contain 'discuss:post'. Got: ${JSON.stringify([...wired])}`, + ); + }); + }); + + // ─── Defect 2 — gen-time wired guard ──────────────────────────────────────── + + describe('Defect 2: validateHooksWired gen-time guard', () => { + /** Minimal capability fixture with one hook at a given point */ + function makeCapWithStep(point) { + return { + id: 'test-cap', + role: 'feature', + steps: [{ point, ref: { skill: 'my-skill' }, produces: [], consumes: [], onError: 'skip' }], + contributions: [], + gates: [], + config: {}, + }; + } + + function makeCapWithContribution(point) { + return { + id: 'test-cap', + role: 'feature', + steps: [], + contributions: [{ point, into: 'orchestrator', fragment: { inline: 'hi' }, produces: [], consumes: [] }], + gates: [], + config: {}, + }; + } + + function makeCapWithGate(point) { + return { + id: 'test-cap', + role: 'feature', + steps: [], + contributions: [], + gates: [{ point, check: { query: 'test-query' }, blocking: false, onError: 'skip' }], + config: {}, + }; + } + + test('returns non-empty error array mentioning "not wired" when step point is not in wiredSet', () => { + const cap = makeCapWithStep('discuss:pre'); + const wiredSet = new Set(['plan:pre', 'plan:post']); // discuss:pre absent + const errs = validateHooksWired(cap, wiredSet); + assert.ok(Array.isArray(errs), 'must return an array'); + assert.ok(errs.length > 0, 'must return errors when point is unwired'); + const joined = errs.join(' '); + assert.match(joined, /not wired/i, 'error must mention "not wired"'); + assert.match(joined, /discuss:pre/, 'error must name the point'); + assert.match(joined, /test-cap/, 'error must name the capability id'); + }); + + test('returns non-empty error array when contribution point is not in wiredSet', () => { + const cap = makeCapWithContribution('discuss:pre'); + const wiredSet = new Set(['plan:pre']); // discuss:pre absent + const errs = validateHooksWired(cap, wiredSet); + assert.ok(errs.length > 0, 'must return errors for unwired contribution point'); + assert.match(errs.join(' '), /not wired/i); + }); + + test('returns non-empty error array when gate point is not in wiredSet', () => { + const cap = makeCapWithGate('discuss:pre'); + const wiredSet = new Set(['plan:pre']); // discuss:pre absent + const errs = validateHooksWired(cap, wiredSet); + assert.ok(errs.length > 0, 'must return errors for unwired gate point'); + assert.match(errs.join(' '), /not wired/i); + }); + + test('returns empty array when all declared points are in wiredSet', () => { + const cap = makeCapWithStep('plan:pre'); + const wiredSet = new Set(['plan:pre', 'plan:post', 'execute:post']); + const errs = validateHooksWired(cap, wiredSet); + assert.deepEqual(errs, [], 'must return empty array when all points are wired'); + }); + + test('boundary: cap declaring discuss:pre is rejected against wiredSet lacking it', () => { + const cap = makeCapWithStep('discuss:pre'); + const smallSet = new Set(['plan:pre', 'plan:post']); + const errs = validateHooksWired(cap, smallSet); + assert.ok(errs.length > 0, 'must reject discuss:pre against a set that lacks it'); + }); + + test('boundary: cap declaring discuss:pre is accepted against real getWiredLoopPoints(ROOT) post-fix', () => { + const cap = makeCapWithStep('discuss:pre'); + const realWired = getWiredLoopPoints(ROOT); + const errs = validateHooksWired(cap, realWired); + assert.deepEqual( + errs, [], + `discuss:pre must be wired after the fix. Errors: ${errs.join('; ')}`, + ); + }); + + test('boundary: cap declaring discuss:post is accepted against real getWiredLoopPoints(ROOT) post-fix', () => { + const cap = makeCapWithContribution('discuss:post'); + const realWired = getWiredLoopPoints(ROOT); + const errs = validateHooksWired(cap, realWired); + assert.deepEqual( + errs, [], + `discuss:post must be wired after the fix. Errors: ${errs.join('; ')}`, + ); + }); + + test('invalid points (not in VALID_LOOP_POINTS) are not flagged as "unwired" (already caught by schema validator)', () => { + const cap = { + id: 'test-cap', + role: 'feature', + steps: [{ point: 'not:a:real:point', ref: { skill: 'x' }, produces: [], consumes: [], onError: 'skip' }], + contributions: [], + gates: [], + config: {}, + }; + const wiredSet = new Set(['plan:pre']); // the invalid point is not here either + const errs = validateHooksWired(cap, wiredSet); + // Should NOT flag it — invalid points are the schema validator's job + const notWiredErrors = errs.filter((e) => /not wired/i.test(e)); + assert.deepEqual( + notWiredErrors, [], + 'validateHooksWired must not flag invalid points as "not wired" (those are caught by schema validation)', + ); + }); + }); + + // ─── Anti-pattern parity guards ────────────────────────────────────────────── + + describe('Anti-pattern parity: host-file set has a single source of truth', () => { + test('every STEP_WORKFLOWS entry file exists on disk and contains a gsd:loop-host marker', () => { + for (const { file, step } of STEP_WORKFLOWS) { + const absPath = path.join(ROOT, 'gsd-core', 'workflows', file); + assert.ok( + fs.existsSync(absPath), + `STEP_WORKFLOWS entry ${file} (step: ${step}) does not exist on disk at ${absPath}`, + ); + const content = fs.readFileSync(absPath, 'utf8'); + assert.match( + content, + /