fix(#1422,#1447): fix sub_repos/.git precedence; guard uncommitted data in new-milestone (#1484)
* fix(#1422): sub_repos config takes precedence over .git in findProjectRoot When heuristic-3 (.git + parent .planning/) fires, do a lookahead walk over ancestors above the matching parent to check if any further ancestor has a sub_repos entry that explicitly claims the starting directory. If found, return that ancestor instead — explicit config wins over the implicit .git signal. Regression tests updated: the heuristic-3 precedence test now asserts the correct new behavior (sub_repos wins), and two additional coverage cases added (startDir directly in sub_repo child, and startDir nested 2+ levels inside the child). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(#1447): guard against uncommitted changes before deleting phase dirs in new-milestone cmdPhasesClear now runs `git status --porcelain <phasesDir>` before executing any rmSync. If uncommitted changes are detected it calls error() and aborts, preventing silent data loss when new-milestone's §6 "phases.clear --confirm" fires before the operator has archived or committed outgoing phase work. A new --force flag is added to bypass the guard for callers that have already verified archival is done (or explicitly accept the loss). When git is unavailable or the directory is not inside a git repo the guard silently skips, preserving the existing behaviour for non-git projects. Five new regression tests cover: untracked files abort, staged-but- uncommitted abort, --force bypasses, committed files pass, non-git project passes without guard. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: add changeset for #1422 and #1447 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: correct changeset format Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
9
.changeset/fix-1422-1447-projectroot-milestone-guards.md
Normal file
9
.changeset/fix-1422-1447-projectroot-milestone-guards.md
Normal file
@@ -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)
|
||||
|
||||
<!-- docs-exempt: internal CLI guard in milestone.cts — no public docs surface change; --force flag is an operator escape hatch, not a user-visible API change -->
|
||||
@@ -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 });
|
||||
|
||||
@@ -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<string, unknown>;
|
||||
const subReposValueA =
|
||||
cfgA['sub_repos'] ??
|
||||
(cfgA['planning'] && typeof cfgA['planning'] === 'object'
|
||||
? (cfgA['planning'] as Record<string, unknown>)['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;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user