From 5f419c0238a7ff5ffd795c0e6425e260e5ec2c77 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 20 Apr 2026 10:10:16 -0400 Subject: [PATCH] fix(bugs): resolve issues #2388, #2431, #2396, #2376 (#2467) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #2388 (plan-phase silently renames feature branch): add explicit Git Branch Invariant section to plan-phase.md prohibiting branch creation/rename/switch during planning; phase slug changes are plan-level only and must not affect the git branch. #2431 (worktree teardown silently swallows errors): replace `git worktree remove --force 2>/dev/null || true` with a lock-aware block in quick.md and execute-phase.md that detects locked worktrees, attempts unlock+retry, and surfaces a user-visible recovery message when removal still fails. #2396 (hardcoded test commands bypass Makefile): add a three-tier test command resolver (project config → Makefile/Justfile → language sniff) in execute-phase.md, verify-phase.md, and audit-fix.md. Makefile with a `test:` target now takes priority over npm/cargo/go. #2376 (OpenCode @$HOME not mapped on Windows): add platform guard in bin/install.js so OpenCode on win32 uses the absolute path instead of `$HOME/...`, which OpenCode does not expand in @file references on Windows. Tests: 29 new assertions across 4 regression test files (all passing). Co-authored-by: Claude Sonnet 4.6 --- bin/install.js | 5 +- get-shit-done/workflows/audit-fix.md | 20 ++- get-shit-done/workflows/execute-phase.md | 78 ++++++++---- get-shit-done/workflows/plan-phase.md | 4 + get-shit-done/workflows/quick.md | 14 ++- get-shit-done/workflows/verify-phase.md | 35 +++--- ...g-2376-opencode-windows-home-path.test.cjs | 118 ++++++++++++++++++ ...-2388-plan-phase-no-branch-rename.test.cjs | 61 +++++++++ .../bug-2396-makefile-test-priority.test.cjs | 112 +++++++++++++++++ ...ug-2431-worktree-locked-surfacing.test.cjs | 98 +++++++++++++++ 10 files changed, 504 insertions(+), 41 deletions(-) create mode 100644 tests/bug-2376-opencode-windows-home-path.test.cjs create mode 100644 tests/bug-2388-plan-phase-no-branch-rename.test.cjs create mode 100644 tests/bug-2396-makefile-test-priority.test.cjs create mode 100644 tests/bug-2431-worktree-locked-surfacing.test.cjs diff --git a/bin/install.js b/bin/install.js index 2ae0466f..0df408e8 100755 --- a/bin/install.js +++ b/bin/install.js @@ -5465,9 +5465,12 @@ function install(isGlobal, runtime = 'claude') { // For global installs: use $HOME/ so paths expand correctly inside double-quoted // shell commands (~ does NOT expand inside double quotes, causing MODULE_NOT_FOUND). // For local installs: use resolved absolute path (may be outside $HOME). + // Exception: OpenCode on Windows does not expand $HOME in @file references — + // use the absolute path instead so @$HOME/... references resolve correctly (#2376). const resolvedTarget = path.resolve(targetDir).replace(/\\/g, '/'); const homeDir = os.homedir().replace(/\\/g, '/'); - const pathPrefix = isGlobal && resolvedTarget.startsWith(homeDir) + const isWindowsHost = process.platform === 'win32'; + const pathPrefix = isGlobal && resolvedTarget.startsWith(homeDir) && !(isOpencode && isWindowsHost) ? '$HOME' + resolvedTarget.slice(homeDir.length) + '/' : `${resolvedTarget}/`; diff --git a/get-shit-done/workflows/audit-fix.md b/get-shit-done/workflows/audit-fix.md index 7f7c0074..6a3e3cf6 100644 --- a/get-shit-done/workflows/audit-fix.md +++ b/get-shit-done/workflows/audit-fix.md @@ -103,7 +103,25 @@ Task( **b. Run tests:** ```bash -npm test 2>&1 | tail -20 +AUDIT_TEST_CMD=$(gsd-sdk query config-get workflow.test_command --default "" 2>/dev/null || true) +if [ -z "$AUDIT_TEST_CMD" ]; then + if [ -f "Makefile" ] && grep -q "^test:" Makefile; then + AUDIT_TEST_CMD="make test" + elif [ -f "Justfile" ] || [ -f "justfile" ]; then + AUDIT_TEST_CMD="just test" + elif [ -f "package.json" ]; then + AUDIT_TEST_CMD="npm test" + elif [ -f "Cargo.toml" ]; then + AUDIT_TEST_CMD="cargo test" + elif [ -f "go.mod" ]; then + AUDIT_TEST_CMD="go test ./..." + elif [ -f "pyproject.toml" ] || [ -f "requirements.txt" ]; then + AUDIT_TEST_CMD="python -m pytest -x -q --tb=short" + else + AUDIT_TEST_CMD="true" + fi +fi +eval "$AUDIT_TEST_CMD" 2>&1 | tail -20 ``` **c. If tests pass** — commit atomically: diff --git a/get-shit-done/workflows/execute-phase.md b/get-shit-done/workflows/execute-phase.md index b89a5e14..2d602e3a 100644 --- a/get-shit-done/workflows/execute-phase.md +++ b/get-shit-done/workflows/execute-phase.md @@ -665,7 +665,19 @@ Execute each selected wave in sequence. Within a wave: parallel if `PARALLELIZAT fi # Remove the worktree - git worktree remove "$WT" --force 2>/dev/null || true + if ! git worktree remove "$WT" --force; then + WT_NAME=$(basename "$WT") + if [ -f ".git/worktrees/${WT_NAME}/locked" ]; then + echo "⚠ Worktree $WT is locked — attempting to unlock and retry" + git worktree unlock "$WT" 2>/dev/null || true + if ! git worktree remove "$WT" --force; then + echo "⚠ Residual worktree at $WT — manual cleanup required after session exits:" + echo " git worktree unlock \"$WT\" && git worktree remove \"$WT\" --force && git branch -D \"$WT_BRANCH\"" + fi + else + echo "⚠ Residual worktree at $WT (remove failed) — investigate manually" + fi + fi # Delete the temporary branch git branch -D "$WT_BRANCH" 2>/dev/null || true @@ -688,22 +700,29 @@ Execute each selected wave in sequence. Within a wave: parallel if `PARALLELIZAT merging their work creates failures. ```bash + # Resolve test command: project config > Makefile > language sniff + TEST_CMD=$(gsd-sdk query config-get workflow.test_command --default "" 2>/dev/null || true) + if [ -z "$TEST_CMD" ]; then + if [ -f "Makefile" ] && grep -q "^test:" Makefile; then + TEST_CMD="make test" + elif [ -f "Justfile" ] || [ -f "justfile" ]; then + TEST_CMD="just test" + elif [ -f "package.json" ]; then + TEST_CMD="npm test" + elif [ -f "Cargo.toml" ]; then + TEST_CMD="cargo test" + elif [ -f "go.mod" ]; then + TEST_CMD="go test ./..." + elif [ -f "pyproject.toml" ] || [ -f "requirements.txt" ]; then + TEST_CMD="python -m pytest -x -q --tb=short 2>&1 || uv run python -m pytest -x -q --tb=short" + else + TEST_CMD="true" + echo "⚠ No test runner detected — skipping post-merge test gate" + fi + fi # Detect test runner and run quick smoke test (timeout: 5 minutes) TEST_EXIT=0 - timeout 300 bash -c ' - if [ -f "package.json" ]; then - npm test 2>&1 - elif [ -f "Cargo.toml" ]; then - cargo test 2>&1 - elif [ -f "go.mod" ]; then - go test ./... 2>&1 - elif [ -f "pyproject.toml" ] || [ -f "requirements.txt" ]; then - python -m pytest -x -q --tb=short 2>&1 || uv run python -m pytest -x -q --tb=short 2>&1 - else - echo "⚠ No test runner detected — skipping post-merge test gate" - exit 0 - fi - ' + timeout 300 bash -c "$TEST_CMD" 2>&1 TEST_EXIT=$? if [ "${TEST_EXIT}" -eq 0 ]; then echo "✓ Post-merge test gate passed — no cross-plan conflicts" @@ -1111,16 +1130,27 @@ Collect all unique test file paths into `REGRESSION_FILES`. **Step 3: Run regression tests (if any found)** ```bash -# Detect test runner and run prior phase tests -if [ -f "package.json" ]; then - npm test 2>&1 -elif [ -f "Cargo.toml" ]; then - cargo test 2>&1 -elif [ -f "go.mod" ]; then - go test ./... 2>&1 -elif [ -f "requirements.txt" ] || [ -f "pyproject.toml" ]; then - python -m pytest ${REGRESSION_FILES} -q --tb=short 2>&1 +# Resolve test command: project config > Makefile > language sniff +REG_TEST_CMD=$(gsd-sdk query config-get workflow.test_command --default "" 2>/dev/null || true) +if [ -z "$REG_TEST_CMD" ]; then + if [ -f "Makefile" ] && grep -q "^test:" Makefile; then + REG_TEST_CMD="make test" + elif [ -f "Justfile" ] || [ -f "justfile" ]; then + REG_TEST_CMD="just test" + elif [ -f "package.json" ]; then + REG_TEST_CMD="npm test" + elif [ -f "Cargo.toml" ]; then + REG_TEST_CMD="cargo test" + elif [ -f "go.mod" ]; then + REG_TEST_CMD="go test ./..." + elif [ -f "requirements.txt" ] || [ -f "pyproject.toml" ]; then + REG_TEST_CMD="python -m pytest ${REGRESSION_FILES} -q --tb=short" + else + REG_TEST_CMD="true" + fi fi +# Detect test runner and run prior phase tests +eval "$REG_TEST_CMD" 2>&1 ``` **Step 4: Report results** diff --git a/get-shit-done/workflows/plan-phase.md b/get-shit-done/workflows/plan-phase.md index 8346f961..8866699c 100644 --- a/get-shit-done/workflows/plan-phase.md +++ b/get-shit-done/workflows/plan-phase.md @@ -22,6 +22,10 @@ Valid GSD subagent types (use exact names — do not fall back to 'general-purpo +## 0. Git Branch Invariant + +**Do not create, rename, or switch git branches during plan-phase.** Branch identity is established at discuss-phase and is owned by the user's git workflow. A phase rename in ROADMAP.md is a plan-level change only — it does not mutate git branch names. If `phase_slug` in the init JSON differs from the current branch name, that is expected and correct; leave the branch unchanged. + ## 1. Initialize Load all context in one call (paths only to minimize orchestrator context): diff --git a/get-shit-done/workflows/quick.md b/get-shit-done/workflows/quick.md index 4b83027b..e7acf3c6 100644 --- a/get-shit-done/workflows/quick.md +++ b/get-shit-done/workflows/quick.md @@ -682,7 +682,19 @@ After executor returns: git merge "$WT_BRANCH" --no-edit -m "chore: merge rescued SUMMARY.md from executor worktree ($WT_BRANCH)" 2>/dev/null || true fi - git worktree remove "$WT" --force 2>/dev/null || true + if ! git worktree remove "$WT" --force; then + WT_NAME=$(basename "$WT") + if [ -f ".git/worktrees/${WT_NAME}/locked" ]; then + echo "⚠ Worktree $WT is locked — attempting to unlock and retry" + git worktree unlock "$WT" 2>/dev/null || true + if ! git worktree remove "$WT" --force; then + echo "⚠ Residual worktree at $WT — manual cleanup required after session exits:" + echo " git worktree unlock \"$WT\" && git worktree remove \"$WT\" --force && git branch -D \"$WT_BRANCH\"" + fi + else + echo "⚠ Residual worktree at $WT (remove failed) — investigate manually" + fi + fi git branch -D "$WT_BRANCH" 2>/dev/null || true fi done diff --git a/get-shit-done/workflows/verify-phase.md b/get-shit-done/workflows/verify-phase.md index 91b6744c..7810cfcf 100644 --- a/get-shit-done/workflows/verify-phase.md +++ b/get-shit-done/workflows/verify-phase.md @@ -197,22 +197,29 @@ inspecting static artifacts. **Step 1: Run test suite** ```bash +# Resolve test command: project config > Makefile > language sniff +TEST_CMD=$(gsd-sdk query config-get workflow.test_command --default "" 2>/dev/null || true) +if [ -z "$TEST_CMD" ]; then + if [ -f "Makefile" ] && grep -q "^test:" Makefile; then + TEST_CMD="make test" + elif [ -f "Justfile" ] || [ -f "justfile" ]; then + TEST_CMD="just test" + elif [ -f "package.json" ]; then + TEST_CMD="npm test" + elif [ -f "Cargo.toml" ]; then + TEST_CMD="cargo test" + elif [ -f "go.mod" ]; then + TEST_CMD="go test ./..." + elif [ -f "pyproject.toml" ] || [ -f "requirements.txt" ]; then + TEST_CMD="python -m pytest -q --tb=short 2>&1 || uv run python -m pytest -q --tb=short" + else + TEST_CMD="false" + echo "⚠ No test runner detected — skipping test suite" + fi +fi # Detect test runner and run all tests (timeout: 5 minutes) TEST_EXIT=0 -timeout 300 bash -c ' -if [ -f "package.json" ]; then - npm test 2>&1 -elif [ -f "Cargo.toml" ]; then - cargo test 2>&1 -elif [ -f "go.mod" ]; then - go test ./... 2>&1 -elif [ -f "pyproject.toml" ] || [ -f "requirements.txt" ]; then - python -m pytest -q --tb=short 2>&1 || uv run python -m pytest -q --tb=short 2>&1 -else - echo "⚠ No test runner detected — skipping test suite" - exit 1 -fi -' +timeout 300 bash -c "$TEST_CMD" 2>&1 TEST_EXIT=$? if [ "${TEST_EXIT}" -eq 0 ]; then echo "✓ Test suite passed" diff --git a/tests/bug-2376-opencode-windows-home-path.test.cjs b/tests/bug-2376-opencode-windows-home-path.test.cjs new file mode 100644 index 00000000..624ae969 --- /dev/null +++ b/tests/bug-2376-opencode-windows-home-path.test.cjs @@ -0,0 +1,118 @@ +/** + * Regression test for #2376: @$HOME not correctly mapped in OpenCode on Windows. + * + * On Windows, $HOME is not expanded by PowerShell/cmd.exe, so OpenCode cannot + * resolve @$HOME/... file references in installed command files. + * + * Fix: install.js must use the absolute path (not $HOME-relative) when installing + * for OpenCode on Windows. + */ + +'use strict'; + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const INSTALL_JS_PATH = path.join(__dirname, '..', 'bin', 'install.js'); + +describe('bug-2376: OpenCode on Windows must use absolute path, not $HOME', () => { + test('install.js exists', () => { + assert.ok(fs.existsSync(INSTALL_JS_PATH), 'bin/install.js should exist'); + }); + + test('install.js pathPrefix computation skips $HOME for OpenCode on Windows', () => { + const content = fs.readFileSync(INSTALL_JS_PATH, 'utf-8'); + + // The fix must include a Windows detection condition for OpenCode + const hasWindowsOpenCodeGuard = ( + content.includes('isWindowsHost') || + (content.includes("'win32'") && content.includes('isOpencode') && content.includes('pathPrefix')) + ); + assert.ok( + hasWindowsOpenCodeGuard, + 'install.js must include a Windows platform guard for OpenCode pathPrefix computation' + ); + }); + + test('install.js pathPrefix guard excludes $HOME for OpenCode+Windows combination', () => { + const content = fs.readFileSync(INSTALL_JS_PATH, 'utf-8'); + + // The pathPrefix assignment must include a guard that prevents $HOME substitution + // for OpenCode on Windows (process.platform === 'win32'). + const pathPrefixBlock = content.match(/const pathPrefix[\s\S]{0,500}resolvedTarget/); + assert.ok(pathPrefixBlock, 'pathPrefix assignment block should be present'); + + const block = pathPrefixBlock[0]; + const excludesOpenCodeWindows = ( + block.includes('isWindowsHost') || + (block.includes('isOpencode') && block.includes('win32')) + ); + assert.ok( + excludesOpenCodeWindows, + 'pathPrefix computation must exclude $HOME substitution for OpenCode on Windows' + ); + }); + + test('pathPrefix simulation: OpenCode on Windows uses absolute path', () => { + // Simulate the fixed pathPrefix computation for Windows+OpenCode + const isGlobal = true; + const isOpencode = true; + const isWindowsHost = true; // simulated Windows + const resolvedTarget = 'C:/Users/user/.config/opencode'; + const homeDir = 'C:/Users/user'; + + const pathPrefix = isGlobal && resolvedTarget.startsWith(homeDir) && !(isOpencode && isWindowsHost) + ? '$HOME' + resolvedTarget.slice(homeDir.length) + '/' + : `${resolvedTarget}/`; + + assert.strictEqual( + pathPrefix, + 'C:/Users/user/.config/opencode/', + 'OpenCode on Windows should use absolute path, not $HOME-relative' + ); + assert.ok( + !pathPrefix.includes('$HOME'), + 'OpenCode on Windows pathPrefix must not contain $HOME' + ); + }); + + test('pathPrefix simulation: OpenCode on Linux/macOS still uses $HOME', () => { + // Non-Windows OpenCode should still use $HOME (POSIX shells expand it) + const isGlobal = true; + const isOpencode = true; + const isWindowsHost = false; // simulated Linux/macOS + const homeDir = '/home/user'; + const resolvedTarget = '/home/user/.config/opencode'; + + const pathPrefix = isGlobal && resolvedTarget.startsWith(homeDir) && !(isOpencode && isWindowsHost) + ? '$HOME' + resolvedTarget.slice(homeDir.length) + '/' + : `${resolvedTarget}/`; + + assert.strictEqual( + pathPrefix, + '$HOME/.config/opencode/', + 'OpenCode on Linux/macOS should still use $HOME-relative path' + ); + }); + + test('pathPrefix simulation: Claude Code on Windows still uses $HOME (unaffected)', () => { + // Claude Code on Windows is handled by Claude Code's own shell, which expands $HOME + const isGlobal = true; + const isOpencode = false; // Claude Code, not OpenCode + const isWindowsHost = true; + const homeDir = 'C:/Users/user'; + const resolvedTarget = 'C:/Users/user/.claude'; + + const pathPrefix = isGlobal && resolvedTarget.startsWith(homeDir) && !(isOpencode && isWindowsHost) + ? '$HOME' + resolvedTarget.slice(homeDir.length) + '/' + : `${resolvedTarget}/`; + + assert.strictEqual( + pathPrefix, + '$HOME/.claude/', + 'Claude Code on Windows should still use $HOME-relative path (Claude Code handles this)' + ); + }); +}); diff --git a/tests/bug-2388-plan-phase-no-branch-rename.test.cjs b/tests/bug-2388-plan-phase-no-branch-rename.test.cjs new file mode 100644 index 00000000..0ea90021 --- /dev/null +++ b/tests/bug-2388-plan-phase-no-branch-rename.test.cjs @@ -0,0 +1,61 @@ +/** + * Regression test for #2388: plan-phase silently renames feature branch + * when phase slug has changed since the branch was created. + * + * Fix: plan-phase.md must include an explicit instruction not to create, + * rename, or switch git branches during the planning workflow. + */ + +'use strict'; + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const PLAN_PHASE_PATH = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'plan-phase.md'); + +describe('bug-2388: plan-phase must not rename or create git branches', () => { + test('plan-phase.md exists', () => { + assert.ok(fs.existsSync(PLAN_PHASE_PATH), 'plan-phase.md should exist'); + }); + + test('plan-phase.md contains explicit no-branch-rename instruction', () => { + const content = fs.readFileSync(PLAN_PHASE_PATH, 'utf-8'); + // Must say "do not" and mention branch in the context of phase slug/rename + const hasBranchGuard = ( + /do not.{0,80}branch/i.test(content) || + /branch.{0,80}do not/i.test(content) || + /NEVER.{0,80}branch/i.test(content) || + /branch.{0,80}NEVER/i.test(content) + ); + assert.ok( + hasBranchGuard, + 'plan-phase.md must include an explicit instruction not to create or rename git branches' + ); + }); + + test('plan-phase.md mentions phase rename does not affect branch name', () => { + const content = fs.readFileSync(PLAN_PHASE_PATH, 'utf-8'); + // Should explain that a phase rename in ROADMAP.md is plan-level, not git-level + const hasPlanLevelExplanation = ( + content.includes('phase rename') || + content.includes('phase_slug') || + content.includes('branch identity') || + content.includes('branch name') + ); + assert.ok( + hasPlanLevelExplanation, + 'plan-phase.md should clarify that phase slug changes do not change the git branch' + ); + }); + + test('plan-phase.md does not contain git checkout -b instruction', () => { + const content = fs.readFileSync(PLAN_PHASE_PATH, 'utf-8'); + // The workflow should not instruct the LLM to run git checkout -b + assert.ok( + !content.includes('git checkout -b'), + 'plan-phase.md must not instruct LLM to create a new branch via git checkout -b' + ); + }); +}); diff --git a/tests/bug-2396-makefile-test-priority.test.cjs b/tests/bug-2396-makefile-test-priority.test.cjs new file mode 100644 index 00000000..f4755bb4 --- /dev/null +++ b/tests/bug-2396-makefile-test-priority.test.cjs @@ -0,0 +1,112 @@ +/** + * Regression test for #2396: hardcoded host-level test commands bypass + * container-only project Makefiles. + * + * Fix: execute-phase.md, verify-phase.md, and audit-fix.md must check for + * Makefile with a test target (and other wrappers) before falling through + * to hardcoded language-sniffed commands. + */ + +'use strict'; + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const EXECUTE_PHASE_PATH = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'execute-phase.md'); +const VERIFY_PHASE_PATH = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'verify-phase.md'); +const AUDIT_FIX_PATH = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'audit-fix.md'); + +function assertMakefileCheckBeforeNpmTest(filePath, label) { + const content = fs.readFileSync(filePath, 'utf-8'); + + // Must check for Makefile with test target + const hasMakefileCheck = /Makefile.*grep.*test:|grep.*test:.*Makefile/s.test(content) || + (content.includes('Makefile') && content.includes('"^test:"')); + assert.ok( + hasMakefileCheck, + `${label}: must check for Makefile with test: target before falling through to hardcoded commands` + ); + + // make test must appear before npm test in the file + const makeTestIdx = content.indexOf('make test'); + const npmTestIdx = content.indexOf('npm test'); + assert.ok(makeTestIdx !== -1, `${label}: must contain "make test"`); + assert.ok(npmTestIdx !== -1, `${label}: must still contain "npm test" as fallback`); + assert.ok( + makeTestIdx < npmTestIdx, + `${label}: "make test" must appear before "npm test" (Makefile takes priority)` + ); +} + +function assertConfigGetBeforeMakefile(filePath, label) { + const content = fs.readFileSync(filePath, 'utf-8'); + // Must check workflow.test_command config before Makefile sniff. + // Verify within each bash code block: the workflow.test_command lookup + // appears before the Makefile grep in the same block. + assert.ok( + content.includes('workflow.test_command'), + `${label}: must check workflow.test_command config before Makefile/language sniff` + ); + + // Extract bash blocks to check ordering within each block. + // Use the actual Makefile test ([ -f "Makefile" ]) not just the word "Makefile" + // (which appears in comments before the config-get call). + const bashBlockRe = /```bash([\s\S]*?)```/g; + let match; + let anyBlockCorrectlyOrdered = false; + while ((match = bashBlockRe.exec(content)) !== null) { + const block = match[1]; + if (block.includes('workflow.test_command') && block.includes('[ -f "Makefile"')) { + const configIdx = block.indexOf('workflow.test_command'); + const makefileIdx = block.indexOf('[ -f "Makefile"'); + if (configIdx < makefileIdx) { + anyBlockCorrectlyOrdered = true; + break; + } + } + } + assert.ok( + anyBlockCorrectlyOrdered, + `${label}: within a bash block, workflow.test_command config check must appear before Makefile test ([ -f "Makefile" ])` + ); +} + +describe('bug-2396: Makefile test target must take priority over hardcoded commands', () => { + test('execute-phase.md exists', () => { + assert.ok(fs.existsSync(EXECUTE_PHASE_PATH), 'execute-phase.md should exist'); + }); + + test('verify-phase.md exists', () => { + assert.ok(fs.existsSync(VERIFY_PHASE_PATH), 'verify-phase.md should exist'); + }); + + test('audit-fix.md exists', () => { + assert.ok(fs.existsSync(AUDIT_FIX_PATH), 'audit-fix.md should exist'); + }); + + test('execute-phase.md: Makefile check precedes npm test (post-merge gate)', () => { + assertMakefileCheckBeforeNpmTest(EXECUTE_PHASE_PATH, 'execute-phase.md'); + }); + + test('verify-phase.md: Makefile check precedes npm test', () => { + assertMakefileCheckBeforeNpmTest(VERIFY_PHASE_PATH, 'verify-phase.md'); + }); + + test('audit-fix.md: Makefile check precedes npm test', () => { + assertMakefileCheckBeforeNpmTest(AUDIT_FIX_PATH, 'audit-fix.md'); + }); + + test('execute-phase.md: workflow.test_command config checked first (within bash block)', () => { + assertConfigGetBeforeMakefile(EXECUTE_PHASE_PATH, 'execute-phase.md'); + }); + + test('verify-phase.md: workflow.test_command config checked first (within bash block)', () => { + assertConfigGetBeforeMakefile(VERIFY_PHASE_PATH, 'verify-phase.md'); + }); + + test('audit-fix.md: workflow.test_command config checked first (within bash block)', () => { + assertConfigGetBeforeMakefile(AUDIT_FIX_PATH, 'audit-fix.md'); + }); +}); diff --git a/tests/bug-2431-worktree-locked-surfacing.test.cjs b/tests/bug-2431-worktree-locked-surfacing.test.cjs new file mode 100644 index 00000000..c976c033 --- /dev/null +++ b/tests/bug-2431-worktree-locked-surfacing.test.cjs @@ -0,0 +1,98 @@ +/** + * Regression test for #2431: quick.md and execute-phase.md worktree teardown + * silently accumulates locked worktrees via `2>/dev/null || true`. + * + * Fix: replace the silent-fail pattern with a lock-aware block that surfaces + * the error and provides a user-visible recovery message. + */ + +'use strict'; + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const QUICK_PATH = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'quick.md'); +const EXECUTE_PHASE_PATH = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'execute-phase.md'); + +function assertNoSilentWorktreeRemove(filePath, label) { + const content = fs.readFileSync(filePath, 'utf-8'); + // The old pattern: git worktree remove "$WT" --force 2>/dev/null || true + const silentRemovePattern = /git worktree remove[^\n]*--force\s+2>\/dev\/null\s*\|\|\s*true/; + assert.ok( + !silentRemovePattern.test(content), + `${label}: must not contain "git worktree remove --force 2>/dev/null || true" (silently swallows errors)` + ); +} + +function assertHasLockAwareBlock(filePath, label) { + const content = fs.readFileSync(filePath, 'utf-8'); + // Fix must include: lock-aware detection (checking .git/worktrees/*/locked) + const hasLockCheck = content.includes('.git/worktrees/') && content.includes('locked'); + assert.ok( + hasLockCheck, + `${label}: must include lock-aware detection (.git/worktrees/.../locked check)` + ); +} + +function assertHasWorktreeUnlock(filePath, label) { + const content = fs.readFileSync(filePath, 'utf-8'); + // Fix must include a git worktree unlock attempt + assert.ok( + content.includes('git worktree unlock'), + `${label}: must include "git worktree unlock" retry attempt` + ); +} + +function assertHasUserVisibleWarning(filePath, label) { + const content = fs.readFileSync(filePath, 'utf-8'); + // Fix must print a user-visible warning on residual worktree failure + const hasWarning = content.includes('Residual worktree') || content.includes('manual cleanup'); + assert.ok( + hasWarning, + `${label}: must include user-visible warning when worktree removal fails` + ); +} + +describe('bug-2431: worktree teardown must surface locked-worktree errors', () => { + test('quick.md exists', () => { + assert.ok(fs.existsSync(QUICK_PATH), 'quick.md should exist'); + }); + + test('execute-phase.md exists', () => { + assert.ok(fs.existsSync(EXECUTE_PHASE_PATH), 'execute-phase.md should exist'); + }); + + test('quick.md: no silent worktree remove pattern', () => { + assertNoSilentWorktreeRemove(QUICK_PATH, 'quick.md'); + }); + + test('execute-phase.md: no silent worktree remove pattern', () => { + assertNoSilentWorktreeRemove(EXECUTE_PHASE_PATH, 'execute-phase.md'); + }); + + test('quick.md: has lock-aware detection block', () => { + assertHasLockAwareBlock(QUICK_PATH, 'quick.md'); + }); + + test('execute-phase.md: has lock-aware detection block', () => { + assertHasLockAwareBlock(EXECUTE_PHASE_PATH, 'execute-phase.md'); + }); + + test('quick.md: has git worktree unlock retry', () => { + assertHasWorktreeUnlock(QUICK_PATH, 'quick.md'); + }); + + test('execute-phase.md: has git worktree unlock retry', () => { + assertHasWorktreeUnlock(EXECUTE_PHASE_PATH, 'execute-phase.md'); + }); + + test('quick.md: has user-visible warning on residual worktree', () => { + assertHasUserVisibleWarning(QUICK_PATH, 'quick.md'); + }); + + test('execute-phase.md: has user-visible warning on residual worktree', () => { + assertHasUserVisibleWarning(EXECUTE_PHASE_PATH, 'execute-phase.md'); + }); +});