diff --git a/.changeset/merry-geese-fly.md b/.changeset/merry-geese-fly.md new file mode 100644 index 000000000..6375ac4ff --- /dev/null +++ b/.changeset/merry-geese-fly.md @@ -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) diff --git a/src/phase-locator.cts b/src/phase-locator.cts index d33b03822..11213dad6 100644 --- a/src/phase-locator.cts +++ b/src/phase-locator.cts @@ -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 — `/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// + // 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; } diff --git a/tests/fix-2855-phase-locator-workstream-archive-scope.test.cjs b/tests/fix-2855-phase-locator-workstream-archive-scope.test.cjs new file mode 100644 index 000000000..8a4b4c58c --- /dev/null +++ b/tests/fix-2855-phase-locator-workstream-archive-scope.test.cjs @@ -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')); + }); +}); diff --git a/tests/phase-locator.test.cjs b/tests/phase-locator.test.cjs index 5a64f43ee..5fcf2966f 100644 --- a/tests/phase-locator.test.cjs +++ b/tests/phase-locator.test.cjs @@ -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')); });