diff --git a/dsh/bundle/index.js b/dsh/bundle/index.js index af25189..76c9774 100644 --- a/dsh/bundle/index.js +++ b/dsh/bundle/index.js @@ -17,13 +17,16 @@ * installation, so only Node builtins and files in this package are safe. */ +import { spawnSync } from 'node:child_process' import { existsSync } from 'node:fs' import { homedir } from 'node:os' -import { dirname, join } from 'node:path' +import { dirname, isAbsolute, join, resolve } from 'node:path' import { fileURLToPath } from 'node:url' import { registerRoleCommand } from './command-role.js' import { registerCheckCommand } from './command-check.js' +import { registerApproveCommand } from './command-approve.js' import { createGuard } from './lib/guards.js' +import { checkApprovalWrite, checkDispatch, checkPlanWrite, checkSddWrite } from './lib/gates.js' import { isBootstrapVisible, prepareBootstrap } from './lib/bootstrap.js' import { isLegacyClaudePath, warnLegacyOnce } from './lib/legacy.js' import { createUserMessage } from './lib/message.js' @@ -73,9 +76,109 @@ function sessionCwd(workingDirectory, agent) { return process.cwd() } +/** DSH tools that write file content (the Edit/Write/MultiEdit family). */ +const WRITE_TOOLS = new Set(['write', 'edit', 'multi_edit', 'str_replace_editor']) + +/** Coarse substrings that make a Bash command worth target-parsing (D-8). */ +const BASH_GATE_HINTS = ['docs/superpowers/plans', 'approval.md', '.superpowers/sdd'] + +/** + * Write targets of one Bash command, parsed by the same parser the hooks use + * (`harness_target.py targets`). `null` on any spawn/parse failure — the gate + * fails open exactly like the hooks do. + */ +function bashWriteTargets(hooksDir, command, cwd, logger) { + const parser = join(hooksDir, 'harness_target.py') + let result + try { + result = spawnSync('python3', [parser, 'targets', command, cwd], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + timeout: 15_000, + }) + } catch (error) { + logger?.warn?.('[dsh-harness] approval gates: bash target parse failed open: %s', error?.message ?? String(error)) + return null + } + if (result.error || result.status !== 0) { + logger?.warn?.( + '[dsh-harness] approval gates: harness_target.py failed open (%s)', + result.error?.message ?? `exit ${result.status}`, + ) + return null + } + const targets = [] + for (const line of String(result.stdout ?? '').split(/\r?\n/u)) { + if (!line.startsWith('file ')) continue + const path = line.slice('file '.length).trim().split(/\s+/u)[0] + if (path !== '') targets.push(path) + } + return targets +} + +/** + * Approval gates A/B/C for one tool call, run after the hook guards. + * Returns the first deny reason or undefined. All failures fail open: a call + * that names no plan/path is never blocked, and a broken bash parser pass is + * allowed, mirroring the hook invariant. + */ +function approvalGateDeny({ exec, repoRoot, hooksDir, logger }) { + const name = exec?.name + const args = exec?.arguments ?? {} + + if (WRITE_TOOLS.has(name)) { + const rawPath = args.file_path ?? args.path + if (typeof rawPath !== 'string' || rawPath === '') return undefined + const filePath = isAbsolute(rawPath) ? resolve(rawPath) : resolve(repoRoot, rawPath) + const content = + typeof args.content === 'string' + ? args.content + : typeof args.new_string === 'string' + ? args.new_string + : null + for (const verdict of [ + checkApprovalWrite({ filePath, repoRoot }), + checkPlanWrite({ filePath, content, repoRoot }), + checkSddWrite({ filePath, repoRoot }), + ]) { + if (verdict.deny !== undefined) return verdict.deny + } + return undefined + } + + if (name === 'subagent' || name === 'subagent_fork' || name === 'workflow') { + return checkDispatch({ + prompt: typeof args.prompt === 'string' ? args.prompt : undefined, + script: typeof args.script === 'string' ? args.script : undefined, + repoRoot, + }).deny + } + + if (name === 'bash') { + const command = args.command + if (typeof command !== 'string' || command === '') return undefined + if (!BASH_GATE_HINTS.some((hint) => command.includes(hint))) return undefined + const targets = bashWriteTargets(hooksDir, command, repoRoot, logger) + if (targets === null) return undefined + for (const target of targets) { + for (const verdict of [ + checkApprovalWrite({ filePath: target, repoRoot }), + checkPlanWrite({ filePath: target, content: null, repoRoot }), + checkSddWrite({ filePath: target, repoRoot }), + ]) { + if (verdict.deny !== undefined) return verdict.deny + } + } + return undefined + } + + return undefined +} + const DEFAULTS = { enabled: true, guards: true, + approvalGates: true, bootstrap: true, bootstrapSubagents: false, roleCommand: true, @@ -130,11 +233,21 @@ export function apply(ctx, rawConfig = {}) { // `tools/pre-execute` waterfall, so no other listener can turn a denial // back into permission. ctx.tools.guard((exec) => { - const reason = guard.check(exec, sessionCwd(ctx.workingDirectory, exec.agent)) + const repoRoot = sessionCwd(ctx.workingDirectory, exec.agent) + const reason = guard.check(exec, repoRoot) if (reason !== undefined) { ctx.logger?.info?.('[dsh-harness] blocked %s: %s', exec.name, reason.split('\n')[0]) + return reason } - return reason + if (config.enabled && config.approvalGates) { + const gateDeny = approvalGateDeny({ exec, repoRoot, hooksDir, logger: ctx.logger }) + if (gateDeny !== undefined) { + const gateReason = `BLOCKED [approval-gates]: ${gateDeny}` + ctx.logger?.info?.('[dsh-harness] blocked %s: %s', exec.name, gateReason.split('\n')[0]) + return gateReason + } + } + return undefined }) } @@ -185,6 +298,14 @@ export function apply(ctx, rawConfig = {}) { logger: ctx.logger, }) } + registerApproveCommand({ + commands: commandCtx.commands, + userQuestions: typeof ctx.get === 'function' ? ctx.get('userQuestions') : undefined, + // The command-injection scope may carry the owning agent/session; + // when it does not, sessionCwd falls back to the process directory. + repoRoot: sessionCwd(ctx.workingDirectory, commandCtx.agent ?? commandCtx), + logger: ctx.logger, + }) }) } diff --git a/dsh/bundle/package.json b/dsh/bundle/package.json index 00b2214..2a4d524 100644 --- a/dsh/bundle/package.json +++ b/dsh/bundle/package.json @@ -13,6 +13,7 @@ "index.js", "command-role.js", "command-check.js", + "command-approve.js", "lib", "dsh-tools.md", "cordis.patch.yml", diff --git a/dsh/tests/approve.test.mjs b/dsh/tests/approve.test.mjs index 8f2016d..2394b33 100644 --- a/dsh/tests/approve.test.mjs +++ b/dsh/tests/approve.test.mjs @@ -22,14 +22,17 @@ const ISO = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/u const APPROVE = 'Approve' /** A throwaway repository with the design artifacts and one demo plan. */ -function repoFixture({ builderCheck = true } = {}) { +function repoFixture({ builderCheck = true, withPlan = true } = {}) { const tmp = tempDir('dsh-approve-') const repo = join(tmp, 'repo') write(join(repo, 'openspec', 'changes', 'demo', 'proposal.md'), '# Proposal\n') write(join(repo, 'openspec', 'changes', 'demo', 'design.md'), '# Design\n') if (builderCheck) write(join(repo, 'openspec', 'changes', 'demo', 'builder-check.md'), '# Check\n') - const planPath = join(repo, 'docs', 'superpowers', 'plans', 'plan-x.md') - write(planPath, PLAN_TEXT) + let planPath = null + if (withPlan) { + planPath = join(repo, 'docs', 'superpowers', 'plans', 'plan-x.md') + write(planPath, PLAN_TEXT) + } return { tmp, repo, planPath } } @@ -221,3 +224,59 @@ test('approve with unknown kind returns usage', async () => { cleanup(tmp) } }) + +// Pins for the Task 2 review findings (R-5): branches without a first test. + +test('approve plan refuses when no plan references the change', async () => { + const { tmp, repo } = repoFixture({ withPlan: false }) + try { + write( + approvalPath(repo), + serializeApproval({ design: { by: 'aep', at: '2026-10-25T10:00:00+04:00' } }), + ) + const before = readFileSync(approvalPath(repo), 'utf8') + + const harness = setup(repo, { answer: approveAnswer('plan') }) + const result = await harness.run('demo plan') + + assert.equal(result.kind, 'error') + assert.match(result.text, /no plan for demo found/u) + assert.match(result.text, /docs\/superpowers\/plans\//u) + assert.equal(readFileSync(approvalPath(repo), 'utf8'), before, 'approval.md is untouched') + assert.equal(harness.calls.length, 0, 'the owner is not asked without a plan') + } finally { + cleanup(tmp) + } +}) + +test('approve refuses an unsafe change id', async () => { + const { tmp, repo } = repoFixture() + try { + const harness = setup(repo, { answer: approveAnswer('design') }) + + const result = await harness.run('../x design') + assert.match(result.text, /Invalid change id "\.\.\/x"\./u) + assert.match(result.text, /Usage: \/approve design\|plan/u) + assert.equal(existsSync(approvalPath(repo)), false, 'no approval.md is written') + assert.equal(harness.calls.length, 0, 'the owner is not asked for an invalid id') + } finally { + cleanup(tmp) + } +}) + +test('approve accepts the fallback answer shapes {approved}/{confirmed}/true', async () => { + const { tmp, repo } = repoFixture() + try { + const shapes = [{ approved: true }, { confirmed: true }, true] + for (const answer of shapes) { + const harness = setup(repo, { answer }) + const result = await harness.run('demo design') + assert.equal(result.kind, 'success', `answer shape ${JSON.stringify(answer)} approves`) + const parsed = parseApproval(readFileSync(approvalPath(repo), 'utf8')) + assert.equal(parsed.design.by, 'owner') + assert.match(parsed.design.at, ISO) + } + } finally { + cleanup(tmp) + } +}) diff --git a/dsh/tests/package.test.mjs b/dsh/tests/package.test.mjs index acb7a5d..010625d 100644 --- a/dsh/tests/package.test.mjs +++ b/dsh/tests/package.test.mjs @@ -14,6 +14,7 @@ test('package declares a DSH bundle patch and the runtime files', () => { assert.equal(pkg.dsh?.bundle?.patch, './cordis.patch.yml') assert.equal(pkg.exports?.['.'], './index.js') assert.ok(pkg.files.includes('index.js')) + assert.ok(pkg.files.includes('command-approve.js')) assert.ok(pkg.files.includes('lib')) assert.ok(pkg.files.includes('hooks')) assert.ok(existsSync(join(bundleDir, 'cordis.patch.yml'))) @@ -56,8 +57,10 @@ test('runtime files have no external @deepseek-ai imports', () => { 'index.js', 'command-role.js', 'command-check.js', + 'command-approve.js', 'lib/message.js', 'lib/guards.js', + 'lib/gates.js', 'lib/bootstrap.js', 'lib/role.js', 'lib/legacy.js', diff --git a/dsh/tests/run.sh b/dsh/tests/run.sh index 6ce8e30..6130c4b 100755 --- a/dsh/tests/run.sh +++ b/dsh/tests/run.sh @@ -23,7 +23,7 @@ ensure_bundle_hooks() { ensure_bundle_hooks -for f in dsh/bundle/index.js dsh/bundle/command-role.js dsh/bundle/command-check.js dsh/bundle/lib/*.js dsh/tests/*.mjs; do +for f in dsh/bundle/index.js dsh/bundle/command-role.js dsh/bundle/command-check.js dsh/bundle/command-approve.js dsh/bundle/lib/*.js dsh/tests/*.mjs; do node --check "$f" done diff --git a/dsh/tests/selfcheck.test.mjs b/dsh/tests/selfcheck.test.mjs index d3d6e9e..d32a086 100644 --- a/dsh/tests/selfcheck.test.mjs +++ b/dsh/tests/selfcheck.test.mjs @@ -1,8 +1,12 @@ import assert from 'node:assert/strict' +import { createHash } from 'node:crypto' +import { join } from 'node:path' import test from 'node:test' import { apply } from '../bundle/index.js' import { collectSelfCheck } from '../bundle/lib/selfcheck.js' +import { checkSddWrite } from '../bundle/lib/gates.js' import { registerCheckCommand } from '../bundle/command-check.js' +import { cleanup, git, makeRepo, write } from './helpers.mjs' const FEATURES = ['guards', 'bootstrap', 'commands', 'workingDirectory'] @@ -167,3 +171,238 @@ test('apply wires guards, bootstrap, and commands with a full ctx', () => { assert.equal(calls.on, 1, 'bootstrap wired') assert.equal(calls.inject, 1, 'commands wired') }) + +// --------------------------------------------------------------------------- +// Approval gates (Task 3): apply() wires the A/B/C gates after the hooks. +// --------------------------------------------------------------------------- + +const PLAN_TEXT = [ + '# Plan X', + '', + '**Spec:** `openspec/changes/demo/`', + '', + '## Task 1', + '', + 'Do the work.', + '', +].join('\n') + +/** + * A throwaway repository on a feature branch (the hooks must pass) with the + * change artifacts and one demo plan. `design` writes a design-approved + * approval.md; `planSha` additionally writes the plan section (default: the + * real sha256 of the fixture plan). + */ +function gateFixture({ design = false, planSha } = {}) { + const repo = makeRepo() + git(repo, ['checkout', '-b', 'feat/x']) + write(join(repo, 'openspec', 'changes', 'demo', 'proposal.md'), '# Proposal\n') + write(join(repo, 'openspec', 'changes', 'demo', 'design.md'), '# Design\n') + const planPath = join(repo, 'docs', 'superpowers', 'plans', 'plan-x.md') + write(planPath, PLAN_TEXT) + if (design || planSha !== undefined) { + const sections = [] + if (design) { + sections.push('design:\n by: owner\n at: 2026-10-25T10:00:00.000Z') + } + if (planSha !== undefined) { + sections.push(`plan:\n by: owner\n at: 2026-10-25T11:00:00.000Z\n sha256: ${planSha}`) + } + write(join(repo, 'openspec', 'changes', 'demo', 'approval.md'), `${sections.join('\n\n')}\n`) + } + return { repo, planPath } +} + +function planShaOf(planText) { + return createHash('sha256').update(planText).digest('hex') +} + +/** apply() against a fake ctx: captured guard + inject, session cwd = repo. */ +function gateHarness(repo, config = {}) { + const guardFns = [] + const injected = [] + const ctx = { + tools: { guard(fn) { guardFns.push(fn) } }, + on() {}, + inject(names, callback) { injected.push({ names, callback }) }, + workingDirectory: { get() { return repo } }, + get() { return undefined }, + logger: { info() {}, warn() {}, error() {} }, + } + apply(ctx, config) + return { + guard: (exec) => guardFns[0]({ agent: { session: 's1' }, ...exec }), + injected, + } +} + +function call(name, args) { + return { name, arguments: args } +} + +test('apply: guard denies plan write without design approval', () => { + const { repo } = gateFixture() + try { + const harness = gateHarness(repo) + const reason = harness.guard(call('write', { + file_path: join(repo, 'docs', 'superpowers', 'plans', 'plan-new.md'), + content: PLAN_TEXT, + })) + assert.match(reason ?? '', /BLOCKED \[approval-gates\]: design not approved; run \/approve demo design/) + } finally { + cleanup(repo) + } +}) + +test('apply: guard allows plan write after approval.md with design', () => { + const { repo } = gateFixture({ design: true }) + try { + const harness = gateHarness(repo) + const reason = harness.guard(call('write', { + file_path: join(repo, 'docs', 'superpowers', 'plans', 'plan-new.md'), + content: PLAN_TEXT, + })) + assert.equal(reason, undefined) + } finally { + cleanup(repo) + } +}) + +test('apply: guard denies subagent dispatch referencing unapproved plan', () => { + const { repo } = gateFixture() + try { + const harness = gateHarness(repo) + const reason = harness.guard(call('subagent', { + prompt: 'Implement docs/superpowers/plans/plan-x.md Task 1.', + })) + assert.match(reason ?? '', /BLOCKED \[approval-gates\]: plan not approved; run \/approve demo plan/) + } finally { + cleanup(repo) + } +}) + +test('apply: guard allows dispatch after plan approval (sha match)', () => { + const { repo } = gateFixture({ design: true, planSha: planShaOf(PLAN_TEXT) }) + try { + const harness = gateHarness(repo) + const reason = harness.guard(call('subagent', { + prompt: 'Implement docs/superpowers/plans/plan-x.md Task 1.', + })) + assert.equal(reason, undefined) + } finally { + cleanup(repo) + } +}) + +test('apply: bash heredoc into approval.md denied', () => { + const { repo } = gateFixture() + try { + const harness = gateHarness(repo) + const reason = harness.guard(call('bash', { + command: "cat > openspec/changes/demo/approval.md <<'EOF'\nby: me\nEOF\n", + })) + assert.match(reason ?? '', /BLOCKED \[approval-gates\]: approval\.md is written only by \/approve/) + } finally { + cleanup(repo) + } +}) + +test('apply: bash write under .superpowers/sdd// denied without approval', () => { + const { repo } = gateFixture() + try { + const harness = gateHarness(repo) + const reason = harness.guard(call('bash', { + command: 'mkdir -p .superpowers/sdd/plan-x && printf "x" > .superpowers/sdd/plan-x/progress.md', + })) + assert.match(reason ?? '', /BLOCKED \[approval-gates\]: plan not approved; run \/approve demo plan/) + } finally { + cleanup(repo) + } +}) + +test('apply: bash write under .superpowers/sdd// allowed with approval', () => { + const { repo } = gateFixture({ design: true, planSha: planShaOf(PLAN_TEXT) }) + try { + const harness = gateHarness(repo) + const reason = harness.guard(call('bash', { + command: 'mkdir -p .superpowers/sdd/plan-x && printf "x" > .superpowers/sdd/plan-x/progress.md', + })) + assert.equal(reason, undefined) + } finally { + cleanup(repo) + } +}) + +test('gates: checkSddWrite sha-mismatch deny', () => { + const { repo } = gateFixture({ design: true, planSha: 'a'.repeat(64) }) + try { + const result = checkSddWrite({ + filePath: join(repo, '.superpowers', 'sdd', 'plan-x', 'progress.md'), + repoRoot: repo, + }) + assert.equal(result.deny, 'plan changed after approval; re-approve') + } finally { + cleanup(repo) + } +}) + +test('gates: bare .superpowers/sdd/ dir ungated', () => { + const { repo } = gateFixture() + try { + const result = checkSddWrite({ + filePath: join(repo, '.superpowers', 'sdd', 'plan-x'), + repoRoot: repo, + }) + assert.equal(result.deny, undefined) + } finally { + cleanup(repo) + } +}) + +test('apply: approvalGates:false disables all gates', () => { + const { repo } = gateFixture() + try { + const harness = gateHarness(repo, { approvalGates: false }) + assert.equal( + harness.guard(call('write', { + file_path: join(repo, 'docs', 'superpowers', 'plans', 'plan-new.md'), + content: PLAN_TEXT, + })), + undefined, + ) + assert.equal( + harness.guard(call('subagent', { + prompt: 'Implement docs/superpowers/plans/plan-x.md Task 1.', + })), + undefined, + ) + assert.equal( + harness.guard(call('bash', { + command: "cat > openspec/changes/demo/approval.md <<'EOF'\nby: me\nEOF\n", + })), + undefined, + ) + } finally { + cleanup(repo) + } +}) + +test('apply: /approve registered when commands present; absent userQuestions still registers', () => { + const { repo } = gateFixture({ design: true }) + try { + const harness = gateHarness(repo) + assert.equal(harness.injected.length, 1, 'the commands inject block ran') + + const registered = [] + harness.injected[0].callback({ + commands: { register(definition) { registered.push(definition) } }, + }) + + const approve = registered.find((definition) => definition.name === 'approve') + assert.ok(approve, '/approve is registered') + assert.equal(typeof approve.handler, 'function') + assert.equal(approve.input.hint, ' design|plan') + } finally { + cleanup(repo) + } +})