diff --git a/.changeset/graceful-koalas-greet.md b/.changeset/graceful-koalas-greet.md new file mode 100644 index 000000000..7fad1c6a8 --- /dev/null +++ b/.changeset/graceful-koalas-greet.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 2251 +--- +**`requirements mark-complete` reports a per-surface write-set** — the command now returns a per-requirement `write_set` (checkbox + traceability surfaces) and a `write_set_complete` that is true only when every surface of every requirement applied, so a partial (checkbox-only) reconcile can no longer masquerade as full success even inside a multi-ID batch. Introduces the reusable ADR-2143 §5/§6 `Result` / `WriteSet` contract. (#2251) diff --git a/.gitignore b/.gitignore index 2fcfa036f..0f399bcad 100644 --- a/.gitignore +++ b/.gitignore @@ -92,6 +92,7 @@ build/ /gsd-core/bin/lib/capability-lock.cjs /gsd-core/bin/lib/markdown-sectionizer.cjs /gsd-core/bin/lib/markdown-table.cjs +/gsd-core/bin/lib/write-set.cjs /gsd-core/bin/lib/resolution.cjs /gsd-core/bin/lib/research-store.cjs /gsd-core/bin/lib/research-provider.cjs diff --git a/CONTEXT.md b/CONTEXT.md index 33632bc2d..f701eac97 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -143,7 +143,10 @@ Module owning the tool's CLI I/O primitives: `output()` result emission (with la Canonical markdown-structure parsing seam (`gsd-core/bin/lib/markdown-sectionizer.cjs`, generated from `src/markdown-sectionizer.cts`). Pure functions, Node built-ins only. Exports: `stripFencedCode(content) → { text, unterminatedFence }` (CommonMark-correct state machine, CRLF-safe, signals unterminated fences); `tokenizeHeadings(content) → HeadingToken[]` (ATX headings outside fenced blocks, `{ level, text, line, offset }`); `collectSections(content, stopPredicate) → Section[]` (line-by-line section collection driven by a heading predicate); `collectSection(content, headingPredicate, { levelBounded, stripFences }) → Section | null` (single named section with level-bounded stop); `iterateBullets(sectionText) → BulletItem[]` (dash/checkbox/numbered markers with indented continuation); `extractTaggedBlocks(content, tagName) → string[]` (inner text of every `…` block in document order, tagName regex-escaped, caller decides fence-stripping — generalises `decisions.cts`'s bespoke extractor for T1); `replaceSection(content, section, newBody) → string` (pure character-offset splice using `Section.bodyStart`/`bodyEnd` for read-modify-write callers — eliminates T6 `state.cts`'s 7× inline `content.replace` pattern); `withSection(content, target, edit) → string` (resolve the section whose heading matches `target` — exact heading text or a `HeadingToken` predicate — and run `edit(body)` against ONLY that section's body before splicing the result back; bounded no-op when no heading matches or `edit` returns the same/non-string body; ADR-2143 §4 structurally retires the #2130/#2067/#2080 boundary-crossing class by confining any regex the caller runs to the matched section). `Section` carries `bodyStart`/`bodyEnd` offsets for `replaceSection`. ADR-1372 (epic #1372) establishes this seam and a tiered migration plan (T0–T7) to retire the 8+ ad-hoc markdown parsers and ~20 inline section-collects across `src/*.cts`. New `src/*.cts` modules must import this seam instead of hand-rolling fence strippers or heading-regex section walks (enforced by the `no-adhoc-markdown-parsing` ESLint rule landing in tier T7). ### Markdown Table Model -Canonical GFM table parsing + schema registry seam (`gsd-core/bin/lib/markdown-table.cjs`, generated from `src/markdown-table.cts`; ADR-2143, epic #2143). Pure functions, Node built-ins only, string-in/value-out, no I/O. Exports: `parseMarkdownTable(sectionText) → Result` (parses the first GFM pipe table found; typed `{ok:false,reason}` parse errors for no-table, missing/misaligned delimiter row, and ragged data rows — never silently drops or coerces a malformed row); `MarkdownTable` (`{columns: string[], rows: Record[]}`, rows addressed by column name, not position); `Result` (`{ok:true,value}\|{ok:false,reason}` — deliberately distinct from command-routing-hub's dispatch `Result` `{ok,data\|kind}`; the two never mix); `TABLE_SCHEMAS` (`Record` — the canonical column-header variants for every GFM table GSD parses or generates: `RoadmapProgress` flat/milestone-grouped, `RequirementsTraceability`, `QuickTasks` no-status/with-status, `Security` trust-boundaries/threat-register/accepted-risks/audit-trail); `matchTableSchema(columns) → {id,label}\|null` (resolves a parsed header back to its canonical schema by exact column-name/order match). This registry is the single source of truth for ROADMAP/STATE/SECURITY canonical tables — a parity test (`tests/markdown-table.test.cjs`) asserts every variant's header appears verbatim in the template/workflow file that generates it, so the registry and templates can never silently drift (ADR-2143 §3 Generative-Fix-Divergence guard). `phase-lifecycle.cts`'s `deriveProgressFromRoadmap` is the first consumer: it locates the Progress section via the Markdown Sectionizer's `collectSection` and reads cells by column NAME through this seam, fixing #2137 (the prior position-anchored regex assumed `Status` was always the 3rd cell, which broke for the 5-column milestone-grouped `Milestone` variant). +Canonical GFM table parsing + schema registry seam (`gsd-core/bin/lib/markdown-table.cjs`, generated from `src/markdown-table.cts`; ADR-2143, epic #2143). Pure functions, Node built-ins only, string-in/value-out, no I/O. Exports: `parseMarkdownTable(sectionText) → Result` (parses the first GFM pipe table found; typed `{ok:false,reason}` parse errors for no-table, missing/misaligned delimiter row, and ragged data rows — never silently drops or coerces a malformed row); `MarkdownTable` (`{columns: string[], rows: Record[]}`, rows addressed by column name, not position); `Result` (`{ok:true,value}\|{ok:false,reason}` — re-exported from the Write-Set Module, the ADR-2143 §5 single source of truth for this shape, so existing importers of `Result` from `markdown-table.cjs` are unaffected; deliberately distinct from command-routing-hub's dispatch `Result` `{ok,data\|kind}`; the two never mix); `TABLE_SCHEMAS` (`Record` — the canonical column-header variants for every GFM table GSD parses or generates: `RoadmapProgress` flat/milestone-grouped, `RequirementsTraceability`, `QuickTasks` no-status/with-status, `Security` trust-boundaries/threat-register/accepted-risks/audit-trail); `matchTableSchema(columns) → {id,label}\|null` (resolves a parsed header back to its canonical schema by exact column-name/order match). This registry is the single source of truth for ROADMAP/STATE/SECURITY canonical tables — a parity test (`tests/markdown-table.test.cjs`) asserts every variant's header appears verbatim in the template/workflow file that generates it, so the registry and templates can never silently drift (ADR-2143 §3 Generative-Fix-Divergence guard). `phase-lifecycle.cts`'s `deriveProgressFromRoadmap` is the first consumer: it locates the Progress section via the Markdown Sectionizer's `collectSection` and reads cells by column NAME through this seam, fixing #2137 (the prior position-anchored regex assumed `Status` was always the 3rd cell, which broke for the 5-column milestone-grouped `Milestone` variant). + +### Write-Set Module +Shared fail-loud `Result` and per-surface write-set contracts (`gsd-core/bin/lib/write-set.cjs`, generated from `src/write-set.cts`; ADR-2143 §5/§6, epic #2143). Pure, Node built-ins only, no I/O. Exports: `Result` (`{ok:true,value}\|{ok:false,reason}` — ADR-2143 §5 fail-loud parse shape, never a bare `null` a caller can mistake for "empty but fine"; the single source of truth `markdown-table.cjs` re-exports so its existing importers are unaffected; deliberately distinct from command-routing-hub's dispatch `Result` `{ok,data\|kind}`); `WriteOutcome` (`{surface: string, applied: boolean}` — one surface's outcome within a multi-surface write); `WriteSet` (`WriteOutcome[]`); `writeSetComplete(ws) → boolean` (true only when the set is non-empty AND every surface applied — ADR-2143 §6's "no OR-into-one-flag" rule: a command that mutates more than one surface must not collapse independent surface outcomes into a single boolean, the anti-pattern that let a checkbox-only partial write (#2140) report full success). `milestone.cts`'s `requirements mark-complete` handler is the first consumer: it reports a `write_set` (`checkbox`/`traceability` surfaces) and `write_set_complete` alongside its existing `updated`/`marked_complete`/`already_complete`/`not_found`/`table_unmatched` fields, which remain computed exactly as before — the write-set is additive, structured ADR-2143 documentation of the same per-surface facts #2140's tactical fix already exposed via `table_unmatched`. ### Roadmap Parser Module Module owning ROADMAP.md parsing: shipped-milestone slicing, current-milestone extraction, milestone/phase lookups, and milestone-phase filtering (`stripShippedMilestones`, `extractCurrentMilestone`, `replaceInCurrentMilestone`, `getRoadmapPhaseInternal`, `getMilestoneInfo`, `getMilestonePhaseFilter`, `withPhaseSection`). `withPhaseSection(content, phaseId, edit)` resolves a phase's `### Phase N` detail-section heading via the #2121 phase-id source (`phaseMarkdownRegexSource`) and delegates to the markdown-sectionizer seam's `withSection`, so a per-phase ROADMAP edit is bounded to that phase's own section (ADR-2143 §4). Depends only on leaf modules (`phase-id`, `planning-workspace`, `shell-command-projection`, `markdown-sectionizer`) — no `loadConfig`, no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2b (#870), resolving the ROADMAP.md parse/write straddle so the Roadmap module (`roadmap.cjs`, which owns ROADMAP.md mutation) imports parsing directly instead of through Core; the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/roadmap-parser.cjs` (generated from `src/roadmap-parser.cts`). diff --git a/docs/INVENTORY-MANIFEST.json b/docs/INVENTORY-MANIFEST.json index f2b42220c..b3ba5789a 100644 --- a/docs/INVENTORY-MANIFEST.json +++ b/docs/INVENTORY-MANIFEST.json @@ -447,7 +447,8 @@ "workstream-name-policy.cjs", "workstream.cjs", "worktree-base-ref.cjs", - "worktree-safety.cjs" + "worktree-safety.cjs", + "write-set.cjs" ], "hooks": [ "gsd-check-update-worker.js", diff --git a/docs/INVENTORY.md b/docs/INVENTORY.md index 10b595fbf..7bc4c1c05 100644 --- a/docs/INVENTORY.md +++ b/docs/INVENTORY.md @@ -533,6 +533,7 @@ Full listing: `gsd-core/bin/lib/*.cjs`. | `workstream.cjs` | Workstream CRUD, migration, session-scoped active pointer | | `worktree-base-ref.cjs` | Worktree base-ref drift detection and degrade decision (`evaluateWorktreeBaseDegrade`) plus no-clobber `worktree.baseRef` settings management for the `base-check`/`set-baseref` subcommands (#683) | | `worktree-safety.cjs` | Worktree-root resolution and non-destructive prune policy decisions; owns W017 health-check logic | +| `write-set.cjs` | Shared fail-loud `Result` (`{ok:true,value}\|{ok:false,reason}`) and per-surface write-set contracts (ADR-2143, epic #2143) — `WriteOutcome` (`{surface,applied}`), `WriteSet` (`WriteOutcome[]`), and `writeSetComplete(ws)` (true only when the set is non-empty AND every surface applied, never an OR-into-one-flag); `markdown-table.cjs` re-exports `Result` from here so existing importers are unaffected; consumed by `milestone.cts`'s `requirements mark-complete` handler to report a structured per-surface (`checkbox`/`traceability`) write-set alongside its existing fields (fixes the structural half of #2140) | [`docs/CLI-TOOLS.md`](CLI-TOOLS.md) may describe a subset of these modules; when it disagrees with the filesystem, this table and the directory listing are authoritative. diff --git a/eslint.config.mjs b/eslint.config.mjs index dbeae1b7c..1c83da7fc 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -215,6 +215,8 @@ export default tseslint.config( 'gsd-core/bin/lib/markdown-sectionizer.cjs', // ADR-2143: tsc-generated runtime artifact — lint the src/markdown-table.cts source. 'gsd-core/bin/lib/markdown-table.cjs', + // ADR-2143: tsc-generated runtime artifact — lint the src/write-set.cts source. + 'gsd-core/bin/lib/write-set.cjs', // ADR-1239 Phase C-1 (#1680): tsc-generated — lint src/embedding-adapter.cts + src/adapter-declarative.cts. 'gsd-core/bin/lib/embedding-adapter.cjs', 'gsd-core/bin/lib/adapter-declarative.cjs', diff --git a/src/markdown-table.cts b/src/markdown-table.cts index bc26eabe4..2627efaaf 100644 --- a/src/markdown-table.cts +++ b/src/markdown-table.cts @@ -3,17 +3,19 @@ * (ADR-2143, epic #2143). Pure functions, Node built-ins only, string-in/value-out, * no I/O. Compiled by tsc to gsd-core/bin/lib/markdown-table.cjs. * - * NOTE: the `Result` here is the ADR-2143 parse-result shape {ok,value|reason} — - * deliberately distinct from command-routing-hub's dispatch `Result` {ok,data|kind}; - * the two never mix (different modules). + * NOTE: the `Result` here is the ADR-2143 §5 parse-result shape {ok,value|reason}, + * now defined once in `./write-set.cjs` (the shared fail-loud + write-set seam) and + * re-exported here so existing importers of `Result` from this module keep working + * unchanged — deliberately distinct from command-routing-hub's dispatch `Result` + * {ok,data|kind}; the two never mix (different modules). */ import { collectSection, replaceSection } from './markdown-sectionizer.cjs'; +import type { Result } from './write-set.cjs'; +export type { Result } from './write-set.cjs'; // ─── Types ──────────────────────────────────────────────────────────────────── -export type Result = { ok: true; value: T } | { ok: false; reason: string }; - /** A parsed GFM pipe table: header column names + rows addressed by column name. */ export interface MarkdownTable { columns: string[]; diff --git a/src/milestone.cts b/src/milestone.cts index 1dbc8142d..b09c64105 100644 --- a/src/milestone.cts +++ b/src/milestone.cts @@ -18,6 +18,8 @@ import { platformWriteSync, platformEnsureDir, execGit, retryRenameSync } from ' import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs'; import { realClock } from './clock.cjs'; import { transitionCore } from './state-transition.cjs'; +import { writeSetComplete } from './write-set.cjs'; +import type { WriteSet } from './write-set.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); const { output, error } = ioMod; @@ -79,6 +81,18 @@ function cmdRequirementsMarkComplete(cwd: string, reqIdsRaw: string[], raw: bool // a missing row only counts as drift when a table actually exists. const hasTable = /^\|\s*Requirement\s*\|/im.test(reqContent); + // ADR-2143 §6 per-surface write-set, tracked PER requirement ID: a + // multi-ID batch must not OR one ID's surface outcome into another's — + // that is the exact #2140 class one level up (an ID whose traceability + // row is absent/unmatched must not have its partial write masked by a + // different ID in the same invocation that fully reconciled). Reported + // additively as `write_set` below — it does not change the existing + // marked_complete/already_complete/not_found/table_unmatched/updated + // computation, which stays byte-for-behaviour identical (#2140's tactical + // fix already surfaces the checkbox-only-partial-write case via + // table_unmatched; this only adds the structured ADR-2143 shape on top). + const writeSet: WriteSet = []; + for (const reqId of reqIds) { const reqEscaped = escapeRegex(reqId); @@ -96,6 +110,16 @@ function cmdRequirementsMarkComplete(cwd: string, reqIdsRaw: string[], raw: bool const tableHit = afterTable !== reqContent; if (tableHit) reqContent = afterTable; + // ADR-2143 §6 per-ID write-set entries: this ID's checkbox surface is + // always tracked; the traceability surface is tracked only when the file + // has a traceability table at all (same `hasTable` gate the existing + // required-surface logic below uses) — omitted entirely, not a false + // `applied:false`, when no table is required of this file. + writeSet.push({ requirement: reqId, surface: 'checkbox', applied: checkboxHit }); + if (hasTable) { + writeSet.push({ requirement: reqId, surface: 'traceability', applied: tableHit }); + } + // Coverage of the traceability surface for this ID (computed after any flip). // hasRow keys on the ID + a second cell (`| ID | |`) so a bare mention // of the ID in a non-traceability table does not masquerade as a real row. @@ -129,6 +153,17 @@ function cmdRequirementsMarkComplete(cwd: string, reqIdsRaw: string[], raw: bool platformWriteSync(reqPath, reqContent); } + // ADR-2143 §6: `writeSet` above already carries one WriteOutcome per + // (requirement, surface) this invocation could have written to — per ID, + // not ORed across the batch. `write_set` and `write_set_complete` are + // additive: they do not replace or gate `updated` / `marked_complete` / + // `already_complete` / `not_found` / `table_unmatched`, which remain + // computed exactly as before (see #2140 note above — that fix already + // surfaces a checkbox-only partial write via `table_unmatched`; + // `write_set_complete` is a structured, ADR-2143-shaped read of the SAME + // per-surface, per-ID facts, `false` if ANY id's ANY required surface did + // not apply, since `writeSetComplete` requires EVERY entry to have + // applied, never an OR across surfaces OR across IDs). output( { updated: updated.length > 0, @@ -137,6 +172,8 @@ function cmdRequirementsMarkComplete(cwd: string, reqIdsRaw: string[], raw: bool not_found: notFound, table_unmatched: tableUnmatched, total: reqIds.length, + write_set: writeSet, + write_set_complete: writeSetComplete(writeSet), }, raw, `${updated.length}/${reqIds.length} requirements marked complete`, diff --git a/src/phase-lifecycle.cts b/src/phase-lifecycle.cts index 315d109c1..e0f4177f5 100644 --- a/src/phase-lifecycle.cts +++ b/src/phase-lifecycle.cts @@ -62,43 +62,49 @@ export function deriveProgressFromRoadmap(roadmapContent: string): RoadmapProgre let totalPhases: number | null = null; let totalPlans: number | null = null; - try { - // ADR-2143 §3: read the Progress table by column NAME (order/injection-invariant), - // via the markdown-table seam. Scope to the `## Progress` section when present - // (#2012 decoy avoidance); a headingless milestone slice (#1445) falls back to the - // whole input. Requires the canonical Phase/Plans Complete/Status/Completed columns - // in any order (extra columns ignored) — supersedes findTableBySchema's exact-schema lookup. - const progressMatch = roadmapContent.match(/^##[ \t]+Progress\b/im); - let scoped = roadmapContent; - if (progressMatch && progressMatch.index !== undefined) { - const afterHeading = roadmapContent.slice(progressMatch.index); - const nextHeading = afterHeading.search(/\n#{1,2}[ \t]/); - scoped = nextHeading >= 0 ? afterHeading.slice(0, nextHeading) : afterHeading; + // ADR-2143 §5 (fail-loud, no null-swallow): this used to be wrapped in a + // try/catch that silently fell through to the existing (null) values on any + // thrown error. `findTableWithColumns`/`parseMarkdownTable` never throw — + // an unparseable or absent table resolves to `null` / + // `{ ok: false, reason }`, not an exception — so the catch was masking + // nothing but dead code paths. Removed per ADR-2143 §5; the public + // `RoadmapProgress` contract (nulls = absent) is unchanged. + // + // ADR-2143 §3: read the Progress table by column NAME (order/injection-invariant), + // via the markdown-table seam. Scope to the `## Progress` section when present + // (#2012 decoy avoidance); a headingless milestone slice (#1445) falls back to the + // whole input. Requires the canonical Phase/Plans Complete/Status/Completed columns + // in any order (extra columns ignored) — supersedes findTableBySchema's exact-schema lookup. + const progressMatch = roadmapContent.match(/^##[ \t]+Progress\b/im); + let scoped = roadmapContent; + if (progressMatch && progressMatch.index !== undefined) { + const afterHeading = roadmapContent.slice(progressMatch.index); + const nextHeading = afterHeading.search(/\n#{1,2}[ \t]/); + scoped = nextHeading >= 0 ? afterHeading.slice(0, nextHeading) : afterHeading; + } + const table = findTableWithColumns(scoped, ['Phase', 'Plans Complete', 'Status', 'Completed']); + + if (table) { + const allRows = table.rows; + + const completed = allRows.filter((r) => /^complete$/i.test((r['Status'] ?? '').trim())).length; + completedPhases = completed > 0 ? completed : null; + + // Data rows only (exclude 999.x backlog phases). Mirrors init.cts /^999(?:\.|$)/ filter. + const dataRows = allRows.filter((r) => { + const phase = (r['Phase'] ?? '').trim(); + return /^\d/.test(phase) && !/^999\b/.test(phase); + }); + totalPhases = dataRows.length > 0 ? dataRows.length : null; + + let totalPlansSum = 0; + for (const r of allRows) { + const cell = (r['Plans Complete'] ?? '').trim(); + const m = /(\d+)\s*\/\s*(\d+)/.exec(cell); + if (m) totalPlansSum += parseInt(m[2], 10); } - const table = findTableWithColumns(scoped, ['Phase', 'Plans Complete', 'Status', 'Completed']); - - if (table) { - const allRows = table.rows; - - const completed = allRows.filter((r) => /^complete$/i.test((r['Status'] ?? '').trim())).length; - completedPhases = completed > 0 ? completed : null; - - // Data rows only (exclude 999.x backlog phases). Mirrors init.cts /^999(?:\.|$)/ filter. - const dataRows = allRows.filter((r) => { - const phase = (r['Phase'] ?? '').trim(); - return /^\d/.test(phase) && !/^999\b/.test(phase); - }); - totalPhases = dataRows.length > 0 ? dataRows.length : null; - - let totalPlansSum = 0; - for (const r of allRows) { - const cell = (r['Plans Complete'] ?? '').trim(); - const m = /(\d+)\s*\/\s*(\d+)/.exec(cell); - if (m) totalPlansSum += parseInt(m[2], 10); - } - totalPlans = totalPlansSum > 0 ? totalPlansSum : null; - } - } catch { /* intentionally empty — fall through to existing values */ } + totalPlans = totalPlansSum > 0 ? totalPlansSum : null; + } return { completedPhases, totalPhases, totalPlans }; } diff --git a/src/write-set.cts b/src/write-set.cts new file mode 100644 index 000000000..51e8cf1cc --- /dev/null +++ b/src/write-set.cts @@ -0,0 +1,55 @@ +/** + * Write-Set — shared fail-loud parse `Result` and per-surface write-set + * contracts (ADR-2143, epic #2143). Pure, Node built-ins only, no I/O. + * Compiled by tsc to gsd-core/bin/lib/write-set.cjs. + * + * ADR-2143 §5 (fail-loud parsing, no null-swallow): seam parse operations + * and document-model accessors return a typed `Result` — never a bare + * `null` a caller can mistake for "empty but fine." This is the same + * `{ ok: true; value: T } | { ok: false; reason: string }` shape + * `markdown-table.cts` already defined for `parseMarkdownTable` / + * `appendQuickTaskRow`; this module is now the single source of truth for + * it and `markdown-table.cjs` re-exports the type so existing importers of + * `Result` from that module keep working unchanged. + * + * NOTE: deliberately distinct from command-routing-hub's dispatch `Result` + * (`{ok,data}|{ok:false,kind}`) — the two never mix (different modules, + * different shapes, different purposes). + * + * ADR-2143 §6 (write-set results for multi-surface commands, no + * OR-into-one-flag): a command that mutates more than one surface returns + * an explicit per-surface write-set — `{ surface, applied }` outcomes — and + * its top-level "did this fully succeed" signal is true only if EVERY + * surface in the set applied. ORing independent surfaces into a single + * boolean is the direct anti-pattern that let a checkbox-only partial + * write (#2140) report full success. + */ + +export type Result = { ok: true; value: T } | { ok: false; reason: string }; + +/** + * One surface's outcome within a multi-surface write (ADR-2143 §6). + * + * `requirement` identifies which entity/ID this surface outcome belongs to, + * so a multi-entity command's write-set does not collapse independent + * entities into one aggregate — the §6 "no OR into one flag" rule applies + * per entity too, not just per surface. + */ +export interface WriteOutcome { + surface: string; + applied: boolean; + requirement?: string; +} + +/** The full set of per-surface outcomes for one multi-surface write. */ +export type WriteSet = WriteOutcome[]; + +/** + * True only if the write-set is non-empty AND every surface in it applied. + * An empty write-set is never "complete" — there is nothing to be complete + * about, so treating it as vacuously true would let a no-op masquerade as + * a full success (the same OR-into-one-flag class ADR-2143 §6 prohibits). + */ +export function writeSetComplete(ws: WriteSet): boolean { + return ws.length > 0 && ws.every((o) => o.applied); +} diff --git a/tests/milestone.test.cjs b/tests/milestone.test.cjs index 60f96aed0..795d2886f 100644 --- a/tests/milestone.test.cjs +++ b/tests/milestone.test.cjs @@ -647,6 +647,19 @@ describe('requirements mark-complete command', () => { assert.ok(content.includes('- [x] **FOO-01**'), 'checkbox should be checked'); // The table is untouched (no FOO-01 row synthesized). assert.ok(!content.includes('FOO-01 | Phase'), 'no FOO-01 row should be invented'); + + // ADR-2143 §6 write-set: additive structured read of the same per-surface + // facts — checkbox surface applied (fresh write this run), traceability + // surface did NOT (no row existed to flip), so the set is not complete. + // This does not change `updated`/`marked_complete` above. Per-ID: each + // outcome carries `requirement` so a multi-ID batch cannot OR one ID's + // outcome into another's (the #2140 class one level up). + assert.deepStrictEqual(out.write_set, [ + { requirement: 'FOO-01', surface: 'checkbox', applied: true }, + { requirement: 'FOO-01', surface: 'traceability', applied: false }, + ]); + assert.strictEqual(out.write_set_complete, false, + 'writeSetComplete requires EVERY surface applied, not an OR — a checkbox-only write is not complete'); }); test('#2140 re-run on the half-written file does NOT mask the drift as already_complete', () => { @@ -664,6 +677,15 @@ describe('requirements mark-complete command', () => { 'nothing flipped on re-run, so not marked_complete'); assert.ok(out.table_unmatched.includes('FOO-01'), 'the drift must still be surfaced as table_unmatched on re-run'); + + // ADR-2143 §6 write-set: nothing was written THIS run on either surface + // (checkbox was already [x], the row still doesn't exist), so both + // surfaces report applied:false and the set is not complete. + assert.deepStrictEqual(out.write_set, [ + { requirement: 'FOO-01', surface: 'checkbox', applied: false }, + { requirement: 'FOO-01', surface: 'traceability', applied: false }, + ]); + assert.strictEqual(out.write_set_complete, false); }); test('#2140 no traceability table at all → still a clean success (no table_unmatched)', () => { @@ -676,6 +698,104 @@ describe('requirements mark-complete command', () => { assert.ok(out.marked_complete.includes('NO-TABLE-01')); assert.ok(!out.table_unmatched || !out.table_unmatched.includes('NO-TABLE-01'), 'no table_unmatched when there is no traceability table'); + + // ADR-2143 §6 write-set: the traceability surface is omitted entirely + // (not reported as a false applied:false) when the file has no + // traceability table — nothing was required of it. A single-surface + // write-set that fully applied is complete. + assert.deepStrictEqual(out.write_set, [ + { requirement: 'NO-TABLE-01', surface: 'checkbox', applied: true }, + ]); + assert.strictEqual(out.write_set_complete, true); + }); + + test('#2140-class multi-ID: one ID\'s partial reconcile is not masked by another ID\'s full one', () => { + // Adversarial-review regression: write_set/write_set_complete were built + // from two INVOCATION-WIDE booleans OR-accumulated across every ID in the + // batch, so a fully-reconciled REQ-02 in the same call could mask a + // checkbox-only partial write on REQ-01. The write-set must be tracked + // PER (requirement, surface) so REQ-01's unmatched traceability row + // cannot be hidden by REQ-02 reconciling cleanly. + writeRequirements( + tmpDir, + `# Requirements + +## Coverage +- [ ] **REQ-01**: feature one (no traceability row) +- [ ] **REQ-02**: feature two (has traceability row) + +## Traceability + +| Requirement | Phase | Status | +|-------------|-------|--------| +| REQ-02 | Phase 1 | Pending | +`, + ); + + const result = runGsdTools('requirements mark-complete REQ-01,REQ-02', tmpDir); + assert.ok(result.success); + const out = JSON.parse(result.output); + + // Pre-existing fields are unchanged: both checkboxes flip (marked_complete), + // REQ-01 has no table row so it surfaces as table_unmatched. + assert.deepStrictEqual(out.marked_complete.sort(), ['REQ-01', 'REQ-02']); + assert.deepStrictEqual(out.table_unmatched, ['REQ-01']); + assert.deepStrictEqual(out.not_found, []); + assert.deepStrictEqual(out.already_complete, []); + + const content = readRequirements(tmpDir); + assert.ok(content.includes('- [x] **REQ-01**'), 'REQ-01 checkbox should be checked'); + assert.ok(content.includes('- [x] **REQ-02**'), 'REQ-02 checkbox should be checked'); + assert.ok(content.includes('| REQ-02 | Phase 1 | Complete |'), 'REQ-02 table row should be Complete'); + assert.ok(!content.includes('REQ-01 | Phase'), 'no REQ-01 row should be invented'); + + // The write-set is now per-ID: REQ-01's traceability surface did NOT + // apply (no row to flip) even though REQ-02's did — the aggregate must + // not OR REQ-02's success into REQ-01's outcome. + assert.deepStrictEqual(out.write_set, [ + { requirement: 'REQ-01', surface: 'checkbox', applied: true }, + { requirement: 'REQ-01', surface: 'traceability', applied: false }, + { requirement: 'REQ-02', surface: 'checkbox', applied: true }, + { requirement: 'REQ-02', surface: 'traceability', applied: true }, + ]); + // Because REQ-01's traceability entry did not apply, the batch as a + // whole is NOT complete — this is the exact bug the fix closes. + assert.strictEqual(out.write_set_complete, false, + 'REQ-01\'s unmatched traceability row must not be masked by REQ-02 fully reconciling'); + }); + + test('#2140-class multi-ID: fully-reconciled batch reports write_set_complete:true', () => { + writeRequirements( + tmpDir, + `# Requirements + +## Coverage +- [ ] **REQ-01**: feature one +- [ ] **REQ-02**: feature two + +## Traceability + +| Requirement | Phase | Status | +|-------------|-------|--------| +| REQ-01 | Phase 1 | Pending | +| REQ-02 | Phase 1 | Pending | +`, + ); + + const result = runGsdTools('requirements mark-complete REQ-01,REQ-02', tmpDir); + assert.ok(result.success); + const out = JSON.parse(result.output); + + assert.deepStrictEqual(out.marked_complete.sort(), ['REQ-01', 'REQ-02']); + assert.deepStrictEqual(out.table_unmatched, []); + + assert.deepStrictEqual(out.write_set, [ + { requirement: 'REQ-01', surface: 'checkbox', applied: true }, + { requirement: 'REQ-01', surface: 'traceability', applied: true }, + { requirement: 'REQ-02', surface: 'checkbox', applied: true }, + { requirement: 'REQ-02', surface: 'traceability', applied: true }, + ]); + assert.strictEqual(out.write_set_complete, true); }); test('marks single requirement complete (checkbox + table)', () => { @@ -692,6 +812,14 @@ describe('requirements mark-complete command', () => { assert.ok(content.includes('- [x] **TEST-01**'), 'checkbox should be checked'); assert.ok(content.includes('| TEST-01 | Phase 1 | Complete |'), 'table row should be Complete'); assert.ok(content.includes('- [ ] **TEST-02**'), 'TEST-02 should remain unchecked'); + + // ADR-2143 §6 write-set: both surfaces got a fresh write this run, so the + // write-set is complete. + assert.deepStrictEqual(output.write_set, [ + { requirement: 'TEST-01', surface: 'checkbox', applied: true }, + { requirement: 'TEST-01', surface: 'traceability', applied: true }, + ]); + assert.strictEqual(output.write_set_complete, true); }); test('handles mixed prefixes in single call (TEST-XX, REG-XX, INFRA-XX)', () => { diff --git a/tests/write-set.test.cjs b/tests/write-set.test.cjs new file mode 100644 index 000000000..2ee4e40f0 --- /dev/null +++ b/tests/write-set.test.cjs @@ -0,0 +1,97 @@ +'use strict'; + +/** + * Behavioral tests for write-set.cjs + * + * Module: gsd-core/bin/lib/write-set.cjs + * Exports: writeSetComplete (Result is a pure type re-exported at compile + * time only — nothing to assert on it at runtime beyond markdown-table.cjs's + * own re-export continuing to work, covered by tests/markdown-table.test.cjs). + * + * Covers (ADR-2143 §5/§6): + * - writeSetComplete happy path: all-applied → true, one-not-applied → false + * - BOUNDARY coverage: empty write-set (0 outcomes), single outcome, two outcomes + * - an empty write-set is never "complete" (no vacuous-true on a no-op) + * - fast-check property: writeSetComplete(ws) === (ws.length > 0 && every applied) + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fc = require('./helpers/fast-check-setup.cjs'); + +const { writeSetComplete } = require('../gsd-core/bin/lib/write-set.cjs'); + +describe('writeSetComplete', () => { + test('every surface applied → true', () => { + assert.equal( + writeSetComplete([ + { surface: 'checkbox', applied: true }, + { surface: 'traceability', applied: true }, + ]), + true, + ); + }); + + test('one surface not applied → false (AND across surfaces, never OR)', () => { + assert.equal( + writeSetComplete([ + { surface: 'checkbox', applied: true }, + { surface: 'traceability', applied: false }, + ]), + false, + ); + }); + + test('no surfaces applied → false', () => { + assert.equal( + writeSetComplete([ + { surface: 'checkbox', applied: false }, + { surface: 'traceability', applied: false }, + ]), + false, + ); + }); + + // BOUNDARY: 0 outcomes (limit-1 relative to the smallest real set), 1 outcome + // (limit), 2 outcomes (limit+1) — the write-set shape has no upper bound, but + // the meaningful boundary here is "is there anything to be complete about". + test('BOUNDARY: empty write-set (0 outcomes) is never complete', () => { + assert.equal(writeSetComplete([]), false, + 'an empty write-set must not vacuously report complete — that would let a ' + + 'no-op masquerade as full success, the exact OR-into-one-flag class ADR-2143 §6 prohibits'); + }); + + test('BOUNDARY: single-outcome write-set (1 outcome) — applied true is complete', () => { + assert.equal(writeSetComplete([{ surface: 'checkbox', applied: true }]), true); + }); + + test('BOUNDARY: single-outcome write-set (1 outcome) — applied false is not complete', () => { + assert.equal(writeSetComplete([{ surface: 'checkbox', applied: false }]), false); + }); + + test('BOUNDARY: two-outcome write-set (2 outcomes) — both true is complete', () => { + assert.equal( + writeSetComplete([ + { surface: 'checkbox', applied: true }, + { surface: 'traceability', applied: true }, + ]), + true, + ); + }); + + // ─── Property test ─────────────────────────────────────────────────────────── + + const outcomeArb = fc.record({ + surface: fc.string({ minLength: 1, maxLength: 12 }), + applied: fc.boolean(), + }); + + test('property: writeSetComplete(ws) === (ws.length > 0 && every outcome applied)', () => { + fc.assert( + fc.property(fc.array(outcomeArb, { maxLength: 8 }), (ws) => { + const expected = ws.length > 0 && ws.every((o) => o.applied); + assert.equal(writeSetComplete(ws), expected); + }), + ); + }); +});