From db61f9dab56cb301bc686faec12054ea2e3b9b11 Mon Sep 17 00:00:00 2001 From: Drew Ritter Date: Tue, 5 May 2026 12:38:55 -0700 Subject: [PATCH] fix: remove global worktree path fallback --- .../plans/2026-04-06-worktree-rototill.md | 29 +++----- .../2026-04-06-worktree-rototill-design.md | 15 ++-- .../finishing-a-development-branch/SKILL.md | 4 +- skills/using-git-worktrees/SKILL.md | 21 ++---- tests/claude-code/run-skill-tests.sh | 3 +- tests/claude-code/test-helpers.sh | 8 +-- .../test-subagent-driven-development.sh | 34 +++++---- .../claude-code/test-worktree-path-policy.sh | 69 +++++++++++++++++++ 8 files changed, 117 insertions(+), 66 deletions(-) create mode 100755 tests/claude-code/test-worktree-path-policy.sh diff --git a/docs/superpowers/plans/2026-04-06-worktree-rototill.md b/docs/superpowers/plans/2026-04-06-worktree-rototill.md index ae5acec1..183ee744 100644 --- a/docs/superpowers/plans/2026-04-06-worktree-rototill.md +++ b/docs/superpowers/plans/2026-04-06-worktree-rototill.md @@ -275,23 +275,16 @@ If no native tool is available, create a worktree manually using git. Follow this priority order: -1. **Check existing directories:** +1. **Check your instructions for a worktree directory preference.** If specified, use it without asking. + +2. **Check existing project-local directories:** ```bash ls -d .worktrees 2>/dev/null # Preferred (hidden) ls -d worktrees 2>/dev/null # Alternative ``` If found, use that directory. If both exist, `.worktrees` wins. -2. **Check for existing global directory:** - ```bash - project=$(basename "$(git rev-parse --show-toplevel)") - ls -d ~/.config/superpowers/worktrees/$project 2>/dev/null - ``` - If found, use it (backward compatibility with legacy global path). - -3. **Check your instructions for a worktree directory preference.** If specified, use it without asking. - -4. **Default to `.worktrees/`.** +3. **Default to `.worktrees/`.** #### Safety Verification (project-local directories only) @@ -305,16 +298,11 @@ git check-ignore -q .worktrees 2>/dev/null || git check-ignore -q worktrees 2>/d **Why critical:** Prevents accidentally committing worktree contents to repository. -Global directories (`~/.config/superpowers/worktrees/`) need no verification. - #### Create the Worktree ```bash -project=$(basename "$(git rev-parse --show-toplevel)") - # Determine path based on chosen location -# For project-local: path="$LOCATION/$BRANCH_NAME" -# For global: path="~/.config/superpowers/worktrees/$project/$BRANCH_NAME" +path="$LOCATION/$BRANCH_NAME" git worktree add "$path" -b "$BRANCH_NAME" cd "$path" @@ -387,7 +375,6 @@ Ready to implement | `worktrees/` exists | Use it (verify ignored) | | Both exist | Use `.worktrees/` | | Neither exists | Check instruction file, then default `.worktrees/` | -| Global path exists | Use it (backward compat) | | Directory not ignored | Add to .gitignore + commit | | Permission error on create | Sandbox fallback, work in place | | Tests fail during baseline | Report failures + ask | @@ -464,7 +451,7 @@ git commit -m "feat: rewrite using-git-worktrees with detect-and-defer (PRI-974) Step 0: GIT_DIR != GIT_COMMON detection (skip if already isolated) Step 0 consent: opt-in prompt before creating worktree (#991) Step 1a: native tool preference (short, first, declarative) -Step 1b: git worktree fallback with hooks symlink and legacy path compat +Step 1b: git worktree fallback with project-local directory policy Submodule guard prevents false detection Platform-neutral instruction file references (#1049)" ``` @@ -663,7 +650,7 @@ WORKTREE_PATH=$(git rev-parse --show-toplevel) **If `GIT_DIR == GIT_COMMON`:** Normal repo, no worktree to clean up. Done. -**If worktree path is under `.worktrees/` or `~/.config/superpowers/worktrees/`:** Superpowers created this worktree — we own cleanup. +**If worktree path is under `.worktrees/` or `worktrees/`:** Superpowers created this worktree — we own cleanup. ```bash MAIN_ROOT=$(git -C "$(git rev-parse --git-common-dir)/.." rev-parse --show-toplevel) @@ -707,7 +694,7 @@ git worktree prune # Self-healing: clean up any stale registrations **Cleaning up harness-owned worktrees** - **Problem:** Removing a worktree the harness created causes phantom state -- **Fix:** Only clean up worktrees under `.worktrees/` or `~/.config/superpowers/worktrees/` +- **Fix:** Only clean up worktrees under `.worktrees/` or `worktrees/` **No confirmation for discard** - **Problem:** Accidentally delete work diff --git a/docs/superpowers/specs/2026-04-06-worktree-rototill-design.md b/docs/superpowers/specs/2026-04-06-worktree-rototill-design.md index a26e7886..11c854e7 100644 --- a/docs/superpowers/specs/2026-04-06-worktree-rototill-design.md +++ b/docs/superpowers/specs/2026-04-06-worktree-rototill-design.md @@ -46,7 +46,7 @@ The skill describes the goal ("ensure work happens in an isolated workspace") an ### Provenance-based ownership -Whoever creates the worktree owns its cleanup. If the harness created it, superpowers doesn't touch it. If superpowers created it (via git fallback), superpowers cleans it up. The heuristic: if the worktree lives under `.worktrees/` or `~/.config/superpowers/worktrees/`, superpowers owns it. Anything else (`.claude/worktrees/`, `~/.codex/worktrees/`, `.gemini/worktrees/`) belongs to the harness. +Whoever creates the worktree owns its cleanup. If the harness created it, superpowers doesn't touch it. If superpowers created it (via git fallback), superpowers cleans it up. The heuristic: if the worktree lives under `.worktrees/` or `worktrees/`, superpowers owns it. Anything else (`.claude/worktrees/`, `~/.codex/worktrees/`, `.gemini/worktrees/`, or old user-global Superpowers paths) belongs to the harness or user and is left alone. ## Design @@ -110,12 +110,11 @@ File splitting (Step 1b in a separate skill) was tested and proven unnecessary. When no native tool is available, create a worktree manually. **Directory selection** (priority order): -1. Check for existing `.worktrees/` or `worktrees/` directory — if found, use it. If both exist, `.worktrees/` wins. -2. Check for existing `~/.config/superpowers/worktrees//` directory — if found, use it (backward compatibility with legacy global path). -3. Check the project's agent instruction file (CLAUDE.md, GEMINI.md, AGENTS.md, .cursorrules, or equivalent) for a worktree directory preference. -4. Default to `.worktrees/`. +1. Check the project's agent instruction file (CLAUDE.md, GEMINI.md, AGENTS.md, .cursorrules, or equivalent) for a worktree directory preference. +2. Check for existing `.worktrees/` or `worktrees/` directory — if found, use it. If both exist, `.worktrees/` wins. +3. Default to `.worktrees/`. -No interactive directory selection prompt. The global path (`~/.config/superpowers/worktrees/`) is no longer offered as a choice to new users, but existing worktrees at that location are detected and used for backward compatibility. +No interactive directory selection prompt. Old user-global Superpowers worktree paths are not detected or offered; new manual worktrees are project-local unless the user explicitly specifies another location. **Safety verification** (project-local directories only): @@ -232,7 +231,7 @@ if GIT_DIR == GIT_COMMON: # Normal repo, no worktree to clean up done -if worktree path is under .worktrees/ or ~/.config/superpowers/worktrees/: +if worktree path is under .worktrees/ or worktrees/: # Superpowers created it — we own cleanup cd to main repo root # Bug #238 fix git worktree remove @@ -318,7 +317,7 @@ As of 2026-04-06, Claude Code is the only harness with an agent-callable mid-ses ### Provenance heuristic -The `.worktrees/` or `~/.config/superpowers/worktrees/` = ours, anything else = hands off` heuristic works for every current harness. If a future harness adopts `.worktrees/` as its convention, we'd have a false positive (superpowers tries to clean up a harness-owned worktree). Similarly, if a user manually runs `git worktree add .worktrees/experiment` without superpowers, we'd incorrectly claim ownership. Both are low risk — every harness uses branded paths, and manual `.worktrees/` creation is unlikely — but worth noting. +The `.worktrees/` or `worktrees/` = ours, anything else = hands off` heuristic works for every current harness. If a future harness adopts one of those project-local directories as its convention, we'd have a false positive (superpowers tries to clean up a harness-owned worktree). Similarly, if a user manually runs `git worktree add .worktrees/experiment` without superpowers, we'd incorrectly claim ownership. Both are low risk — every harness uses branded paths, and manual `.worktrees/` creation is unlikely — but worth noting. ### Detached HEAD finishing diff --git a/skills/finishing-a-development-branch/SKILL.md b/skills/finishing-a-development-branch/SKILL.md index 43da0ae1..bb38facb 100644 --- a/skills/finishing-a-development-branch/SKILL.md +++ b/skills/finishing-a-development-branch/SKILL.md @@ -180,7 +180,7 @@ WORKTREE_PATH=$(git rev-parse --show-toplevel) **If `GIT_DIR == GIT_COMMON`:** Normal repo, no worktree to clean up. Done. -**If worktree path is under `.worktrees/`, `worktrees/`, or `~/.config/superpowers/worktrees/`:** Superpowers created this worktree — we own cleanup. +**If worktree path is under `.worktrees/` or `worktrees/`:** Superpowers created this worktree — we own cleanup. ```bash MAIN_ROOT=$(git -C "$(git rev-parse --git-common-dir)/.." rev-parse --show-toplevel) @@ -224,7 +224,7 @@ git worktree prune # Self-healing: clean up any stale registrations **Cleaning up harness-owned worktrees** - **Problem:** Removing a worktree the harness created causes phantom state -- **Fix:** Only clean up worktrees under `.worktrees/`, `worktrees/`, or `~/.config/superpowers/worktrees/` +- **Fix:** Only clean up worktrees under `.worktrees/` or `worktrees/` **No confirmation for discard** - **Problem:** Accidentally delete work diff --git a/skills/using-git-worktrees/SKILL.md b/skills/using-git-worktrees/SKILL.md index 134d3714..dae1e3d6 100644 --- a/skills/using-git-worktrees/SKILL.md +++ b/skills/using-git-worktrees/SKILL.md @@ -73,14 +73,7 @@ Follow this priority order. Explicit user preference always beats observed files ``` If found, use it. If both exist, `.worktrees` wins. -3. **Check for an existing global directory:** - ```bash - project=$(basename "$(git rev-parse --show-toplevel)") - ls -d ~/.config/superpowers/worktrees/$project 2>/dev/null - ``` - If found, use it (backward compatibility with legacy global path). - -4. **If there is no other guidance available**, default to `.worktrees/` at the project root. +3. **If there is no other guidance available**, default to `.worktrees/` at the project root. #### Safety Verification (project-local directories only) @@ -94,16 +87,11 @@ git check-ignore -q .worktrees 2>/dev/null || git check-ignore -q worktrees 2>/d **Why critical:** Prevents accidentally committing worktree contents to repository. -Global directories (`~/.config/superpowers/worktrees/`) need no verification. - #### Create the Worktree ```bash -project=$(basename "$(git rev-parse --show-toplevel)") - # Determine path based on chosen location -# For project-local: path="$LOCATION/$BRANCH_NAME" -# For global: path="~/.config/superpowers/worktrees/$project/$BRANCH_NAME" +path="$LOCATION/$BRANCH_NAME" git worktree add "$path" -b "$BRANCH_NAME" cd "$path" @@ -163,7 +151,6 @@ Ready to implement | `worktrees/` exists | Use it (verify ignored) | | Both exist | Use `.worktrees/` | | Neither exists | Check instruction file, then default `.worktrees/` | -| Global path exists | Use it (backward compat) | | Directory not ignored | Add to .gitignore + commit | | Permission error on create | Sandbox fallback, work in place | | Tests fail during baseline | Report failures + ask | @@ -189,7 +176,7 @@ Ready to implement ### Assuming directory location - **Problem:** Creates inconsistency, violates project conventions -- **Fix:** Follow priority: existing > global legacy > instruction file > default +- **Fix:** Follow priority: explicit instructions > existing project-local directory > default ### Proceeding with failing tests @@ -209,7 +196,7 @@ Ready to implement **Always:** - Run Step 0 detection first - Prefer native tools over git fallback -- Follow directory priority: existing > global legacy > instruction file > default +- Follow directory priority: explicit instructions > existing project-local directory > default - Verify directory is ignored for project-local - Auto-detect and run project setup - Verify clean test baseline diff --git a/tests/claude-code/run-skill-tests.sh b/tests/claude-code/run-skill-tests.sh index 023e9794..cee4fd94 100755 --- a/tests/claude-code/run-skill-tests.sh +++ b/tests/claude-code/run-skill-tests.sh @@ -25,7 +25,7 @@ fi # Parse command line arguments VERBOSE=false SPECIFIC_TEST="" -TIMEOUT=300 # Default 5 minute timeout per test +TIMEOUT=600 # Default 10 minute timeout per test RUN_INTEGRATION=false while [[ $# -gt 0 ]]; do @@ -73,6 +73,7 @@ done # List of skill tests to run (fast unit tests) tests=( + "test-worktree-path-policy.sh" "test-subagent-driven-development.sh" ) diff --git a/tests/claude-code/test-helpers.sh b/tests/claude-code/test-helpers.sh index f83a7d22..1b5ead3b 100755 --- a/tests/claude-code/test-helpers.sh +++ b/tests/claude-code/test-helpers.sh @@ -9,14 +9,14 @@ run_claude() { local allowed_tools="${3:-}" local output_file=$(mktemp) - # Build command - local cmd="claude -p \"$prompt\"" + # Build command as an argv array so timeout wraps claude directly. + local cmd=(claude -p "$prompt") if [ -n "$allowed_tools" ]; then - cmd="$cmd --allowed-tools=$allowed_tools" + cmd+=(--allowed-tools="$allowed_tools") fi # Run Claude in headless mode with timeout - if timeout "$timeout" bash -c "$cmd" > "$output_file" 2>&1; then + if timeout "$timeout" "${cmd[@]}" > "$output_file" 2>&1; then cat "$output_file" rm -f "$output_file" return 0 diff --git a/tests/claude-code/test-subagent-driven-development.sh b/tests/claude-code/test-subagent-driven-development.sh index 20d8d4c7..93b11187 100755 --- a/tests/claude-code/test-subagent-driven-development.sh +++ b/tests/claude-code/test-subagent-driven-development.sh @@ -6,13 +6,15 @@ set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" source "$SCRIPT_DIR/test-helpers.sh" +CLAUDE_PROMPT_TIMEOUT="${CLAUDE_PROMPT_TIMEOUT:-90}" + echo "=== Test: subagent-driven-development skill ===" echo "" # Test 1: Verify skill can be loaded echo "Test 1: Skill loading..." -output=$(run_claude "What is the subagent-driven-development skill? Describe its key steps briefly." 30) +output=$(run_claude "What is the subagent-driven-development skill? Describe its key steps briefly." "$CLAUDE_PROMPT_TIMEOUT") if assert_contains "$output" "subagent-driven-development\|Subagent-Driven Development\|Subagent Driven" "Skill is recognized"; then : # pass @@ -31,9 +33,11 @@ echo "" # Test 2: Verify skill describes correct workflow order echo "Test 2: Workflow ordering..." -output=$(run_claude "In the subagent-driven-development skill, what comes first: spec compliance review or code quality review? Be specific about the order." 30) +output=$(run_claude "In the subagent-driven-development skill, what comes first: spec compliance review or code quality review? Answer using exactly this structure: +First: +Second: " "$CLAUDE_PROMPT_TIMEOUT") -if assert_order "$output" "spec.*compliance" "code.*quality" "Spec compliance before code quality"; then +if assert_order "$output" "First:.*spec.*compliance" "Second:.*code.*quality" "Spec compliance before code quality"; then : # pass else exit 1 @@ -44,15 +48,17 @@ echo "" # Test 3: Verify self-review is mentioned echo "Test 3: Self-review requirement..." -output=$(run_claude "Does the subagent-driven-development skill require implementers to do self-review? What should they check?" 30) +output=$(run_claude "Does the subagent-driven-development skill require implementers to self-review before handoff, and can self-review replace the external reviews? Answer using exactly this structure: +Self-review required: +Self-review replaces external review: " "$CLAUDE_PROMPT_TIMEOUT") -if assert_contains "$output" "self-review\|self review" "Mentions self-review"; then +if assert_contains "$output" "Self-review required:.*yes" "Mentions self-review"; then : # pass else exit 1 fi -if assert_contains "$output" "completeness\|Completeness" "Checks completeness"; then +if assert_contains "$output" "Self-review replaces external review:.*no" "Self-review does not replace external review"; then : # pass else exit 1 @@ -63,7 +69,7 @@ echo "" # Test 4: Verify plan is read once echo "Test 4: Plan reading efficiency..." -output=$(run_claude "In subagent-driven-development, how many times should the controller read the plan file? When does this happen?" 30) +output=$(run_claude "In subagent-driven-development, how many times should the controller read the plan file? When does this happen?" "$CLAUDE_PROMPT_TIMEOUT") if assert_contains "$output" "once\|one time\|single" "Read plan once"; then : # pass @@ -82,7 +88,7 @@ echo "" # Test 5: Verify spec compliance reviewer is skeptical echo "Test 5: Spec compliance reviewer mindset..." -output=$(run_claude "What is the spec compliance reviewer's attitude toward the implementer's report in subagent-driven-development?" 30) +output=$(run_claude "What is the spec compliance reviewer's attitude toward the implementer's report in subagent-driven-development?" "$CLAUDE_PROMPT_TIMEOUT") if assert_contains "$output" "not trust\|don't trust\|skeptical\|verify.*independently\|suspiciously" "Reviewer is skeptical"; then : # pass @@ -101,7 +107,7 @@ echo "" # Test 6: Verify review loops echo "Test 6: Review loop requirements..." -output=$(run_claude "In subagent-driven-development, what happens if a reviewer finds issues? Is it a one-time review or a loop?" 30) +output=$(run_claude "In subagent-driven-development, what happens if a reviewer finds issues? Is it a one-time review or a loop?" "$CLAUDE_PROMPT_TIMEOUT") if assert_contains "$output" "loop\|again\|repeat\|until.*approved\|until.*compliant" "Review loops mentioned"; then : # pass @@ -120,7 +126,9 @@ echo "" # Test 7: Verify full task text is provided echo "Test 7: Task context provision..." -output=$(run_claude "In subagent-driven-development, how does the controller provide task information to the implementer subagent? Does it make them read a file or provide it directly?" 30) +output=$(run_claude "In subagent-driven-development, how does the controller provide task information to the implementer subagent? Answer using exactly this structure: +Controller provides: +Implementer must read plan file: " "$CLAUDE_PROMPT_TIMEOUT") if assert_contains "$output" "provide.*directly\|full.*text\|paste\|include.*prompt" "Provides text directly"; then : # pass @@ -128,7 +136,7 @@ else exit 1 fi -if assert_not_contains "$output" "read.*file\|open.*file" "Doesn't make subagent read file"; then +if assert_contains "$output" "Implementer must read plan file:.*no" "Doesn't make subagent read file"; then : # pass else exit 1 @@ -139,7 +147,7 @@ echo "" # Test 8: Verify worktree requirement echo "Test 8: Worktree requirement..." -output=$(run_claude "What workflow skills are required before using subagent-driven-development? List any prerequisites or required skills." 30) +output=$(run_claude "What workflow skills are required before using subagent-driven-development? List any prerequisites or required skills." "$CLAUDE_PROMPT_TIMEOUT") if assert_contains "$output" "using-git-worktrees\|worktree" "Mentions worktree requirement"; then : # pass @@ -152,7 +160,7 @@ echo "" # Test 9: Verify main branch warning echo "Test 9: Main branch red flag..." -output=$(run_claude "In subagent-driven-development, is it okay to start implementation directly on the main branch?" 30) +output=$(run_claude "In subagent-driven-development, is it okay to start implementation directly on the main branch?" "$CLAUDE_PROMPT_TIMEOUT") if assert_contains "$output" "worktree\|feature.*branch\|not.*main\|never.*main\|avoid.*main\|don't.*main\|consent\|permission" "Warns against main branch"; then : # pass diff --git a/tests/claude-code/test-worktree-path-policy.sh b/tests/claude-code/test-worktree-path-policy.sh new file mode 100755 index 00000000..58caad7a --- /dev/null +++ b/tests/claude-code/test-worktree-path-policy.sh @@ -0,0 +1,69 @@ +#!/usr/bin/env bash +# Regression check: Superpowers should not route new worktrees through the old +# global worktree directory. + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +REPO_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)" + +USING_SKILL="$REPO_ROOT/skills/using-git-worktrees/SKILL.md" +FINISHING_SKILL="$REPO_ROOT/skills/finishing-a-development-branch/SKILL.md" +ROTOTILL_SPEC="$REPO_ROOT/docs/superpowers/specs/2026-04-06-worktree-rototill-design.md" +ROTOTILL_PLAN="$REPO_ROOT/docs/superpowers/plans/2026-04-06-worktree-rototill.md" + +failures=0 + +assert_contains() { + local file="$1" + local pattern="$2" + local label="$3" + + if grep -Fq "$pattern" "$file"; then + echo " [PASS] $label" + else + echo " [FAIL] $label" + echo " Expected to find: $pattern" + echo " In file: $file" + failures=$((failures + 1)) + fi +} + +assert_not_contains() { + local file="$1" + local pattern="$2" + local label="$3" + + if grep -Fq "$pattern" "$file"; then + echo " [FAIL] $label" + echo " Did not expect to find: $pattern" + echo " In file: $file" + failures=$((failures + 1)) + else + echo " [PASS] $label" + fi +} + +echo "=== Worktree Path Policy Test ===" +echo "" + +assert_not_contains "$USING_SKILL" "~/.config/superpowers/worktrees" "using-git-worktrees does not mention old global path" +assert_not_contains "$USING_SKILL" "global legacy" "using-git-worktrees does not use unclear global legacy shorthand" +assert_not_contains "$USING_SKILL" "Global path" "using-git-worktrees has no global path quick-reference row" +assert_contains "$USING_SKILL" 'default to `.worktrees/` at the project root' "using-git-worktrees defaults new manual worktrees to .worktrees/" + +assert_not_contains "$FINISHING_SKILL" "~/.config/superpowers/worktrees" "finishing-a-development-branch does not treat old global path as owned" +assert_contains "$FINISHING_SKILL" '`.worktrees/` or `worktrees/`' "finishing-a-development-branch keeps project-local cleanup ownership" + +assert_not_contains "$ROTOTILL_SPEC" "~/.config/superpowers/worktrees" "rototill spec does not preserve old global path policy" +assert_not_contains "$ROTOTILL_PLAN" "~/.config/superpowers/worktrees" "rototill plan does not preserve old global path policy" +assert_not_contains "$ROTOTILL_PLAN" "legacy path compat" "rototill plan does not advertise legacy path compatibility" + +echo "" + +if [ "$failures" -gt 0 ]; then + echo "STATUS: FAILED ($failures failures)" + exit 1 +fi + +echo "STATUS: PASSED"