From 570dddd1cd0d57030906a6e600ceff1bc1fd673e Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 10 May 2026 19:48:28 -0400 Subject: [PATCH] fix: make worktree cleanup fail closed (#3385) * fix: make worktree cleanup fail closed * chore: add changeset for worktree cleanup safety * fix: address worktree cleanup review findings * docs: label remove-workspace failure block * fix: initialize worktree manifest before dispatch --- .changeset/fix-3384-worktree-merge-safety.md | 6 + get-shit-done/bin/gsd-tools.cjs | 14 +- get-shit-done/bin/lib/verify.cjs | 9 +- get-shit-done/bin/lib/worktree-safety.cjs | 252 +++++++++++++++++- get-shit-done/workflows/diagnose-issues.md | 2 +- get-shit-done/workflows/execute-phase.md | 94 ++++--- get-shit-done/workflows/quick.md | 45 +++- get-shit-done/workflows/remove-workspace.md | 23 +- .../query/command-static-catalog-domain.ts | 3 + sdk/src/query/worktree.ts | 39 +++ tests/bug-3384-secondary-defects.test.cjs | 59 ++++ ...ug-3384-worktree-cleanup-manifest.test.cjs | 235 ++++++++++++++++ ...cute-phase-step-5-5-deviation-doc.test.cjs | 42 ++- 13 files changed, 750 insertions(+), 73 deletions(-) create mode 100644 .changeset/fix-3384-worktree-merge-safety.md create mode 100644 sdk/src/query/worktree.ts create mode 100644 tests/bug-3384-secondary-defects.test.cjs create mode 100644 tests/bug-3384-worktree-cleanup-manifest.test.cjs diff --git a/.changeset/fix-3384-worktree-merge-safety.md b/.changeset/fix-3384-worktree-merge-safety.md new file mode 100644 index 000000000..b3fe08f7e --- /dev/null +++ b/.changeset/fix-3384-worktree-merge-safety.md @@ -0,0 +1,6 @@ +--- +type: Fixed +pr: 3385 +--- + +**Worktree cleanup now uses a per-wave manifest and fails closed** — `/gsd-execute-phase`, `/gsd-quick`, debug issue diagnosis, and workspace removal no longer broad-scan active agent worktrees or continue after cleanup failures that could lose work. (#3384) diff --git a/get-shit-done/bin/gsd-tools.cjs b/get-shit-done/bin/gsd-tools.cjs index b805dd721..fe1071e02 100755 --- a/get-shit-done/bin/gsd-tools.cjs +++ b/get-shit-done/bin/gsd-tools.cjs @@ -373,7 +373,7 @@ async function main() { 'generate-dev-preferences, generate-slug, graphify, history-digest, init, intel, ' + 'learnings, list-todos, milestone, phase, phase-plan-index, phases, profile-questionnaire, ' + 'profile-sample, progress, requirements, resolve-model, roadmap, scaffold, state, ' + - 'template, validate, verify, verify-path-exists, verify-summary, workstream\n\n' + + 'template, validate, verify, verify-path-exists, verify-summary, workstream, worktree\n\n' + 'Global flags:\n' + ' --raw Emit raw output without post-processing\n' + ' --pick Extract a single field from JSON output (dot/bracket notation)\n' + @@ -418,6 +418,7 @@ async function main() { const SKIP_ROOT_RESOLUTION = new Set([ 'generate-slug', 'current-timestamp', 'verify-path-exists', 'verify-summary', 'template', 'frontmatter', 'detect-custom-files', + 'worktree', ]); if (!SKIP_ROOT_RESOLUTION.has(command)) { cwd = findProjectRoot(cwd); @@ -981,6 +982,17 @@ async function runCommand(command, args, cwd, raw, defaultValue, originalCommand break; } + case 'worktree': { + const subcommand = args[1]; + const worktreeSafety = require('./lib/worktree-safety.cjs'); + if (subcommand === 'cleanup-wave') { + worktreeSafety.cmdWorktreeCleanupWave(cwd, args.slice(2)); + } else { + error('Unknown worktree subcommand. Available: cleanup-wave', ERROR_REASON.SDK_UNKNOWN_COMMAND); + } + break; + } + // ─── Intel ──────────────────────────────────────────────────────────── case 'intel': { diff --git a/get-shit-done/bin/lib/verify.cjs b/get-shit-done/bin/lib/verify.cjs index 762842a8a..5346f120f 100644 --- a/get-shit-done/bin/lib/verify.cjs +++ b/get-shit-done/bin/lib/verify.cjs @@ -921,8 +921,13 @@ function cmdValidateHealth(cwd, options, raw) { 'Worktree health check degraded: git worktree list timed out after 10s — orphan/stale worktrees could not be inspected', 'Run: git worktree list --porcelain to diagnose; check for .git/index.lock or a hung git process'); } - // Other non-ok reasons (not_a_git_repo, git_list_failed) are silent — not - // meaningful for users who have no git repo or whose git is not configured. + if (worktreeHealth.reason === 'git_list_failed') { + addIssue('warning', 'W020', + 'Worktree health check degraded: git worktree list failed — orphan/stale worktrees could not be inspected', + 'Run: git worktree list --porcelain to diagnose; check git repository state and permissions'); + } + // Other non-ok reasons (not_a_git_repo) are silent — not meaningful for + // users who have no git repo. } else { for (const finding of worktreeHealth.findings) { if (finding.kind === 'orphan') { diff --git a/get-shit-done/bin/lib/worktree-safety.cjs b/get-shit-done/bin/lib/worktree-safety.cjs index e818bd3ad..2cd23fe80 100644 --- a/get-shit-done/bin/lib/worktree-safety.cjs +++ b/get-shit-done/bin/lib/worktree-safety.cjs @@ -35,6 +35,11 @@ function execGitDefault(cwd, args, options = {}) { stdio: 'pipe', encoding: 'utf-8', timeout, + env: { + ...process.env, + GIT_TERMINAL_PROMPT: '0', + GCM_INTERACTIVE: 'never', + }, }); // spawnSync sets signal='SIGTERM' and error.code='ETIMEDOUT' when the timeout // fires and the subprocess is killed. @@ -90,9 +95,12 @@ function readWorktreeList(repoRoot, deps = {}) { }; } if (listResult.exitCode !== 0) { + const stderr = String(listResult.stderr || ''); return { ok: false, - reason: 'git_list_failed', + reason: /not a git repository|not a git repo/i.test(stderr) + ? 'not_a_git_repo' + : 'git_list_failed', porcelain: '', entries: [], }; @@ -327,6 +335,244 @@ function snapshotWorktreeInventory(repoRoot, options = {}, deps = {}) { }; } +function normalizeCleanupManifestEntry(entry) { + if (!entry || typeof entry !== 'object') return null; + const worktreePath = typeof entry.worktree_path === 'string' + ? entry.worktree_path + : (typeof entry.path === 'string' ? entry.path : ''); + const branch = typeof entry.branch === 'string' ? entry.branch : ''; + const expectedBase = typeof entry.expected_base === 'string' ? entry.expected_base : ''; + if (!worktreePath || !branch || !expectedBase) return null; + if (!/^worktree-agent-[A-Za-z0-9._/-]+$/.test(branch)) return null; + return { + agent_id: typeof entry.agent_id === 'string' ? entry.agent_id : null, + worktree_path: worktreePath, + branch, + expected_base: expectedBase, + }; +} + +function normalizeCleanupManifest(manifest) { + let parsed = manifest; + if (typeof manifest === 'string') { + try { + parsed = JSON.parse(manifest); + } catch { + return { ok: false, reason: 'invalid_manifest_json', entries: [] }; + } + } + + const rawEntries = Array.isArray(parsed) + ? parsed + : (Array.isArray(parsed?.worktrees) ? parsed.worktrees : []); + const seen = new Set(); + const entries = []; + for (const raw of rawEntries) { + const entry = normalizeCleanupManifestEntry(raw); + if (!entry) continue; + const key = `${entry.worktree_path}\0${entry.branch}`; + if (seen.has(key)) continue; + seen.add(key); + entries.push(entry); + } + + if (entries.length === 0) { + return { ok: false, reason: 'empty_manifest', entries: [] }; + } + + return { ok: true, reason: 'ok', entries }; +} + +function planWorktreeWaveCleanup(repoRoot, manifest) { + const normalized = normalizeCleanupManifest(manifest); + if (!normalized.ok) { + return { + ok: false, + repoRoot, + action: 'skip', + discovery: 'manifest', + reason: normalized.reason, + entries: [], + }; + } + + return { + ok: true, + repoRoot, + action: 'cleanup_wave', + discovery: 'manifest', + reason: 'manifest_entries_present', + entries: normalized.entries, + }; +} + +function gitResultOk(result) { + return result && result.exitCode === 0 && !result.timedOut; +} + +function executeWorktreeWaveCleanupPlan(plan, deps = {}) { + const execGit = deps.execGit || execGitDefault; + const entries = Array.isArray(plan?.entries) ? plan.entries : []; + if (!plan || plan.action !== 'cleanup_wave' || entries.length === 0) { + return { + ok: false, + action: plan ? plan.action : 'skip', + reason: plan ? (plan.reason || 'missing_entries') : 'missing_plan', + entries: [], + pending: entries, + }; + } + + const results = []; + const pending = []; + let ok = true; + + for (let i = 0; i < entries.length; i += 1) { + const entry = entries[i]; + const result = { + ...entry, + status: 'pending', + reason: null, + stderr: '', + }; + + const branchCheck = execGit(plan.repoRoot, ['-C', entry.worktree_path, 'rev-parse', '--abbrev-ref', 'HEAD']); + if (!gitResultOk(branchCheck) || branchCheck.stdout !== entry.branch) { + result.status = 'blocked'; + result.reason = 'branch_mismatch'; + result.stderr = branchCheck?.stderr || ''; + results.push(result); + pending.push(...entries.slice(i + 1)); + ok = false; + break; + } + + const mergeBase = execGit(plan.repoRoot, ['merge-base', 'HEAD', entry.branch]); + if (!gitResultOk(mergeBase) || mergeBase.stdout !== entry.expected_base) { + result.status = 'blocked'; + result.reason = 'base_mismatch'; + result.stderr = mergeBase?.stderr || ''; + results.push(result); + pending.push(...entries.slice(i + 1)); + ok = false; + break; + } + + const deletions = execGit(plan.repoRoot, ['diff', '--diff-filter=D', '--name-only', `HEAD...${entry.branch}`]); + if (!gitResultOk(deletions)) { + result.status = 'blocked'; + result.reason = 'deletion_check_failed'; + result.stderr = deletions?.stderr || ''; + results.push(result); + pending.push(...entries.slice(i + 1)); + ok = false; + break; + } + if (deletions.stdout) { + result.status = 'blocked'; + result.reason = 'branch_contains_deletions'; + result.stderr = deletions.stdout; + results.push(result); + pending.push(...entries.slice(i + 1)); + ok = false; + break; + } + + const worktreeStatus = execGit(plan.repoRoot, ['-C', entry.worktree_path, 'status', '--porcelain', '--untracked-files=all']); + if (!gitResultOk(worktreeStatus) || worktreeStatus.stdout) { + result.status = 'blocked'; + result.reason = 'worktree_dirty'; + result.stderr = worktreeStatus?.stdout || worktreeStatus?.stderr || ''; + results.push(result); + pending.push(...entries.slice(i + 1)); + ok = false; + break; + } + + const merge = execGit(plan.repoRoot, ['merge', entry.branch, '--no-ff', '--no-edit', '-m', `chore: merge executor worktree (${entry.branch})`]); + if (!gitResultOk(merge)) { + result.status = 'blocked'; + result.reason = 'merge_failed'; + result.stderr = merge?.stderr || merge?.stdout || ''; + results.push(result); + pending.push(...entries.slice(i + 1)); + ok = false; + break; + } + + const remove = execGit(plan.repoRoot, ['worktree', 'remove', entry.worktree_path, '--force']); + if (!gitResultOk(remove)) { + result.status = 'blocked'; + result.reason = 'worktree_remove_failed'; + result.stderr = remove?.stderr || ''; + results.push(result); + pending.push(...entries.slice(i + 1)); + ok = false; + break; + } + + const branchDelete = execGit(plan.repoRoot, ['branch', '-D', entry.branch]); + if (!gitResultOk(branchDelete)) { + result.status = 'warning'; + result.reason = 'branch_delete_failed'; + result.stderr = branchDelete?.stderr || ''; + ok = false; + } else { + result.status = 'merged_removed'; + result.reason = 'ok'; + } + results.push(result); + } + + return { + ok, + action: plan.action, + reason: ok ? 'ok' : 'cleanup_blocked', + entries: results, + pending, + }; +} + +function cmdWorktreeCleanupWave(cwd, args = []) { + const manifestFlagIndex = args.indexOf('--manifest'); + const manifestPath = manifestFlagIndex >= 0 ? args[manifestFlagIndex + 1] : ''; + if (!manifestPath) { + process.stderr.write('Usage: worktree cleanup-wave --manifest \n'); + process.exitCode = 2; + return; + } + + let manifest; + try { + manifest = fs.readFileSync(path.resolve(cwd, manifestPath), 'utf8'); + } catch (err) { + process.stdout.write(`${JSON.stringify({ + ok: false, + reason: 'manifest_read_failed', + error: err.message, + }, null, 2)}\n`); + process.exitCode = 1; + return; + } + + const plan = planWorktreeWaveCleanup(cwd, manifest); + const result = executeWorktreeWaveCleanupPlan(plan); + const response = { + ok: result.ok, + plan: { + action: plan.action, + discovery: plan.discovery, + reason: plan.reason, + entries: plan.entries.length, + }, + result, + }; + process.stdout.write(`${JSON.stringify(response, null, 2)}\n`); + if (!result.ok) { + process.exitCode = 1; + } +} + module.exports = { resolveWorktreeContext, parseWorktreePorcelain, @@ -335,4 +581,8 @@ module.exports = { listLinkedWorktreePaths, inspectWorktreeHealth, snapshotWorktreeInventory, + normalizeCleanupManifest, + planWorktreeWaveCleanup, + executeWorktreeWaveCleanupPlan, + cmdWorktreeCleanupWave, }; diff --git a/get-shit-done/workflows/diagnose-issues.md b/get-shit-done/workflows/diagnose-issues.md index 0eaaf27c9..5c2a37049 100644 --- a/get-shit-done/workflows/diagnose-issues.md +++ b/get-shit-done/workflows/diagnose-issues.md @@ -97,7 +97,7 @@ For each gap, fill the debug-subagent-prompt template and spawn: ``` Agent( - prompt=filled_debug_subagent_prompt + "\n\n\nFIRST ACTION: run git merge-base HEAD {EXPECTED_BASE} — if result differs from {EXPECTED_BASE}, run git reset --hard {EXPECTED_BASE} to correct the branch base (safe — runs before any agent work). Then verify: if [ \"$(git rev-parse HEAD)\" != \"{EXPECTED_BASE}\" ]; then echo \"ERROR: Could not correct worktree base\"; exit 1; fi. Fixes EnterWorktree creating branches from main on all platforms.\n\n\n\n- {phase_dir}/{phase_num}-UAT.md\n- .planning/STATE.md\n\n${AGENT_SKILLS_DEBUGGER}", + prompt=filled_debug_subagent_prompt + "\n\n\nFIRST ACTION: assert this is a disposable worktree branch before any repair. Run:\n```bash\nHEAD_REF=$(git symbolic-ref --quiet HEAD || echo \"DETACHED\")\nACTUAL_BRANCH=$(git rev-parse --abbrev-ref HEAD)\nif [ \"$HEAD_REF\" = \"DETACHED\" ] || echo \"$ACTUAL_BRANCH\" | grep -Eq '^(main|master|develop|trunk|release/.*)$'; then\n echo \"FATAL: diagnose worktree HEAD on '$ACTUAL_BRANCH'; refusing reset --hard on a protected branch.\" >&2\n exit 1\nfi\nif ! echo \"$ACTUAL_BRANCH\" | grep -Eq '^worktree-agent-[A-Za-z0-9._/-]+$'; then\n echo \"FATAL: diagnose worktree HEAD '$ACTUAL_BRANCH' is not in the worktree-agent-* namespace; refusing reset --hard.\" >&2\n exit 1\nfi\nACTUAL_BASE=$(git merge-base HEAD {EXPECTED_BASE})\nif [ \"$ACTUAL_BASE\" != \"{EXPECTED_BASE}\" ]; then\n git reset --hard {EXPECTED_BASE}\n [ \"$(git rev-parse HEAD)\" != \"{EXPECTED_BASE}\" ] && { echo \"ERROR: Could not correct worktree base\"; exit 1; }\nfi\n```\nFixes EnterWorktree creating branches from main on all platforms while preventing protected-branch data loss.\n\n\n\n- {phase_dir}/{phase_num}-UAT.md\n- .planning/STATE.md\n\n${AGENT_SKILLS_DEBUGGER}", subagent_type="gsd-debugger", ${USE_WORKTREES !== "false" ? 'isolation="worktree",' : ''} description="Debug: {truth_short}" diff --git a/get-shit-done/workflows/execute-phase.md b/get-shit-done/workflows/execute-phase.md index 5e47fd7ac..f3c547d49 100644 --- a/get-shit-done/workflows/execute-phase.md +++ b/get-shit-done/workflows/execute-phase.md @@ -519,6 +519,11 @@ increases monotonically across waves. `{status}` is `complete` (success), EXPECTED_BASE=$(git rev-parse HEAD) DISPATCH_TS=$(date -u +"%Y-%m-%dT%H:%M:%SZ") EXPECTED_BRANCH=$(git rev-parse --abbrev-ref HEAD) + if [ "${USE_WORKTREES_FOR_PLAN:-true}" != "false" ] && [ -z "${WAVE_WORKTREE_MANIFEST:-}" ]; then + WAVE_WORKTREE_MANIFEST=$(mktemp "${TMPDIR:-/tmp}/gsd-worktree-wave-XXXXXX.json") + printf '{"worktrees":[]}\n' > "$WAVE_WORKTREE_MANIFEST" + export WAVE_WORKTREE_MANIFEST + fi ``` **Sequential dispatch for parallel execution (waves with 2+ agents):** @@ -636,6 +641,8 @@ increases monotonically across waves. `{status}` is `complete` (success), ) ``` + Immediately after each worktree `Agent()` spawn returns metadata, atomically append `{agent_id, worktree_path, branch, expected_base}` to `WAVE_WORKTREE_MANIFEST`. If any field is missing, stop and ask for recovery instead of scanning all agent worktrees. + > **ORCHESTRATOR RULE — CODEX RUNTIME**: After calling Agent() above to spawn executor agent(s), stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. **Sequential mode** (`USE_WORKTREES_FOR_PLAN` is `false` — either project-level `USE_WORKTREES=false`, or per-plan submodule intersection forced it false in step 2.5): @@ -727,35 +734,37 @@ increases monotonically across waves. `{status}` is `complete` (success), When executor agents ran in worktree isolation, their commits land on temporary branches in separate working trees. After the wave completes, merge these changes back and clean up: + **Manifest source of truth (#3384):** Cleanup consumes the `WAVE_WORKTREE_MANIFEST` created and populated during executor dispatch in step 3. Do not recreate or truncate it here. + + Prefer the bounded helper, which validates branch identity, expected base, deletion + diffs, merge result, and worktree removal before deleting the temporary branch. + If the helper reports a blocked cleanup, resolve the reported manifest entry and + rerun the same command. Do not fall back to broad worktree discovery. + ```bash - # List worktrees created by this wave's agents. - # Inclusion-based filter (#2774): match ONLY agent-spawned worktrees under - # `.claude/worktrees/agent-` (the namespace Claude Code's `isolation="worktree"` - # uses). The previous exclusion filter (`grep -v "$(pwd)$"`) destroyed the parent - # workspace's `.git` whenever the workspace itself was a worktree (multi-workspace - # setups, and the cross-drive Windows case where `git worktree list` reports the - # registry path on a different drive than `$(pwd)`). - # Read line-by-line so worktree paths containing whitespace are preserved (#2774). + [ -n "${WAVE_WORKTREE_MANIFEST:-}" ] && [ -f "$WAVE_WORKTREE_MANIFEST" ] || { + echo "BLOCKED: missing WAVE_WORKTREE_MANIFEST; refusing broad worktree cleanup (#3384)." >&2 + exit 1 + } + + if command -v gsd-sdk >/dev/null 2>&1; then + gsd-sdk query worktree.cleanup-wave --manifest "$WAVE_WORKTREE_MANIFEST" || exit 1 + else + echo "WARN: gsd-sdk unavailable; using manifest-scoped shell fallback (#3384)." >&2 + + WT_PATHS_FILE=$(mktemp "${TMPDIR:-/tmp}/gsd-worktree-paths-XXXXXX") + node -e 'const fs=require("fs");const p=process.env.WAVE_WORKTREE_MANIFEST;try{if(!p)throw new Error("WAVE_WORKTREE_MANIFEST is unset");if(!fs.existsSync(p))throw new Error("manifest does not exist");const s=fs.readFileSync(p,"utf8");if(!s.trim())throw new Error("manifest is empty");const j=JSON.parse(s);for(const w of j.worktrees||[])if(w.worktree_path)console.log(w.worktree_path)}catch(e){console.error(`ERROR: cannot read worktree manifest ${p||"(unset)"}: ${e.message}`);process.exit(1)}' > "$WT_PATHS_FILE" || { echo "BLOCKED: cannot read WAVE_WORKTREE_MANIFEST; refusing cleanup (#3384)." >&2; exit 1; } while IFS= read -r WT; do [ -z "$WT" ] && continue - # Get the branch name for this worktree WT_BRANCH=$(git -C "$WT" rev-parse --abbrev-ref HEAD 2>/dev/null) if [ -n "$WT_BRANCH" ] && [ "$WT_BRANCH" != "HEAD" ]; then CURRENT_BRANCH=$(git rev-parse --abbrev-ref HEAD) - # --- Orchestrator file protection (#1756) --- - # Snapshot orchestrator-owned files BEFORE merge. If the worktree - # branch outlived a milestone transition, its versions of STATE.md - # and ROADMAP.md are stale. Main always wins for these files. STATE_BACKUP=$(mktemp) ROADMAP_BACKUP=$(mktemp) [ -f .planning/STATE.md ] && cp .planning/STATE.md "$STATE_BACKUP" || true [ -f .planning/ROADMAP.md ] && cp .planning/ROADMAP.md "$ROADMAP_BACKUP" || true - # Snapshot list of files on main BEFORE merge to detect resurrections - PRE_MERGE_FILES=$(git ls-files .planning/) - - # Pre-merge deletion check: warn if the worktree branch deletes tracked files DELETIONS=$(git diff --diff-filter=D --name-only HEAD..."$WT_BRANCH" 2>/dev/null || true) if [ -n "$DELETIONS" ]; then echo "BLOCKED: Worktree branch $WT_BRANCH contains file deletions: $DELETIONS" @@ -764,7 +773,6 @@ increases monotonically across waves. `{status}` is `complete` (success), continue fi - # Merge the worktree branch into the current branch (--no-ff ensures a merge commit so HEAD~1 is reliable) git merge "$WT_BRANCH" --no-ff --no-edit -m "chore: merge executor worktree ($WT_BRANCH)" 2>&1 || { echo "⚠ Merge conflict from worktree $WT_BRANCH — resolve manually" echo " STATE.md backup: $STATE_BACKUP" @@ -773,10 +781,6 @@ increases monotonically across waves. `{status}` is `complete` (success), break } - # Post-merge deletion audit: detect bulk file deletions in merge commit (#2384) - # --diff-filter=D HEAD~1 HEAD shows files deleted by the merge commit itself. - # Exclude .planning/ — orchestrator-owned deletions there are expected (resurrections - # are handled below). Require ALLOW_BULK_DELETE=1 to bypass for intentional large refactors. MERGE_DEL_COUNT=$(git diff --diff-filter=D --name-only HEAD~1 HEAD 2>/dev/null | grep -vc '^\.planning/' || true) if [ "$MERGE_DEL_COUNT" -gt 5 ] && [ "${ALLOW_BULK_DELETE:-0}" != "1" ]; then MERGE_DELETIONS=$(git diff --diff-filter=D --name-only HEAD~1 HEAD 2>/dev/null | grep -v '^\.planning/' || true) @@ -788,7 +792,6 @@ increases monotonically across waves. `{status}` is `complete` (success), continue fi - # Restore orchestrator-owned files (main always wins) if [ -s "$STATE_BACKUP" ]; then cp "$STATE_BACKUP" .planning/STATE.md fi @@ -797,25 +800,17 @@ increases monotonically across waves. `{status}` is `complete` (success), fi rm -f "$STATE_BACKUP" "$ROADMAP_BACKUP" - # Detect files deleted on main but re-added by worktree merge - # (e.g., archived phase directories that were intentionally removed) - # A "resurrected" file must have a deletion event in main's ancestry — - # brand-new files (e.g. SUMMARY.md just created by the executor) have no - # such history and must NOT be removed (#2501). + # Detect files deleted on main but re-added by worktree merge (#2501). DELETED_FILES=$(git diff --diff-filter=A --name-only HEAD~1 -- .planning/ 2>/dev/null || true) for RESURRECTED in $DELETED_FILES; do - # Only delete if this file was previously tracked on main and then - # deliberately removed (has a deletion event in git history). WAS_DELETED=$(git log --follow --diff-filter=D --name-only --format="" HEAD~1 -- "$RESURRECTED" 2>/dev/null | grep -c . || true) if [ "${WAS_DELETED:-0}" -gt 0 ]; then git rm -f "$RESURRECTED" 2>/dev/null || true fi done - # Amend merge commit with restored files if any changed if ! git diff --quiet .planning/STATE.md .planning/ROADMAP.md 2>/dev/null || \ [ -n "$DELETED_FILES" ]; then - # Only amend the commit with .planning/ files if commit_docs is enabled (#1783) COMMIT_DOCS=$(gsd-sdk query config-get commit_docs 2>/dev/null || echo "true") if [ "$COMMIT_DOCS" != "false" ]; then git add .planning/STATE.md .planning/ROADMAP.md 2>/dev/null || true @@ -824,10 +819,6 @@ increases monotonically across waves. `{status}` is `complete` (success), fi # Safety net: rescue uncommitted SUMMARY.md before worktree removal (#2070, #2838). - # Filesystem-level (find + cp) bypasses git's --exclude-standard filter, which silently - # drops .planning/SUMMARY.md when projects gitignore .planning/ — the rescue's prior - # `git ls-files --exclude-standard` form returned empty in that case and the SUMMARY - # was lost on `git worktree remove --force`. while IFS= read -r SUMMARY; do [ -z "$SUMMARY" ] && continue REL_PATH="${SUMMARY#$WT/}" @@ -838,13 +829,17 @@ increases monotonically across waves. `{status}` is `complete` (success), fi done < <(find "$WT/.planning" -name "*SUMMARY.md" 2>/dev/null) - # Remove the worktree - if ! git worktree remove "$WT" --force; then + REMOVE_OK=false + if git worktree remove "$WT" --force; then + REMOVE_OK=true + else 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 + if git worktree remove "$WT" --force; then + REMOVE_OK=true + else 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 @@ -853,22 +848,25 @@ increases monotonically across waves. `{status}` is `complete` (success), fi fi - # Delete the temporary branch - git branch -D "$WT_BRANCH" 2>/dev/null || true + if [ "$REMOVE_OK" = "true" ]; then + git branch -D "$WT_BRANCH" 2>/dev/null || true + else + echo "⚠ Keeping branch $WT_BRANCH because worktree removal failed (#3384)" + fi fi - done < <(git worktree list --porcelain | grep "^worktree " | grep "\.claude/worktrees/agent-" | sed 's/^worktree //') + done < "$WT_PATHS_FILE" + fi ``` **Cleanup-tail snippet (use after any wave whose merges did not flow through the templated path above):** - If the orchestrator deviated from the standard wave merge path (e.g., custom inter-worktree base-update merges with `merge: bring …` style messages), run this snippet after the custom merges are complete. It discovers and removes any residual `worktree-agent-*` worktrees. Safe to run when no residuals exist — it is a no-op in that case. + If the orchestrator deviated from the standard wave merge path (e.g., custom inter-worktree base-update merges with `merge: bring …` style messages), run this snippet after the custom merges are complete. It reads only `WAVE_WORKTREE_MANIFEST`; do not discover unrelated `worktree-agent-*` worktrees. ```bash # Cleanup-tail: remove residual agent worktrees after a cross-wave-dependency deviation. - # Inclusion-based filter (#2774): match ONLY agent-spawned worktrees under - # `.claude/worktrees/agent-`. Do NOT use exclusion filters (grep -v "$(pwd)$") — - # they destroy the parent workspace's .git in multi-workspace or cross-drive setups. - # Read line-by-line so worktree paths containing whitespace are preserved (#2774). + # Uses only the current wave manifest to avoid touching unrelated active agents (#3384). + WT_PATHS_FILE=$(mktemp "${TMPDIR:-/tmp}/gsd-worktree-paths-XXXXXX") + node -e 'const fs=require("fs");const p=process.env.WAVE_WORKTREE_MANIFEST;try{if(!p)throw new Error("WAVE_WORKTREE_MANIFEST is unset");if(!fs.existsSync(p))throw new Error("manifest does not exist");const s=fs.readFileSync(p,"utf8");if(!s.trim())throw new Error("manifest is empty");const j=JSON.parse(s);for(const w of j.worktrees||[])if(w.worktree_path)console.log(w.worktree_path)}catch(e){console.error(`ERROR: cannot read worktree manifest ${p||"(unset)"}: ${e.message}`);process.exit(1)}' > "$WT_PATHS_FILE" || { echo "BLOCKED: cannot read WAVE_WORKTREE_MANIFEST; refusing cleanup (#3384)." >&2; exit 1; } while IFS= read -r WT; do [ -z "$WT" ] && continue WT_BRANCH=$(git -C "$WT" rev-parse --abbrev-ref HEAD 2>/dev/null) @@ -886,7 +884,7 @@ increases monotonically across waves. `{status}` is `complete` (success), else git branch -D "$WT_BRANCH" 2>/dev/null || true fi - done < <(git worktree list --porcelain | grep "^worktree " | grep "\.claude/worktrees/agent-" | sed 's/^worktree //') + done < "$WT_PATHS_FILE" git worktree prune ``` diff --git a/get-shit-done/workflows/quick.md b/get-shit-done/workflows/quick.md index 9c3c60be5..c5627d2c0 100644 --- a/get-shit-done/workflows/quick.md +++ b/get-shit-done/workflows/quick.md @@ -663,6 +663,11 @@ fi Capture current HEAD before spawning (used for worktree branch check): ```bash EXPECTED_BASE=$(git rev-parse HEAD) +if [ "${USE_WORKTREES:-true}" != "false" ]; then + QUICK_WORKTREE_MANIFEST=$(mktemp "${TMPDIR:-/tmp}/gsd-quick-worktree-XXXXXX.json") + printf '{"worktrees":[]}\n' > "$QUICK_WORKTREE_MANIFEST" + export QUICK_WORKTREE_MANIFEST +fi ``` Spawn gsd-executor with plan reference: @@ -763,10 +768,26 @@ SUMMARY.md and stop — the user must rerun with worktrees disabled. > **ORCHESTRATOR RULE — CODEX RUNTIME**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. +If the executor ran with `isolation="worktree"`, append its returned `{agent_id, worktree_path, branch, expected_base}` metadata to `QUICK_WORKTREE_MANIFEST` before cleanup. If any field is unavailable, stop and ask for recovery; do not discover global worktrees. + After executor returns: 1. **Worktree cleanup:** If the executor ran with `isolation="worktree"`, merge the worktree branch back and clean up: ```bash - # Find worktrees created by the executor. + QUICK_WORKTREE_MANIFEST=${QUICK_WORKTREE_MANIFEST:-$WAVE_WORKTREE_MANIFEST} + [ -n "${QUICK_WORKTREE_MANIFEST:-}" ] && [ -f "$QUICK_WORKTREE_MANIFEST" ] || { + echo "BLOCKED: missing QUICK_WORKTREE_MANIFEST; refusing broad worktree cleanup (#3384)." >&2 + exit 1 + } + + # Prefer the bounded cleanup helper. It verifies branch identity, expected + # base, deletion diffs, merge result, and worktree removal before branch + # deletion. If it blocks, resolve the reported manifest entry and rerun. + if command -v gsd-sdk >/dev/null 2>&1; then + gsd-sdk query worktree.cleanup-wave --manifest "$QUICK_WORKTREE_MANIFEST" || exit 1 + else + echo "WARN: gsd-sdk unavailable; using manifest-scoped shell fallback (#3384)." >&2 + + # Find worktrees recorded by the executor manifest only. # Inclusion-based filter (#2774): match ONLY agent-spawned worktrees under # `.claude/worktrees/agent-` (the namespace Claude Code's `isolation="worktree"` # uses). The previous exclusion filter (`grep -v "$(pwd)$"`) destroyed the parent @@ -774,6 +795,8 @@ After executor returns: # setups, and the cross-drive Windows case where `git worktree list` reports the # registry path on a different drive than `$(pwd)`). # Read line-by-line so worktree paths containing whitespace are preserved (#2774). + WT_PATHS_FILE=$(mktemp "${TMPDIR:-/tmp}/gsd-worktree-paths-XXXXXX") + node -e 'const fs=require("fs");const p=process.env.QUICK_WORKTREE_MANIFEST||process.env.WAVE_WORKTREE_MANIFEST;try{if(!p)throw new Error("QUICK_WORKTREE_MANIFEST is unset");if(!fs.existsSync(p))throw new Error("manifest does not exist");const s=fs.readFileSync(p,"utf8");if(!s.trim())throw new Error("manifest is empty");const j=JSON.parse(s);for(const w of j.worktrees||[])if(w.worktree_path)console.log(w.worktree_path)}catch(e){console.error(`ERROR: cannot read worktree manifest ${p||"(unset)"}: ${e.message}`);process.exit(1)}' > "$WT_PATHS_FILE" || { echo "BLOCKED: cannot read QUICK_WORKTREE_MANIFEST; refusing cleanup (#3384)." >&2; exit 1; } while IFS= read -r WT; do [ -z "$WT" ] && continue WT_BRANCH=$(git -C "$WT" rev-parse --abbrev-ref HEAD 2>/dev/null) @@ -846,12 +869,19 @@ After executor returns: fi done < <(find "$WT/.planning" -name "*SUMMARY.md" 2>/dev/null) - if ! git worktree remove "$WT" --force; then + # Remove the worktree before deleting the branch. If removal fails, + # leave the branch in place so the worktree remains recoverable (#3384). + REMOVE_OK=false + if git worktree remove "$WT" --force; then + REMOVE_OK=true + else 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 + if git worktree remove "$WT" --force; then + REMOVE_OK=true + else 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 @@ -859,9 +889,14 @@ After executor returns: echo "⚠ Residual worktree at $WT (remove failed) — investigate manually" fi fi - git branch -D "$WT_BRANCH" 2>/dev/null || true + if [ "$REMOVE_OK" = "true" ]; then + git branch -D "$WT_BRANCH" 2>/dev/null || true + else + echo "⚠ Keeping branch $WT_BRANCH because worktree removal failed (#3384)" + fi fi - done < <(git worktree list --porcelain | grep "^worktree " | grep "\.claude/worktrees/agent-" | sed 's/^worktree //') + done < "$WT_PATHS_FILE" + fi ``` If `workflow.use_worktrees` is `false`, skip this step. 2. Verify summary exists at `${QUICK_DIR}/${quick_id}-SUMMARY.md` diff --git a/get-shit-done/workflows/remove-workspace.md b/get-shit-done/workflows/remove-workspace.md index 98b189ea0..68f2c54c9 100644 --- a/get-shit-done/workflows/remove-workspace.md +++ b/get-shit-done/workflows/remove-workspace.md @@ -62,21 +62,36 @@ Use AskUserQuestion: **If strategy is `worktree`:** +Initialize the failure flag once before iterating repos: + +```bash +REMOVE_FAILED=false +``` + For each repo in the workspace: ```bash cd "$SOURCE_REPO_PATH" -git worktree remove "$WORKSPACE_PATH/$REPO_NAME" 2>&1 || true +if ! git worktree remove "$WORKSPACE_PATH/$REPO_NAME" 2>&1; then + echo "Warning: Could not remove worktree for $REPO_NAME — source repo may have been moved, deleted, locked, or dirty." >&2 + REMOVE_FAILED=true +fi ``` -If `git worktree remove` fails, warn but continue: -``` -Warning: Could not remove worktree for $REPO_NAME — source repo may have been moved or deleted. +If any `git worktree remove` fails, stop before deleting the workspace directory: +```text +Refusing to delete "$WORKSPACE_PATH" because one or more git worktrees could not be removed. +Resolve the failed worktree removal manually, then rerun remove-workspace. ``` ## 5. Delete Workspace Directory ```bash +if [ "${REMOVE_FAILED:-false}" = "true" ]; then + echo "Refusing to delete \"$WORKSPACE_PATH\" because one or more git worktrees could not be removed." >&2 + exit 1 +fi + rm -rf "$WORKSPACE_PATH" ``` diff --git a/sdk/src/query/command-static-catalog-domain.ts b/sdk/src/query/command-static-catalog-domain.ts index 59e656f3e..f08f2e06b 100644 --- a/sdk/src/query/command-static-catalog-domain.ts +++ b/sdk/src/query/command-static-catalog-domain.ts @@ -16,6 +16,7 @@ import { uatRenderCheckpoint, auditUat } from './uat.js'; import { intelStatus, intelDiff, intelSnapshot, intelValidate, intelQuery, intelExtractExports, intelPatchMeta, intelUpdate } from './intel.js'; import { writeProfile, generateClaudeProfile, generateDevPreferences, generateClaudeMd } from './profile-output.js'; import { phaseMvpMode, taskIsBehaviorAdding, userStoryValidate } from './mvp.js'; +import { worktreeCleanupWave } from './worktree.js'; export const DOMAIN_STATIC_CATALOG: ReadonlyArray = [ ['agent-skills', agentSkills], @@ -59,6 +60,8 @@ export const DOMAIN_STATIC_CATALOG: ReadonlyArray { + const toolsPath = resolveGsdToolsPath(projectDir); + const result = spawnSync(process.execPath, [toolsPath, 'worktree', 'cleanup-wave', ...args], { + cwd: projectDir, + encoding: 'utf-8', + stdio: ['pipe', 'pipe', 'pipe'], + timeout: 30000, + maxBuffer: 1024 * 1024, + env: { + ...process.env, + GIT_TERMINAL_PROMPT: '0', + GCM_INTERACTIVE: 'never', + }, + }); + + if (result.error) { + return { data: { ok: false, reason: result.error.message || 'gsd-tools invocation failed' } }; + } + + const stdout = (result.stdout || '').trim(); + if (stdout) { + try { + return { data: JSON.parse(stdout) }; + } catch { + return { data: { ok: result.status === 0, reason: stdout } }; + } + } + + return { + data: { + ok: result.status === 0, + reason: result.stderr?.trim() || (result.status === 0 ? 'ok' : 'gsd-tools error'), + }, + }; +}; diff --git a/tests/bug-3384-secondary-defects.test.cjs b/tests/bug-3384-secondary-defects.test.cjs new file mode 100644 index 000000000..ba36f247d --- /dev/null +++ b/tests/bug-3384-secondary-defects.test.cjs @@ -0,0 +1,59 @@ +// allow-test-rule: source-text-is-the-product +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const repoRoot = path.resolve(__dirname, '..'); + +function read(relPath) { + return fs.readFileSync(path.join(repoRoot, relPath), 'utf8'); +} + +describe('bug #3384: adjacent worktree data-loss guards', () => { + test('worktree cleanup CLI preserves caller cwd instead of resolving project root', () => { + const source = read('get-shit-done/bin/gsd-tools.cjs'); + const skipSet = source.slice( + source.indexOf('const SKIP_ROOT_RESOLUTION = new Set(['), + source.indexOf('if (!SKIP_ROOT_RESOLUTION.has(command))'), + ); + + assert.match(skipSet, /'worktree'/); + }); + + test('diagnose-issues agents assert disposable worktree branch before reset --hard', () => { + const source = read('get-shit-done/workflows/diagnose-issues.md'); + const branchCheck = source.indexOf('HEAD_REF=$(git symbolic-ref --quiet HEAD || echo'); + const namespaceCheck = source.indexOf('worktree-agent-* namespace'); + const reset = source.indexOf('git reset --hard {EXPECTED_BASE}'); + + assert.ok(branchCheck > 0, 'diagnose prompt must assert HEAD before repair'); + assert.ok(namespaceCheck > branchCheck, 'diagnose prompt must require disposable worktree-agent branch'); + assert.ok(reset > namespaceCheck, 'reset --hard must come only after branch namespace check'); + }); + + test('remove-workspace fails closed when git worktree remove fails', () => { + const source = read('get-shit-done/workflows/remove-workspace.md'); + const init = source.indexOf('REMOVE_FAILED=false'); + const loop = source.indexOf('For each repo in the workspace'); + const remove = source.indexOf('git worktree remove "$WORKSPACE_PATH/$REPO_NAME"'); + + assert.doesNotMatch( + source, + /git worktree remove "\$WORKSPACE_PATH\/\$REPO_NAME" 2>&1 \|\| true/, + 'worktree removal failures must not be swallowed', + ); + assert.ok(init > 0 && init < loop, 'REMOVE_FAILED must initialize once before the per-repo loop'); + assert.ok(remove > loop, 'worktree removal should remain inside the per-repo loop'); + assert.match(source, /Refusing to delete "\$WORKSPACE_PATH"/); + }); + + test('validate health warns when worktree inventory cannot be listed', () => { + const source = read('get-shit-done/bin/lib/verify.cjs'); + const failureBranch = source.indexOf("worktreeHealth.reason === 'git_list_failed'"); + const warning = source.indexOf("addIssue('warning', 'W020'", failureBranch); + + assert.ok(failureBranch > 0, 'verify health should branch on git_list_failed'); + assert.ok(warning > failureBranch, 'git_list_failed should emit W020 degraded-health warning'); + }); +}); diff --git a/tests/bug-3384-worktree-cleanup-manifest.test.cjs b/tests/bug-3384-worktree-cleanup-manifest.test.cjs new file mode 100644 index 000000000..2f8524866 --- /dev/null +++ b/tests/bug-3384-worktree-cleanup-manifest.test.cjs @@ -0,0 +1,235 @@ +// allow-test-rule: source-text-is-the-product +// Workflow markdown is the installed orchestration contract, and the CJS policy +// module is the callable safety seam for worktree cleanup. + +'use strict'; + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { + planWorktreeWaveCleanup, + executeWorktreeWaveCleanupPlan, +} = require('../get-shit-done/bin/lib/worktree-safety.cjs'); + +const EXECUTE_PHASE_PATH = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'execute-phase.md'); +const QUICK_PATH = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'quick.md'); + +function readWorkflow(filePath) { + return fs.readFileSync(filePath, 'utf8'); +} + +describe('bug #3384: worktree cleanup is manifest-scoped and fail-closed', () => { + test('cleanup plan includes only manifest entries and never discovers global agent worktrees', () => { + const plan = planWorktreeWaveCleanup('/repo/main', { + worktrees: [ + { + agent_id: 'a1', + worktree_path: '/repo/.claude/worktrees/agent-a1', + branch: 'worktree-agent-a1', + expected_base: 'abc123', + }, + ], + }); + + assert.equal(plan.ok, true); + assert.deepEqual(plan.entries.map((entry) => ({ + agent_id: entry.agent_id, + worktree_path: entry.worktree_path, + branch: entry.branch, + expected_base: entry.expected_base, + })), [{ + agent_id: 'a1', + worktree_path: '/repo/.claude/worktrees/agent-a1', + branch: 'worktree-agent-a1', + expected_base: 'abc123', + }]); + assert.equal(plan.discovery, 'manifest'); + }); + + test('cleanup plan rejects entries without expected base or disposable branch namespace', () => { + const plan = planWorktreeWaveCleanup('/repo/main', { + worktrees: [ + { + agent_id: 'missing-base', + worktree_path: '/repo/.claude/worktrees/agent-missing-base', + branch: 'worktree-agent-missing-base', + }, + { + agent_id: 'feature-branch', + worktree_path: '/repo/.claude/worktrees/agent-feature', + branch: 'feature/user-work', + expected_base: 'abc123', + }, + ], + }); + + assert.equal(plan.ok, false); + assert.equal(plan.reason, 'empty_manifest'); + assert.deepEqual(plan.entries, []); + }); + + test('cleanup executor does not delete a branch when worktree removal fails', () => { + const calls = []; + const plan = { + ok: true, + repoRoot: '/repo/main', + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ + agent_id: 'a1', + worktree_path: '/repo/.claude/worktrees/agent-a1', + branch: 'worktree-agent-a1', + expected_base: 'abc123', + }], + }; + + const result = executeWorktreeWaveCleanupPlan(plan, { + execGit: (cwd, args) => { + calls.push({ cwd, args }); + const key = args.join(' '); + if (key === '-C /repo/.claude/worktrees/agent-a1 rev-parse --abbrev-ref HEAD') { + return { exitCode: 0, stdout: 'worktree-agent-a1', stderr: '' }; + } + if (key === 'merge-base HEAD worktree-agent-a1') { + return { exitCode: 0, stdout: 'abc123', stderr: '' }; + } + if (key === 'diff --diff-filter=D --name-only HEAD...worktree-agent-a1') { + return { exitCode: 0, stdout: '', stderr: '' }; + } + if (key.startsWith('merge worktree-agent-a1')) { + return { exitCode: 0, stdout: '', stderr: '' }; + } + if (key === 'worktree remove /repo/.claude/worktrees/agent-a1 --force') { + return { exitCode: 1, stdout: '', stderr: 'locked' }; + } + if (key === 'branch -D worktree-agent-a1') { + throw new Error('branch deletion must not run after remove failure'); + } + return { exitCode: 0, stdout: '', stderr: '' }; + }, + }); + + assert.equal(result.ok, false); + assert.equal(result.entries[0].status, 'blocked'); + assert.equal(result.entries[0].reason, 'worktree_remove_failed'); + assert.equal(calls.some((call) => call.args.join(' ') === 'branch -D worktree-agent-a1'), false); + }); + + test('cleanup executor stops on merge conflict and records remaining manifest entries', () => { + const plan = { + ok: true, + repoRoot: '/repo/main', + action: 'cleanup_wave', + discovery: 'manifest', + entries: [ + { + agent_id: 'a1', + worktree_path: '/repo/.claude/worktrees/agent-a1', + branch: 'worktree-agent-a1', + expected_base: 'abc123', + }, + { + agent_id: 'a2', + worktree_path: '/repo/.claude/worktrees/agent-a2', + branch: 'worktree-agent-a2', + expected_base: 'abc123', + }, + ], + }; + + const result = executeWorktreeWaveCleanupPlan(plan, { + execGit: (_cwd, args) => { + const key = args.join(' '); + if (key === '-C /repo/.claude/worktrees/agent-a1 rev-parse --abbrev-ref HEAD') { + return { exitCode: 0, stdout: 'worktree-agent-a1', stderr: '' }; + } + if (key === 'merge-base HEAD worktree-agent-a1') { + return { exitCode: 0, stdout: 'abc123', stderr: '' }; + } + if (key === 'diff --diff-filter=D --name-only HEAD...worktree-agent-a1') { + return { exitCode: 0, stdout: '', stderr: '' }; + } + if (key === '-C /repo/.claude/worktrees/agent-a1 status --porcelain --untracked-files=all') { + return { exitCode: 0, stdout: '', stderr: '' }; + } + if (key.startsWith('merge worktree-agent-a1')) { + return { exitCode: 1, stdout: '', stderr: 'CONFLICT' }; + } + throw new Error(`unexpected git call after conflict: ${key}`); + }, + }); + + assert.equal(result.ok, false); + assert.equal(result.entries[0].status, 'blocked'); + assert.equal(result.entries[0].reason, 'merge_failed'); + assert.deepEqual(result.pending.map((entry) => entry.branch), ['worktree-agent-a2']); + }); + + test('cleanup executor blocks dirty worktrees before merge/remove/delete', () => { + const calls = []; + const plan = { + ok: true, + repoRoot: '/repo/main', + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ + agent_id: 'a1', + worktree_path: '/repo/.claude/worktrees/agent-a1', + branch: 'worktree-agent-a1', + expected_base: 'abc123', + }], + }; + + const result = executeWorktreeWaveCleanupPlan(plan, { + execGit: (_cwd, args) => { + calls.push(args.join(' ')); + const key = args.join(' '); + if (key === '-C /repo/.claude/worktrees/agent-a1 rev-parse --abbrev-ref HEAD') { + return { exitCode: 0, stdout: 'worktree-agent-a1', stderr: '' }; + } + if (key === 'merge-base HEAD worktree-agent-a1') { + return { exitCode: 0, stdout: 'abc123', stderr: '' }; + } + if (key === 'diff --diff-filter=D --name-only HEAD...worktree-agent-a1') { + return { exitCode: 0, stdout: '', stderr: '' }; + } + if (key === '-C /repo/.claude/worktrees/agent-a1 status --porcelain --untracked-files=all') { + return { exitCode: 0, stdout: '?? scratch.txt', stderr: '' }; + } + throw new Error(`unexpected git call after dirty check: ${key}`); + }, + }); + + assert.equal(result.ok, false); + assert.equal(result.entries[0].reason, 'worktree_dirty'); + assert.equal(calls.some((call) => call.startsWith('merge worktree-agent-a1')), false); + assert.equal(calls.some((call) => call === 'worktree remove /repo/.claude/worktrees/agent-a1 --force'), false); + assert.equal(calls.some((call) => call === 'branch -D worktree-agent-a1'), false); + }); + + test('execute-phase contract requires a cleanup manifest instead of global worktree discovery', () => { + const content = readWorkflow(EXECUTE_PHASE_PATH); + assert.match(content, /WAVE_WORKTREE_MANIFEST/); + assert.match(content, /worktree\.cleanup-wave/); + assert.match(content, /atomically append `\{agent_id, worktree_path, branch, expected_base\}`/); + assert.match(content, /try\{if\(!p\)throw new Error\("WAVE_WORKTREE_MANIFEST is unset"\)/); + assert.match(content, /WT_PATHS_FILE=.*gsd-worktree-paths-/); + assert.doesNotMatch(content, /done < <\(node -e 'const fs=require\("fs"\);const p=process\.env\.WAVE_WORKTREE_MANIFEST/); + assert.doesNotMatch(content, /done < <\(git worktree list --porcelain \| grep "\^worktree " \| grep "\\\.claude\/worktrees\/agent-"/); + }); + + test('quick contract requires a cleanup manifest instead of global worktree discovery', () => { + const content = readWorkflow(QUICK_PATH); + assert.match(content, /WAVE_WORKTREE_MANIFEST|QUICK_WORKTREE_MANIFEST/); + assert.match(content, /worktree\.cleanup-wave/); + assert.match(content, /mktemp "\$\{TMPDIR:-\/tmp\}\/gsd-quick-worktree-/); + assert.match(content, /append its returned `\{agent_id, worktree_path, branch, expected_base\}`/); + assert.match(content, /try\{if\(!p\)throw new Error\("QUICK_WORKTREE_MANIFEST is unset"\)/); + assert.match(content, /WT_PATHS_FILE=.*gsd-worktree-paths-/); + assert.doesNotMatch(content, /done < <\(node -e 'const fs=require\("fs"\);const p=process\.env\.QUICK_WORKTREE_MANIFEST/); + assert.doesNotMatch(content, /done < <\(git worktree list --porcelain \| grep "\^worktree " \| grep "\\\.claude\/worktrees\/agent-"/); + }); +}); diff --git a/tests/execute-phase-step-5-5-deviation-doc.test.cjs b/tests/execute-phase-step-5-5-deviation-doc.test.cjs index d1acd58b5..639ffbbdd 100644 --- a/tests/execute-phase-step-5-5-deviation-doc.test.cjs +++ b/tests/execute-phase-step-5-5-deviation-doc.test.cjs @@ -39,7 +39,13 @@ function extractStep55Block(content) { } describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3264)', () => { - const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + function readWorkflow() { + try { + return fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + } catch (err) { + throw new Error(`failed to read workflow fixture at ${WORKFLOW_PATH}: ${err.message}`); + } + } test('workflow file exists', () => { assert.ok(fs.existsSync(WORKFLOW_PATH), 'workflows/execute-phase.md should exist'); @@ -47,11 +53,13 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3 test('step 5.5 block exists and is bounded', () => { // extractStep55Block throws on failure — this test validates the helper itself + const content = readWorkflow(); const block = extractStep55Block(content); assert.ok(block.length > 0, 'step 5.5 block must be non-empty'); }); test('step 5.5 documents the standard wave contract', () => { + const content = readWorkflow(); const block = extractStep55Block(content); assert.ok( block.includes('Standard wave contract'), @@ -60,6 +68,7 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3 }); test('step 5.5 names cross-wave dependency deviation as a supported execution mode', () => { + const content = readWorkflow(); const block = extractStep55Block(content); assert.ok( block.includes('Cross-wave dependency deviation'), @@ -68,6 +77,7 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3 }); test('cleanup-tail snippet contains git worktree prune', () => { + const content = readWorkflow(); const block = extractStep55Block(content); assert.ok( block.includes('git worktree prune'), @@ -76,6 +86,7 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3 }); test('cleanup-tail snippet contains git worktree remove --force', () => { + const content = readWorkflow(); const block = extractStep55Block(content); assert.ok( block.includes('git worktree remove') && block.includes('--force'), @@ -84,6 +95,7 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3 }); test('cleanup-tail snippet contains git worktree unlock', () => { + const content = readWorkflow(); const block = extractStep55Block(content); assert.ok( block.includes('git worktree unlock'), @@ -92,6 +104,7 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3 }); test('cleanup-tail snippet contains git branch -D', () => { + const content = readWorkflow(); const block = extractStep55Block(content); assert.ok( block.includes('git branch -D'), @@ -100,6 +113,7 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3 }); test('skip conditions enumerate empty-WAVE_WORKTREE_PLANS case', () => { + const content = readWorkflow(); const block = extractStep55Block(content); assert.ok( block.includes('WAVE_WORKTREE_PLANS'), @@ -108,6 +122,7 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3 }); test('skip conditions enumerate custom-merge-deviation case', () => { + const content = readWorkflow(); const block = extractStep55Block(content); // The deviation skip condition must reference the cleanup-tail as the alternative assert.ok( @@ -116,25 +131,30 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3 ); }); - test('cleanup-tail uses inclusion-based filter for agent namespace', () => { + test('cleanup-tail uses wave manifest instead of agent namespace discovery', () => { + const content = readWorkflow(); const block = extractStep55Block(content); - // Must use .claude/worktrees/agent- inclusion filter, not exclusion (per #2774 precedent) assert.ok( - block.includes('.claude/worktrees/agent-'), - 'cleanup-tail must use inclusion-based filter matching .claude/worktrees/agent- namespace', + block.includes('WAVE_WORKTREE_MANIFEST'), + 'cleanup-tail must consume the current wave manifest', + ); + assert.ok( + block.includes('avoid touching unrelated active agents'), + 'cleanup-tail must document why manifest-scoped cleanup is required', ); }); - test('cleanup-tail reads git worktree list --porcelain line-by-line', () => { + test('cleanup-tail does not rediscover global agent worktrees', () => { + const content = readWorkflow(); const block = extractStep55Block(content); - assert.ok( - block.includes('git worktree list --porcelain'), - 'cleanup-tail must parse git worktree list --porcelain output', + assert.doesNotMatch( + block, + /git worktree list --porcelain.*\.claude\/worktrees\/agent-/s, + 'cleanup-tail must not parse global git worktree list output for agent worktrees', ); - // Line-by-line reading requires IFS= read -r pattern assert.ok( block.includes('IFS= read -r'), - 'cleanup-tail must read line-by-line to preserve paths with whitespace', + 'cleanup-tail still reads manifest paths line-by-line to preserve paths with whitespace', ); }); });