From e115909fd0246907f0e68d360ca76e3e32ccae44 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 25 Mar 2026 02:06:04 -0400 Subject: [PATCH] fix: detect and preserve submodule state during worktree teardown (#2337) (#2425) Worktree teardown with --force destroyed uncommitted changes in submodule directories. Now detects .gitmodules, checks submodule status for uncommitted changes, and stashes them before removal. When submodules have dirty state, attempts non-force removal first. Fixes #2337 Co-authored-by: Claude Opus 4.6 (1M context) --- .../tests/worktree-submodule-safety.test.ts | 65 +++++++++++++++++++ .../extensions/gsd/worktree-manager.ts | 45 ++++++++++++- 2 files changed, 108 insertions(+), 2 deletions(-) create mode 100644 src/resources/extensions/gsd/tests/worktree-submodule-safety.test.ts diff --git a/src/resources/extensions/gsd/tests/worktree-submodule-safety.test.ts b/src/resources/extensions/gsd/tests/worktree-submodule-safety.test.ts new file mode 100644 index 000000000..c32b8fe80 --- /dev/null +++ b/src/resources/extensions/gsd/tests/worktree-submodule-safety.test.ts @@ -0,0 +1,65 @@ +/** + * worktree-submodule-safety.test.ts — #2337 + * + * Worktree teardown (removeWorktree) uses --force which destroys + * uncommitted changes in submodule directories. This test verifies + * that the removal logic detects submodules and preserves their state. + */ + +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { createTestContext } from "./test-helpers.ts"; + +const { assertTrue, report } = createTestContext(); + +const srcPath = join(import.meta.dirname, "..", "worktree-manager.ts"); +const src = readFileSync(srcPath, "utf-8"); + +console.log("\n=== #2337: Worktree teardown preserves submodule state ==="); + +// ── Test 1: removeWorktree function exists ────────────────────────────── + +const removeWorktreeIdx = src.indexOf("export function removeWorktree"); +assertTrue(removeWorktreeIdx > 0, "worktree-manager.ts exports removeWorktree"); + +const fnBody = src.slice(removeWorktreeIdx, removeWorktreeIdx + 3000); + +// ── Test 2: The function checks for submodules before force removal ───── + +const checksSubmodules = + fnBody.includes("submodule") || + fnBody.includes(".gitmodules"); + +assertTrue( + checksSubmodules, + "removeWorktree checks for submodules before force removal (#2337)", +); + +// ── Test 3: Submodule changes are stashed or warned about ─────────────── + +const preservesSubmoduleState = + fnBody.includes("stash") || + fnBody.includes("uncommitted") || + fnBody.includes("dirty") || + fnBody.includes("submodule") && (fnBody.includes("warn") || fnBody.includes("preserv")); + +assertTrue( + preservesSubmoduleState, + "removeWorktree preserves or warns about submodule uncommitted changes (#2337)", +); + +// ── Test 4: Force removal is skipped when submodules have changes ─────── + +// The key fix: when submodules have dirty state, we should NOT use force +// removal. Instead, use non-force first and fall back to force only after +// submodule state is preserved. +const hasConditionalForce = + fnBody.includes("submodule") && + (fnBody.includes("force") || fnBody.includes("--force")); + +assertTrue( + hasConditionalForce, + "removeWorktree has conditional force logic around submodules (#2337)", +); + +report(); diff --git a/src/resources/extensions/gsd/worktree-manager.ts b/src/resources/extensions/gsd/worktree-manager.ts index 23ba831a6..238077abd 100644 --- a/src/resources/extensions/gsd/worktree-manager.ts +++ b/src/resources/extensions/gsd/worktree-manager.ts @@ -16,6 +16,7 @@ */ import { existsSync, mkdirSync, readFileSync, realpathSync, rmSync } from "node:fs"; +import { execFileSync } from "node:child_process"; import { join, resolve, sep } from "node:path"; import { GSDError, GSD_PARSE_ERROR, GSD_STALE_STATE, GSD_LOCK_HELD, GSD_GIT_ERROR, GSD_MERGE_CONFLICT } from "./errors.js"; import { @@ -321,8 +322,48 @@ export function removeWorktree( return; } - // Remove worktree using the resolved path (force if requested, to handle dirty worktrees) - try { nativeWorktreeRemove(basePath, resolvedWtPath, force); } catch { /* may fail */ } + // Submodule safety (#2337): detect submodules with uncommitted changes + // before force-removing the worktree. Force removal destroys all uncommitted + // state, which is especially destructive for submodule directories. + let hasSubmoduleChanges = false; + const gitmodulesPath = join(resolvedWtPath, ".gitmodules"); + if (existsSync(gitmodulesPath)) { + try { + const submoduleStatus = execFileSync( + "git", ["submodule", "status"], + { cwd: resolvedWtPath, stdio: ["ignore", "pipe", "pipe"], encoding: "utf-8" }, + ).trim(); + // Lines starting with '+' indicate uncommitted submodule changes + hasSubmoduleChanges = submoduleStatus.split("\n").some( + (line: string) => line.startsWith("+") || line.startsWith("-"), + ); + if (hasSubmoduleChanges) { + // Stash submodule changes so they are not lost during force removal. + // The stash is created in the worktree before it's torn down. + try { + execFileSync( + "git", ["stash", "push", "-m", "gsd: auto-stash submodule changes before worktree teardown"], + { cwd: resolvedWtPath, stdio: ["ignore", "pipe", "pipe"], encoding: "utf-8" }, + ); + process.stderr.write( + `[GSD] WARNING: Stashed uncommitted submodule changes in ${resolvedWtPath} before worktree teardown.\n`, + ); + } catch { + // Stash failed — warn the user that submodule changes may be lost + process.stderr.write( + `[GSD] WARNING: Submodule changes detected in ${resolvedWtPath} — stash failed, changes may be lost during force removal.\n`, + ); + } + } + } catch { + // submodule status failed — proceed with normal removal + } + } + + // Remove worktree: try non-force first when submodules have changes, + // falling back to force only after submodule state has been preserved. + const useForce = hasSubmoduleChanges ? false : force; + try { nativeWorktreeRemove(basePath, resolvedWtPath, useForce); } catch { /* may fail */ } // If the directory is still there (e.g. locked), try harder with force if (existsSync(resolvedWtPath)) {