golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 5 Packages Projects Releases Wiki Activity

fix(#2855): scope the phase-locator archived-milestone fallback to the active workstream (#3008)

Browse Source 
* test(#2855): add failing-first regression test for cross-workstream archive leak

Covers findPhaseInternal/getArchivedPhaseDirs in src/phase-locator.cts
resolving a pending workstream phase to an unrelated workstream's (or
flat-mode's) archived phase because the archive fallback hardcodes the
project-root .planning/milestones/ tree. Fails against the current
implementation; the fix lands in a follow-up commit.

* fix(#2855): scope phase-locator archived-milestone fallback to the active workstream

findPhaseInternal and getArchivedPhaseDirs in src/phase-locator.cts hardcoded
the project-root .planning/milestones/ tree when falling back to search
archived phases, ignoring GSD_WORKSTREAM. A pending phase in one workstream
whose own phases/ directory didn't exist yet would silently resolve to a
same-numbered phase archived under an unrelated workstream's (or flat-mode's)
history, complete with stale plan/summary counts and an archived status.

Route the archive fallback through planningDir(cwd) instead — the same
workstream-aware helper the active-phase search (three lines above) and the
archive-write path (archivePhaseDirectories in milestone.cts) already use.
Flat/non-workstream projects are unaffected: planningDir(cwd) with no
GSD_WORKSTREAM resolves to the same root .planning path as before.

Also switch the reported relBase/basePath from a hardcoded
'.planning/milestones/...' literal to path.relative(cwd, archivePath), so the
paths returned to callers stay consistent with wherever the archive actually
resolved to (root or workstream-scoped).

* chore(#2855): add changeset for phase-locator workstream archive fix

* fix(#2855): normalize getArchivedPhaseDirs basePath to posix separators

Orthogonal code-review finding: findPhaseInternal's relBase/directory field
was explicitly toPosixPath-normalized, but getArchivedPhaseDirs's basePath
used a bare path.relative() call, leaving it native-separator on Windows —
an inconsistency between two sibling "relative path from cwd" report fields
introduced by the same #2855 fix. Wrap basePath in toPosixPath to match, and
update the two existing assertions that compared basePath against path.join
output (which would break on Windows now that the field is guaranteed posix)
to compare against forward-slash literals instead, matching how the sibling
`directory` field is already asserted elsewhere in this suite.

* refactor(#2855): share archive-directory resolution between findPhaseInternal and getArchivedPhaseDirs

Orthogonal code-review finding: the two functions carried independent copies
of the same resolve-milestonesDir-then-enumerate-archive-dirs logic — the
exact shape that let the original #2855 bug (hardcoded root path) exist in
one copy while the workstream-aware active-phase search sat three lines
above it. Extract listArchiveVersionDirs(cwd) as the single seam both
functions now consume, so a future change to how the archive tree is located
only needs to happen once. Byte-for-behaviour preserved: readSubdirectories
and searchPhaseInDir already self-contain their own try/catch and never
throw, so moving the iteration outside the old inline try block changes
nothing observable (verified via manual repro scripts covering leak
prevention, positive resolution, flat-mode parity, and multi-milestone
reverse-sort ordering).

* test(#2855): demonstrate ROADMAP.md presence does not affect the archive-leak guard

Orthogonal code-review (spec axis) finding: issue #2855's AC1 states the
guard must hold "regardless of whether workstream A's roadmap already lists
the phase and when it doesn't yet" — an explicit two-value dimension that
had no direct test coverage; it was only inferable by reading
findPhaseInternal's source and confirming it never touches ROADMAP.md.
Add a parametrized test creating the workstream's ROADMAP.md with and
without a matching Phase heading, asserting the archive-leak guard resolves
identically (null) either way.

* chore(#2855): backfill changeset PR number to 3008

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-02 19:24:53 -04:00
committed by GitHub
parent 51f32d2d40
commit 67e2ff7b25
4 changed files with 323 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/merry-geese-fly.md Normal file
View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 3008
---
**Phases no longer leak archived data from another workstream** — resolving a phase in one workstream whose own directory doesn't exist yet no longer falls back to an unrelated workstream's (or a flat-mode project's) same-numbered archived phase; it correctly resolves as pending. (#2855)

114
src/phase-locator.cts
View File

@@ -54,8 +54,45 @@ interface ArchivedPhaseDir {
fullPath: string;
}
interface ArchiveVersionDir {
version: string;
archivePath: string;
}
// ─── Phase search helpers ─────────────────────────────────────────────────────
/**
* #2855: single source of truth for resolving and enumerating a project's
* (or, when a workstream is active, that workstream's OWN) archived-milestone
* directories — `<planningDir(cwd)>/milestones/vX.Y-phases/`. Both
* `findPhaseInternal`'s archive fallback and `getArchivedPhaseDirs` used to
* carry independent copies of this resolve-then-enumerate logic, which is
* exactly the shape that let the original #2855 bug (hardcoded root path)
* exist in one copy and not the other. Sharing this seam means a future
* change to how the archive tree is located only needs to happen once.
* Most-recent-milestone-first order (reverse-sorted directory names).
* Never throws: an absent/unreadable milestones/ dir yields [].
*/
function listArchiveVersionDirs(cwd: string): ArchiveVersionDir[] {
const milestonesDir = path.join(planningDir(cwd), 'milestones');
if (!fs.existsSync(milestonesDir)) return [];
try {
const milestoneEntries = fs.readdirSync(milestonesDir, { withFileTypes: true });
return milestoneEntries
.filter(e => e.isDirectory() && /^v[\d.]+-phases$/.test(e.name))
.map(e => e.name)
.sort()
.reverse()
.map(archiveName => ({
version: archiveName.match(/^(v[\d.]+)-phases$/)![1],
archivePath: path.join(milestonesDir, archiveName),
}));
} catch {
return [];
}
}
function searchPhaseInDir(baseDir: string, relBase: string, normalized: string): PhaseSearchResult | null {
try {
const dirs = readSubdirectories(baseDir, true);
@@ -136,63 +173,46 @@ function findPhaseInternal(cwd: string, phase: unknown): PhaseSearchResult | nul
const current = searchPhaseInDir(phasesDir, relPhasesDir, normalized);
if (current) return current;
const milestonesDir = path.join(cwd, '.planning', 'milestones');
if (!fs.existsSync(milestonesDir)) return null;
try {
const milestoneEntries = fs.readdirSync(milestonesDir, { withFileTypes: true });
const archiveDirs = milestoneEntries
.filter(e => e.isDirectory() && /^v[\d.]+-phases$/.test(e.name))
.map(e => e.name)
.sort()
.reverse();
for (const archiveName of archiveDirs) {
const versionMatch = archiveName.match(/^(v[\d.]+)-phases$/);
const version = versionMatch![1];
const archivePath = path.join(milestonesDir, archiveName);
const relBase = '.planning/milestones/' + archiveName;
const result = searchPhaseInDir(archivePath, relBase, normalized);
if (result) {
result.archived = version;
return result;
}
// #2855: scope the archived-milestone fallback to the SAME workstream as the
// active-phase search above (planningDir(cwd) resolves GSD_WORKSTREAM/GSD_PROJECT
// the identical way both places), not the hardcoded project-root tree. Archived
// phases genuinely live under a workstream's own `.planning/workstreams/<ws>/
// milestones/` — that is where archivePhaseDirectories (milestone.cts) writes
// them via the same planningDir(cwd) resolution. Hardcoding root here let a
// pending workstream phase resolve to an unrelated workstream's (or a flat-mode
// project's) archived phase that merely shares a phase number. Shared with
// getArchivedPhaseDirs via listArchiveVersionDirs (see its doc comment).
for (const { version, archivePath } of listArchiveVersionDirs(cwd)) {
const relBase = toPosixPath(path.relative(cwd, archivePath));
const result = searchPhaseInDir(archivePath, relBase, normalized);
if (result) {
result.archived = version;
return result;
}
} catch { /* intentionally empty */ }
}
return null;
}
function getArchivedPhaseDirs(cwd: string): ArchivedPhaseDir[] {
const milestonesDir = path.join(cwd, '.planning', 'milestones');
// #2855: same workstream-scoped resolution as findPhaseInternal above, via
// the shared listArchiveVersionDirs helper. `phase.list --include-archived`
// (the primary non-init consumer) must not leak a different workstream's
// archive either.
const results: ArchivedPhaseDir[] = [];
if (!fs.existsSync(milestonesDir)) return results;
for (const { version, archivePath } of listArchiveVersionDirs(cwd)) {
const dirs = readSubdirectories(archivePath, true);
try {
const milestoneEntries = fs.readdirSync(milestonesDir, { withFileTypes: true });
const phaseDirs = milestoneEntries
.filter(e => e.isDirectory() && /^v[\d.]+-phases$/.test(e.name))
.map(e => e.name)
.sort()
.reverse();
for (const archiveName of phaseDirs) {
const versionMatch = archiveName.match(/^(v[\d.]+)-phases$/);
const version = versionMatch![1];
const archivePath = path.join(milestonesDir, archiveName);
const dirs = readSubdirectories(archivePath, true);
for (const dir of dirs) {
results.push({
name: dir,
milestone: version,
basePath: path.join('.planning', 'milestones', archiveName),
fullPath: path.join(archivePath, dir),
});
}
for (const dir of dirs) {
results.push({
name: dir,
milestone: version,
basePath: toPosixPath(path.relative(cwd, archivePath)),
fullPath: path.join(archivePath, dir),
});
}
} catch { /* intentionally empty */ }
}
return results;
}

246
tests/fix-2855-phase-locator-workstream-archive-scope.test.cjs Normal file
View File

@@ -0,0 +1,246 @@
/**
* Regression tests for #2855: the phase-locator's archived-milestone fallback
* hardcoded the project-root `.planning/milestones/` tree instead of routing
* through the workstream-aware `planningDir(cwd)` helper. A pending phase in
* workstream A, whose own `phases/` directory does not exist yet, would
* silently resolve to a same-numbered phase archived under the ROOT tree
* (an unrelated workstream's history, or a flat-mode project's archive) —
* complete with stale plan/summary counts and an "archived" status for a
* phase that is actually brand new.
*
* Root cause: src/phase-locator.cts:139 (`findPhaseInternal`) and
* src/phase-locator.cts:167 (`getArchivedPhaseDirs`) both used
* `path.join(cwd, '.planning', 'milestones')` instead of
* `path.join(planningDir(cwd), 'milestones')` — the same seam the
* active-phase search (line 132) and the archive-write path
* (`archivePhaseDirectories`, src/milestone.cts) already use.
*
* Ambient-env hermeticity: GSD_WORKSTREAM/GSD_PROJECT are read directly from
* process.env by planningDir() when omitted, so every test here explicitly
* saves and restores both (pattern from
* tests/fix-2297-resolve-model-ids-runtime-scoping.test.cjs) to avoid leaking
* state across tests or picking up a developer's ambient shell env.
*/
'use strict';
const { test, describe, beforeEach, afterEach } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('node:fs');
const path = require('node:path');
const phaseLocator = require('../gsd-core/bin/lib/phase-locator.cjs');
const { createTempProject, cleanup } = require('./helpers.cjs');
let _origWorkstream;
let _origProject;
function isolateWorkstreamEnv() {
_origWorkstream = process.env.GSD_WORKSTREAM;
_origProject = process.env.GSD_PROJECT;
delete process.env.GSD_WORKSTREAM;
delete process.env.GSD_PROJECT;
}
function restoreWorkstreamEnv() {
if (_origWorkstream === undefined) delete process.env.GSD_WORKSTREAM;
else process.env.GSD_WORKSTREAM = _origWorkstream;
if (_origProject === undefined) delete process.env.GSD_PROJECT;
else process.env.GSD_PROJECT = _origProject;
}
describe('#2855: findPhaseInternal does not leak cross-workstream archived phases', () => {
let tmpDir;
beforeEach(() => { isolateWorkstreamEnv(); });
afterEach(() => {
restoreWorkstreamEnv();
if (tmpDir) { cleanup(tmpDir); tmpDir = null; }
});
test('does not leak root-tree archived phase into an unrelated workstream', () => {
tmpDir = createTempProject('gsd-2855-');
// Root archive holds phase 03 (unrelated workstream's / flat-mode history).
const rootArchive = path.join(tmpDir, '.planning', 'milestones', 'v1.0-phases', '03-legacy');
fs.mkdirSync(rootArchive, { recursive: true });
fs.writeFileSync(path.join(rootArchive, 'SOME-SUMMARY.md'), '# stale');
// Workstream "beta" exists with an empty phases/ dir — phase 03 is pending.
fs.mkdirSync(path.join(tmpDir, '.planning', 'workstreams', 'beta', 'phases'), { recursive: true });
process.env.GSD_WORKSTREAM = 'beta';
const result = phaseLocator.findPhaseInternal(tmpDir, '3');
assert.strictEqual(result, null, 'pending workstream phase must not resolve to the root archive');
});
test('does not leak root archive when workstream phases dir is entirely absent', () => {
tmpDir = createTempProject('gsd-2855-');
const rootArchive = path.join(tmpDir, '.planning', 'milestones', 'v2.0-phases', '01-legacy');
fs.mkdirSync(rootArchive, { recursive: true });
// Brand-new workstream: no .planning/workstreams/gamma/ directory at all yet.
process.env.GSD_WORKSTREAM = 'gamma';
const result = phaseLocator.findPhaseInternal(tmpDir, '1');
assert.strictEqual(result, null, 'a workstream with no directory yet must not resolve to the root archive');
});
// Issue #2855 AC1: "...regardless of whether workstream A's roadmap already
// lists the phase and when it doesn't yet." findPhaseInternal never reads
// ROADMAP.md (it is a pure filesystem lookup), so this dimension cannot
// change its behavior — demonstrated directly rather than left as an
// inference from reading the source.
for (const roadmapHasEntry of [true, false]) {
test(`does not leak root archive whether or not the workstream's ROADMAP.md already lists the phase (roadmapHasEntry=${roadmapHasEntry})`, () => {
tmpDir = createTempProject('gsd-2855-');
const rootArchive = path.join(tmpDir, '.planning', 'milestones', 'v1.0-phases', '03-legacy');
fs.mkdirSync(rootArchive, { recursive: true });
const wsDir = path.join(tmpDir, '.planning', 'workstreams', 'beta');
fs.mkdirSync(path.join(wsDir, 'phases'), { recursive: true });
if (roadmapHasEntry) {
fs.writeFileSync(
path.join(wsDir, 'ROADMAP.md'),
['# Roadmap', '', '### Phase 03: Pending Work', ''].join('\n'),
);
}
process.env.GSD_WORKSTREAM = 'beta';
const result = phaseLocator.findPhaseInternal(tmpDir, '3');
assert.strictEqual(result, null, 'ROADMAP.md presence/absence must not affect the archive-leak guard');
});
}
test('still finds a phase genuinely archived under the active workstream\'s own tree', () => {
tmpDir = createTempProject('gsd-2855-');
const ownArchive = path.join(tmpDir, '.planning', 'workstreams', 'beta', 'milestones', 'v1.0-phases', '03-real');
fs.mkdirSync(ownArchive, { recursive: true });
process.env.GSD_WORKSTREAM = 'beta';
const result = phaseLocator.findPhaseInternal(tmpDir, '3');
assert.ok(result !== null, 'workstream\'s own archived phase must still resolve');
assert.strictEqual(result.found, true);
assert.strictEqual(result.archived, 'v1.0');
assert.strictEqual(
result.directory,
'.planning/workstreams/beta/milestones/v1.0-phases/03-real',
);
});
test('flat/non-workstream project archive resolution is unchanged', () => {
tmpDir = createTempProject('gsd-2855-');
const rootArchive = path.join(tmpDir, '.planning', 'milestones', 'v1.0-phases', '03-flat');
fs.mkdirSync(rootArchive, { recursive: true });
// No GSD_WORKSTREAM / GSD_PROJECT set — flat mode.
const result = phaseLocator.findPhaseInternal(tmpDir, '3');
assert.ok(result !== null, 'flat-mode archive lookup must be unaffected by the fix');
assert.strictEqual(result.found, true);
assert.strictEqual(result.archived, 'v1.0');
assert.strictEqual(result.directory, '.planning/milestones/v1.0-phases/03-flat');
});
test('two workstreams with same-numbered archived phases never cross-resolve', () => {
tmpDir = createTempProject('gsd-2855-');
const alphaArchive = path.join(tmpDir, '.planning', 'workstreams', 'alpha', 'milestones', 'v1.0-phases', '03-alpha-work');
const betaArchive = path.join(tmpDir, '.planning', 'workstreams', 'beta', 'milestones', 'v1.0-phases', '03-beta-work');
fs.mkdirSync(alphaArchive, { recursive: true });
fs.mkdirSync(betaArchive, { recursive: true });
process.env.GSD_WORKSTREAM = 'alpha';
const alphaResult = phaseLocator.findPhaseInternal(tmpDir, '3');
assert.ok(alphaResult !== null);
assert.strictEqual(alphaResult.phase_name, 'alpha-work');
process.env.GSD_WORKSTREAM = 'beta';
const betaResult = phaseLocator.findPhaseInternal(tmpDir, '3');
assert.ok(betaResult !== null);
assert.strictEqual(betaResult.phase_name, 'beta-work');
});
test('project+workstream combination scopes the archive fallback', () => {
tmpDir = createTempProject('gsd-2855-');
const rootArchive = path.join(tmpDir, '.planning', 'milestones', 'v1.0-phases', '05-root-legacy');
fs.mkdirSync(rootArchive, { recursive: true });
const scopedArchive = path.join(tmpDir, '.planning', 'proj-x', 'workstreams', 'beta', 'milestones', 'v1.0-phases', '05-scoped');
fs.mkdirSync(scopedArchive, { recursive: true });
process.env.GSD_PROJECT = 'proj-x';
process.env.GSD_WORKSTREAM = 'beta';
const result = phaseLocator.findPhaseInternal(tmpDir, '5');
assert.ok(result !== null, 'project+workstream scoped archive must resolve');
assert.strictEqual(result.phase_name, 'scoped');
assert.strictEqual(
result.directory,
'.planning/proj-x/workstreams/beta/milestones/v1.0-phases/05-scoped',
);
});
test('workstream-scoped archived directory is posix-style', () => {
tmpDir = createTempProject('gsd-2855-');
const ownArchive = path.join(tmpDir, '.planning', 'workstreams', 'beta', 'milestones', 'v1.0-phases', '07-posix');
fs.mkdirSync(ownArchive, { recursive: true });
process.env.GSD_WORKSTREAM = 'beta';
const result = phaseLocator.findPhaseInternal(tmpDir, '7');
assert.ok(result !== null);
assert.ok(!result.directory.includes('\\'), 'directory must use forward slashes on every platform');
});
});
describe('#2855: getArchivedPhaseDirs does not leak cross-workstream archived phases', () => {
let tmpDir;
beforeEach(() => { isolateWorkstreamEnv(); });
afterEach(() => {
restoreWorkstreamEnv();
if (tmpDir) { cleanup(tmpDir); tmpDir = null; }
});
test('does not leak root-tree archive entries under an active workstream', () => {
tmpDir = createTempProject('gsd-2855-');
const rootArchive = path.join(tmpDir, '.planning', 'milestones', 'v1.0-phases', '03-legacy');
fs.mkdirSync(rootArchive, { recursive: true });
fs.mkdirSync(path.join(tmpDir, '.planning', 'workstreams', 'beta', 'phases'), { recursive: true });
process.env.GSD_WORKSTREAM = 'beta';
const result = phaseLocator.getArchivedPhaseDirs(tmpDir);
assert.deepEqual(result, [], 'getArchivedPhaseDirs must not surface the root archive for a workstream');
});
test('still finds phases genuinely archived under the active workstream\'s own tree', () => {
tmpDir = createTempProject('gsd-2855-');
const ownArchive = path.join(tmpDir, '.planning', 'workstreams', 'beta', 'milestones', 'v2.0-phases', '04-own');
fs.mkdirSync(ownArchive, { recursive: true });
process.env.GSD_WORKSTREAM = 'beta';
const result = phaseLocator.getArchivedPhaseDirs(tmpDir);
assert.strictEqual(result.length, 1);
assert.strictEqual(result[0].name, '04-own');
assert.strictEqual(result[0].milestone, 'v2.0');
// basePath is posix-normalized (toPosixPath) — a forward-slash literal is
// the correct cross-platform expectation, not path.join.
assert.strictEqual(
result[0].basePath,
'.planning/workstreams/beta/milestones/v2.0-phases',
);
});
test('flat/non-workstream project resolution is unchanged', () => {
tmpDir = createTempProject('gsd-2855-');
const archiveDir = path.join(tmpDir, '.planning', 'milestones', 'v2.1.0-phases');
fs.mkdirSync(path.join(archiveDir, '03-auth'), { recursive: true });
// No GSD_WORKSTREAM / GSD_PROJECT set — flat mode.
const result = phaseLocator.getArchivedPhaseDirs(tmpDir);
assert.strictEqual(result.length, 1);
const entry = result[0];
assert.strictEqual(entry.name, '03-auth');
assert.strictEqual(entry.milestone, 'v2.1.0');
// basePath is posix-normalized (toPosixPath) — a forward-slash literal is
// the correct cross-platform expectation, not path.join.
assert.strictEqual(entry.basePath, '.planning/milestones/v2.1.0-phases');
assert.strictEqual(entry.fullPath, path.join(archiveDir, '03-auth'));
});
});

6
tests/phase-locator.test.cjs
View File

@@ -288,7 +288,11 @@ describe('getArchivedPhaseDirs', () => {
const entry = result[0];
assert.strictEqual(entry.name, '03-auth');
assert.strictEqual(entry.milestone, 'v2.1.0');
assert.strictEqual(entry.basePath, path.join('.planning', 'milestones', 'v2.1.0-phases'));
// #2855: basePath is posix-normalized (toPosixPath), matching the sibling
// `directory` field's contract — a hardcoded forward-slash literal is the
// correct expectation on every platform, not path.join (which would emit
// backslashes on Windows and break this assertion there).
assert.strictEqual(entry.basePath, '.planning/milestones/v2.1.0-phases');
assert.strictEqual(entry.fullPath, path.join(archiveDir, '03-auth'));
});