From db7d6c0fc92cf56db8913d4af2778d0ddb6dc73e Mon Sep 17 00:00:00 2001 From: bigbes Date: Wed, 22 Jul 2026 11:24:10 +0300 Subject: [PATCH] docs: correct the merge model against implementation Resolves a contradiction: the Space section called policy changes reviewable, but the merge is keyed by document ID and .spec.yml has none, so a proposal could never express one. Policy joins deletion and rename as human-push-only, which costs nothing with a single reviewer. Corrects the claimed cost of ID-keyed staleness. It needs an ID-to-path index over both whole trees, not one parse of the changed set; free at our volume, two full-tree reads at ten thousand documents. Adds the three staleness cases the two-line rule omitted, each of which is silent corruption if unhandled, and records that an already-merged proposal needs an ancestry check because the staleness rule reports a confusing 409 instead. Records that malformed documents on the approved branch are tolerated rather than fatal, since skip-validation guarantees they can exist and failing the index build would turn one typo into a space-wide outage. --- docs/DESIGN.md | 52 ++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 48 insertions(+), 4 deletions(-) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index eaa9fc81a8da49f37a7f50446d77cac780586dd8..41f2d1883314321bfd53db0397a25f220d384eda 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -428,13 +428,43 @@ commit keeps the proposal visible in `git log`. `blob(f)@H` to `blob(f)@B` by path, inside a system whose entire premise is "paths move, IDs do not". A rename between `B` and `H` would then surface as a baffling 409 — or worse, a proposal re-adding the old path would silently -resurrect a document that had been moved. Resolving each ID to its path *in `H`* -before comparing costs one frontmatter parse of the changed set and removes both -failure modes. +resurrect a document that had been moved. + +**Correction on its cost.** An earlier draft claimed this costs "one frontmatter +parse of the changed set". It does not: resolving IDs to paths requires an +ID→path index over *all* of `H` and all of `B`, so every document in both trees +is parsed, not just the changed ones. At the confirmed volume this is free; at +ten thousand documents it is two full-tree reads per merge. The `document_id` +table is exactly this index, but `gitx` must not depend on `db/` — and git is the +source of truth regardless — so the index is rebuilt from the trees. + +**Four staleness cases, not one.** The two-line rule above only covers "the blob +changed". The others are silent corruption if unhandled, and are implemented: + +| Case | Why it must 409 | +|---|---| +| blob changed under us | the ordinary conflict | +| document removed from `H` since `B` | splicing it back silently resurrects a deletion | +| same ID appeared on `H` after `B` | two documents now claim one identity | +| a new document's path is occupied on `H` | the splice would overwrite an unrelated document | + +**Already-merged proposals need an ancestry check, not a staleness check.** Once +`H` carries the proposal's own blobs, the blob comparison is trivially "changed" +and returns a confusing 409. `service/` must test `IsAncestor(proposalHead, H)` +first and report "already merged" — the staleness rule cannot distinguish the two +cases on its own. + +**Malformed documents on the approved branch are tolerated, not fatal.** Because +`--push-option=skip-validation` exists, a document with a broken or duplicated +`id:` *can* be on the approved branch. Failing the ID-index build on encountering +one would let a single typo block every future merge in that space — an outage +caused by a cosmetic error. Such a document is instead recorded by *path*, so its +path stays occupied and nothing silently overwrites it, and excluded from the ID +map; a duplicate ID is refused only when a merge actually needs to resolve it. **Deletion and rename need an explicit surface.** The write plane is whole-document `PUT`, which gives an agent no way to express "delete this" or -"move this". Two coherent answers; **the second is the v1 recommendation**: +"move this". Two coherent answers; **the second is the v1 decision**: 1. Add `DELETE` and a move operation to the proposal API. 2. **Deletion and rename are human-push-only.** They are rare, destructive, and @@ -442,6 +472,20 @@ whole-document `PUT`, which gives an agent no way to express "delete this" or rights to the approved branch. An agent that thinks a document should go proposes `status: superseded` instead, which is reviewable and reversible. +**So is `.spec.yml`, and the design contradicted itself here.** The Space section +says "policy changes are themselves reviewable", implying an agent proposes one — +but the merge is defined purely over document IDs, and `.spec.yml` has none, so +there is no way to express it. Rather than add a path-keyed side channel to the +merge for a single file, **policy changes reach the approved branch only by human +push**, consistent with deletion and rename. A proposal containing any +non-document change is refused outright. With one reviewer this costs nothing: +the person who would approve the policy change is the person pushing it. + +**Refs outside the two namespaces are rejected for both principals.** The design +covers the approved branch and `proposals/*` and was silent on tags and other +branches. Silence would mean "allowed", and a ref nothing ever reads is a place +for content to rot unnoticed. Loosening this later is one branch in a switch. + ## Consistency and recovery Three systems are touched by a merge — git refs, the bleve index, and Postgres —