From 6f15ddcbf7628677f744dee0c6931abb0c98df25 Mon Sep 17 00:00:00 2001 From: Jeremy McSpadden Date: Fri, 20 Mar 2026 14:15:11 -0500 Subject: [PATCH] fix(gsd): close residual #1364 data-loss vectors on v2.36.0+ (#1637) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two targeted fixes that close the three remaining paths where .gsd/ tracked files can still be silently deleted after the v2.36.0 fix. --- Path 1: hasGitTrackedGsdFiles fails open on git error (gitignore.ts) nativeLsFiles() swallows git failures via allowFailure=true and returns [], making hasGitTrackedGsdFiles() indistinguishable between "nothing tracked" and "git failed". On any transient git failure (locked index, binary not on PATH, corrupted .git/index), the function returned false and .gsd was added to .gitignore, deleting all tracked state. Fix: after nativeLsFiles returns [], verify git is reachable with a cheap rev-parse call. If git is unavailable, return true (fail safe — assume tracked). The outer catch also returns true instead of false. --- Path 2: migration never cleans git index (migrate-external.ts) migrateToExternalState() correctly creates the .gsd symlink/junction but never ran `git rm -r --cached .gsd/`. All previously tracked .gsd/* files remained in the git index pointing through the new symlink, which git cannot follow — causing PROJECT.md, milestones/, REQUIREMENTS.md etc. to appear as deleted in git status immediately after every migration. Fix: after the symlink is verified, run: git rm -r --cached --ignore-unmatch .gsd --ignore-unmatch makes this a no-op on fresh/untracked projects. --- Path 3: race between migration and ensureGitignore Resolved by Path 2. If migration always cleans the index, the race window (another process converting .gsd/ to a symlink between the migrateToExternalState() and ensureGitignore() calls) is harmless — the index is already clean and there is nothing to lose. --- Tests added (gitignore-tracked-gsd.test.ts) - hasGitTrackedGsdFiles returns true (fail-safe) when git is unavailable (simulated via .git/index.lock to force git ls-files failure) - migrateToExternalState cleans git index so tracked files don't show as deleted after successful migration Fixes residual vectors from #1364 (original fix: #1367, v2.36.0) --- src/resources/extensions/gsd/gitignore.ts | 20 ++++++-- .../extensions/gsd/migrate-external.ts | 19 ++++++- .../gsd/tests/gitignore-tracked-gsd.test.ts | 50 +++++++++++++++++++ 3 files changed, 85 insertions(+), 4 deletions(-) diff --git a/src/resources/extensions/gsd/gitignore.ts b/src/resources/extensions/gsd/gitignore.ts index 04b166225..cb65f8c00 100644 --- a/src/resources/extensions/gsd/gitignore.ts +++ b/src/resources/extensions/gsd/gitignore.ts @@ -7,9 +7,11 @@ */ import { join } from "node:path"; +import { execFileSync } from "node:child_process"; import { existsSync, lstatSync, readFileSync, writeFileSync } from "node:fs"; import { nativeRmCached, nativeLsFiles } from "./native-git-bridge.js"; import { gsdRoot } from "./paths.js"; +import { GIT_NO_PROMPT_ENV } from "./git-constants.js"; /** * GSD runtime patterns for git index cleanup. @@ -104,10 +106,22 @@ export function hasGitTrackedGsdFiles(basePath: string): boolean { // Check if git tracks any files under .gsd/ try { const tracked = nativeLsFiles(basePath, ".gsd"); - return tracked.length > 0; - } catch { - // Not a git repo or git not available — safe to proceed + if (tracked.length > 0) return true; + + // nativeLsFiles swallows git failures and returns []. An empty result + // could mean "nothing tracked" OR "git failed silently". Verify git is + // reachable before trusting the empty result — if it isn't, fail safe + // by assuming files ARE tracked to prevent data loss. + execFileSync("git", ["rev-parse", "--git-dir"], { + cwd: basePath, + stdio: "pipe", + env: GIT_NO_PROMPT_ENV, + }); + return false; + } catch { + // git unavailable, index locked, or repo corrupt — fail safe + return true; } } diff --git a/src/resources/extensions/gsd/migrate-external.ts b/src/resources/extensions/gsd/migrate-external.ts index 4e36f8978..4fd53e7d1 100644 --- a/src/resources/extensions/gsd/migrate-external.ts +++ b/src/resources/extensions/gsd/migrate-external.ts @@ -6,11 +6,13 @@ * symlink replaces the original directory so all paths remain valid. */ +import { execFileSync } from "node:child_process"; import { existsSync, lstatSync, mkdirSync, readdirSync, realpathSync, renameSync, cpSync, rmSync, symlinkSync } from "node:fs"; import { join } from "node:path"; import { externalGsdRoot } from "./repo-identity.js"; import { getErrorMessage } from "./error-utils.js"; import { hasGitTrackedGsdFiles } from "./gitignore.js"; +import { GIT_NO_PROMPT_ENV } from "./git-constants.js"; export interface MigrationResult { migrated: boolean; @@ -144,7 +146,22 @@ export function migrateToExternalState(basePath: string): MigrationResult { return { migrated: false, error: `Migration verification failed: ${getErrorMessage(verifyErr)}` }; } - // Remove .gsd.migrating only after symlink is verified + // Clean the git index — any .gsd/* files tracked before migration now + // sit behind the symlink and git can't follow it, causing them to show + // as deleted. Remove them from the index so the working tree stays clean. + // --ignore-unmatch makes this a no-op on fresh projects with no tracked .gsd/. + try { + execFileSync("git", ["rm", "-r", "--cached", "--ignore-unmatch", ".gsd"], { + cwd: basePath, + stdio: ["ignore", "pipe", "ignore"], + env: GIT_NO_PROMPT_ENV, + timeout: 10_000, + }); + } catch { + // Non-fatal — git may be unavailable or nothing was tracked + } + + // Remove .gsd.migrating only after symlink is verified and index is clean rmSync(migratingPath, { recursive: true, force: true }); return { migrated: true }; diff --git a/src/resources/extensions/gsd/tests/gitignore-tracked-gsd.test.ts b/src/resources/extensions/gsd/tests/gitignore-tracked-gsd.test.ts index b0b7301fd..b9bda919a 100644 --- a/src/resources/extensions/gsd/tests/gitignore-tracked-gsd.test.ts +++ b/src/resources/extensions/gsd/tests/gitignore-tracked-gsd.test.ts @@ -183,6 +183,28 @@ test("ensureGitignore with tracked .gsd/ does not cause git to see files as dele } }); +test("hasGitTrackedGsdFiles returns true (fail-safe) when git is not available", () => { + const dir = makeTempRepo(); + try { + // Create and track .gsd/ files + mkdirSync(join(dir, ".gsd"), { recursive: true }); + writeFileSync(join(dir, ".gsd", "PROJECT.md"), "# Project\n"); + git(dir, "add", ".gsd/"); + git(dir, "commit", "-m", "track gsd"); + + // Corrupt the git index to simulate git failure + const indexPath = join(dir, ".git", "index.lock"); + writeFileSync(indexPath, "locked"); + + // Should fail safe — assume tracked rather than silently returning false + // (The index lock causes git ls-files to fail; rev-parse also fails → true) + const result = hasGitTrackedGsdFiles(dir); + assert.equal(result, true, "Should return true (fail-safe) when git is unavailable"); + } finally { + cleanup(dir); + } +}); + // ─── migrateToExternalState — tracked .gsd/ protection ────────────── test("migrateToExternalState aborts when .gsd/ has tracked files (#1364)", () => { @@ -212,3 +234,31 @@ test("migrateToExternalState aborts when .gsd/ has tracked files (#1364)", () => cleanup(dir); } }); + +test("migrateToExternalState cleans git index so tracked files don't show as deleted (#1364 path 2)", () => { + const dir = makeTempRepo(); + try { + // Track .gsd/ files, then untrack them so migration proceeds + mkdirSync(join(dir, ".gsd", "milestones", "M001"), { recursive: true }); + writeFileSync(join(dir, ".gsd", "PROJECT.md"), "# Project\n"); + writeFileSync(join(dir, ".gsd", "milestones", "M001", "PLAN.md"), "# Plan\n"); + git(dir, "add", ".gsd/"); + git(dir, "commit", "-m", "track gsd state"); + git(dir, "rm", "-r", "--cached", ".gsd/"); + git(dir, "commit", "-m", "untrack gsd (simulates pre-migration project)"); + + const result = migrateToExternalState(dir); + assert.equal(result.migrated, true, "Migration should succeed"); + + // git status must show NO deleted files after migration + const status = git(dir, "status", "--porcelain"); + const deletions = status.split("\n").filter((l) => /^\s*D\s/.test(l) || /^D\s/.test(l)); + assert.equal( + deletions.length, + 0, + `Expected no deleted files after migration, but found:\n${deletions.join("\n")}`, + ); + } finally { + cleanup(dir); + } +});