fix(dsh): #62 — PATH-изоляция теста, WARN DSH_HOME, TDD-порядок в sdd (#62)

2026-10-25 12:19:53 +04:00 · 2026-10-25 12:19:53 +04:00 · 183f8d8956
commit 183f8d8956
parent 622dd32e1a
4 changed files with 103 additions and 16 deletions
--- a/dsh/install.sh
+++ b/dsh/install.sh
@ -21,6 +21,14 @@ set -euo pipefail

 ROOT="$(cd "$(dirname "$0")/.." && pwd)"
 BUNDLE="$ROOT/dsh/bundle"
+# O-2: remember whether DSH_HOME came from the environment. Isolating HOME
+# alone is not enough: an explicit DSH_HOME still writes outside the isolated
+# tree (a temp HOME with the real DSH_HOME, typically a test or a detached
+# shell), so the install steps warn about that mixed state.
+DSH_HOME_FROM_ENV=0
+if [ -n "${DSH_HOME:-}" ]; then
+  DSH_HOME_FROM_ENV=1
+fi
 DSH_HOME="${DSH_HOME:-$HOME/.dsh}"
 DSH_SUPERPOWERS_REF="${DSH_SUPERPOWERS_REF:-v7.0.1}"
 DSH_SUPERPOWERS_REPO="${DSH_SUPERPOWERS_REPO:-https://github.com/obra/superpowers}"
@ -498,7 +506,21 @@ copy_skills_from() {
  echo "copied $count superpowers skills into $DSH_HOME/skills (source: $label)"
 }

