From fac0e9de86bda3bf559ca19b921b2734aa81ae33 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 21 Sep 2026 00:17:10 -0400 Subject: [PATCH] =?UTF-8?q?docs(#4906):=20ADR-4910=20amendment=20=E2=80=94?= =?UTF-8?q?=20a=20write=20refuses=20on=20a=20document=20with=20any=20unrea?= =?UTF-8?q?dable=20node=20(#4913)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * docs(#4906): ADR-4910 amendment — a write refuses on a document with any unreadable node §5 scoped the parse error to the node. That is correct for reads and was never examined for writes. §5 was reasoned entirely from #4899, a read bug. Node-scoping is right there: a ragged Progress table must not make phase list and init.progress fail, because those are the commands a user needs to see what to repair. But the rule was stated unqualified, and it licensed something never considered — phase.complete mutating a ROADMAP.md whose Progress table it could not read. A partial-view write persists a wrong answer rather than merely returning one. Amendment: reads stay node-scoped; a write refuses when ANY node in the document carries a parse error, whether or not the mutation targets it. A reader answers a bounded question from a bounded region; a writer asserts that the document it emits is the document it read, and cannot make that assertion about a region it could not parse. §3's byte-stability does not rescue it — splicing untouched bytes faithfully is not the same as knowing they were consistent with the change. Rejected: refusing only when the mutation's own target node is unreadable. It is the appealing middle and it does not hold — phase.complete writes **Plans:** while deriving that value from plan/summary counts in a different region. The regions a write depends on are not statically the regions it touches. Downstream: Phase 1 ships both scopes plus a hasUnreadableNodes predicate the serializer consults; Phase 2 gains a new acceptance criterion (a refused write leaves the file byte-identical on disk); Phase 4's census gains a second axis and must publish both counts. Appended as a dated section per docs/contributor-standards.md pattern 1 — the original Decision body is unchanged. Refs #4906 Refs #4910 Co-Authored-By: Claude Opus 5 * docs(#4906): correct the amendment's motivating example and state the limit it exposes An isolated review pass falsified the first draft's central example. The correction is recorded in the amendment rather than quietly patched, because it bounds what the rule can promise. - The draft claimed phase.complete derives the value it writes into **Plans:** from a different REGION of the same document. It does not. planCount and summaryCount come from findPhaseInternal at src/phase.cts:3429-3434 — a filesystem scan of the phase directory. No PlanningDoc node holds them. That widens the dependency rather than narrowing it, and it makes an explicit limit necessary: a document-scoped write refusal protects the document's own consistency and says nothing about the correctness of a value sourced from outside the document. Recorded as a stated limit and named out of scope for this epic. - The rejected alternative (refuse only when the mutation's own target node is unreadable) is re-grounded on two arguments that survive: it would almost never fire, since a verb locates the node in order to write it; and it guards something smaller than the operation performs, because serialization re-emits the whole file. - §5's body names `phase list` as a consumer an unreadable Progress table would block. It is not one — cmdPhasesList (src/phase.cts:207) enumerates phases/ and never opens ROADMAP.md. init.progress and roadmap.analyze are the real instances; the argument never needed three. §5's body left unmodified per the append-only rule, correction carried in the amendment, fold in at ratification. - Clarified that the new WriteOutcome refusal sits on a different axis from §5's reserved document-level Result parse failure, so no reader can conclude that reservation was reopened. A document can be valid to open and still refuse to be written. Refs #4906 Refs #4910 Co-Authored-By: Claude Opus 5 --------- Co-authored-by: sim Co-authored-by: Claude Opus 5 --- docs/adr/4910-planning-document-seam.md | 116 ++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/docs/adr/4910-planning-document-seam.md b/docs/adr/4910-planning-document-seam.md index dd0d46e8f..c52cfe1db 100644 --- a/docs/adr/4910-planning-document-seam.md +++ b/docs/adr/4910-planning-document-seam.md @@ -497,3 +497,119 @@ Two consequences for the phases: - Fail-loud precedent: [ADR-1411](1411-resolution-provenance.md) — report provenance rather than fall open silently - Sibling consolidation: [#2121](https://github.com/open-gsd/gsd-core/issues/2121) (`phase-id.cts`) - Absorbed: [#4736](https://github.com/open-gsd/gsd-core/issues/4736), [#4793](https://github.com/open-gsd/gsd-core/issues/4793), [#4852](https://github.com/open-gsd/gsd-core/issues/4852), [#4862](https://github.com/open-gsd/gsd-core/issues/4862), [#4899](https://github.com/open-gsd/gsd-core/issues/4899), [#4900](https://github.com/open-gsd/gsd-core/issues/4900), [#4837](https://github.com/open-gsd/gsd-core/issues/4837), [#4865](https://github.com/open-gsd/gsd-core/issues/4865), [#4661](https://github.com/open-gsd/gsd-core/issues/4661), [#4605](https://github.com/open-gsd/gsd-core/issues/4605), [#4606](https://github.com/open-gsd/gsd-core/issues/4606), [#4499](https://github.com/open-gsd/gsd-core/issues/4499) + +## Amendment (2026-09-21): a write refuses on a document with any unreadable node + +**§5's node-scoping is correct for reads and was never examined for writes.** This amendment adds +the write rule. The original `## Decision` body is unchanged and §5 still holds as written. + +### What was missed + +§5 was reasoned entirely from [#4899](https://github.com/open-gsd/gsd-core/issues/4899), which is a +**read** bug — `roadmap.analyze` returning `phases: []`. Node-scoping is the right answer there: a +ragged Progress table should not make `init.progress` fail as well, and it must not, because that is +a command a user needs in order to *see what to repair*. A hand-editable format whose single typo +bricks every command that could diagnose it is a worse tool. + +> **Correction to §5's own wording, carried here rather than silently.** §5's body names `phase list` +> alongside `init.progress` as a consumer that would be blocked. That is wrong: `cmdPhasesList` +> (`src/phase.cts:207`) enumerates the `phases/` directory and never opens `ROADMAP.md`, so an +> unreadable Progress table cannot affect it either way. `init.progress` (`src/init.cts:3625`) does +> read `ROADMAP.md` and is a real instance; `roadmap.analyze` is the other. The argument stands on +> those two — it never needed three — but the example was not checked when §5 was written. §5's body +> is left unmodified per the append-only rule; fold this correction in at ratification. + +But "the error lives on the node" was then stated unqualified, and it silently licensed something the +original never considered: **`phase.complete` re-serializing a `ROADMAP.md` whose Progress table it +could not read.** Serialization re-emits the whole file, so the verb republishes every region — +including the one nobody could parse — while having understood only part of it. That is a +partial-view write, and it is a strictly worse failure than the one #4899 reports, because the read +bug returns a wrong answer while this one *persists* one. + +### The rule + +**Reads are node-scoped. Writes are document-scoped.** + +- A **read** of node *n* resolves independently: it returns *n*'s values, or `could-not-parse` with + *n*'s span. A sibling node's parse failure is invisible to it. Unchanged from §5. +- A **write** — any `PlanningDoc` mutation reaching the serializer — **refuses** when *any* node in + the document carries a parse error, whether or not the mutation targets that node. The refusal is a + typed `WriteOutcome` naming the unreadable node and its span, never a silent skip and never a + partial apply. + +**This does not reopen §5's "reserved for".** §5 reserves the document-level `Result` +*parse* failure for a document that is not a planning artifact at all, and that reservation stands +untouched. The refusal added here is a different type on a different axis — a `WriteOutcome` at +mutate/serialize time, on a document that parsed successfully and merely contains one or more nodes +carrying their own errors. A document can therefore be perfectly valid to *open* and still refuse to +be *written*, which is the intended state. + +The asymmetry is deliberate and is the point of the amendment: a reader is answering a bounded +question and can honestly answer it from a bounded region, while a writer is asserting that the +document it emits is the document it read. A writer that cannot read a region cannot make that +assertion about it, and §3's byte-stability guarantee does not rescue it — splicing untouched bytes +through faithfully is not the same as knowing they were consistent with the change being written. + +### Why not the alternatives + +- **Document-scoped for reads too** (strictest, one error type, simplest to test). Rejected: one + unreadable table blocks every consumer of that file, including the diagnostic commands, and the + artifacts are hand-editable by design — the epic's own constraint. +- **Node-scoped for writes** (what §5 left implied). Rejected on the partial-view write above. +- **Refuse only when the mutation's own target node is unreadable.** Rejected, for two reasons that + survive scrutiny: + 1. **It would almost never fire.** A verb locates the node in order to write it, so the target is + readable in the ordinary case by construction. A precondition that is satisfied whenever the + write is attempted is not a precondition. + 2. **It does not match the blast radius of the operation it guards.** Serialization re-emits the + **whole file** (§3). A write is therefore a document-wide assertion — *this is the document I + read* — regardless of how narrow the mutated span is. A document-wide act takes a + document-wide precondition; scoping it to one node guards something smaller than what is + actually happening. + +### Stated limit: this rule does not cover a write whose input comes from outside the document + +An isolated review of this amendment falsified its first draft's motivating example, and the true +mechanism is worth recording because it bounds what the rule can promise. + +The draft claimed `phase.complete` derives the value it writes into `**Plans:**` from *a different +region of the same document*. It does not. `planCount` and `summaryCount` are read at +`src/phase.cts:3429-3434` from `findPhaseInternal(cwd, phaseNum)` — a **filesystem scan of the phase +directory**. No `PlanningDoc` node holds those counts at all. + +That makes the dependency wider than the draft claimed, and it makes the limit explicit: + +> **A document-scoped write refusal protects the document's own consistency. It says nothing about +> the correctness of a value sourced from outside the document.** If the phase directory is +> mid-write, partially synced, or otherwise disagrees with reality, `phase.complete` writes a wrong +> count into a perfectly readable `**Plans:**` line and this rule will not object — correctly, since +> every node parsed. + +The write rule still holds on its own terms (see the two reasons above; neither depends on where the +written *value* came from). But the seam does not make a write *correct*, only *whole*, and an +implementer must not read §5-plus-this-amendment as a guarantee that a successful write was right. +Closing the outside-input gap is a separate concern about where verbs source their values, not about +how documents are parsed, and it is **not** in this epic's scope. It is recorded here so that the +next reader finds it stated rather than re-derives it from a surprise in production. + +### What changes downstream + +- **Phase 1** ships both scopes: the node-scoped read error of §5, **and** a document-level + `hasUnreadableNodes` predicate the serializer consults before applying any mutation. Both get + positive controls. +- **Phase 2** gains an acceptance criterion that is new, not reworded: *a mutation against a document + carrying any unreadable node refuses, with a typed outcome naming that node — and the file on disk + is byte-identical afterwards.* The last clause matters: "refused" must mean nothing was written, + which is a filesystem assertion, not a return-value one. +- **Phase 4's census gains a second axis.** It was "every read path that can return `[]` for an input + it failed to parse". It is now that, **plus** every write path that can apply a mutation to a + document with an unreadable node. Those are different call sets and the phase must publish both + counts. + +### Provenance + +Raised by a maintainer ruling on 2026-09-21, after §5 shipped in +[#4911](https://github.com/open-gsd/gsd-core/pull/4911). The gap was real: §5's node-scoping had been +recorded in that PR as an interpretation of #4906's *"an unparseable shape surfaces `could-not-parse` +with the offending span"* — a sentence that carries no read-or-write qualifier — and the +interpretation was made without examining the write side at all.