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

fix(#4487): stamp broken-windows ledger entries with the resolved milestone (#4583)

Browse Source 
* enhance(#4487): stamp windows-ledger entries with the resolved milestone

Broken-windows ledger entries (`.planning/WINDOWS.md`) carry `phase` as
a bare number. 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 sharing
the same phase value with nothing distinguishing them. Since
`/gsd-ship` blocks while any entry is open, an already-archived
milestone's open entries could silently block shipping the CURRENT
milestone, with no supported way to attribute which entry belonged to
which milestone short of manually cross-referencing MILESTONES.md
timestamps against decision IDs that happened to appear in description
prose.

Added an optional `milestone: string | null` field to WindowEntry,
stamped by `windows append` (cmdWindowsAppend, which already does file
I/O) from the workstream's resolved milestone version. Reused the
existing `readCurrentMilestoneVersion` (workstream-inventory.cts --
STATE.md `milestone:` frontmatter first, ROADMAP.md in-progress marker
as fallback) rather than writing a parallel implementation: exported it
via that module's existing `export = {...}` CJS-interop convention
(matching the `import ... = require(...)` pattern already used in
workstream.cts/init.cts). appendWindow itself stays pure -- it accepts
milestone as an optional input field and passes it through; only the
CLI-facing cmdWindowsAppend resolves it from disk.

Backward compatible by construction: validateEntryShape does NOT add
`milestone` to its required fields, so an existing ledger entry with no
milestone key at all parses without error and reads back as null --
exactly "recorded before this change," no migration needed. The
rendered markdown table is deliberately left unchanged (the issue's own
words: "the JSON is the source of truth"); adding a table column would
be a separate, larger change than adding an optional JSON field.

Two smaller gaps the issue itself flags as separable ("happy to split
them out") are explicitly NOT addressed here: no verb to amend an
entry's description, and the table/JSON drift-repair advice that can
destroy table-only edits on a parse failure.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(#4487): preserve absent-vs-null milestone through parse/render roundtrip

validateEntryShape stamped an explicit `milestone: null` onto every
entry lacking the key, so a pre-#4487 ledger entry gained permanent
JSON churn ("milestone": null) the first time ANY entry in the ledger
was touched -- breaking the pure parse/render roundtrip-identity
property test (render(parse(render(ledger))) must equal render(ledger))
and, in real usage, contaminating unrelated entries' diffs on every
append/waive/fixed of an old ledger.

Fixed by distinguishing "key genuinely absent" (undefined -- JSON.
stringify drops it, matching pre-#4487 behavior exactly) from "recorded
but unresolvable" (explicit null, the real signal appendWindow stamps
on brand-new entries). WindowEntry.milestone is now optional
(`milestone?: string | null`) so returning undefined type-checks.

Updated tests/broken-windows.test.cjs's roundtrip property generator to
exercise all three states (absent/null/string) -- its prior silence on
this field is exactly what let the regression through. Also corrected
the earlier backward-compatibility test's assertion: a pre-#4487 entry
reads as milestone: undefined, not null, and re-rendering it must not
introduce a milestone key at all.

Also ran npm run regen:derived: docs/features/broken-windows-ledger.md
(edited in an earlier commit) had never been propagated to its
generated docs/FEATURES.md projection, which is what was independently
failing tests/features-index-gate.test.cjs and, as a side effect of
staleness, tripping tests/fragment-single-edit-propagation.install.
test.cjs's second-source-surface check.

Manually verified via the compiled lib (500 fast-check iterations plus
direct legacy/new-entry roundtrip checks) before wiring the test file,
since this repo blocks local node --test.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(#4487): materialize milestone via conditional spread, not undefined assignment

An object literal property set to `milestone: undefined` is still an
OWN property -- `'milestone' in entry` reads true regardless of the
assigned value, only JSON.stringify treats undefined specially. My
prior commit's own new backward-compat test asserted `'milestone' in
entry === false` for a pre-#4487 entry and failed on exactly this.
Switched to conditionally spreading the key in only when the source
object actually had it, so a genuinely absent milestone is not
materialized at all -- matching both the `in` check and JSON
serialization. Re-verified via the compiled lib (500 fast-check
roundtrip iterations, plus the specific in/undefined/JSON assertions
the failing test makes) before re-running gsd-test.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* docs(#4487): backfill changeset pr number to 4583

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: sim <sim@local>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-09-09 15:37:50 -04:00
committed by GitHub
parent 8deb40722a
commit 385ed619f1
6 changed files with 194 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/eager-tigers-wander.md Normal file
View File

@@ -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.

2
docs/FEATURES.md
View File

@@ -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`
---

2
docs/features/broken-windows-ledger.md
View File

@@ -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`

42
src/broken-windows.cts
View File

@@ -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<WindowEntry, 'kind' | 'phase' | 'description'> &
Partial<Pick<WindowEntry, 'file' | 'line'>>;
Partial<Pick<WindowEntry, 'file' | 'line' | 'milestone'>>;
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() },
);

1
src/workstream-inventory.cts
View File

@@ -860,6 +860,7 @@ export = {
inspectWorkstream,
isCompletedInventory,
listWorkstreamInventories,
readCurrentMilestoneVersion,
sortWorkstreamInventories,
workstreamsRoot,
};

143
tests/broken-windows.test.cjs
View File

@@ -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));