+# O-2: an explicit DSH_HOME that already exists while $HOME is another tree is
+# the mixed case: the install writes into the existing DSH_HOME instead of the
+# isolated one. Warn once per run and continue — the target is valid, only the
+# HOME/DSH_HOME pair is not isolated together.
+warn_mixed_dsh_home() {
+  if [ "$DSH_HOME_FROM_ENV" != 1 ]; then return 0; fi
+  if [ "$DSH_HOME" = "$HOME/.dsh" ]; then return 0; fi
+  if [ ! -d "$DSH_HOME" ]; then return 0; fi
+  if [ "${DSH_HOME_WARNED:-0}" = 1 ]; then return 0; fi
+  DSH_HOME_WARNED=1
+  echo "WARN: DSH_HOME=$DSH_HOME points to an existing directory with a non-standard HOME; both HOME and DSH_HOME must be isolated together" >&2
+}
+
 install_skills() {
+  warn_mixed_dsh_home
  mkdir -p "$DSH_HOME/skills"

  # Explicit source: sync it directly, no network, no pin enforcement.
@ -576,6 +598,7 @@ install_local_skills() {
 }

 install_roles() {
+  warn_mixed_dsh_home
  mkdir -p "$DSH_HOME/roles"
  local src="$ROOT/roles"
  local count=0
--- a/dsh/skills/sdd/SKILL.md
+++ b/dsh/skills/sdd/SKILL.md
@ -9,7 +9,7 @@ description: Use when executing an implementation plan in DeepSeek Harness (DSH)
 только в DSH-сессиях: у Claude Code синхронный `Task`, у DSH — асинхронный
 `subagent`. Это **не паритет** с Claude-SDD и не имитация: протокол построен
 на том, что DSH умеет (уведомление о завершении, `send_message`, ledger), а не
-на том, чего у него нет (возврат результата из вызова инструмента).
+на том, чего у него нет.

 ## Когда применять

@ -62,6 +62,9 @@ overlay ребёнок наследует модель сессии.
 6. Замеченное расхождение с брифом или спекой — это `DONE_WITH_CONCERNS`, даже
   если задача формально выполнена: ревью его проверит, контроллер — решит.

+**Порядок TDD.** Шаг «напиши тест и убедись, что он падает (RED)» обязателен
+ДО кода; реализация без RED — `DONE_WITH_CONCERNS`, ревью это проверяет.
+
 ## Ожидание без блокировки

 `subagent` возвращает id и сразу завершается; результата в значении
@ -135,9 +138,8 @@ ledger лежит в git-ignored scratch (после `git clean -fdx` восст
 фиксирует это наблюдением `O-n`. `model` нельзя передать один, «унаследовав»
 провайдера: DSH требует `provider` и `model` вместе и на односторонний
 маршрут отвечает «child LLM provider and model must be supplied together».
-Эта ошибка означает, что в вызове не хватает `provider` (в прогоне #41 так
-случилось дважды), а не сбой валидации: проверь аргументы и повтори вызов,
-передав оба поля.
+Эта ошибка означает, что в вызове не хватает `provider`, а не сбой валидации:
+проверь аргументы и повтори вызов, передав оба поля.

 ## Отличие от Claude-SDD

--- a/dsh/tests/install.test.mjs
+++ b/dsh/tests/install.test.mjs
@ -54,6 +54,9 @@ test('install.sh copies skills and roles instead of linking into ~/.claude', ()
    assert.match(sddText, /name: sdd/)
    assert.match(sddText, /\/approve/, 'the sdd skill must check the plan approval marker before dispatch')
    assert.match(sddText, /approval\.md/, 'the sdd skill must name the approval marker file')
+    // T3.4: TDD order is part of the dispatch protocol: RED before the code.
+    assert.match(sddText, /падает \(RED\)/, 'the sdd skill must require a failing test (RED) before the code')
+    assert.match(sddText, /реализация без RED — `DONE_WITH_CONCERNS`/, 'a missing RED must degrade the report status')

    for (const role of ['dev', 'admin', 'analyst', 'qa']) {
      assert.ok(existsSync(join(dshHome, 'roles', `${role}.md`)), `missing ${role}.md`)
@ -638,6 +641,42 @@ test('--global installs DSH role paths and explains the instruction format', ()
  }
 })

+test('--skills and --roles warn when DSH_HOME exists while HOME is isolated (O-2)', () => {
+  const work = tempDir()
+  try {
+    const home = join(work, 'home')
+    // An existing DSH_HOME outside the isolated HOME is the mixed state: the
+    // install would write outside the sandbox. The warning must not abort.
+    const sharedDsh = join(work, 'shared-dsh')
+    mkdirSync(home, { recursive: true })
+    mkdirSync(sharedDsh, { recursive: true })
+    const src = fixtureSuperpowers(home, null, join(home, 'src', 'superpowers'))
+    const warn = `WARN: DSH_HOME=${sharedDsh} points to an existing directory with a non-standard HOME; both HOME and DSH_HOME must be isolated together`
+
+    const skills = runInstaller(home, ['--skills', '--superpowers-from', src], { DSH_HOME: sharedDsh })
+    const skillsOut = skills.stdout + skills.stderr
+    assert.equal(skills.status, 0, skillsOut)
+    assert.ok(skillsOut.includes(warn), skillsOut)
+    assert.ok(
+      existsSync(join(sharedDsh, 'skills', 'using-superpowers', 'SKILL.md')),
+      'the install still runs after the warning',
+    )
+
+    const roles = runInstaller(home, ['--roles'], { DSH_HOME: sharedDsh })
+    const rolesOut = roles.stdout + roles.stderr
+    assert.equal(roles.status, 0, rolesOut)
+    assert.ok(rolesOut.includes(warn), rolesOut)
+
+    // Isolated together ($DSH_HOME=$HOME/.dsh): no warning.
+    const normal = runInstaller(home, ['--skills', '--superpowers-from', src])
+    const normalOut = normal.stdout + normal.stderr
+    assert.equal(normal.status, 0, normalOut)
+    assert.doesNotMatch(normalOut, /WARN: DSH_HOME=/)
+  } finally {
+    cleanup(work)
+  }
+})
+
 test('a legacy cache without using-superpowers is not a source', () => {
  const home = tempDir()
  try {
--- a/dsh/tests/openspec.test.mjs
+++ b/dsh/tests/openspec.test.mjs
@ -19,6 +19,32 @@ const NAMES = [
  'openspec-update-change',
 ]

+// #62 T2.1/M-2: a negative test must not inherit the host PATH — on Linux a
+// global npm prefix puts openspec into /usr/bin, so a fixed system PATH is not
+// openspec-free. The child gets a guard dir holding symlinks to the binaries
+// the script needs and nothing else. `bash` is addressed by absolute path
+// because libuv resolves the command with the child's restricted PATH.
+function which(name) {
+  const out = spawnSync('bash', ['-c', `command -v ${name}`], { encoding: 'utf8' })
+  return out.stdout.trim()
+}
+
+const BASH = which('bash') || '/bin/bash'
+
+function isolatedPath(home) {
+  const bin = join(home, 'guard-bin')
+  mkdirSync(bin)
+  // The toolchain --check verifies (python3, git, node) plus the POSIX tools
+  // install.sh calls on this path (dirname for ROOT, head for versions, the
+  // ls/sort/tail legacy lookup). openspec is deliberately absent.
+  for (const name of ['python3', 'git', 'node', 'dirname', 'head', 'ls', 'sort', 'tail']) {
+    const target = which(name)
+    assert.ok(target, `cannot resolve ${name}: the isolated PATH would be incomplete`)
+    symlinkSync(target, join(bin, name))
+  }
+  return bin
+}
+
 test('repository ships six agents skills for DSH', () => {
  assert.equal(readFileSync(join(agentsSkills, '.openspec-target'), 'utf8').trim(), 'agents')
  for (const name of NAMES) {
@ -49,17 +75,16 @@ test('install.sh --check reports the openspec CLI and the agents skills', () =>
 test('install.sh --check warns but does not fail without the openspec CLI', () => {
  const home = tempDir()
  try {
-    const bin = join(home, 'bin')
-    mkdirSync(bin, { recursive: true })
-    symlinkSync(process.execPath, join(bin, 'node'))
-    // openspec ставится в /opt/homebrew/bin; здесь только системные каталоги — CLI не найден
+    // #62 T2.1: the guard dir is the whole PATH, so a host openspec CLI (e.g.
+    // /usr/bin on Linux) can never leak into this negative test.
+    const bin = isolatedPath(home)
    const env = {
      ...process.env,
      HOME: home,
      DSH_HOME: join(home, '.dsh'),
-      PATH: `${bin}:/usr/bin:/bin:/usr/sbin:/sbin`,
+      PATH: bin,
    }
-    const run = spawnSync('bash', [script, '--check'], { env, encoding: 'utf8' })
+    const run = spawnSync(BASH, [script, '--check'], { env, encoding: 'utf8' })
    assert.equal(run.status, 0, run.stderr)
    assert.match(run.stdout + run.stderr, /WARN: openspec not found/)
  } finally {
@ -134,17 +159,15 @@ test('--openspec fails without the openspec CLI', () => {
  try {
    const project = join(home, 'project')
    mkdirSync(join(project, 'openspec'), { recursive: true })
-    const bin = join(home, 'bin')
-    mkdirSync(bin, { recursive: true })
-    symlinkSync(process.execPath, join(bin, 'node'))
-    // openspec ставится в /opt/homebrew/bin; здесь только системные каталоги — CLI не найден
+    // #62 T2.1: same guard-dir isolation as the --check negative test.
+    const bin = isolatedPath(home)
    const env = {
      ...process.env,
      HOME: home,
      DSH_HOME: join(home, '.dsh'),
-      PATH: `${bin}:/usr/bin:/bin:/usr/sbin:/sbin`,
+      PATH: bin,
    }
-    const run = spawnSync('bash', [script, '--openspec'], { cwd: project, env, encoding: 'utf8' })
+    const run = spawnSync(BASH, [script, '--openspec'], { cwd: project, env, encoding: 'utf8' })
    assert.notEqual(run.status, 0)
    assert.match(run.stdout + run.stderr, /FAIL: openspec CLI not found/)
  } finally {