From 4815b3c97205390fc2e807ca757fb36b01839e62 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 29 Apr 2026 08:07:12 -0400 Subject: [PATCH] fix(#2838): SUMMARY rescue handles gitignored .planning (#2850) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2838): SUMMARY rescue handles gitignored .planning explicitly The pre-fix rescue used `git ls-files --modified --others --exclude-standard` to detect uncommitted SUMMARY.md before worktree removal. When projects gitignore .planning/, --exclude-standard filters out the very files the rescue is meant to save, the rescue branch is skipped, and `git worktree remove --force` permanently deletes the SUMMARY. Replace both rescue blocks (quick.md, execute-phase.md) with a filesystem-level find + cp rescue that bypasses gitignore entirely and avoids the worktree↔main commit/merge cascade. cmp -s makes it idempotent. Adds tests/bug-2838-summary-rescue-gitignored-planning.test.cjs which extracts each rescue block, runs it against a real temp repo with a gitignored .planning/, and asserts the SUMMARY survives worktree removal. * test(#2838): assert rescue block exits 0 in idempotency test CodeRabbit (Minor): the idempotency test pre-creates the destination SUMMARY.md, so even a syntax/runtime error in the rescue block would silently false-pass. Add an explicit r.status === 0 assertion. --- get-shit-done/workflows/execute-phase.md | 24 +- get-shit-done/workflows/quick.md | 22 +- ...ummary-rescue-gitignored-planning.test.cjs | 218 ++++++++++++++++++ 3 files changed, 246 insertions(+), 18 deletions(-) create mode 100644 tests/bug-2838-summary-rescue-gitignored-planning.test.cjs diff --git a/get-shit-done/workflows/execute-phase.md b/get-shit-done/workflows/execute-phase.md index 2e1c8c53a..b7774a264 100644 --- a/get-shit-done/workflows/execute-phase.md +++ b/get-shit-done/workflows/execute-phase.md @@ -742,16 +742,20 @@ increases monotonically across waves. `{status}` is `complete` (success), fi fi - # Safety net: commit any uncommitted SUMMARY.md before force-removing the worktree. - # This guards against executors that skipped the git_commit_metadata step (#2070). - UNCOMMITTED_SUMMARY=$(git -C "$WT" ls-files --modified --others --exclude-standard -- "*SUMMARY.md" 2>/dev/null || true) - if [ -n "$UNCOMMITTED_SUMMARY" ]; then - echo "⚠ SUMMARY.md was not committed by executor — committing now to prevent data loss" - git -C "$WT" add -- "*SUMMARY.md" 2>/dev/null || true - git -C "$WT" commit --no-verify -m "docs(recovery): rescue uncommitted SUMMARY.md before worktree removal (#2070)" 2>/dev/null || true - # Re-merge the recovery commit - git merge "$WT_BRANCH" --no-edit -m "chore: merge rescued SUMMARY.md from executor worktree ($WT_BRANCH)" 2>/dev/null || true - 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/}" + if [ ! -f "$REL_PATH" ] || ! cmp -s "$SUMMARY" "$REL_PATH"; then + mkdir -p "$(dirname "$REL_PATH")" + cp "$SUMMARY" "$REL_PATH" + echo "⚠ Rescued $REL_PATH from worktree before removal" + fi + done < <(find "$WT/.planning" -name "*SUMMARY.md" 2>/dev/null) # Remove the worktree if ! git worktree remove "$WT" --force; then diff --git a/get-shit-done/workflows/quick.md b/get-shit-done/workflows/quick.md index 22d1086dd..f8a93c00b 100644 --- a/get-shit-done/workflows/quick.md +++ b/get-shit-done/workflows/quick.md @@ -752,14 +752,20 @@ After executor returns: fi fi - # Safety net: rescue uncommitted SUMMARY.md before worktree removal (#2296, mirrors #2070) - UNCOMMITTED_SUMMARY=$(git -C "$WT" ls-files --modified --others --exclude-standard -- "*SUMMARY.md" 2>/dev/null || true) - if [ -n "$UNCOMMITTED_SUMMARY" ]; then - echo "⚠ SUMMARY.md was not committed by executor — committing now to prevent data loss" - git -C "$WT" add -- "*SUMMARY.md" 2>/dev/null || true - git -C "$WT" commit --no-verify -m "docs(recovery): rescue uncommitted SUMMARY.md before worktree removal (#2070)" 2>/dev/null || true - git merge "$WT_BRANCH" --no-edit -m "chore: merge rescued SUMMARY.md from executor worktree ($WT_BRANCH)" 2>/dev/null || true - fi + # Safety net: rescue uncommitted SUMMARY.md before worktree removal (#2296, mirrors #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/}" + if [ ! -f "$REL_PATH" ] || ! cmp -s "$SUMMARY" "$REL_PATH"; then + mkdir -p "$(dirname "$REL_PATH")" + cp "$SUMMARY" "$REL_PATH" + echo "⚠ Rescued $REL_PATH from worktree before removal" + fi + done < <(find "$WT/.planning" -name "*SUMMARY.md" 2>/dev/null) if ! git worktree remove "$WT" --force; then WT_NAME=$(basename "$WT") diff --git a/tests/bug-2838-summary-rescue-gitignored-planning.test.cjs b/tests/bug-2838-summary-rescue-gitignored-planning.test.cjs new file mode 100644 index 000000000..41b71c1cf --- /dev/null +++ b/tests/bug-2838-summary-rescue-gitignored-planning.test.cjs @@ -0,0 +1,218 @@ +/** + * Regression tests for #2838: SUMMARY rescue silently fails when .planning/ + * is gitignored. + * + * The pre-fix rescue used `git ls-files --modified --others --exclude-standard` + * to detect uncommitted SUMMARY.md files. When projects gitignore .planning/ + * (a common policy), --exclude-standard filters out the very files the rescue + * was meant to save, producing an empty result and skipping the rescue branch. + * The next line `git worktree remove --force` then permanently deleted the + * SUMMARY. + * + * The fix replaces git ls-files with a filesystem-level `find` + `cp` rescue + * that bypasses gitignore entirely. + * + * This test file: + * 1. Extracts the rescue block from each workflow file (parsed structurally + * by locating the labeled comment + closing fence — not free-form regex + * over file contents). + * 2. Runs the extracted block against a real temp repo whose .planning/ + * directory is gitignored. + * 3. Asserts the SUMMARY is rescued into the main repo before worktree + * removal. + */ + +'use strict'; + +const { describe, test, before, after } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const os = require('os'); +const { execFileSync, spawnSync } = require('child_process'); + +const REPO_ROOT = path.join(__dirname, '..'); +const EXECUTE_PHASE_PATH = path.join(REPO_ROOT, 'get-shit-done', 'workflows', 'execute-phase.md'); +const QUICK_PATH = path.join(REPO_ROOT, 'get-shit-done', 'workflows', 'quick.md'); + +/** + * Extract the rescue block (the bash lines that detect+rescue the + * uncommitted SUMMARY.md). We locate it by: + * - Finding the line that contains "Safety net" AND "SUMMARY" + * - Reading forward until the indent drops back to the surrounding level + * OR we hit a blank line followed by a non-comment, non-rescue line. + * + * To keep this robust, we scan from the safety-net comment forward until we + * either reach `done` (new fix) or `fi` (old fix) followed by a blank line. + */ +function extractRescueBlock(filePath) { + const lines = fs.readFileSync(filePath, 'utf-8').split('\n'); + let startIdx = -1; + for (let i = 0; i < lines.length; i++) { + if (/Safety net/.test(lines[i]) && /SUMMARY/.test(lines[i])) { + startIdx = i; + break; + } + } + assert.notStrictEqual(startIdx, -1, `Could not find Safety net SUMMARY rescue block in ${filePath}`); + + // Capture from comment through the terminator. The new fix ends with + // `done < <(find ... )`. The old fix ended with `fi` followed by a blank + // line. We accumulate until we find one of those terminators; if we see + // `done < <(find` we always stop there (it can only appear after `fi`). + const collected = []; + let sawFi = false; + for (let i = startIdx; i < lines.length; i++) { + collected.push(lines[i]); + const trimmed = lines[i].trim(); + if (/^done\s*<\s*<\(find/.test(trimmed)) return collected.join('\n'); + if (/^fi\s*$/.test(trimmed) && i > startIdx + 3) { + sawFi = true; + // Peek next line — if it's blank and the line after is not part of + // the new-fix `done`, treat fi as terminator (old block). + const next = (lines[i + 1] || '').trim(); + const next2 = (lines[i + 2] || '').trim(); + const newFixContinues = /^done\s*<\s*<\(find/.test(next) || /^done\s*<\s*<\(find/.test(next2); + if (!newFixContinues) { + return collected.join('\n'); + } + } + } + assert.fail(`No terminator found in rescue block of ${filePath}`); + return collected.join('\n'); +} + +function sh(cwd, cmd) { + const r = spawnSync('bash', ['-c', cmd], { cwd, encoding: 'utf-8' }); + if (r.status !== 0) { + throw new Error(`Command failed (${r.status}): ${cmd}\nstdout: ${r.stdout}\nstderr: ${r.stderr}`); + } + return r.stdout; +} + +/** + * Build a temp repo that mirrors the bug repro from issue #2838 and run + * the rescue block against it. Returns { tmp, wt, summaryFinalPath }. + */ +function runRescueScenario(rescueBlock) { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2838-')); + const wt = path.join(tmp, 'wt'); + + sh(tmp, 'git init -q -b main'); + sh(tmp, 'git config user.email test@example.com && git config user.name test'); + fs.writeFileSync(path.join(tmp, '.gitignore'), '.planning/\n'); + fs.writeFileSync(path.join(tmp, 'README.md'), 'init\n'); + sh(tmp, 'git add .gitignore README.md && git commit -q -m init'); + + // Create worktree on a feature branch + sh(tmp, `git worktree add -q -b sim-executor "${wt}"`); + + // Simulate the executor: untracked SUMMARY.md under gitignored .planning/ + const summaryDir = path.join(wt, '.planning', 'quick', '260429-repro'); + fs.mkdirSync(summaryDir, { recursive: true }); + const summarySrc = path.join(summaryDir, '260429-repro-SUMMARY.md'); + fs.writeFileSync(summarySrc, 'status: complete\nrescued: yes\n'); + + // Confirm precondition: --exclude-standard misses the file (this is the bug) + const ls = sh(tmp, `git -C "${wt}" ls-files --modified --others --exclude-standard -- "*SUMMARY.md" || true`); + assert.strictEqual(ls.trim(), '', 'Precondition: --exclude-standard must hide gitignored SUMMARY'); + + // Run the rescue block. It expects WT, WT_BRANCH to be set, and to be run + // from the main repo root. + const script = ` +set -u +WT="${wt}" +WT_BRANCH="sim-executor" +cd "${tmp}" +${rescueBlock} +`; + const r = spawnSync('bash', ['-c', script], { cwd: tmp, encoding: 'utf-8' }); + // Don't fail on non-zero — original block has || true everywhere; we + // judge by outcome. + + // Now do the worktree removal that would have lost the file + sh(tmp, `git worktree remove "${wt}" --force`); + + const summaryFinalPath = path.join(tmp, '.planning', 'quick', '260429-repro', '260429-repro-SUMMARY.md'); + return { tmp, summaryFinalPath, rescueOut: r.stdout + r.stderr }; +} + +function cleanup(tmp) { + try { fs.rmSync(tmp, { recursive: true, force: true }); } catch (_) {} +} + +describe('bug-2838: SUMMARY rescue handles gitignored .planning/', () => { + test('execute-phase.md rescue block recovers SUMMARY when .planning/ is gitignored', () => { + const block = extractRescueBlock(EXECUTE_PHASE_PATH); + const { tmp, summaryFinalPath, rescueOut } = runRescueScenario(block); + try { + assert.ok( + fs.existsSync(summaryFinalPath), + `SUMMARY was lost — rescue did not surface the file into main repo.\nRescue output:\n${rescueOut}` + ); + const content = fs.readFileSync(summaryFinalPath, 'utf-8'); + assert.match(content, /rescued: yes/); + } finally { + cleanup(tmp); + } + }); + + test('quick.md rescue block recovers SUMMARY when .planning/ is gitignored', () => { + const block = extractRescueBlock(QUICK_PATH); + const { tmp, summaryFinalPath, rescueOut } = runRescueScenario(block); + try { + assert.ok( + fs.existsSync(summaryFinalPath), + `SUMMARY was lost — rescue did not surface the file into main repo.\nRescue output:\n${rescueOut}` + ); + const content = fs.readFileSync(summaryFinalPath, 'utf-8'); + assert.match(content, /rescued: yes/); + } finally { + cleanup(tmp); + } + }); + + test('rescue is idempotent when SUMMARY already present in main repo', () => { + const block = extractRescueBlock(EXECUTE_PHASE_PATH); + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2838-idem-')); + const wt = path.join(tmp, 'wt'); + try { + sh(tmp, 'git init -q -b main'); + sh(tmp, 'git config user.email test@example.com && git config user.name test'); + fs.writeFileSync(path.join(tmp, '.gitignore'), '.planning/\n'); + fs.writeFileSync(path.join(tmp, 'README.md'), 'init\n'); + sh(tmp, 'git add .gitignore README.md && git commit -q -m init'); + sh(tmp, `git worktree add -q -b sim-executor "${wt}"`); + + const dir = path.join(wt, '.planning', 'quick', 'x'); + fs.mkdirSync(dir, { recursive: true }); + const body = 'identical\n'; + fs.writeFileSync(path.join(dir, 'x-SUMMARY.md'), body); + + // Pre-place the same content in main repo + const mainDir = path.join(tmp, '.planning', 'quick', 'x'); + fs.mkdirSync(mainDir, { recursive: true }); + fs.writeFileSync(path.join(mainDir, 'x-SUMMARY.md'), body); + + const script = ` +set -u +WT="${wt}" +WT_BRANCH="sim-executor" +cd "${tmp}" +${block} +`; + const r = spawnSync('bash', ['-c', script], { cwd: tmp, encoding: 'utf-8' }); + assert.strictEqual( + r.status, + 0, + `Rescue block failed unexpectedly.\nstdout: ${r.stdout}\nstderr: ${r.stderr}` + ); + // No "Rescued" message expected because cmp -s matches. + assert.doesNotMatch(r.stdout + r.stderr, /Rescued .*x-SUMMARY\.md/); + assert.strictEqual(fs.readFileSync(path.join(mainDir, 'x-SUMMARY.md'), 'utf-8'), body); + } finally { + try { sh(tmp, `git worktree remove "${wt}" --force`); } catch (_) {} + cleanup(tmp); + } + }); +});