From 653d1df30f8ff678c814c28ff5f40aa1764a7cda Mon Sep 17 00:00:00 2001 From: Andrey Epifancev Date: Sun, 25 Oct 2026 02:20:27 +0400 Subject: [PATCH] =?UTF-8?q?fix(dsh):=20/approve=20=E2=80=94=20per-invocati?= =?UTF-8?q?on=20repoRoot,=20=D1=80=D0=B5=D0=B3=D0=B8=D1=81=D1=82=D1=80?= =?UTF-8?q?=D0=B0=D1=86=D0=B8=D1=8F=20=D0=BF=D0=BE=20approvalGates,=20chec?= =?UTF-8?q?kBashTargets=20(T3.1-T3.2,T3.7,=20#59)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- dsh/bundle/command-approve.js | 20 +++++++++- dsh/bundle/index.js | 35 +++++++++++----- dsh/tests/approve.test.mjs | 39 +++++++++++++++++- dsh/tests/selfcheck.test.mjs | 75 ++++++++++++++++++++++++++++++++--- 4 files changed, 149 insertions(+), 20 deletions(-) diff --git a/dsh/bundle/command-approve.js b/dsh/bundle/command-approve.js index 56aa63f..bfece3a 100644 --- a/dsh/bundle/command-approve.js +++ b/dsh/bundle/command-approve.js @@ -7,6 +7,11 @@ * merges the new section into the existing file. A missing user-questions * service refuses instead of writing, so no model tool can approve itself. * + * The repository root is resolved per invocation from the receiving agent's + * session (`workingDirectory.get(agent.session)`, T3.1) — the registration + * itself carries no root, because the command runs against whatever agent + * invoked it. + * * Mirrors `command-role.js`: object-form `commands.register`, the same * `{ kind, text }` result shape. */ @@ -53,6 +58,17 @@ function readText(path) { } } +/** Session working directory of the invoking agent; process.cwd() fallback. */ +function currentDirectory(workingDirectory, agent) { + try { + const cwd = workingDirectory?.get?.(agent?.session) + if (typeof cwd === 'string' && cwd !== '') return cwd + } catch { + // Fall through to the process directory. + } + return process.cwd() +} + function refusal(text) { return { kind: 'error', text: `Refused: ${text}` } } @@ -228,7 +244,7 @@ async function approvePlan({ root, changeId, userQuestions, agent, signal }) { } } -export function registerApproveCommand({ commands, userQuestions, repoRoot, logger }) { +export function registerApproveCommand({ commands, userQuestions, workingDirectory, logger }) { commands.register({ name: 'approve', description: 'Утвердить design или plan change (единственный писатель approval.md)', @@ -249,7 +265,7 @@ export function registerApproveCommand({ commands, userQuestions, repoRoot, logg return usage(`Unknown kind "${kind}".`) } - const root = resolve(repoRoot || process.cwd()) + const root = resolve(currentDirectory(workingDirectory, agent)) try { if (kind === 'design') { return await approveDesign({ root, changeId, userQuestions, agent, signal }) diff --git a/dsh/bundle/index.js b/dsh/bundle/index.js index 76c9774..761578e 100644 --- a/dsh/bundle/index.js +++ b/dsh/bundle/index.js @@ -26,7 +26,13 @@ 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 { + checkApprovalWrite, + checkBashTargets, + 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' @@ -160,14 +166,13 @@ function approvalGateDeny({ exec, repoRoot, hooksDir, logger }) { if (!BASH_GATE_HINTS.some((hint) => command.includes(hint))) return undefined const targets = bashWriteTargets(hooksDir, command, repoRoot, logger) if (targets === null) return undefined + // Plans/approval.md through the shared target check (T3.7), then the + // additional sdd pass for `.superpowers/sdd/` targets. + const bashVerdict = checkBashTargets(targets, repoRoot) + if (bashVerdict.deny !== undefined) return bashVerdict.deny 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 - } + const sdd = checkSddWrite({ filePath: target, repoRoot }) + if (sdd.deny !== undefined) return sdd.deny } return undefined } @@ -298,12 +303,20 @@ export function apply(ctx, rawConfig = {}) { logger: ctx.logger, }) } + }) + } + + // /approve has its own activation switch (T3.2), independent of /role and + // /dsh-harness-check: the command resolves its repoRoot per invocation from + // the receiving agent, so the wiring passes the service, not a root. The + // commands availability is checked directly — the self-check skips the + // `commands` feature when both roleCommand and checkCommand are off. + if (config.enabled && config.approvalGates && typeof ctx.inject === 'function') { + ctx.inject(['commands'], (commandCtx) => { 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), + workingDirectory: ctx.workingDirectory, logger: ctx.logger, }) }) diff --git a/dsh/tests/approve.test.mjs b/dsh/tests/approve.test.mjs index 2394b33..f993060 100644 --- a/dsh/tests/approve.test.mjs +++ b/dsh/tests/approve.test.mjs @@ -47,7 +47,8 @@ function approveAnswer(kind = 'design') { /** * Wire the command against a fake `commands.register` that captures the - * registered definition, and a scripted `userQuestions.ask`. + * registered definition, and a scripted `userQuestions.ask`. The fake + * `workingDirectory` resolves every session to the fixture repo. */ function setup(repo, { answer, missingService = false } = {}) { const captured = {} @@ -69,7 +70,7 @@ function setup(repo, { answer, missingService = false } = {}) { registerApproveCommand({ commands, userQuestions, - repoRoot: repo, + workingDirectory: { get() { return repo } }, logger: { warn() {} }, }) return { captured, calls, run: (rawInput) => captured.handler({ rawInput }) } @@ -280,3 +281,37 @@ test('approve accepts the fallback answer shapes {approved}/{confirmed}/true', a cleanup(tmp) } }) + +// T3.1 regression: the repo root is resolved per handler invocation from the +// invocation agent's session, not fixed at registration. + +test('approve resolves the session cwd per invocation (T3.1)', async () => { + const first = repoFixture() + const second = repoFixture() + try { + const roots = new Map([ + ['s1', first.repo], + ['s2', second.repo], + ]) + const captured = {} + const commands = { register(definition) { Object.assign(captured, definition) } } + registerApproveCommand({ + commands, + userQuestions: { async ask() { return approveAnswer('design') } }, + workingDirectory: { get(session) { return roots.get(session) } }, + logger: { warn() {} }, + }) + + const firstResult = await captured.handler({ agent: { session: 's1' }, rawInput: 'demo design' }) + assert.equal(firstResult.kind, 'success') + assert.equal(existsSync(approvalPath(first.repo)), true, 'approval.md written in the s1 repo') + assert.equal(existsSync(approvalPath(second.repo)), false, 'the s2 repo is untouched') + + const secondResult = await captured.handler({ agent: { session: 's2' }, rawInput: 'demo design' }) + assert.equal(secondResult.kind, 'success') + assert.equal(existsSync(approvalPath(second.repo)), true, 'approval.md written in the s2 repo') + } finally { + cleanup(first.tmp) + cleanup(second.tmp) + } +}) diff --git a/dsh/tests/selfcheck.test.mjs b/dsh/tests/selfcheck.test.mjs index d32a086..093fb16 100644 --- a/dsh/tests/selfcheck.test.mjs +++ b/dsh/tests/selfcheck.test.mjs @@ -169,7 +169,7 @@ test('apply wires guards, bootstrap, and commands with a full ctx', () => { assert.doesNotThrow(() => apply(ctx)) assert.equal(calls.guard, 1, 'guards wired') assert.equal(calls.on, 1, 'bootstrap wired') - assert.equal(calls.inject, 1, 'commands wired') + assert.equal(calls.inject, 2, 'commands wired (role/check + /approve)') }) // --------------------------------------------------------------------------- @@ -391,12 +391,23 @@ test('apply: /approve registered when commands present; absent userQuestions sti const { repo } = gateFixture({ design: true }) try { const harness = gateHarness(repo) - assert.equal(harness.injected.length, 1, 'the commands inject block ran') + assert.equal(harness.injected.length, 2, 'role/check and /approve inject blocks ran') + // A cordis-like command context: declared services resolve, any other key + // (e.g. `agent`) throws — the wiring must not read it (T3.1). const registered = [] - harness.injected[0].callback({ - commands: { register(definition) { registered.push(definition) } }, - }) + const commandCtx = new Proxy( + { commands: { register(definition) { registered.push(definition) } } }, + { + get(target, prop) { + if (prop in target) return target[prop] + throw new Error(`unknown service "${String(prop)}"`) + }, + }, + ) + for (const { callback } of harness.injected) { + assert.doesNotThrow(() => callback(commandCtx), 'the inject callback must not touch ctx.agent') + } const approve = registered.find((definition) => definition.name === 'approve') assert.ok(approve, '/approve is registered') @@ -406,3 +417,57 @@ test('apply: /approve registered when commands present; absent userQuestions sti cleanup(repo) } }) + +test('apply: /approve registers on approvalGates alone (role/check commands off)', () => { + const { repo } = gateFixture({ design: true }) + try { + const harness = gateHarness(repo, { roleCommand: false, checkCommand: false }) + assert.equal(harness.injected.length, 1, 'only the /approve inject block runs') + + const registered = [] + harness.injected[0].callback({ + commands: { register(definition) { registered.push(definition) } }, + }) + assert.ok( + registered.some((definition) => definition.name === 'approve'), + '/approve is registered without role/check commands', + ) + assert.ok( + !registered.some((definition) => definition.name === 'role'), + '/role is not registered', + ) + } finally { + cleanup(repo) + } +}) + +test('apply: approvalGates:false leaves /approve unregistered', () => { + const { repo } = gateFixture() + try { + const harness = gateHarness(repo, { approvalGates: false }) + + const registered = [] + for (const { callback } of harness.injected) { + callback({ commands: { register(definition) { registered.push(definition) } } }) + } + assert.ok( + !registered.some((definition) => definition.name === 'approve'), + '/approve is not registered when approvalGates is off', + ) + } finally { + cleanup(repo) + } +}) + +test('apply: bash targets prefer approval/plan denies before sdd denies', () => { + const { repo } = gateFixture() + try { + const harness = gateHarness(repo) + const reason = harness.guard(call('bash', { + command: 'printf "y" > .superpowers/sdd/plan-x/progress.md && printf "x" > openspec/changes/demo/approval.md', + })) + assert.match(reason ?? '', /BLOCKED \[approval-gates\]: approval\.md is written only by \/approve/) + } finally { + cleanup(repo) + } +})