Просмотр исходного кода

fix(sdd): ownership markers stop same-basename plans sharing a workspace

sdd-workspace slugged workspaces by basename alone, so docs/alpha/plan.md
and docs/beta/plan.md resolved to one directory and task-brief silently
overwrote the other plan's brief — the single gitignored source of task
requirements, unrecoverable once clobbered.

Each workspace now records its owning plan in a plan-path marker
(repo-relative in-repo, absolute outside). Lookup keeps basename slugs
and existing behavior for the common case: a markerless workspace is
adopted in place (no migration break for in-flight plans), a marker
naming this plan is a match, and a marker naming a different plan
disambiguates with the plan's parent-directory name, then a counter.
Plan paths are normalized (CDPATH-guarded physical cd) so relative,
absolute, and ../ spellings of one plan share one workspace.

task-brief and review-package delegate to sdd-workspace and need no
changes. SKILL.md's workspace bullet no longer promises the exact
<plan-basename> path, since disambiguated workspaces differ.

Reported by @CRGDan; reproduction and test groundwork by @crisnahine
in PR #2120.

Fixes #2045
Jesse Vincent 1 месяц назад
Родитель
Сommit
0e798e1dc2

+ 1 - 1
skills/subagent-driven-development/SKILL.md

@@ -135,7 +135,7 @@ a ledger file, not only in todos.
 
 - Each plan owns a workspace: at skill start, run this skill's
   `scripts/sdd-workspace PLAN_FILE` — it prints the plan's git-ignored
-  directory (`<repo-root>/.superpowers/sdd/<plan-basename>/`), home to
+  directory (under `<repo-root>/.superpowers/sdd/`), home to
   every artifact for THIS plan: ledger, briefs, reports, review packages.
   Another plan's directory is never yours to read or write.
 - Check for this plan's ledger at `<workspace>/progress.md`. If its first

+ 44 - 2
skills/subagent-driven-development/scripts/sdd-workspace

@@ -8,6 +8,16 @@
 # artifacts. A stale ledger misread as current progress makes controllers
 # skip whole task sequences — plan-scoping removes that failure structurally.
 #
+# Basename slugs collide when two plans share a filename (docs/alpha/plan.md
+# vs docs/beta/plan.md), so each workspace records its owning plan's path in
+# a plan-path marker (repo-relative in-repo, absolute outside). A workspace
+# owned by a different plan is skipped and the slug disambiguated with the
+# plan's parent-directory name, then a counter. A workspace with no marker
+# predates the marker scheme and is adopted for the current plan so in-flight
+# workspaces keep resolving — which means the first collision on such a
+# legacy workspace adopts instead of detecting; acceptable, marker-less
+# workspaces age out as plans finish.
+#
 # The workspace lives in the working tree (not under .git/) because Claude Code
 # treats .git/ as a protected path and denies agent writes there — which blocks
 # an implementer subagent from writing its report file. A self-ignoring
@@ -34,7 +44,39 @@ slug=$(basename "$plan" .md)
 
 root=$(git rev-parse --show-toplevel)
 base="$root/.superpowers/sdd"
