diff --git a/.changeset/fix-1422-1447-projectroot-milestone-guards.md b/.changeset/fix-1422-1447-projectroot-milestone-guards.md new file mode 100644 index 000000000..a2c23456a --- /dev/null +++ b/.changeset/fix-1422-1447-projectroot-milestone-guards.md @@ -0,0 +1,9 @@ +--- +type: Fixed +pr: 1484 +--- +**`findProjectRoot` now respects explicit `sub_repos` config over implicit `.git`** — when a parent workspace's `.planning/config.json` lists a child directory in `sub_repos`, that declaration takes precedence over the child's own `.git/` directory. Previously, if the child had both `.planning/` and `.git/`, the `.git` heuristic fired first and resolved to the child rather than the parent workspace, making the `sub_repos` declaration ineffective. (#1422) + +**`phases clear` now refuses to delete phase directories with uncommitted changes** — `cmdPhasesClear` runs `git status --porcelain` over the phases directory before executing any deletion. If uncommitted or staged-but-not-committed files are found it aborts with a clear error message, preventing silent data loss at `new-milestone` time. Pass `--force` to bypass the guard when archival is already complete. Non-git projects are unaffected. (#1447, data-loss fix) + + diff --git a/src/milestone.cts b/src/milestone.cts index e06dc6926..f15c9baad 100644 --- a/src/milestone.cts +++ b/src/milestone.cts @@ -14,7 +14,7 @@ import planningWorkspace = require('./planning-workspace.cjs'); import frontmatterMod = require('./frontmatter.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports -- state.cjs is an export= CommonJS module import stateMod = require('./state.cjs'); -import { platformWriteSync, platformEnsureDir } from './shell-command-projection.cjs'; +import { platformWriteSync, platformEnsureDir, execGit } from './shell-command-projection.cjs'; import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); @@ -390,6 +390,9 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo function cmdPhasesClear(cwd: string, raw: boolean, args: string[]): void { const phasesDir = planningPaths(cwd).phases; const confirm = Array.isArray(args) && args.includes('--confirm'); + // --force bypasses the uncommitted-changes guard. Only use when the caller + // has already archived or explicitly accepts loss of uncommitted work. (#1447) + const force = Array.isArray(args) && args.includes('--force'); let cleared = 0; if (fs.existsSync(phasesDir)) { @@ -403,6 +406,45 @@ function cmdPhasesClear(cwd: string, raw: boolean, args: string[]): void { ); } + // Guard (#1447): refuse to hard-delete phase directories that contain + // uncommitted changes. This prevents data loss when `new-milestone` runs + // `phases.clear --confirm` before the operator has archived or committed + // phase work from the outgoing milestone. + // Use `--force` to bypass this guard only when you have verified that + // archive or commit of the outgoing phases is already done. + if (dirs.length > 0 && !force) { + // Compute the path relative to cwd for git status + let relPhasesDir: string; + try { + relPhasesDir = path.relative(cwd, phasesDir); + } catch { + relPhasesDir = phasesDir; + } + + let gitStatusOutput = ''; + try { + const gitResult = execGit(['status', '--porcelain', relPhasesDir], { cwd, timeout: 10_000 }); + if (gitResult.exitCode === 0) { + gitStatusOutput = gitResult.stdout ?? ''; + } + // If git is not available or this is not a git repo, skip the guard + // (gitResult.exitCode non-zero → not a git repo → no uncommitted changes to protect). + } catch { + // git unavailable — skip guard + } + + const uncommittedLines = gitStatusOutput + .split('\n') + .filter((line) => line.trim().length > 0); + if (uncommittedLines.length > 0) { + error( + `phases clear aborted: ${uncommittedLines.length} uncommitted change${uncommittedLines.length === 1 ? '' : 's'} detected in phase directories. ` + + `Archive or commit outgoing phase work before running this command, ` + + `or pass --force to skip this check and permanently delete the phase directories. (#1447)`, + ); + } + } + try { for (const entry of dirs) { fs.rmSync(path.join(phasesDir, entry.name), { recursive: true, force: true }); diff --git a/src/project-root.cts b/src/project-root.cts index a897644fc..821e93b9a 100644 --- a/src/project-root.cts +++ b/src/project-root.cts @@ -102,8 +102,43 @@ export function findProjectRoot(startDir: string): string { // config.json missing or unparseable — fall through to .git heuristic. } if (matched) return parent; - // Heuristic: parent has .planning/ and we're inside a git repo. + // Heuristic (3): parent has .planning/ and we're inside a git repo. + // Before returning, check if any further ancestor has sub_repos that explicitly + // claims our startDir — explicit sub_repos config takes precedence over the + // implicit .git signal. (#1422) if (isInsideGitRepo(parent)) { + // Lookahead: walk ancestors above `parent` to find a sub_repos claim. + let ancestor = path.dirname(parent); + let ancestorDepth = 0; + while (ancestor !== fsRoot && ancestor !== home && ancestorDepth < FIND_PROJECT_ROOT_MAX_DEPTH) { + const ancestorPlanning = ancestor + path.sep + '.planning'; + try { + if (fs.existsSync(ancestorPlanning) && fs.statSync(ancestorPlanning).isDirectory()) { + const ancestorConfig = ancestor + path.sep + '.planning' + path.sep + 'config.json'; + const rawA = fs.readFileSync(ancestorConfig, 'utf-8'); + const cfgA = JSON.parse(rawA) as Record; + const subReposValueA = + cfgA['sub_repos'] ?? + (cfgA['planning'] && typeof cfgA['planning'] === 'object' + ? (cfgA['planning'] as Record)['sub_repos'] + : undefined); + const subReposA = Array.isArray(subReposValueA) ? (subReposValueA as unknown[]) : []; + if (subReposA.length > 0) { + const relPathA = path.relative(ancestor, resolvedStart); + const topSegmentA = relPathA.split(path.sep)[0]; + if (subReposA.includes(topSegmentA)) { + return ancestor; + } + } + } + } catch { + // ignore — config missing or unparseable, keep walking + } + const nextAncestor = path.dirname(ancestor); + if (nextAncestor === ancestor) break; + ancestor = nextAncestor; + ancestorDepth += 1; + } return parent; } } diff --git a/tests/new-milestone-clear-phases.test.cjs b/tests/new-milestone-clear-phases.test.cjs index 76e81124a..8aa0ad752 100644 --- a/tests/new-milestone-clear-phases.test.cjs +++ b/tests/new-milestone-clear-phases.test.cjs @@ -1,15 +1,19 @@ /** - * GSD Tools Tests - New Milestone Clear Phases (#1588) + * GSD Tools Tests - New Milestone Clear Phases (#1588, #1447) * * Verifies that `phases clear` removes all phase subdirectories from * .planning/phases/, leaving the directory itself intact. + * + * Also covers the #1447 uncommitted-changes guard: phases clear must refuse + * to delete phase directories that contain uncommitted work. */ const { test, describe, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); +const { execSync } = require('child_process'); const fs = require('fs'); const path = require('path'); -const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); +const { runGsdTools, createTempProject, createTempGitProject, cleanup } = require('./helpers.cjs'); describe('phases clear command', () => { let tmpDir; @@ -110,3 +114,101 @@ describe('phases clear command', () => { assert.ok(!fs.existsSync(phase1), 'phase directory including nested content should be removed'); }); }); + +// ─── #1447: uncommitted-changes guard ─────────────────────────────────────── + +describe('phases clear: uncommitted-changes guard (#1447)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempGitProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('aborts with error when phase dirs contain uncommitted files', () => { + // Add a phase directory with an untracked (uncommitted) file + const phasesDir = path.join(tmpDir, '.planning', 'phases'); + const phase1 = path.join(phasesDir, '01-foundation'); + fs.mkdirSync(phase1, { recursive: true }); + fs.writeFileSync(path.join(phase1, 'PLAN.md'), '# Plan (uncommitted)'); + // Do NOT commit — leave as untracked/uncommitted changes + + const result = runGsdTools('phases clear --confirm', tmpDir); + assert.ok(!result.success, 'phases clear should fail when uncommitted changes exist'); + assert.ok( + result.error.includes('uncommitted') || result.error.includes('aborted'), + `expected error about uncommitted changes, got: ${result.error}` + ); + // Phase directory must still exist (was not deleted) + assert.ok(fs.existsSync(phase1), 'phase directory must survive when guard fires'); + }); + + test('aborts when phase dirs have staged but uncommitted changes', () => { + const phasesDir = path.join(tmpDir, '.planning', 'phases'); + const phase1 = path.join(phasesDir, '01-foundation'); + fs.mkdirSync(phase1, { recursive: true }); + fs.writeFileSync(path.join(phase1, 'PLAN.md'), '# Plan (staged)'); + // Stage the file but do not commit + execSync('git add .planning/phases/', { cwd: tmpDir, stdio: 'pipe' }); + + const result = runGsdTools('phases clear --confirm', tmpDir); + assert.ok(!result.success, 'phases clear should fail when staged-but-uncommitted changes exist'); + assert.ok( + result.error.includes('uncommitted') || result.error.includes('aborted'), + `expected error about uncommitted changes, got: ${result.error}` + ); + assert.ok(fs.existsSync(phase1), 'phase directory must survive when guard fires'); + }); + + test('--force bypasses the uncommitted-changes guard and deletes anyway', () => { + const phasesDir = path.join(tmpDir, '.planning', 'phases'); + const phase1 = path.join(phasesDir, '01-foundation'); + fs.mkdirSync(phase1, { recursive: true }); + fs.writeFileSync(path.join(phase1, 'PLAN.md'), '# Plan (uncommitted)'); + // Do NOT commit + + const result = runGsdTools('phases clear --confirm --force', tmpDir); + assert.ok(result.success, `--force should bypass guard and succeed: ${result.error}`); + const output = JSON.parse(result.output); + assert.strictEqual(output.cleared, 1, 'should clear 1 phase directory'); + assert.ok(!fs.existsSync(phase1), 'phase directory must be removed when --force is passed'); + }); + + test('succeeds without --force when all phase files are committed', () => { + const phasesDir = path.join(tmpDir, '.planning', 'phases'); + const phase1 = path.join(phasesDir, '01-foundation'); + fs.mkdirSync(phase1, { recursive: true }); + fs.writeFileSync(path.join(phase1, 'PLAN.md'), '# Plan (committed)'); + // Commit the phase files + execSync('git add .planning/phases/', { cwd: tmpDir, stdio: 'pipe' }); + execSync('git commit -m "add phase"', { cwd: tmpDir, stdio: 'pipe' }); + + const result = runGsdTools('phases clear --confirm', tmpDir); + assert.ok(result.success, `should succeed when phase files are committed: ${result.error}`); + const output = JSON.parse(result.output); + assert.strictEqual(output.cleared, 1, 'should clear 1 phase directory'); + assert.ok(!fs.existsSync(phase1), 'committed phase directory should be removed'); + }); + + test('guard skips gracefully when not in a git repo (no guard, proceeds normally)', () => { + // Non-git project: createTempProject creates a plain project without git + const nonGitDir = createTempProject(); + try { + const phasesDir = path.join(nonGitDir, '.planning', 'phases'); + const phase1 = path.join(phasesDir, '01-foundation'); + fs.mkdirSync(phase1, { recursive: true }); + fs.writeFileSync(path.join(phase1, 'PLAN.md'), '# Plan'); + + // Without git, the guard cannot check status — it should skip and proceed + const result = runGsdTools('phases clear --confirm', nonGitDir); + assert.ok(result.success, `should succeed in non-git repo: ${result.error}`); + const output = JSON.parse(result.output); + assert.strictEqual(output.cleared, 1, 'should clear 1 phase directory in non-git project'); + } finally { + cleanup(nonGitDir); + } + }); +}); diff --git a/tests/project-root.test.cjs b/tests/project-root.test.cjs index 33210db8d..0942a4fab 100644 --- a/tests/project-root.test.cjs +++ b/tests/project-root.test.cjs @@ -175,20 +175,19 @@ describe('findProjectRoot nearest-.planning resolution (#1414)', () => { } }); - // REGRESSION (pre-existing heuristic-3 behavior, orthogonal to heuristic 4): + // REGRESSION (#1422): sub_repos explicit config wins over .git implicit signal. // A sub_repos workspace where the child has BOTH its own .planning/ AND its own - // .git/ — invoked from inside the child — RESOLVES TO THE CHILD (not the parent). - // Pre-existing heuristic-3 precedence: a sub-repo that is itself a full project - // (.git + .planning) resolves to itself; this is orthogonal to heuristic 4 and - // tracked separately. Documents current behavior. - test('sub_repos child with BOTH .planning/ and .git/ resolves to child itself (heuristic-3 precedence)', () => { + // .git/ — invoked from inside the child — MUST resolve to the PARENT workspace + // because the parent's config.json explicitly lists the child in sub_repos. + // The implicit .git heuristic (heuristic-3) must not override explicit sub_repos. + test('sub_repos child with BOTH .planning/ and .git/ resolves to PARENT workspace (sub_repos wins, #1422)', () => { // Layout: // workspaceRoot/ // .planning/ // config.json ← sub_repos: ['child'] // child/ // .planning/ ← child has own .planning/ - // .git/ ← child ALSO has own .git/ → heuristic-3 makes it self-resolving + // .git/ ← child ALSO has own .git/ → was triggering heuristic-3 prematurely // src/ ← startDir const workspaceRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-pr-subrepos-git-')); try { @@ -203,10 +202,33 @@ describe('findProjectRoot nearest-.planning resolution (#1414)', () => { const childSrc = mkDeep(childDir, 'src'); const result = findProjectRoot(childSrc); - // Pre-existing heuristic-3 precedence: child is a full project (.git + .planning) - // → resolves to the child, not the workspace root. - assert.strictEqual(result, childDir, - 'A sub-repo with both .planning/ and .git/ should resolve to itself (heuristic-3 precedence)'); + // Explicit sub_repos config in the ancestor workspace must take precedence + // over the implicit .git heuristic — resolves to the workspace root. + assert.strictEqual(result, workspaceRoot, + 'sub_repos config in parent workspace must win over child .git: should resolve to workspaceRoot (#1422)'); + } finally { + cleanup(workspaceRoot); + } + }); + + // REGRESSION (#1422): sub_repos child with .git resolves to parent even when + // startDir is nested more than one level inside the child. + test('sub_repos child with .git: startDir nested 2+ levels inside child still resolves to parent (#1422)', () => { + const workspaceRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-pr-subrepos-nested-')); + try { + fs.mkdirSync(path.join(workspaceRoot, '.planning'), { recursive: true }); + fs.writeFileSync( + path.join(workspaceRoot, '.planning', 'config.json'), + JSON.stringify({ sub_repos: ['child'] }) + ); + const childDir = path.join(workspaceRoot, 'child'); + fs.mkdirSync(path.join(childDir, '.planning'), { recursive: true }); + fs.mkdirSync(path.join(childDir, '.git'), { recursive: true }); + const deepChild = mkDeep(childDir, 'src', 'lib', 'utils'); + + const result = findProjectRoot(deepChild); + assert.strictEqual(result, workspaceRoot, + 'sub_repos config must win over .git even when startDir is deeply nested inside the child (#1422)'); } finally { cleanup(workspaceRoot); }