diff --git a/.changeset/eager-tigers-wander.md b/.changeset/eager-tigers-wander.md new file mode 100644 index 000000000..30754a2d4 --- /dev/null +++ b/.changeset/eager-tigers-wander.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 4583 +--- +**Broken-windows ledger entries now record which milestone they belong to** — `windows append` stamps a new `milestone` field from the workstream's resolved milestone version. Phase numbers are unique only within one active phases directory, so two milestones routinely produced entries sharing the same phase number with nothing to distinguish them; `/gsd-ship`'s open-count gate could be silently blocked by another, already-shipped milestone's entries. Absence (an entry recorded before this field existed) reads as null — existing ledgers keep working with no migration. diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 9df3b2723..2793536dc 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -3554,6 +3554,8 @@ The load-bearing wire is the `plan-phase` lift into `must_haves.prohibitions`, s **Backward compatibility:** A project with no `.planning/WINDOWS.md` reports `open_count: 0` and ships cleanly; the gate only activates once windows are recorded. +**Milestone attribution (#4487):** each entry carries a `milestone` field, stamped at record time from the workstream's resolved milestone version (STATE.md `milestone:` frontmatter, or the ROADMAP.md in-progress marker as a fallback). Phase numbers are unique only within one active `phases/` directory — `milestone complete` frees them for reuse — so this is what lets an entry be attributed to the milestone it was actually recorded under, even after that milestone is archived and its phase numbers reused. `null` when no milestone could be resolved, including every entry recorded before this field existed. + **Configuration:** `graphify.graph_path` --- diff --git a/docs/features/broken-windows-ledger.md b/docs/features/broken-windows-ledger.md index 8f356eac2..be9268d88 100644 --- a/docs/features/broken-windows-ledger.md +++ b/docs/features/broken-windows-ledger.md @@ -12,4 +12,6 @@ group: v1.7.0 Features **Backward compatibility:** A project with no `.planning/WINDOWS.md` reports `open_count: 0` and ships cleanly; the gate only activates once windows are recorded. +**Milestone attribution (#4487):** each entry carries a `milestone` field, stamped at record time from the workstream's resolved milestone version (STATE.md `milestone:` frontmatter, or the ROADMAP.md in-progress marker as a fallback). Phase numbers are unique only within one active `phases/` directory — `milestone complete` frees them for reuse — so this is what lets an entry be attributed to the milestone it was actually recorded under, even after that milestone is archived and its phase numbers reused. `null` when no milestone could be resolved, including every entry recorded before this field existed. + **Configuration:** `graphify.graph_path` diff --git a/src/broken-windows.cts b/src/broken-windows.cts index e1162154e..aeafb5b7a 100644 --- a/src/broken-windows.cts +++ b/src/broken-windows.cts @@ -45,6 +45,8 @@ import fs from 'node:fs'; import path from 'node:path'; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import workstreamInventory = require('./workstream-inventory.cjs'); // ─── Constants ───────────────────────────────────────────────────────────── @@ -115,11 +117,25 @@ export interface WindowEntry { reason: string; // '' unless status === 'waived' recorded_at: string; // ISO-8601 resolved_at: string | null; + // #4487: the workstream's resolved milestone version (e.g. "v2.0") at the + // moment the entry was recorded, or null when it could not be resolved. + // Phase numbers are unique only within one active phases/ directory -- + // `milestone complete` archives phases and frees their numbers for reuse, + // so two milestones routinely produce entries under the same `phase` + // value with nothing to distinguish them. Absence (on an entry recorded + // before this field existed) reads as "recorded before this change" -- + // validateEntryShape below does NOT require this key, so existing ledgers + // stay valid with no migration. Optional (not `| null` alone): a genuinely + // absent key must survive parseLedger -> renderLedger as absent, not as an + // explicit `null`, or every legacy entry picks up permanent JSON noise + // the first time ANY entry in the ledger is touched (JSON.stringify drops + // an `undefined` property but keeps an explicit `null` one). + milestone?: string | null; } /** Input shape for appendWindow — id/status/timestamps are assigned by the fn. */ export type WindowInput = Pick & - Partial>; + Partial>; export interface Ledger { schema_version: number; @@ -302,6 +318,7 @@ export function appendWindow( reason: '', recorded_at: opts.now, resolved_at: null, + milestone: input.milestone ?? null, }; const entries = [...ledger.entries, entry]; @@ -614,6 +631,19 @@ function validateEntryShape(e: unknown, i: number): WindowEntry { reason: o.reason, recorded_at: recordedStr, resolved_at: resolvedStr, + // #4487: NOT in `required` above -- an entry recorded before this field + // existed has no `milestone` key at all, and that must parse cleanly. + // Preserve the absence itself -- by not materializing the property at + // all, via the conditional spread below, rather than assigning it + // `milestone: undefined` (an object literal property set to `undefined` + // is still an OWN property; `'milestone' in entry` reads true either + // way) -- so a genuinely absent key stays absent both to `in` and to + // JSON.stringify on re-render. Collapsing absence to an explicit null + // instead would stamp every legacy entry with permanent + // `"milestone": null` noise the moment the ledger is next touched. A + // genuinely-recorded-but-unresolvable milestone (set by appendWindow) + // is still an explicit null and round-trips as one. + ...('milestone' in o ? { milestone: typeof o.milestone === 'string' ? o.milestone : null } : {}), }; } @@ -1107,6 +1137,15 @@ export function cmdWindowsAppend( throw new WindowsError(REASON.WINDOWS_LEDGER_MALFORMED, (e as Error).message); } + // #4487: stamp the workstream's resolved milestone at record time -- the + // same STATE.md-first, ROADMAP-fallback resolution workstream-inventory.cts + // already uses. Best-effort: an unreadable/missing STATE.md or ROADMAP.md + // resolves to null, same as an entry recorded before this field existed. + const milestone = workstreamInventory.readCurrentMilestoneVersion( + path.join(cwd, '.planning', 'STATE.md'), + path.join(cwd, '.planning', 'ROADMAP.md'), + ); + const result = appendWindow( ledger, { @@ -1115,6 +1154,7 @@ export function cmdWindowsAppend( file: parsed.values['--file'] ?? '', line: parsed.values['--line'] == null ? null : Number(parsed.values['--line']), description: parsed.values['--description'] ?? '', + milestone, }, { now: nowIso() }, ); diff --git a/src/workstream-inventory.cts b/src/workstream-inventory.cts index d053f05d2..248295e18 100644 --- a/src/workstream-inventory.cts +++ b/src/workstream-inventory.cts @@ -860,6 +860,7 @@ export = { inspectWorkstream, isCompletedInventory, listWorkstreamInventories, + readCurrentMilestoneVersion, sortWorkstreamInventories, workstreamsRoot, }; diff --git a/tests/broken-windows.test.cjs b/tests/broken-windows.test.cjs index 0924d3dc5..9716e0432 100644 --- a/tests/broken-windows.test.cjs +++ b/tests/broken-windows.test.cjs @@ -183,6 +183,25 @@ describe('broken-windows: appendWindow', () => { // 10 cells = 11 cell-separator pipes per row (leading + 9 internal + trailing). assert.equal(unescapedPipes, 11, 'table row must have exactly 11 unescaped pipes (10 cells) — backslash-pipe in description must NOT add a split'); }); + + // #4487: phase numbers are unique only within one active phases/ directory — + // milestone complete archives phases and frees their numbers for reuse, so + // two milestones can produce entries sharing the same `phase` value with + // nothing to distinguish them. appendWindow itself is pure (no I/O), so the + // milestone value is a caller-supplied input, not resolved here — this pins + // that it flows through untouched, and defaults to null when omitted + // (an entry recorded before this field existed reads the same way). + test('milestone input flows through to the entry (#4487)', () => { + const led = emptyLedger('now'); + const { entry } = appendWindow(led, makeEntry({ milestone: 'v2.0' }), { now: 't' }); + assert.equal(entry.milestone, 'v2.0'); + }); + + test('milestone defaults to null when the caller omits it (#4487)', () => { + const led = emptyLedger('now'); + const { entry } = appendWindow(led, makeEntry(), { now: 't' }); + assert.equal(entry.milestone, null); + }); }); // --------------------------------------------------------------------------- @@ -203,6 +222,13 @@ describe('broken-windows: markWaived', () => { assert.equal(openCount(led), 0); // waived does not block }); + test('waive preserves the entry\'s milestone (#4487 — object-spread transition must not drop it)', () => { + let led = emptyLedger('now'); + ({ ledger: led } = appendWindow(led, makeEntry({ milestone: 'v2.0' }), { now: 't1' })); + led = markWaived(led, 1, 'reason', { now: 't2' }); + assert.equal(led.entries[0].milestone, 'v2.0'); + }); + test('waive with empty reason throws (boundary: limit-1 = 0 chars)', () => { let led = emptyLedger('now'); ({ ledger: led } = appendWindow(led, makeEntry(), { now: 't1' })); @@ -264,6 +290,13 @@ describe('broken-windows: markFixed', () => { assert.equal(openCount(led), 0); }); + test('fixed preserves the entry\'s milestone (#4487 — object-spread transition must not drop it)', () => { + let led = emptyLedger('now'); + ({ ledger: led } = appendWindow(led, makeEntry({ milestone: 'v2.0' }), { now: 't1' })); + led = markFixed(led, 1, { now: 't2' }); + assert.equal(led.entries[0].milestone, 'v2.0'); + }); + test('fixed on unknown id throws', () => { const led = emptyLedger('now'); assert.throws( @@ -299,6 +332,12 @@ describe('broken-windows: parse/render roundtrip property', () => { phase: arbPhase, description: arbText, status: arbStatus, + // #4487: three-way split so the roundtrip property actually exercises + // all of "key absent" (pre-#4487 entry), "explicit null" (recorded but + // unresolvable), and "a real string" -- these three states must each + // survive parse/render identically, which is exactly the distinction + // validateEntryShape/renderLedger have to get right. + milestoneCase: fc.constantFrom('absent', 'null', 'string'), }).map((e) => ({ id: e.id, kind: e.kind, @@ -310,6 +349,7 @@ describe('broken-windows: parse/render roundtrip property', () => { reason: e.status === 'waived' ? 'justified' : '', recorded_at: '2026-07-19T00:00:00Z', resolved_at: e.status === 'open' ? null : '2026-07-19T01:00:00Z', + ...(e.milestoneCase === 'absent' ? {} : { milestone: e.milestoneCase === 'null' ? null : 'v2.0' }), })); const arbLedger = fc.array(arbEntry, { maxLength: 6 }).map((entries) => { @@ -397,6 +437,58 @@ describe('broken-windows: parseLedger fail-closed', () => { }); }); +// --------------------------------------------------------------------------- +// #4487: milestone field backward compatibility +// --------------------------------------------------------------------------- + +describe('broken-windows: parseLedger backward compatibility for the #4487 milestone field', () => { + const rawWithNoMilestoneKey = [ + '---', + 'schema_version: 1', + 'open_count: 1', + 'waived_count: 0', + 'fixed_count: 0', + 'total_count: 1', + 'last_updated: 2026-01-01T00:00:00.000Z', + '---', + '', + '```json', + JSON.stringify([ + { + id: 1, + kind: 'stub', + phase: '3', + file: '', + line: null, + description: 'old entry', + status: 'open', + reason: '', + recorded_at: '2026-01-01T00:00:00.000Z', + resolved_at: null, + // deliberately no `milestone` key + }, + ], null, 2), + '```', + '', + ].join('\n'); + + test('an entry with NO milestone key at all (pre-#4487 shape) parses without error, reading as undefined', () => { + const ledger = parseLedger(rawWithNoMilestoneKey); + // Not `null`: an explicit null is the "recorded, but unresolvable" + // signal appendWindow stamps on NEW entries. A pre-#4487 entry never + // recorded anything -- it must read as genuinely absent (`undefined`), + // the only representation JSON.stringify will also omit on re-render. + assert.equal(ledger.entries[0].milestone, undefined, 'an entry recorded before this field existed must read as milestone: undefined (absent), not null'); + assert.equal('milestone' in ledger.entries[0], false, 'the key itself must not be materialized for a pre-#4487 entry'); + }); + + test('re-rendering a parsed pre-#4487 entry does not stamp a milestone key into the JSON (no drive-by churn)', () => { + const ledger = parseLedger(rawWithNoMilestoneKey); + const rendered = renderLedger(ledger); + assert.doesNotMatch(rendered, /"milestone"/, 'parsing then re-rendering a legacy entry must not introduce milestone: null noise the entry never had'); + }); +}); + // --------------------------------------------------------------------------- // CLI: gsd-tools windows status (acceptance: clean-ship on empty) // --------------------------------------------------------------------------- @@ -724,6 +816,57 @@ describe('broken-windows CLI: windows append', () => { assert.equal(obj2.ledger.entries[0].id, 1); }); + // #4487: cmdWindowsAppend (unlike the pure appendWindow) resolves the + // milestone itself from disk, reusing workstream-inventory.cts's existing + // readCurrentMilestoneVersion (STATE.md `milestone:` frontmatter first, + // ROADMAP.md in-progress marker as fallback) rather than a new parallel + // implementation. + test('append stamps the milestone resolved from STATE.md frontmatter (#4487)', (t) => { + const tmp = createTempDir('bw-append-milestone-state-'); + t.after(() => cleanup(tmp)); + fs.mkdirSync(path.join(tmp, '.planning'), { recursive: true }); + fs.writeFileSync( + path.join(tmp, '.planning', 'STATE.md'), + '---\nmilestone: v2.0\n---\n# Project State\n', + 'utf-8', + ); + const res = runGsdTools( + ['windows', 'append', '--kind', 'stub', '--phase', '5', '--description', 'x'], + tmp, + ); + assert.equal(res.success, true, `stderr: ${res.error || ''}`); + assert.equal(JSON.parse(res.output).entry.milestone, 'v2.0'); + }); + + test('append stamps null when no milestone is resolvable (#4487)', (t) => { + const tmp = createTempDir('bw-append-milestone-none-'); + t.after(() => cleanup(tmp)); + const res = runGsdTools( + ['windows', 'append', '--kind', 'stub', '--phase', '5', '--description', 'x'], + tmp, + ); + assert.equal(res.success, true, `stderr: ${res.error || ''}`); + assert.equal(JSON.parse(res.output).entry.milestone, null); + }); + + test('append falls back to the ROADMAP in-progress marker when STATE.md has no milestone field (#4487)', (t) => { + const tmp = createTempDir('bw-append-milestone-roadmap-fallback-'); + t.after(() => cleanup(tmp)); + fs.mkdirSync(path.join(tmp, '.planning'), { recursive: true }); + fs.writeFileSync(path.join(tmp, '.planning', 'STATE.md'), '---\nstatus: executing\n---\n# Project State\n', 'utf-8'); + fs.writeFileSync( + path.join(tmp, '.planning', 'ROADMAP.md'), + '# Roadmap\n\n## 🚧 **v3.1** — In Progress\n', + 'utf-8', + ); + const res = runGsdTools( + ['windows', 'append', '--kind', 'stub', '--phase', '5', '--description', 'x'], + tmp, + ); + assert.equal(res.success, true, `stderr: ${res.error || ''}`); + assert.equal(JSON.parse(res.output).entry.milestone, 'v3.1'); + }); + test('append a second entry gets id=2', (t) => { const tmp = createTempDir('bw-append-second-'); t.after(() => cleanup(tmp));