fix(dsh): /approve — per-invocation repoRoot, регистрация по approvalGates, checkBashTargets (T3.1-T3.2,T3.7, #59)

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 })

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 [
+      const sdd = checkSddWrite({ filePath: target, repoRoot })
-        checkApprovalWrite({ filePath: target, repoRoot }),
+      if (sdd.deny !== undefined) return sdd.deny
-        checkPlanWrite({ filePath: target, content: null, repoRoot }),
-        checkSddWrite({ filePath: target, repoRoot }),
-      ]) {
-        if (verdict.deny !== undefined) return verdict.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;
+        workingDirectory: ctx.workingDirectory,
-        // when it does not, sessionCwd falls back to the process directory.
-        repoRoot: sessionCwd(ctx.workingDirectory, commandCtx.agent ?? commandCtx),
         logger: ctx.logger,
       })
     })

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)
+  }
+})

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({
+    const commandCtx = new Proxy(
-      commands: { register(definition) { registered.push(definition) } },
+      { 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)
+  }
+})