diff --git a/.changeset/calm-seals-chatter.md b/.changeset/calm-seals-chatter.md new file mode 100644 index 000000000..44e431b0b --- /dev/null +++ b/.changeset/calm-seals-chatter.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 3934 +--- +**Quick planning now preserves `plan:pre` guidance across revisions.** `/gsd-quick` renders one hook snapshot and reuses its planner-targeted contributions for the initial planner and `--full`/`--validate` revision planner. Non-planner contributions are omitted. Because security enforcement is enabled by default, Quick plans may now receive `` guidance unless `workflow.security_enforcement` is disabled. (#3778) diff --git a/docs/tutorials/build-your-first-capability.md b/docs/tutorials/build-your-first-capability.md index 7f273b4cf..2300d51a4 100644 --- a/docs/tutorials/build-your-first-capability.md +++ b/docs/tutorials/build-your-first-capability.md @@ -180,7 +180,7 @@ The envelope is `{ point, activeHooks, rendered }`. Your contribution appears in } ``` -Notice that `fragment.inline` now holds the materialised text from `fragments/plan-pre.md` — GSD inlined it at load time, while keeping the original `path` for reference. The top-level `rendered` field of the envelope contains the same fragment formatted as a `…` block, which is what the planner actually receives. +Notice that `fragment.inline` now holds the materialised text from `fragments/plan-pre.md` — GSD inlined it at load time, while keeping the original `path` for reference. The top-level `rendered` field is an unfiltered diagnostic digest across kinds and roles; planner hosts do not paste it. They read `activeHooks` in registry order and inject only active `kind: "contribution"` entries whose `into` is `planner`, using each entry's `fragment.inline` and resolved `configValues`. --- @@ -192,7 +192,9 @@ Planning is driven by a slash command, not a `gsd` subcommand. In your AI assist /gsd-plan-phase ``` -When the planner runs, the `plan:pre` hook set is rendered into its prompt, so it receives the `hello-note` contribution and, following the fragment's instruction, records a one-line note in `HELLO.md`. +When the planner runs — whether from `/gsd-plan-phase` or `/gsd-quick` — the active planner-targeted `plan:pre` contributions are injected into its prompt, so it receives `hello-note` and, following the fragment's instruction, records a one-line note in `HELLO.md`. In `/gsd-quick --full` and `/gsd-quick --validate`, the same rendered hook snapshot is reused if the plan checker sends the plan back to the planner for revision. + +The first-party security capability is enabled by default and also contributes at `plan:pre` into the planner role. Its guidance requires generated plans to include `` blocks and carries the configured ASVS level and blocking threshold. Disable `workflow.security_enforcement` if that behavior is not wanted. Contributions targeting any non-planner role are not inserted into Quick's planner prompts. You do not need to run a full planning session to confirm the wiring, though. The `loop render-hooks` command shows exactly what the loop would hand the planner — the same output you saw in Step 5: @@ -200,7 +202,7 @@ You do not need to run a full planning session to confirm the wiring, though. Th gsd loop render-hooks plan:pre --raw ``` -Find `hello-note` in `activeHooks` and read the `rendered` field: the `` block is the literal text the planner receives. That confirms the capability is wired into the loop, without dispatching a single agent. +Find `hello-note` in `activeHooks` and inspect its `fragment.inline`, `into`, and optional `configValues`. Those are the fields the planner host filters and injects. The unfiltered top-level `rendered` digest is useful for inspection, but it is not the planner prompt. This confirms the capability is wired into the loop without dispatching an agent. --- diff --git a/gsd-core/bin/lib/capability-validator.cjs b/gsd-core/bin/lib/capability-validator.cjs index 3bfdc6ccf..f99d35eb7 100644 --- a/gsd-core/bin/lib/capability-validator.cjs +++ b/gsd-core/bin/lib/capability-validator.cjs @@ -3619,11 +3619,12 @@ const HOOK_GROUP_KINDS = Object.freeze({ * double-report. * * KNOWN LIMITATION (#3606): coverage is the UNION across all call sites for a - * point in the five STEP_WORKFLOWS host files. Consumers outside that universe - * (quick.md, autonomous.md, code-review*.md, audit-milestone.md, + * point in HOST_LOOP_FILES. Quick's plan:pre planner contribution seam has an + * additional generator-owned per-file check. Other consumers outside that + * universe (autonomous.md, code-review*.md, audit-milestone.md, * secure-phase.md, validate-phase.md) are not per-file checked — a narrowed - * consumer there passes as long as one host file covers the point. Per-file - * coverage maps are the tightening path. + * consumer there passes as long as one host file covers the point. More + * per-file coverage maps are the tightening path. * * @param {object} cap Validated capability object. * @param {Map>} wiredKinds Per point, the hook kinds the diff --git a/gsd-core/workflows/quick.md b/gsd-core/workflows/quick.md index b71d5ec75..2a37ed786 100644 --- a/gsd-core/workflows/quick.md +++ b/gsd-core/workflows/quick.md @@ -287,6 +287,13 @@ If `section_manifest` is `null` or `"research-phase"` is in its `included` list: **Step 5: Spawn planner (quick mode)** +**Capability gate:** +```bash +PLAN_PRE_HOOKS_JSON=$(gsd_run loop render-hooks plan:pre --raw) +``` + +**Contribution dispatch (#3778):** read `PLAN_PRE_HOOKS_JSON.activeHooks` directly in context. In registry order, inject only active entries with `kind == "contribution"` and `into == "planner"` into each Quick planner prompt below, using `fragment.inline` verbatim plus resolved `configValues`. Do not paste `rendered`. Empty, inactive, incompatible, or non-planner entries inject nothing and do not error. Reuse this snapshot for revisions; do not render again. + **If `$VALIDATE_MODE`:** Use `quick-full` mode with stricter constraints. **If NOT `$VALIDATE_MODE`:** Use standard `quick` mode. @@ -313,6 +320,8 @@ ${AGENT_SKILLS_PLANNER} **Project skills:** Check .claude/skills/ or .agents/skills/ directory (if either exists) — read SKILL.md files, plans should account for project skill rules +{For each active entry in `PLAN_PRE_HOOKS_JSON` where `kind == "contribution"` and `into == "planner"` (in array order): inject the entry's `fragment.inline` verbatim here, plus its resolved `configValues` when the entry carries them. If no active planner contributions exist, omit this block entirely.} + diff --git a/gsd-core/workflows/quick/steps/plan-checker-loop.md b/gsd-core/workflows/quick/steps/plan-checker-loop.md index cd6b54c21..d11d8b249 100644 --- a/gsd-core/workflows/quick/steps/plan-checker-loop.md +++ b/gsd-core/workflows/quick/steps/plan-checker-loop.md @@ -68,6 +68,8 @@ Display: `Sending back to planner for revision... (iteration ${N}/2)` Revision prompt: +Reuse the `PLAN_PRE_HOOKS_JSON` snapshot captured by Quick Step 5; do not render hooks again. + ```markdown **Mode:** quick-full (revision) @@ -78,6 +80,8 @@ Revision prompt: ${AGENT_SKILLS_PLANNER} +{For each active entry in `PLAN_PRE_HOOKS_JSON` where `kind == "contribution"` and `into == "planner"` (in array order): inject the entry's `fragment.inline` verbatim here, plus its resolved `configValues` when the entry carries them. If no active planner contributions exist, omit this block entirely.} + **Checker issues:** ${structured_issues_from_checker} diff --git a/scripts/gen-loop-host-contract.cjs b/scripts/gen-loop-host-contract.cjs index 426db9e9e..1be932336 100644 --- a/scripts/gen-loop-host-contract.cjs +++ b/scripts/gen-loop-host-contract.cjs @@ -30,7 +30,13 @@ const CONTRACT_PATH = path.join(ROOT, 'gsd-core', 'bin', 'lib', 'loop-host-contr // The five step workflows in pipeline order const STEP_WORKFLOWS = [ { file: 'discuss-phase.md', step: 'discuss' }, - { file: 'plan-phase.md', step: 'plan' }, + { + file: 'plan-phase.md', + step: 'plan', + auxiliaryHosts: [ + { file: 'quick.md', point: 'plan:pre', kinds: ['contribution'], into: 'planner' }, + ], + }, { file: 'execute-phase.md', step: 'execute' }, { file: 'verify-work.md', step: 'verify' }, { file: 'ship.md', step: 'ship' }, @@ -295,7 +301,7 @@ function buildContract(workflowsDir) { const contract = []; const allErrors = []; - for (const { file, step } of STEP_WORKFLOWS) { + for (const { file, step, auxiliaryHosts = [] } of STEP_WORKFLOWS) { const filePath = path.join(resolvedDir, file); let content; try { @@ -325,6 +331,34 @@ function buildContract(workflowsDir) { const roleErrors = crossCheckRoles(content, entry.agentRoles, file); allErrors.push(...roleErrors); + for (const auxiliary of auxiliaryHosts) { + let auxiliaryContent; + try { + auxiliaryContent = fs.readFileSync(path.join(resolvedDir, auxiliary.file), 'utf8'); + } catch (err) { + allErrors.push('Could not read auxiliary host ' + auxiliary.file + ': ' + String(err.message)); + continue; + } + + const wiredPoints = scanWiredPoints(auxiliaryContent); + if (!wiredPoints.has(auxiliary.point)) { + allErrors.push( + auxiliary.file + ': auxiliary host missing expected point "' + auxiliary.point + '"', + ); + continue; + } + + const wiredKinds = scanWiredKinds(auxiliaryContent, auxiliary.into).get(auxiliary.point) || new Set(); + for (const kind of auxiliary.kinds) { + if (!wiredKinds.has(kind)) { + allErrors.push( + auxiliary.file + ': auxiliary host point "' + auxiliary.point + + '" missing expected kind "' + kind + '"', + ); + } + } + } + contract.push(entry); } @@ -437,7 +471,10 @@ function main() { * 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); +const HOST_LOOP_FILES = STEP_WORKFLOWS.flatMap(({ file, auxiliaryHosts = [] }) => [ + 'gsd-core/workflows/' + file, + ...auxiliaryHosts.map((host) => 'gsd-core/workflows/' + host.file), +]); /** * Pure function: scan a text string for `loop render-hooks ` call sites. @@ -475,9 +512,11 @@ const HOOK_KINDS = ['contribution', 'step', 'gate']; * - A deferral line (one carrying an `@`-included path to the generic * contract, e.g. `@gsd-core/references/loop-hook-dispatch.md`) that names a * kind (`kind == "step"`) covers that kind; a deferral line with no kind - * discriminator ("apply each entry") covers every kind. A bare §-citation - * of the reference (validation guidance only, no `@`) covers nothing — - * plan-phase cites the gate-validation section while dispatching only gates. + * discriminator ("apply each entry") covers every kind only when no role + * target is required. With `expectedInto`, the same segment must explicitly + * name both the kind and that exact `into` target. A bare §-citation of the + * reference (validation guidance only, no `@`) covers nothing — plan-phase + * cites the gate-validation section while dispatching only gates. * - Otherwise a kind is covered when some LINE dispatches it unconditionally: * a `kind == ""` discriminator with NO same-line narrowing to one * hook (`ref.skill ==`, `ref.agent ==`, `ref.command ==`). A narrowed line @@ -491,19 +530,26 @@ const HOOK_KINDS = ['contribution', 'step', 'gate']; * Pure: same input, same output; CRLF-safe (line splitting tolerates \r). * * @param {string} region Dispatch text following one call site. + * @param {string} [expectedInto] Optional role target required in the same segment. * @returns {Set} */ -function coveredKindsInRegion(region) { +function coveredKindsInRegion(region, expectedInto) { const covered = new Set(); // Same-SEGMENT narrowing to ONE hook voids credit: `ref.skill ==`, `capId ==`, - // and `into ==` each special-case a subset, not the kind generally + // and `into ==` each special-case a subset, not the kind generally. An + // auxiliary host may name the one role it actually hosts via `expectedInto`; + // any other role target still voids credit. // (plan-phase's `kind == "contribution" and capId == "security"` is the // hand-rolled shape; `into == "planner"` covers only planner-targeted // contributions). Segments, not lines: execute-phase legitimately writes // "dispatch `kind == "step"` hooks per … . `ref.skill == "code-review"`:" — // the deferral is one sentence, the specialization the next; narrowing in a // DIFFERENT segment must not void the deferral's credit. - const narrowingRe = /(?:ref\.(?:skill|agent|command)|capId|into)\s*={2,3}/; + const identityNarrowingRe = /(?:ref\.(?:skill|agent|command)|capId)\s*={2,3}/; + const intoNarrowingRe = /into\s*={2,3}/; + const expectedIntoRe = expectedInto + ? new RegExp(`into\\s*={2,3}\\s*["']${escapeRegExp(expectedInto)}["']`) + : null; // Negated mentions describe an absence, not a dispatch ("Branch 1 — no active // step hooks (`activeHooks` has no entry with `kind == "step"`)" — ship.md). const negationRe = /\b(?:no|without|absent|lacks?|missing)\b[^.|]*kind\s*={2,3}/; @@ -519,17 +565,22 @@ function coveredKindsInRegion(region) { } if (kindDiscriminators.length === 0) { // A deferral with no kind discriminator ("apply each entry per …") - // still covers every kind. - if (deferralRe.test(segment)) for (const kind of HOOK_KINDS) covered.add(kind); + // still covers every kind for generic hosts. An auxiliary host with a + // required role target must state both its kind and target explicitly. + if (!expectedInto && deferralRe.test(segment)) for (const kind of HOOK_KINDS) covered.add(kind); continue; } if (negationRe.test(segment)) continue; + const narrowed = identityNarrowingRe.test(segment) || + (expectedInto + ? !expectedIntoRe.test(segment) + : intoNarrowingRe.test(segment)); if (deferralRe.test(segment)) { // Deferral naming kinds ("dispatch `kind == "step"` hooks per …"). - if (!narrowingRe.test(segment)) for (const kind of kindDiscriminators) covered.add(kind); + if (!narrowed) for (const kind of kindDiscriminators) covered.add(kind); continue; } - if (!narrowingRe.test(segment)) for (const kind of kindDiscriminators) covered.add(kind); + if (!narrowed) for (const kind of kindDiscriminators) covered.add(kind); } } return covered; @@ -540,9 +591,10 @@ function coveredKindsInRegion(region) { * per point, the union of hook kinds its dispatch regions cover (#3606). * * @param {string} text Content of a workflow file (or any text). + * @param {string} [expectedInto] Optional role target an auxiliary host must dispatch. * @returns {Map>} point → covered kinds. */ -function scanWiredKinds(text) { +function scanWiredKinds(text, expectedInto) { const result = new Map(); const siteRe = CALL_SITE_RE; const sites = []; @@ -552,7 +604,7 @@ function scanWiredKinds(text) { for (let i = 0; i < sites.length; i++) { const regionEnd = i + 1 < sites.length ? sites[i + 1].start : Math.min(text.length, sites[i].start + REGION_CAP); const region = text.slice(sites[i].start, regionEnd); - const covered = coveredKindsInRegion(region); + const covered = coveredKindsInRegion(region, expectedInto); if (!result.has(sites[i].point)) result.set(sites[i].point, new Set()); for (const kind of covered) result.get(sites[i].point).add(kind); } diff --git a/tests/capability-registry.test.cjs b/tests/capability-registry.test.cjs index cfcd8991b..2007a267e 100644 --- a/tests/capability-registry.test.cjs +++ b/tests/capability-registry.test.cjs @@ -61,6 +61,7 @@ const { const { STEP_WORKFLOWS, HOST_LOOP_FILES, + buildContract, scanWiredPoints, getWiredLoopPoints, CANONICAL_POINTS, @@ -913,6 +914,28 @@ describe('ADR-857 phase 6 planning feature capabilities', () => { }); }); +describe('#3778 — plan:pre contribution set feeding the quick.md planner dispatch', () => { + test('Quick host registration is generator-owned without changing canonical contract shape', () => { + const plan = STEP_WORKFLOWS.find((workflow) => workflow.step === 'plan'); + assert.deepEqual(plan.auxiliaryHosts, [ + { file: 'quick.md', point: 'plan:pre', kinds: ['contribution'], into: 'planner' }, + ]); + assert.ok( + HOST_LOOP_FILES.includes('gsd-core/workflows/quick.md'), + 'Quick must be enumerated by the generator-owned host set', + ); + + const contract = buildContract(); + assert.strictEqual(STEP_WORKFLOWS.length, 5, 'canonical step rows must stay at five'); + assert.strictEqual(contract.length, 5, 'serialized contract must stay at five entries'); + assert.deepEqual(contract, LOOP_HOST_CONTRACT, 'auxiliary metadata must not be serialized'); + + const points = contract.flatMap((entry) => entry.points); + assert.strictEqual(points.length, 12, 'serialized contract must stay at 12 points'); + assert.strictEqual(new Set(points).size, 12, 'serialized lifecycle points must remain unique'); + }); +}); + // ─── 6. Fix regression guards ──────────────────────────────────────────────── describe('Fix #1: consumes-satisfiability is point-order-aware', () => { @@ -5312,12 +5335,15 @@ describe('#1196 — discuss loop wiring + wired-point guard', () => { } }); - test('HOST_LOOP_FILES matches STEP_WORKFLOWS (single source of truth)', () => { - const expectedFromStepWorkflows = STEP_WORKFLOWS.map((w) => 'gsd-core/workflows/' + w.file); + test('HOST_LOOP_FILES matches STEP_WORKFLOWS rows and auxiliary hosts', () => { + const expectedFromStepWorkflows = STEP_WORKFLOWS.flatMap(({ file, auxiliaryHosts = [] }) => [ + 'gsd-core/workflows/' + file, + ...auxiliaryHosts.map((host) => 'gsd-core/workflows/' + host.file), + ]); assert.deepEqual( HOST_LOOP_FILES, expectedFromStepWorkflows, - 'HOST_LOOP_FILES must be derived from STEP_WORKFLOWS, not a separate hardcoded list', + 'HOST_LOOP_FILES must be derived from STEP_WORKFLOWS rows and their auxiliary hosts', ); }); diff --git a/tests/loop-host-contract.test.cjs b/tests/loop-host-contract.test.cjs index d5d84ffbf..3435c8eda 100644 --- a/tests/loop-host-contract.test.cjs +++ b/tests/loop-host-contract.test.cjs @@ -74,6 +74,11 @@ function makeValidWorkflowFiles() { // Cross-check content: must contain the agent names 'gsd-phase-researcher gsd-planner gsd-plan-checker', ), + 'quick.md': [ + 'PLAN_PRE_HOOKS_JSON=$(gsd_run loop render-hooks plan:pre --raw)', + 'For each active entry where `kind == "contribution"` and `into == "planner"`: inject.', + '', + ].join('\n'), 'execute-phase.md': makeWorkflow( 'execute', ['execute:pre', 'execute:wave:pre', 'execute:wave:post', 'execute:post'], ['executor', 'verifier'], @@ -438,6 +443,97 @@ describe('module exports', () => { }); }); +// ─── #3778 — quick.md is auxiliary without becoming a 6th serialized step ──── + +describe('Quick auxiliary loop-host contract (#3778)', () => { + function makeQuickWorkflow({ includePoint = true, includeKind = true, into = 'planner' } = {}) { + const target = into === null ? '' : ` and \`into == "${into}"\``; + return [ + includePoint ? 'PLAN_PRE_HOOKS_JSON=$(gsd_run loop render-hooks plan:pre --raw)' : '', + includeKind ? `For each active entry where \`kind == "contribution"\`${target}: inject.` : '', + '', + ].join('\n'); + } + + test('Quick is a generator-owned plan auxiliary host with its own contribution expectation', () => { + const plan = STEP_WORKFLOWS.find((workflow) => workflow.step === 'plan'); + assert.deepEqual(plan.auxiliaryHosts, [ + { file: 'quick.md', point: 'plan:pre', kinds: ['contribution'], into: 'planner' }, + ]); + + const files = makeValidWorkflowFiles(); + files['quick.md'] = makeQuickWorkflow(); + const tmpDir = makeTempWorkflowsDir(files); + try { + const contract = buildContract(tmpDir); + assert.strictEqual(contract.length, 5, 'auxiliary hosts must not create a sixth serialized step'); + assert.strictEqual( + new Set(contract.flatMap((entry) => entry.points)).size, + 12, + 'auxiliary hosts must not create a duplicate lifecycle point', + ); + } finally { + cleanup(tmpDir); + } + }); + + test('buildContract rejects Quick without its plan:pre render seam', () => { + const files = makeValidWorkflowFiles(); + files['quick.md'] = makeQuickWorkflow({ includePoint: false }); + const tmpDir = makeTempWorkflowsDir(files); + try { + assert.throws( + () => buildContract(tmpDir), + /quick\.md: auxiliary host missing expected point "plan:pre"/, + ); + } finally { + cleanup(tmpDir); + } + }); + + test('buildContract rejects Quick without its contribution dispatch', () => { + const files = makeValidWorkflowFiles(); + files['quick.md'] = makeQuickWorkflow({ includeKind: false }); + const tmpDir = makeTempWorkflowsDir(files); + try { + assert.throws( + () => buildContract(tmpDir), + /quick\.md: auxiliary host point "plan:pre" missing expected kind "contribution"/, + ); + } finally { + cleanup(tmpDir); + } + }); + + test('buildContract rejects a Quick contribution dispatch targeting the wrong role', () => { + const files = makeValidWorkflowFiles(); + files['quick.md'] = makeQuickWorkflow({ into: 'orchestrator' }); + const tmpDir = makeTempWorkflowsDir(files); + try { + assert.throws( + () => buildContract(tmpDir), + /quick\.md: auxiliary host point "plan:pre" missing expected kind "contribution"/, + ); + } finally { + cleanup(tmpDir); + } + }); + + test('buildContract rejects a Quick contribution dispatch without its planner target', () => { + const files = makeValidWorkflowFiles(); + files['quick.md'] = makeQuickWorkflow({ into: null }); + const tmpDir = makeTempWorkflowsDir(files); + try { + assert.throws( + () => buildContract(tmpDir), + /quick\.md: auxiliary host point "plan:pre" missing expected kind "contribution"/, + ); + } finally { + cleanup(tmpDir); + } + }); +}); + // ─── 8. Regression: FIX 1 — per-step point ownership ──────────────────────── describe('assertPointsCoverage per-step ownership (FIX 1)', () => { diff --git a/tests/quick-plan-pre-capabilities.test.cjs b/tests/quick-plan-pre-capabilities.test.cjs new file mode 100644 index 000000000..b9956f680 --- /dev/null +++ b/tests/quick-plan-pre-capabilities.test.cjs @@ -0,0 +1,106 @@ +// Quick workflow Markdown is the installed orchestration contract. + +'use strict'; + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); +const { readFileNormalized } = require('./helpers.cjs'); + +const WORKFLOW_DIR = path.join(__dirname, '..', 'gsd-core', 'workflows'); +const QUICK_PATH = path.join(WORKFLOW_DIR, 'quick.md'); +const REVISION_PATH = path.join(WORKFLOW_DIR, 'quick', 'steps', 'plan-checker-loop.md'); + +function sliceBetween(content, startAnchor, endAnchor, label) { + const start = content.indexOf(startAnchor); + assert.notEqual(start, -1, `${label}: missing start anchor ${JSON.stringify(startAnchor)}`); + const bodyStart = start + startAnchor.length; + const end = content.indexOf(endAnchor, bodyStart); + assert.notEqual(end, -1, `${label}: missing end anchor ${JSON.stringify(endAnchor)}`); + assert.ok(end > bodyStart, `${label}: anchors must enclose non-empty text`); + return content.slice(bodyStart, end); +} + +function initialPlannerPrompt(quick) { + const step = sliceBetween( + quick, + '**Step 5: Spawn planner (quick mode)**', + ''); + assert.notEqual(initialStep, -1, 'standard initial planner step must exist'); + assert.ok(initialStep < revisionGate, 'initial planner must run before the optional revision gate'); + assert.match(quick, /--full.*\$VALIDATE_MODE=true/, '--full must enable the validate path'); + assert.match(quick, /--validate.*\$VALIDATE_MODE=true/, '--validate must enable the validate path'); + assert.match(revision, /Revision loop \(max 2 iterations\)/, 'validate path must contain revision loop'); + assert.equal((quick.match(/subagent_type="gsd-planner"/g) || []).length, 1); + assert.equal((revision.match(/subagent_type="gsd-planner"/g) || []).length, 1); + }); +});