From 5c0e801322f7838e50d0a19ddaaf4ec0e5ac28cd Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 10 Apr 2026 21:37:08 -0400 Subject: [PATCH] fix(executor): prohibit git clean in worktree context to prevent file deletions (#2075) (#2076) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running git clean inside a worktree treats files committed on the feature branch as untracked — from the worktree's perspective they were never staged. The executor deletes them, then commits only its own deliverables; when the worktree branch merges back the deletions land on the main branch, destroying prior-wave work (documented across 8 incidents, including commit c6f4753 "Wave 2 executor incorrectly ran git-clean on the worktree"). - Add block to gsd-executor.md explaining exactly why git clean is unsafe in worktree context and what to use instead - Add regression tests (bug-2075-worktree-deletion-safeguards.test.cjs) covering Failure Mode B (git clean prohibition), Failure Mode A (worktree_branch_check presence audit across all worktree-spawning workflows), and both defense-in-depth deletion checks from #1977 Failure Mode A and defense-in-depth checks (post-commit --diff-filter=D in gsd-executor.md, pre-merge --diff-filter=D in execute-phase.md) were already implemented — tests confirm they remain in place. Fixes #2075 Co-authored-by: Claude Sonnet 4.6 --- agents/gsd-executor.md | 25 ++ ...2075-worktree-deletion-safeguards.test.cjs | 219 ++++++++++++++++++ 2 files changed, 244 insertions(+) create mode 100644 tests/bug-2075-worktree-deletion-safeguards.test.cjs diff --git a/agents/gsd-executor.md b/agents/gsd-executor.md index de8a038ec..6cd22195e 100644 --- a/agents/gsd-executor.md +++ b/agents/gsd-executor.md @@ -400,6 +400,31 @@ Intentional deletions (e.g., removing a deprecated file as part of the task) are **7. Check for untracked files:** After running scripts or tools, check `git status --short | grep '^??'`. For any new untracked files: commit if intentional, add to `.gitignore` if generated/runtime output. Never leave generated files untracked. + +**NEVER run `git clean` inside a worktree. This is an absolute rule with no exceptions.** + +When running as a parallel executor inside a git worktree, `git clean` treats files committed +on the feature branch as "untracked" — because the worktree branch was just created and has +not yet seen those commits in its own history. Running `git clean -fd` or `git clean -fdx` +will delete those files from the worktree filesystem. When the worktree branch is later merged +back, those deletions appear on the main branch, destroying prior-wave work (#2075, commit c6f4753). + +**Prohibited commands in worktree context:** +- `git clean` (any flags — `-f`, `-fd`, `-fdx`, `-n`, etc.) +- `git rm` on files not explicitly created by the current task +- `git checkout -- .` or `git restore .` (blanket working-tree resets that discard files) +- `git reset --hard` except inside the `` step at agent startup + +If you need to discard changes to a specific file you modified during this task, use: +```bash +git checkout -- path/to/specific/file +``` +Never use blanket reset or clean operations that affect the entire working tree. + +To inspect what is untracked vs. genuinely new, use `git status --short` and evaluate each +file individually. If a file appears untracked but is not part of your task, leave it alone. + + After all tasks complete, create `{phase}-{plan}-SUMMARY.md` at `.planning/phases/XX-name/`. diff --git a/tests/bug-2075-worktree-deletion-safeguards.test.cjs b/tests/bug-2075-worktree-deletion-safeguards.test.cjs new file mode 100644 index 000000000..e8d53104d --- /dev/null +++ b/tests/bug-2075-worktree-deletion-safeguards.test.cjs @@ -0,0 +1,219 @@ +/** + * Regression tests for #2075: gsd-executor worktree merge systematically + * deletes prior-wave committed files. + * + * Three failure modes documented in issue #2075: + * + * Failure Mode B (PRIMARY — unaddressed before this fix): + * Executor agent runs `git clean` inside the worktree, removing files + * committed on the feature branch. git clean treats them as "untracked" + * from the worktree's perspective and deletes them. The executor then + * commits only its own deliverables; the subsequent merge brings the + * deletions onto the main branch. + * + * Failure Mode A (partially addressed in PR #1982): + * Worktree created from wrong branch base. Audit all worktree-spawning + * workflows for worktree_branch_check presence. + * + * Failure Mode C: + * Stale content from wrong base overwrites shared files. Covered by + * the --hard reset in the worktree_branch_check. + * + * Defense-in-depth (from #1977): + * Post-commit deletion check: already in gsd-executor.md (--diff-filter=D). + * Pre-merge deletion check: already in execute-phase.md (--diff-filter=D). + */ + +'use strict'; + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const EXECUTOR_AGENT_PATH = path.join(__dirname, '..', 'agents', 'gsd-executor.md'); +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'); +const DIAGNOSE_PATH = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'diagnose-issues.md'); + +describe('bug-2075: worktree deletion safeguards', () => { + + describe('Failure Mode B: git clean prohibition in executor agent', () => { + test('gsd-executor.md explicitly prohibits git clean in worktree context', () => { + const content = fs.readFileSync(EXECUTOR_AGENT_PATH, 'utf-8'); + + // Must have an explicit prohibition section mentioning git clean + const prohibitsGitClean = ( + content.includes('git clean') && + ( + /NEVER.*git clean/i.test(content) || + /git clean.*NEVER/i.test(content) || + /do not.*git clean/i.test(content) || + /git clean.*prohibited/i.test(content) || + /prohibited.*git clean/i.test(content) || + /forbidden.*git clean/i.test(content) || + /git clean.*forbidden/i.test(content) || + /must not.*git clean/i.test(content) || + /git clean.*must not/i.test(content) + ) + ); + + assert.ok( + prohibitsGitClean, + 'gsd-executor.md must explicitly prohibit git clean — running it inside a worktree deletes files committed on the feature branch (#2075 Failure Mode B)' + ); + }); + + test('gsd-executor.md git clean prohibition explains the worktree data-loss risk', () => { + const content = fs.readFileSync(EXECUTOR_AGENT_PATH, 'utf-8'); + + // The prohibition must be accompanied by a reason — not just a bare rule + // Look for the word "worktree" near the git clean prohibition + const gitCleanIdx = content.indexOf('git clean'); + assert.ok(gitCleanIdx > -1, 'gsd-executor.md must mention git clean (to prohibit it)'); + + // Extract context around the git clean mention (500 chars either side) + const contextStart = Math.max(0, gitCleanIdx - 500); + const contextEnd = Math.min(content.length, gitCleanIdx + 500); + const context = content.slice(contextStart, contextEnd); + + const hasWorktreeRationale = ( + /worktree/i.test(context) || + /delete/i.test(context) || + /untracked/i.test(context) + ); + + assert.ok( + hasWorktreeRationale, + 'The git clean prohibition in gsd-executor.md must explain why: git clean in a worktree deletes files that appear untracked but are committed on the feature branch' + ); + }); + }); + + describe('Failure Mode A: worktree_branch_check audit across all worktree-spawning workflows', () => { + test('execute-phase.md has worktree_branch_check block with --hard reset', () => { + const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf-8'); + + const blockMatch = content.match(/([\s\S]*?)<\/worktree_branch_check>/); + assert.ok( + blockMatch, + 'execute-phase.md must contain a block' + ); + + const block = blockMatch[1]; + assert.ok( + block.includes('reset --hard'), + 'execute-phase.md worktree_branch_check must use git reset --hard (not --soft)' + ); + assert.ok( + !block.includes('reset --soft'), + 'execute-phase.md worktree_branch_check must not use git reset --soft' + ); + }); + + test('quick.md has worktree_branch_check block with --hard reset', () => { + const content = fs.readFileSync(QUICK_PATH, 'utf-8'); + + const blockMatch = content.match(/([\s\S]*?)<\/worktree_branch_check>/); + assert.ok( + blockMatch, + 'quick.md must contain a block' + ); + + const block = blockMatch[1]; + assert.ok( + block.includes('reset --hard'), + 'quick.md worktree_branch_check must use git reset --hard (not --soft)' + ); + assert.ok( + !block.includes('reset --soft'), + 'quick.md worktree_branch_check must not use git reset --soft' + ); + }); + + test('diagnose-issues.md has worktree_branch_check instruction for spawned agents', () => { + const content = fs.readFileSync(DIAGNOSE_PATH, 'utf-8'); + + assert.ok( + content.includes('worktree_branch_check'), + 'diagnose-issues.md must include worktree_branch_check instruction for spawned debug agents' + ); + + assert.ok( + content.includes('reset --hard'), + 'diagnose-issues.md worktree_branch_check must instruct agents to use git reset --hard' + ); + }); + }); + + describe('Defense-in-depth: post-commit deletion check (from #1977)', () => { + test('gsd-executor.md task_commit_protocol has post-commit deletion verification', () => { + const content = fs.readFileSync(EXECUTOR_AGENT_PATH, 'utf-8'); + + assert.ok( + content.includes('--diff-filter=D'), + 'gsd-executor.md must include --diff-filter=D to detect accidental file deletions after each commit' + ); + + // Must have a warning about unexpected deletions + assert.ok( + content.includes('DELETIONS') || content.includes('WARNING'), + 'gsd-executor.md must emit a warning when a commit includes unexpected file deletions' + ); + }); + }); + + describe('Defense-in-depth: pre-merge deletion check (from #1977)', () => { + test('execute-phase.md worktree merge section has pre-merge deletion check', () => { + const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf-8'); + + const worktreeCleanupStart = content.indexOf('Worktree cleanup'); + assert.ok( + worktreeCleanupStart > -1, + 'execute-phase.md must have a worktree cleanup section' + ); + + const cleanupSection = content.slice(worktreeCleanupStart); + + assert.ok( + cleanupSection.includes('--diff-filter=D'), + 'execute-phase.md worktree cleanup must use --diff-filter=D to block deletion-introducing merges' + ); + + // Deletion check must appear before git merge + const deletionCheckIdx = cleanupSection.indexOf('--diff-filter=D'); + const gitMergeIdx = cleanupSection.indexOf('git merge'); + assert.ok( + deletionCheckIdx < gitMergeIdx, + '--diff-filter=D deletion check must appear before git merge in the worktree cleanup section' + ); + + assert.ok( + cleanupSection.includes('BLOCKED') || cleanupSection.includes('deletion'), + 'execute-phase.md must block or warn when the worktree branch contains file deletions' + ); + }); + + test('quick.md worktree merge section has pre-merge deletion check', () => { + const content = fs.readFileSync(QUICK_PATH, 'utf-8'); + + const mergeIdx = content.indexOf('git merge'); + assert.ok(mergeIdx > -1, 'quick.md must contain a git merge operation'); + + // Find the worktree cleanup block (starts after "Worktree cleanup") + const worktreeCleanupStart = content.indexOf('Worktree cleanup'); + assert.ok( + worktreeCleanupStart > -1, + 'quick.md must have a worktree cleanup section' + ); + + const cleanupSection = content.slice(worktreeCleanupStart); + + assert.ok( + cleanupSection.includes('--diff-filter=D') || cleanupSection.includes('diff-filter'), + 'quick.md worktree cleanup must check for file deletions before merging' + ); + }); + }); + +});