+
+# Normalize the plan path (physical directory, so relative/absolute/../
+# spellings of one plan compare equal) and express it as the marker value:
+# repo-relative when the plan lives under the repo root, absolute otherwise.
+plan_dir=$(CDPATH= cd -- "$(dirname "$plan")" && pwd -P)
+plan_abs="$plan_dir/$(basename "$plan")"
+case "$plan_abs" in
+  "$root"/*) plan_id=${plan_abs#"$root"/} ;;
+  *)         plan_id=$plan_abs ;;
+esac
+
+# True when the workspace at $1 is (or becomes) this plan's: an existing
+# marker must name this plan; a missing marker means a new workspace or a
+# pre-marker legacy one, and either way the plan claims it by writing one.
+owns() {
+  if [ -e "$1/plan-path" ]; then
+    [ "$(cat "$1/plan-path")" = "$plan_id" ]
+  else
+    mkdir -p "$1"
+    printf '%s\n' "$plan_id" > "$1/plan-path"
+  fi
+}
+
 dir="$base/$slug"
-mkdir -p "$dir"
+if ! owns "$dir"; then
+  parent=$(basename "$plan_dir")
+  dir="$base/$slug-$parent"
+  if ! owns "$dir"; then
+    n=2
+    while ! owns "$base/$slug-$parent-$n"; do n=$((n + 1)); done
+    dir="$base/$slug-$parent-$n"
+  fi
+fi
+
 printf '*\n' > "$base/.gitignore"
-cd "$dir" && pwd
+CDPATH= cd -- "$dir" && pwd

+ 122 - 0
tests/claude-code/test-sdd-workspace.sh

@@ -189,6 +189,128 @@ PLAN
         echo "    status: $wt_status"
     fi
 
+    # --- Ownership markers: two plans with the same basename (#2045) ---
+    mkdir -p "$repo/docs/alpha" "$repo/docs/beta"
+    cat > "$repo/docs/alpha/plan.md" <<'PLAN'
+# Alpha Plan
+
+## Task 1: Alpha work
+
+Alpha-only requirement text.
+PLAN
+    cat > "$repo/docs/beta/plan.md" <<'PLAN'
+# Beta Plan
+
+## Task 1: Beta work
+
+Beta-only requirement text.
+PLAN
+
+    local dir_alpha dir_beta
+    dir_alpha="$(cd "$repo" && "$SDD_SCRIPTS/sdd-workspace" docs/alpha/plan.md)"
+    dir_beta="$(cd "$repo" && "$SDD_SCRIPTS/sdd-workspace" docs/beta/plan.md)"
+    if [[ "$dir_alpha" != "$dir_beta" ]]; then
+        pass "same-basename plans resolve to distinct workspaces"
+    else
+        fail "same-basename plans resolve to distinct workspaces"
+        echo "    alpha: $dir_alpha"
+        echo "    beta:  $dir_beta"
+    fi
+
+    ( cd "$repo" && "$SDD_SCRIPTS/task-brief" docs/alpha/plan.md 1 >/dev/null )
+    ( cd "$repo" && "$SDD_SCRIPTS/task-brief" docs/beta/plan.md 1 >/dev/null )
+    if grep -q "Alpha-only requirement text." "$dir_alpha/task-1-brief.md" 2>/dev/null \
+        && grep -q "Beta-only requirement text." "$dir_beta/task-1-brief.md" 2>/dev/null; then
+        pass "same-basename plans keep both task briefs intact"
+    else
+        fail "same-basename plans keep both task briefs intact"
+        echo "    alpha brief: $(cat "$dir_alpha/task-1-brief.md" 2>/dev/null)"
+        echo "    beta brief:  $(cat "$dir_beta/task-1-brief.md" 2>/dev/null)"
+    fi
+
+    # --- Legacy adoption: pre-existing workspace without a marker ---
+    printf '# Foo\n\n## Task 1: Foo\n\nFoo.\n' > "$repo/foo.md"
+    mkdir -p "$repo/.superpowers/sdd/foo"
+    printf 'ledger\n' > "$repo/.superpowers/sdd/foo/progress.md"
+    local dir_foo
+    dir_foo="$(cd "$repo" && "$SDD_SCRIPTS/sdd-workspace" foo.md)"
+    if [[ "$dir_foo" == "$repo/.superpowers/sdd/foo" \
+        && -f "$dir_foo/progress.md" \
+        && "$(cat "$dir_foo/plan-path" 2>/dev/null)" == "foo.md" ]]; then
+        pass "legacy markerless workspace is adopted in place and marked"
+    else
+        fail "legacy markerless workspace is adopted in place and marked"
+        echo "    dir: $dir_foo"
+        echo "    marker: $(cat "$dir_foo/plan-path" 2>/dev/null)"
+    fi
+
+    # --- Ownership conflict: marker names a different plan ---
+    printf '# Bar\n\n## Task 1: Bar\n\nBar.\n' > "$repo/bar.md"
+    mkdir -p "$repo/.superpowers/sdd/bar"
+    printf 'somewhere-else/bar.md\n' > "$repo/.superpowers/sdd/bar/plan-path"
+    printf 'other ledger\n' > "$repo/.superpowers/sdd/bar/progress.md"
+    local dir_bar
+    dir_bar="$(cd "$repo" && "$SDD_SCRIPTS/sdd-workspace" bar.md)"
+    if [[ "$dir_bar" == "$repo/.superpowers/sdd/bar-repo" \
+        && "$(cat "$dir_bar/plan-path" 2>/dev/null)" == "bar.md" ]]; then
+        pass "owned workspace disambiguates with parent-dir suffix"
+    else
+        fail "owned workspace disambiguates with parent-dir suffix"
+        echo "    got: $dir_bar"
+    fi
+    if [[ "$(cat "$repo/.superpowers/sdd/bar/plan-path")" == "somewhere-else/bar.md" \
+        && "$(cat "$repo/.superpowers/sdd/bar/progress.md")" == "other ledger" ]]; then
+        pass "conflicting plan leaves the original workspace untouched"
+    else
+        fail "conflicting plan leaves the original workspace untouched"
+    fi
+
+    # --- Counter fallback: parent-suffixed workspace is owned too ---
+    printf '# Baz\n\n## Task 1: Baz\n\nBaz.\n' > "$repo/baz.md"
+    mkdir -p "$repo/.superpowers/sdd/baz" "$repo/.superpowers/sdd/baz-repo"
+    printf 'one/baz.md\n' > "$repo/.superpowers/sdd/baz/plan-path"
+    printf 'two/baz.md\n' > "$repo/.superpowers/sdd/baz-repo/plan-path"
+    local dir_baz
+    dir_baz="$(cd "$repo" && "$SDD_SCRIPTS/sdd-workspace" baz.md)"
+    if [[ "$dir_baz" == "$repo/.superpowers/sdd/baz-repo-2" \
+        && "$(cat "$dir_baz/plan-path" 2>/dev/null)" == "baz.md" ]]; then
+        pass "double conflict falls back to a counter suffix"
+    else
+        fail "double conflict falls back to a counter suffix"
+        echo "    got: $dir_baz"
+    fi
+
+    # --- Same plan spelled differently resolves to one workspace ---
+    local dir_rel dir_abs dir_dotdot
+    dir_rel="$(cd "$repo" && "$SDD_SCRIPTS/sdd-workspace" docs/alpha/plan.md)"
+    dir_abs="$(cd "$repo" && "$SDD_SCRIPTS/sdd-workspace" "$repo/docs/alpha/plan.md")"
+    dir_dotdot="$(cd "$repo/docs/beta" && "$SDD_SCRIPTS/sdd-workspace" ../alpha/plan.md)"
+    if [[ "$dir_rel" == "$dir_abs" && "$dir_rel" == "$dir_dotdot" \
+        && "$(cat "$dir_rel/plan-path" 2>/dev/null)" == "docs/alpha/plan.md" ]]; then
+        pass "relative, absolute, and ../ spellings share one workspace and marker"
+    else
+        fail "relative, absolute, and ../ spellings share one workspace and marker"
+        echo "    rel:    $dir_rel"
+        echo "    abs:    $dir_abs"
+        echo "    dotdot: $dir_dotdot"
+        echo "    marker: $(cat "$dir_rel/plan-path" 2>/dev/null)"
+    fi
+
+    # --- Out-of-repo plans keep working, marker holds the absolute path ---
+    mkdir -p "$TEST_ROOT/outside"
+    printf '# Remote\n\n## Task 1: Remote\n\nRemote.\n' > "$TEST_ROOT/outside/remote-plan.md"
+    local outside_abs dir_out
+    outside_abs="$(cd "$TEST_ROOT/outside" && pwd -P)/remote-plan.md"
+    dir_out="$(cd "$repo" && "$SDD_SCRIPTS/sdd-workspace" "$TEST_ROOT/outside/remote-plan.md")"
+    if [[ "$dir_out" == "$repo/.superpowers/sdd/remote-plan" \
+        && "$(cat "$dir_out/plan-path" 2>/dev/null)" == "$outside_abs" ]]; then
+        pass "out-of-repo plan gets a basename slug and an absolute-path marker"
+    else
+        fail "out-of-repo plan gets a basename slug and an absolute-path marker"
+        echo "    dir:    $dir_out"
+        echo "    marker: $(cat "$dir_out/plan-path" 2>/dev/null)"
+    fi
+
     echo ""
     if [[ "$FAILURES" -ne 0 ]]; then
         echo "FAILED: $FAILURES assertion(s)."