fix(gsd): worktree teardown path validation prevents data loss (#3311)

* fix(gsd): validate worktree paths before removal to prevent data loss

Fixes #2365

removeWorktree() overrides its computed path with whatever git reports
from `git worktree list`. When .gsd/ was a symlink, git resolves it at
creation time, so the registered path can point to an external directory.
Teardown with --force on that path destroys user data.

Add isInsideWorktreesDir() containment check that resolves ".." traversals
and validates the target is under .gsd/worktrees/ before any destructive
operation (nativeWorktreeRemove --force, rmSync). Paths outside containment
get a non-force git worktree remove only, with a warning logged.

Also guard the fallback rmSync in teardownAutoWorktree() with the same
containment check.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor: convert worktree-teardown-safety test to node:test format

Replace main() function wrapper pattern with proper describe/it blocks
using node:test, matching the project's standard test conventions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: replace empty catch blocks with logWarning in worktree-manager

* fix(test): increase removeWorktree slice window from 3000 to 6000 chars

The PR's path-validation additions expanded removeWorktree beyond the
3000-char snapshot window, pushing the submodule safety section out of
scope and causing Tests 3 and 4 to see empty strings.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: trek-e <trek-e@users.noreply.github.com>

src/resources/extensions/gsd/auto-worktree.ts

@@ -36,6 +36,7 @@ import {
   removeWorktree,
   resolveGitDir,
   worktreePath,
+  isInsideWorktreesDir,
 } from "./worktree-manager.js";
 import {
   detectWorktreeName,
@@ -1204,12 +1205,19 @@ export function teardownAutoWorktree(
         `Remove it manually with: rm -rf "${wtDir.replaceAll("\\", "/")}"`,
       { worktree: milestoneId },
     );
-    // Attempt a direct filesystem removal as a fallback
-    try {
-      rmSync(wtDir, { recursive: true, force: true });
-    } catch (err) {
-      // Non-fatal — the warning above tells the user how to clean up
-      logWarning("worktree", `worktree directory removal failed: ${err instanceof Error ? err.message : String(err)}`);
+    // Attempt a direct filesystem removal as a fallback — but ONLY if the
+    // path is safely inside .gsd/worktrees/ to prevent #2365 data loss.
+    if (isInsideWorktreesDir(originalBasePath, wtDir)) {
+      try {
+        rmSync(wtDir, { recursive: true, force: true });
+      } catch (err) {
+        // Non-fatal — the warning above tells the user how to clean up
+        logWarning("worktree", `worktree directory removal failed: ${err instanceof Error ? err.message : String(err)}`);
+      }
+    } else {
+      console.error(
+        `[GSD] REFUSING fallback rmSync — path is outside .gsd/worktrees/: ${wtDir}`,
+      );
     }
   }
 }

src/resources/extensions/gsd/tests/worktree-submodule-safety.test.ts

@@ -22,7 +22,7 @@ console.log("\n=== #2337: Worktree teardown preserves submodule state ===");
 const removeWorktreeIdx = src.indexOf("export function removeWorktree");
 assertTrue(removeWorktreeIdx > 0, "worktree-manager.ts exports removeWorktree");
 
-const fnBody = src.slice(removeWorktreeIdx, removeWorktreeIdx + 3000);
+const fnBody = src.slice(removeWorktreeIdx, removeWorktreeIdx + 6000);
 
 // ── Test 2: The function checks for submodules before force removal ─────
 

src/resources/extensions/gsd/tests/worktree-teardown-safety.test.ts

@@ -0,0 +1,148 @@
+/**
+ * worktree-teardown-safety.test.ts — Regression test for #2365.
+ *
+ * Ensures that removeWorktree() and teardownAutoWorktree() never delete
+ * directories outside .gsd/worktrees/.  The bug: removeWorktree overrides
+ * the computed worktree path with whatever `git worktree list` reports.
+ * When .gsd/ was (or is) a symlink, git resolves the symlink at creation
+ * time, so its registered path can point to an external directory.  If that
+ * external path happens to be a project data directory, teardown destroys it.
+ *
+ * The fix adds path validation so rmSync / nativeWorktreeRemove only operate
+ * on paths that are actually under .gsd/worktrees/.
+ */
+
+import {
+  mkdtempSync,
+  mkdirSync,
+  writeFileSync,
+  rmSync,
+  existsSync,
+  realpathSync,
+  readFileSync,
+} from "node:fs";
+import { join } from "node:path";
+import { tmpdir } from "node:os";
+import { execSync } from "node:child_process";
+import { describe, it, after } from "node:test";
+
+import { createWorktree, removeWorktree, worktreePath, isInsideWorktreesDir } from "../worktree-manager.ts";
+import { createTestContext } from "./test-helpers.ts";
+
+const { assertEq, assertTrue, report } = createTestContext();
+
+// ─── Helpers ──────────────────────────────────────────────────────────────
+
+function run(command: string, cwd: string): string {
+  return execSync(command, { cwd, stdio: ["ignore", "pipe", "pipe"], encoding: "utf-8" }).trim();
+}
+
+function createTempRepo(): string {
+  const dir = realpathSync(mkdtempSync(join(tmpdir(), "wt-safety-test-")));
+  run("git init", dir);
+  run("git config user.email test@test.com", dir);
+  run("git config user.name Test", dir);
+  writeFileSync(join(dir, "README.md"), "# test\n");
+  run("git add .", dir);
+  run("git commit -m init", dir);
+  run("git branch -M main", dir);
+  return dir;
+}
+
+// ─── Tests ────────────────────────────────────────────────────────────────
+
+describe("worktree-teardown-safety", () => {
+  const dirs: string[] = [];
+
+  after(() => {
+    for (const d of dirs) rmSync(d, { recursive: true, force: true });
+    report();
+  });
+
+  it("removeWorktree does not delete sibling data directories", () => {
+    const tempDir = createTempRepo();
+    dirs.push(tempDir);
+
+    // Create a project data directory that lives alongside .gsd/
+    const dataDir = join(tempDir, "project-data");
+    mkdirSync(dataDir, { recursive: true });
+    writeFileSync(join(dataDir, "important.db"), "precious data");
+
+    // Create a worktree normally
+    const wt = createWorktree(tempDir, "test-wt");
+    assertTrue(existsSync(wt.path), "worktree created successfully");
+
+    // Remove the worktree
+    removeWorktree(tempDir, "test-wt");
+
+    // The worktree directory should be gone
+    assertTrue(!existsSync(wt.path), "worktree directory removed");
+
+    // The project data directory MUST still exist
+    assertTrue(existsSync(dataDir), "project data directory survives teardown");
+    assertTrue(
+      existsSync(join(dataDir, "important.db")),
+      "project data files survive teardown",
+    );
+  });
+
+  it("path validation rejects paths outside .gsd/worktrees/", () => {
+    const tempDir = createTempRepo();
+    dirs.push(tempDir);
+
+    const externalDir = join(tempDir, "external-state");
+    mkdirSync(externalDir, { recursive: true });
+    writeFileSync(join(externalDir, "state.json"), '{"critical": true}');
+
+    // Create and then remove a worktree that has a legitimate path
+    const wt2 = createWorktree(tempDir, "safe-wt");
+    assertTrue(existsSync(wt2.path), "second worktree created");
+
+    removeWorktree(tempDir, "safe-wt");
+    assertTrue(!existsSync(wt2.path), "second worktree removed cleanly");
+
+    // External directory must be untouched
+    assertTrue(existsSync(externalDir), "external directory survives second teardown");
+    assertEq(
+      readFileSync(join(externalDir, "state.json"), "utf-8"),
+      '{"critical": true}',
+      "external directory contents intact after teardown",
+    );
+  });
+
+  it("worktreePath always returns paths under .gsd/worktrees/", () => {
+    const tempDir = createTempRepo();
+    dirs.push(tempDir);
+
+    const wtPathResult = worktreePath(tempDir, "anything");
+    assertTrue(
+      wtPathResult.startsWith(join(tempDir, ".gsd", "worktrees")),
+      "worktreePath returns path under .gsd/worktrees/",
+    );
+  });
+
+  it("isInsideWorktreesDir rejects path traversal attempts", () => {
+    const tempDir = createTempRepo();
+    dirs.push(tempDir);
+
+    assertTrue(
+      isInsideWorktreesDir(tempDir, join(tempDir, ".gsd", "worktrees", "my-wt")),
+      "path inside .gsd/worktrees/ is accepted",
+    );
+
+    assertTrue(
+      !isInsideWorktreesDir(tempDir, join(tempDir, "project-data")),
+      "path outside .gsd/worktrees/ is rejected",
+    );
+
+    assertTrue(
+      !isInsideWorktreesDir(tempDir, join(tempDir, ".gsd", "worktrees", "..", "..", "project-data")),
+      "path traversal via .. is rejected",
+    );
+
+    assertTrue(
+      !isInsideWorktreesDir(tempDir, "/tmp/some-other-dir"),
+      "completely external path is rejected",
+    );
+  });
+});

src/resources/extensions/gsd/worktree-manager.ts

@@ -113,6 +113,22 @@ export function worktreeBranchName(name: string): string {
   return `worktree/${name}`;
 }
 
+/**
+ * Validate that a path is inside the .gsd/worktrees/ directory.
+ * Resolves symlinks and normalizes ".." traversals before comparison
+ * so that a symlink-resolved or crafted path cannot escape containment.
+ *
+ * Used as a safety gate before any destructive operation (rmSync,
+ * nativeWorktreeRemove --force) to prevent #2365-style data loss.
+ */
+export function isInsideWorktreesDir(basePath: string, targetPath: string): boolean {
+  const wtDir = resolve(worktreesDir(basePath));
+  const resolved = resolve(targetPath);
+  // The resolved path must start with the worktrees dir followed by a separator,
+  // not merely be a prefix match (e.g. ".gsd/worktrees-extra" must not match).
+  return resolved === wtDir || resolved.startsWith(wtDir + sep);
+}
+
 // ─── Core Operations ───────────────────────────────────────────────────────
 
 /**
@@ -370,16 +386,37 @@ export function removeWorktree(
   // time, so its registered path points to the resolved external location.
   // If syncStateToProjectRoot later creates a real .gsd/ directory that
   // shadows the symlink, the computed path diverges from git's record.
+  let gitReportedPath: string | null = null;
   try {
     const entries = nativeWorktreeList(basePath);
     const entry = entries.find(e => e.branch === branch);
     if (entry?.path) {
-      wtPath = entry.path;
+      gitReportedPath = entry.path;
     }
   } catch (e) { logWarning("worktree", `nativeWorktreeList parse failed: ${(e as Error).message}`); }
 
+  // Safety gate (#2365): only use the git-reported path if it is actually
+  // inside .gsd/worktrees/.  When .gsd/ was a symlink, git may have resolved
+  // it to an external directory (e.g. a project data folder).  Using that
+  // path for removal would destroy user data.
+  if (gitReportedPath && isInsideWorktreesDir(basePath, gitReportedPath)) {
+    wtPath = gitReportedPath;
+  } else if (gitReportedPath) {
+    console.error(
+      `[GSD] WARNING: git worktree list reported path outside .gsd/worktrees/: ${gitReportedPath}\n` +
+        `  Refusing to use it for removal — falling back to computed path: ${wtPath}`,
+    );
+    // Still tell git to unregister the worktree entry via its reported path,
+    // but do NOT use force and do NOT fall back to rmSync on this path.
+    try { nativeWorktreeRemove(basePath, gitReportedPath, false); } catch (e) { logWarning("worktree", `non-force worktree remove failed for ${gitReportedPath}: ${e instanceof Error ? e.message : String(e)}`); }
+  }
+
   const resolvedWtPath = existsSync(wtPath) ? realpathSync(wtPath) : wtPath;
 
+  // Double-check: the resolved path (after symlink resolution) must also be
+  // inside .gsd/worktrees/ — a symlink inside the directory could point out.
+  const resolvedPathSafe = isInsideWorktreesDir(basePath, resolvedWtPath);
+
   // If we're inside the worktree, move out first — git can't remove an in-use directory
   const cwd = process.cwd();
   const resolvedCwd = existsSync(cwd) ? realpathSync(cwd) : cwd;
@@ -453,37 +490,48 @@ export function removeWorktree(
     }
   }
 
-  // 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 (e) { logWarning("worktree", `nativeWorktreeRemove failed: ${(e as Error).message}`); }
+  // Remove worktree — only use force/rmSync when the path is safely contained
+  if (resolvedPathSafe) {
+    // 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 (e) { logWarning("worktree", `nativeWorktreeRemove failed: ${(e as Error).message}`); }
 
-  // If the directory is still there (e.g. locked), try harder with force
-  if (existsSync(resolvedWtPath)) {
-    try { nativeWorktreeRemove(basePath, resolvedWtPath, true); } catch (e) { logWarning("worktree", `nativeWorktreeRemove (force) failed: ${(e as Error).message}`); }
-  }
-
-  // (#2821) If the worktree directory STILL exists after both native removal
-  // attempts (e.g. untracked files like ASSESSMENT/UAT-RESULT prevent git
-  // worktree remove), force-remove the git internal worktree metadata first,
-  // then remove the filesystem directory. Without this, the .git/worktrees/<name>
-  // lock prevents rmSync from cleaning up, and the orphaned worktree directory
-  // causes every subsequent `/gsd auto` to re-enter the stale worktree.
-  if (existsSync(resolvedWtPath)) {
-    try {
-      const wtInternalDir = join(basePath, ".git", "worktrees", name);
-      if (existsSync(wtInternalDir)) {
-        rmSync(wtInternalDir, { recursive: true, force: true });
-      }
-      rmSync(resolvedWtPath, { recursive: true, force: true });
-    } catch {
-      logWarning(
-        "reconcile",
-        `Worktree directory could not be removed after git internal cleanup: ${resolvedWtPath}. ` +
-          `Manual cleanup: rm -rf "${resolvedWtPath.replaceAll("\\", "/")}"`,
-        { worktree: name },
-      );
+    // If the directory is still there (e.g. locked), try harder with force
+    if (existsSync(resolvedWtPath)) {
+      try { nativeWorktreeRemove(basePath, resolvedWtPath, true); } catch (e) { logWarning("worktree", `nativeWorktreeRemove (force) failed: ${(e as Error).message}`); }
     }
+
+    // (#2821) If the worktree directory STILL exists after both native removal
+    // attempts (e.g. untracked files like ASSESSMENT/UAT-RESULT prevent git
+    // worktree remove), force-remove the git internal worktree metadata first,
+    // then remove the filesystem directory. Without this, the .git/worktrees/<name>
+    // lock prevents rmSync from cleaning up, and the orphaned worktree directory
+    // causes every subsequent `/gsd auto` to re-enter the stale worktree.
+    if (existsSync(resolvedWtPath)) {
+      try {
+        const wtInternalDir = join(basePath, ".git", "worktrees", name);
+        if (existsSync(wtInternalDir)) {
+          rmSync(wtInternalDir, { recursive: true, force: true });
+        }
+        rmSync(resolvedWtPath, { recursive: true, force: true });
+      } catch {
+        logWarning(
+          "reconcile",
+          `Worktree directory could not be removed after git internal cleanup: ${resolvedWtPath}. ` +
+            `Manual cleanup: rm -rf "${resolvedWtPath.replaceAll("\\", "/")}"`,
+          { worktree: name },
+        );
+      }
+    }
+  } else {
+    // Path is outside containment — only do a non-force git worktree remove
+    // (which refuses to delete dirty worktrees) and never fall back to rmSync.
+    console.error(
+      `[GSD] WARNING: Resolved worktree path is outside .gsd/worktrees/: ${resolvedWtPath}\n` +
+        `  Skipping forced removal to prevent data loss.`,
+    );
+    try { nativeWorktreeRemove(basePath, resolvedWtPath, false); } catch (e) { logWarning("worktree", `non-force worktree remove failed for ${resolvedWtPath}: ${e instanceof Error ? e.message : String(e)}`); }
   }
 
   // Prune stale entries so git knows the worktree is gone