From 874457527111baa9cf57e764e8d043721cec06c9 Mon Sep 17 00:00:00 2001 From: Andrey Epifancev Date: Sun, 25 Oct 2026 01:36:12 +0400 Subject: [PATCH] =?UTF-8?q?fix(dsh):=20gates=20=E2=80=94=20write-=D0=B3?= =?UTF-8?q?=D0=B5=D0=B9=D1=82=20.superpowers/sdd=20=D0=B8=20=D0=BF=D1=80?= =?UTF-8?q?=D0=BE=D0=B2=D0=B5=D1=80=D0=BA=D0=B0=20=D0=B2=D1=81=D0=B5=D1=85?= =?UTF-8?q?=20=D0=BF=D0=BB=D0=B0=D0=BD-=D1=81=D1=81=D1=8B=D0=BB=D0=BE?= =?UTF-8?q?=D0=BA=20(T1.1-T1.2,=20#59)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- dsh/bundle/lib/gates.js | 124 ++++++++++++++++++++++++++------------- dsh/tests/gates.test.mjs | 99 +++++++++++++++++++++++++++++++ 2 files changed, 181 insertions(+), 42 deletions(-) diff --git a/dsh/bundle/lib/gates.js b/dsh/bundle/lib/gates.js index b3deb48..8500aa6 100644 --- a/dsh/bundle/lib/gates.js +++ b/dsh/bundle/lib/gates.js @@ -19,6 +19,7 @@ import { basename, isAbsolute, join, relative, resolve, sep } from 'node:path' export const APPROVAL_FILE = 'approval.md' export const PLANS_DIR = join('docs', 'superpowers', 'plans') export const CHANGE_DIR = join('openspec', 'changes') +export const SDD_DIR = join('.superpowers', 'sdd') const SPEC_DENY = 'plan must carry **Spec:** pointing to openspec/changes/' const APPROVAL_DENY = 'approval.md is written only by /approve' @@ -256,6 +257,22 @@ function readApproval(repoRoot, changeId) { return parseApproval(readText(join(repoRoot, CHANGE_DIR, changeId, APPROVAL_FILE))) } +/** + * Does approval.md carry a real design approval? An empty `design:` block is + * treated as no approval (R-4/T1.6f): the section must carry the `by` and `at` + * scalars `/approve` writes, so a bare header cannot unlock the plan gate. + */ +export function hasDesignApproval(approval) { + const design = approval?.design + if (!design || typeof design !== 'object') return false + return ( + typeof design.by === 'string' && + design.by !== '' && + typeof design.at === 'string' && + design.at !== '' + ) +} + /** * Gate A: a plan may only be written for a change whose design is approved. * The `**Spec:**` reference is mandatory and must resolve to an openspec @@ -269,7 +286,7 @@ export function checkPlanWrite({ filePath, content, repoRoot } = {}) { const changeId = resolveChangeFromSpec(extractSpec(content)) if (changeId === null) return { deny: SPEC_DENY } - if (!readApproval(root, changeId).design) { + if (!hasDesignApproval(readApproval(root, changeId))) { return { deny: `design not approved; run /approve ${changeId} design` } } return {} @@ -293,37 +310,29 @@ function tokenStart(text, index) { } /** - * First plan reference in one string: a direct `docs/superpowers/plans/x.md` - * path (absolute or repository-relative) or an `.superpowers/sdd/x/` ledger - * path. Returns null when the string references no plan. + * All plan references in one string, in textual order: a direct + * `docs/superpowers/plans/x.md` path (absolute or repository-relative) or an + * `.superpowers/sdd/x/` ledger path. Empty when the string references none. */ -function planReference(text) { - if (typeof text !== 'string' || text === '') return null +function planReferences(text) { + if (typeof text !== 'string' || text === '') return [] const found = [] - const directAt = text.indexOf(DIRECT_PREFIX) - if (directAt !== -1) { - const match = DIRECT_PLAN.exec(text.slice(directAt)) - if (match) { - const start = tokenStart(text, directAt) - found.push({ index: directAt, path: text.slice(start, directAt + match[0].length) }) - } + for (let at = text.indexOf(DIRECT_PREFIX); at !== -1; at = text.indexOf(DIRECT_PREFIX, at + 1)) { + const match = DIRECT_PLAN.exec(text.slice(at)) + if (!match) continue + const start = tokenStart(text, at) + found.push({ index: at, path: text.slice(start, at + match[0].length) }) } - const sddAt = text.indexOf(SDD_PREFIX) - if (sddAt !== -1) { - const match = SDD_PATH.exec(text.slice(sddAt)) - if (match) { - const name = match[1].replace(/[/.,;:]+$/u, '') - if (name !== '' && name !== '.' && name !== '..') { - found.push({ index: sddAt, name }) - } - } + for (let at = text.indexOf(SDD_PREFIX); at !== -1; at = text.indexOf(SDD_PREFIX, at + 1)) { + const match = SDD_PATH.exec(text.slice(at)) + if (!match) continue + const name = match[1].replace(/[/.,;:]+$/u, '') + if (name !== '' && name !== '.' && name !== '..') found.push({ index: at, name }) } - if (found.length === 0) return null - found.sort((left, right) => left.index - right.index) - return found[0] + return found.sort((left, right) => left.index - right.index) } /** Deny text for an unapproved plan; `` stays literal when unknown. */ @@ -331,7 +340,23 @@ function denyPlanNotApproved(changeId) { return { deny: `plan not approved; run /approve ${changeId ?? ''} plan` } } -function checkReferencedPlan(repoRoot, planPath) { +/** Plan path a parsed reference points to (the file may be missing). */ +function referencePlanPath(reference, repoRoot) { + if (typeof reference.name === 'string') { + return ( + resolvePlanFromSdd(`${SDD_PREFIX}${reference.name}`, repoRoot) ?? + join(repoRoot, PLANS_DIR, `${reference.name}.md`) + ) + } + return isAbsolute(reference.path) ? resolve(reference.path) : resolve(repoRoot, reference.path) +} + +/** + * Full gate-B chain for one referenced plan: read it, take its `**Spec:**`, + * resolve the change, verify the plan approval against the file content. + * Missing/spec-less plans fail closed. + */ +function planVerdict(repoRoot, planPath) { const content = readText(planPath) if (content === null) return denyPlanNotApproved() const changeId = resolveChangeFromSpec(extractSpec(content)) @@ -344,27 +369,42 @@ function checkReferencedPlan(repoRoot, planPath) { } /** - * Gate B: a dispatch may only start when the plan it references is approved + * Gate B: a dispatch may only start when every plan it references is approved * for its current content (D-10). The check covers the prompt and, when - * given, the workflow script; no plan reference means no gate. A reference to - * a missing or spec-less plan fails closed. + * given, the workflow script; the first failing reference denies, so an + * approved plan cannot mask a later unapproved one. No plan reference at all + * means no gate. */ export function checkDispatch({ prompt, script, repoRoot } = {}) { const root = resolve(repoRoot || '.') - const reference = planReference(prompt) ?? planReference(script) - if (reference === null) return {} - - let planPath - if (typeof reference.name === 'string') { - planPath = - resolvePlanFromSdd(`${SDD_PREFIX}${reference.name}`, root) ?? - join(root, PLANS_DIR, `${reference.name}.md`) - } else { - planPath = isAbsolute(reference.path) - ? resolve(reference.path) - : resolve(root, reference.path) + const references = [...planReferences(prompt), ...planReferences(script)] + for (const reference of references) { + const verdict = planVerdict(root, referencePlanPath(reference, root)) + if (verdict.deny !== undefined) return verdict } - return checkReferencedPlan(root, planPath) + return {} +} + +/** + * Gate B for Mode 1 starts: a write under `.superpowers/sdd//` + * (ledger, briefs) is allowed only when that plan is approved for its current + * content (D-7). A missing plan file fails closed with `plan not found`. + */ +export function checkSddWrite({ filePath, repoRoot } = {}) { + if (typeof filePath !== 'string' || filePath === '') return {} + const root = resolve(repoRoot || '.') + const target = resolve(root, filePath) + const sddDir = join(root, SDD_DIR) + if (!isInside(sddDir, target)) return {} + + const segments = relative(sddDir, target).split(sep) + if (segments.length < 2) return {} + const name = segments[0] + if (name === '' || name === '.' || name === '..') return {} + + const planPath = resolvePlanFromSdd(`${SDD_PREFIX}${name}`, root) + if (planPath === null) return { deny: `plan not found: ${name}` } + return planVerdict(root, planPath) } /** diff --git a/dsh/tests/gates.test.mjs b/dsh/tests/gates.test.mjs index e208ab7..8ed1e26 100644 --- a/dsh/tests/gates.test.mjs +++ b/dsh/tests/gates.test.mjs @@ -8,6 +8,7 @@ import { checkBashTargets, checkDispatch, checkPlanWrite, + checkSddWrite, extractSpec, hashPlan, parseApproval, @@ -367,3 +368,101 @@ test('checkBashTargets: heredoc-equivalent targets deny', () => { cleanup(tmp) } }) + +test('checkSddWrite: plan not approved -> deny', () => { + const { tmp, repo } = repoFixture() + try { + writeApproval(repo) + const result = checkSddWrite({ + filePath: join(repo, '.superpowers', 'sdd', 'plan-x', 'progress.md'), + repoRoot: repo, + }) + assert.equal(result.deny, 'plan not approved; run /approve demo plan') + } finally { + cleanup(tmp) + } +}) + +test('checkSddWrite: approved plan -> ok, files elsewhere are not gated', () => { + const { tmp, repo, planPath } = repoFixture() + try { + writeApproval(repo, { planSha: shaOf(planPath) }) + const ledger = checkSddWrite({ + filePath: join(repo, '.superpowers', 'sdd', 'plan-x', 'briefs', 'task-1.md'), + repoRoot: repo, + }) + assert.equal(ledger.deny, undefined) + + assert.equal( + checkSddWrite({ filePath: join(repo, 'README.md'), repoRoot: repo }).deny, + undefined, + ) + assert.equal( + checkSddWrite({ + filePath: join(repo, 'docs', 'superpowers', 'plans', 'plan-x.md'), + repoRoot: repo, + }).deny, + undefined, + ) + } finally { + cleanup(tmp) + } +}) + +test('checkSddWrite: missing plan -> fail-closed deny', () => { + const { tmp, repo } = repoFixture() + try { + writeApproval(repo) + const result = checkSddWrite({ + filePath: join(repo, '.superpowers', 'sdd', 'ghost-plan', 'progress.md'), + repoRoot: repo, + }) + assert.equal(result.deny, 'plan not found: ghost-plan') + } finally { + cleanup(tmp) + } +}) + +test('checkDispatch: all plan references are checked, not only the first', () => { + const { tmp, repo, planPath } = repoFixture() + try { + const otherPlan = join(repo, 'docs', 'superpowers', 'plans', 'plan-y.md') + write(otherPlan, '# Plan Y\n\n**Spec:** `openspec/changes/demo2/`\n') + writeApproval(repo, { planSha: shaOf(planPath) }) + + // The first reference is approved; the second one is not. A first-only + // check would pass, so this pin proves every reference is checked. + const result = checkDispatch({ + prompt: `Read ${planPath} first, then ${otherPlan}.`, + repoRoot: repo, + }) + assert.equal(result.deny, 'plan not approved; run /approve demo2 plan') + } finally { + cleanup(tmp) + } +}) + +test('checkDispatch: sdd reference with an approved plan -> ok', () => { + const { tmp, repo, planPath } = repoFixture() + try { + writeApproval(repo, { planSha: shaOf(planPath) }) + const result = checkDispatch({ + prompt: 'Start Mode 1: write .superpowers/sdd/plan-x/progress.md, then the briefs.', + repoRoot: repo, + }) + assert.equal(result.deny, undefined) + } finally { + cleanup(tmp) + } +}) + +test('checkPlanWrite: empty design section counts as not approved', () => { + const { tmp, repo, planPath } = repoFixture() + try { + write(join(repo, 'openspec', 'changes', 'demo', 'approval.md'), 'design:\n') + const result = checkPlanWrite({ filePath: planPath, content: PLAN_TEXT, repoRoot: repo }) + assert.equal(result.deny, 'design not approved; run /approve demo design') + } finally { + cleanup(tmp) + } +})