amadan
Import spec revision 3: settle what two implementers would guess at
The second review came back "not ready -- but close": the architecture held, and ten points were under-specified in ways two people would resolve differently. Those are now settled -- the canonical form's full rule set (including that a block's runs share one Y.XmlText, or y-prosemirror rewrites the block on first edit), what an empty file does, the file-size caps as actual numbers rather than a promise that they were derived, where the import report lives and when it is written, what happens to a failed file in the no-JS path, Markdown beyond this app's own export subset, and the fact that splitting snapshotEncodingVersion off the ABI number overrides a ruling in the snapshots design and so is Paul's call. One prescription is withdrawn rather than implemented. Revision 2, having established that the editor writes heading.level as a number, told the builder to do the same. Implementing it showed that yrs 0.23.5's public API cannot store a non-string XML attribute at all: insert_attribute is typed V: Into<String>, and the typed path runs through MapRef, which wraps a BranchPtr that is private to the crate. So the builder writes a string, which is what this repo's fixtures already contain, and §4 D2 records exactly what that costs -- including that prosemirror-model's validate:"number" would reject it if anything ever called node.check(), a tripwire that already exists for every heading in the fixtures.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RMNxUbTqqxehd9JdtkYfbe
1 file changed,
+155
−20
docs/superpowers/specs/2026-09-01-import-design.md+155 −20
CI passed — run details
diff --git a/docs/superpowers/specs/2026-09-01-import-design.md b/docs/superpowers/specs/2026-09-01-import-design.md| index 7ab3180..b3cb3d8 100644 |
| --- a/docs/superpowers/specs/2026-09-01-import-design.md |
| +++ b/docs/superpowers/specs/2026-09-01-import-design.md |
| @@ -8,6 +8,12 @@ This is the other half of §11 of the [main design](2026-08-30-docs-design.md), |
| which has always said Docs is an editor "with DOCX, PDF and Markdown **in and |
| out**". Out shipped (`internal/export`, `internal/pdf`). In is this document. |
| +**Revision 3, after a second review returned "not ready — but close": the |
| +architecture held, and ten points were under-specified in ways two |
| +implementers would resolve differently. Revision 3 settles them, and |
| +withdraws one prescription that turned out not to be implementable at all — |
| +yrs cannot write a numeric attribute (§4 D2).** |
| + |
| **Revision 2, after a Fable review of revision 1 returned "not ready" with |
| sixteen defects.** One of them holed the centre: revision 1 had the importer |
| author its content under the document row's own `ClientID`, and **861 of |
| @@ -103,9 +109,14 @@ importer trips the first of them constantly: |
| So the contract is stated over a **canonical tree** instead: |
| -> **Canonical form.** Adjacent runs with identical mark sets are merged; no |
| -> empty text runs; marks in `MARKS` order, deduplicated; `attrs` only on |
| -> `heading`; `content` always present. Importers emit canonical trees. |
| +> **Canonical form.** Within one block, consecutive text runs whose mark-name |
| +> *sets* are equal are merged (sets, not sequences — order does not matter, |
| +> because marks are then written in `MARKS` order). No empty text runs. Marks |
| +> in `MARKS` order, deduplicated, and the `marks` key is **omitted entirely** |
| +> when a run has none — never `"marks":[]`. A mark is exactly |
| +> `{"type": name}`, with no `attrs`. A text node carries no `content`; an |
| +> element carries no `text` and no `marks`. `attrs` appears only on |
| +> `heading`; `content` always appears. Importers emit canonical trees. |
| > |
| > **Round-trip gate:** for any canonical tree, `fragment_json(build(tree))` |
| > parses to a structurally equal value (`serde_json::Value`, never string |
| @@ -126,6 +137,28 @@ list_item → paragraph block* (fixtures/editor-entry.js:38 |
| blockquote → block+ |
| ``` |
| +where **block** is `paragraph | heading | bullet_list | ordered_list | |
| +blockquote`. `list_item` is not a block and appears only inside a list. |
| + |
| +**One `XmlText` per block.** y-prosemirror groups every adjacent ProseMirror |
| +text node in a block into a single `Y.XmlText` (`sync-plugin.js`'s |
| +`normalizePNodeContent`), and `fragment_json` emits one `text_json` call per |
| +`XmlText` child, never merging across two siblings. So a builder that gave |
| +each run its own `XmlText` would round-trip perfectly and still differ from |
| +what the browser would have written — which makes y-prosemirror rewrite the |
| +whole block on the first edit. Runs go into one `XmlText` per block. |
| + |
| +**An empty document is refused** (`doc → block+`), so a file that yields no |
| +blocks — an empty or whitespace-only `.txt`, a `.md` of nothing but blank |
| +lines — fails with a clear message and creates no document, exactly as a |
| +text-free PDF does (§8). It does not quietly create a blank document: "I |
| +dragged a file in and got an empty document" is indistinguishable from the |
| +feature being broken. |
| + |
| +Nested lists are legal here and reachable only through import: the editor's |
| +schema permits them, but its toolbar binds no `sinkListItem`, so until now |
| +only fixtures have produced one. That widens what documents contain, not what |
| +the schema allows. |
| The vocabulary is a strict **subset** of the editor's — revision 1 said |
| "what the editor's schema already speaks, no more", which was inaccurate: |
| @@ -147,6 +180,10 @@ mark as `{ name: mark.attrs }`, i.e. `{}` for the attribute-free marks |
| browser writes gets rewritten by y-prosemirror's attribute comparison on the |
| first edit. |
| +**`heading.level` is the exception, written as a string because yrs cannot |
| +write anything else.** See §4 D2. It is the one place where matching the |
| +browser exactly is not achievable, and it is documented rather than hidden. |
| + |
| ### 3.1 The Go side |
| Revision 1 left the Go tree type undefined and implied `export.Node` could |
| @@ -197,11 +234,10 @@ and yrs's reader hides the difference — `get_attribute` stringifies, so |
| `2.0` prints as `2` and a spike reading through Rust cannot tell the two |
| apart. |
| - **The builder writes `level` as a numeric `Any`.** And |
| - `generate-fragment.mjs:50` is corrected to a number, so the fixture |
| - represents the editor it claims to represent. The `node.check()` remark in |
| - revision 1 was inverted: today's editor-written headings pass validation; |
| - string-level ones would be the failures. |
| + The `node.check()` remark in revision 1 was also inverted: today's |
| + editor-written headings pass validation, and string-level ones are the |
| + failures. Revision 2 concluded the builder should therefore write a |
| + number. **It cannot — see D2.** |
| The general lesson, since this repo has now been bitten by it twice |
| (AGENTS.md's "tools that answer wrongly instead of failing"): **a fixture |
| @@ -239,6 +275,34 @@ been on the wire — and every wire fixture uses `clientID` 1, 2 or 3. |
| real edits are attributed to the row's `ClientID` — they are attributed to |
| the browser's own `new Y.Doc()` id. |
| +**D2 — the builder writes `heading.level` as a string, and that is forced.** |
| +Revision 2, having established that the editor writes a number, told the |
| +builder to write `Any::Number`. Implementing it showed that **yrs 0.23.5's |
| +public API cannot store a non-string XML attribute at all.** The only setter |
| +the `Xml` trait exposes is `insert_attribute`, typed `V: Into<String>` |
| +(`xml.rs:1002`). The typed path — an `XmlElement`'s attributes live in the |
| +same branch map a `Y.Map` uses, so a `MapRef` over it would take an `Any` — |
| +is closed, because `MapRef` wraps `BranchPtr` (`map.rs:62`, `:512`) and |
| +`BranchPtr` is private to the crate. Short of forking a library that is half |
| +of a wire contract and pinned at `=0.23.5`, a string is the only value the |
| +builder can write. |
| + |
| + What that costs, stated here rather than discovered later: it is what this |
| + repo's fixtures already contain (`generate-fragment.mjs:50`); the editor |
| + opens such a document, verified by the cross-implementation gate, and a |
| + level-2 heading still renders `<h2>`; `fragment_json` cannot see the |
| + difference, because it reads through `get_attribute`, which stringifies |
| + (`xml.rs:1026`); it differs from what the browser would write, so |
| + y-prosemirror may rewrite an imported heading on its first edit — CRDT |
| + churn, not lost text; and `prosemirror-model` 1.25's `validate: "number"` |
| + rejects a string level, so any future code that calls `node.check()` |
| + breaks. That tripwire already exists for every heading in this repo's |
| + fixtures; import does not introduce it, and does not clear it either. |
| + |
| + `generate-fragment.mjs:50` stays a string, then — it turns out to |
| + represent what the *server* writes, even though it does not represent what |
| + the editor writes. |
| + |
| **Reported, not fixed: `fragment_text` renders marks as literal markup.** |
| A bold word comes back as `plain <strong>bold</strong> plain`, because yrs's |
| `XmlTextRef::get_string` serialises formatting as tags. I fixed this, with a |
| @@ -248,7 +312,7 @@ verified that yrs's `get_string` and `Y.XmlText.toString()` agree on it — |
| "XmlText's own plain-text convention in both implementations". |
| It is still wrong for its consumers. `fragment_text`'s own doc comment says |
| - it is "the words without the markup", it feeds the list preview, and the |
| + it is "the words without the markup", it would feed the list preview if anything called it, and the |
| two fixture families already disagree with each other |
| (`fixtures/data/formatted.expected.json` says `plain bold plain`; |
| `fixtures/vectors/fragment/formatted.expected.json` says |
| @@ -291,11 +355,22 @@ directly contradicting §8's rule that a text-free PDF creates no document. |
| Corrected: |
| 1. parse the file → a canonical tree (§3) |
| -2. open a scratch core document: fresh GUID, **fresh client id in `[0, 2^32)`** (§4 D1) |
| +2. open a scratch core document: fresh GUID, **fresh client id in `[0, 2^32)`** |
| + (§4 D1). Both drawn **in Go** (`uuid.NewString()`, and a `uint32` from |
| + `math/rand/v2`), never from yrs — the rule `Store.Create` already follows |
| + and `clippy.toml` enforces on the Rust side. Drawn per import, never a |
| + fixed "importer id", which would collide the day "import into an existing |
| + document" (§14) is built |
| 3. `docs_core_build` |
| 4. `Snapshot` → the update bytes; check them against the update budget (§10) |
| 5. **only now** `Store.Create` — the row |
| -6. `Store.ApplyUpdate` — the ordinary persistence path, deleting the row if it fails |
| +6. `Store.ApplyUpdate` — the ordinary persistence path |
| + |
| +If step 6 fails, the cleanup deletes `documents`, `doc_updates` **and** |
| +`doc_import_reports` for that id, through `owned()`. Not the row alone: |
| +`ApplyUpdate` can fail after its `doc_updates` insert has committed |
| +(`store.go:837`), so deleting only `documents` orphans the log. `Store` |
| +exposes no delete at all today, so plan 1 adds one. |
| Step 6 is the point of the rest. Import writes nothing the editor does not; |
| there is no second write path, and an imported document is indistinguishable |
| @@ -317,7 +392,18 @@ natural fuzz target, and gets one. |
| snapshot; each document then replays its whole log until its next write, and |
| one past 4096 rows re-seeds on every read, logging a warning each time. The |
| state *encoding* does not change, so this is pure waste. **Split a |
| -`snapshotEncodingVersion` from the ABI number** and bump only the ABI. |
| +`snapshotEncodingVersion` from the ABI number** and bump only the ABI. The |
| +column stays `abi_version` — a misnomer afterwards, worth a comment rather |
| +than a migration — existing rows stay valid if the new constant starts at 4, |
| +and `TestNewestSnapshotIgnoresAnotherABIsRows` is re-keyed to it. The new |
| +constant bumps on any yrs upgrade or any change to the encode call, because |
| +nothing else will remind anyone. |
| + |
| +**This overrides a ruling, so it is Paul's call.** The snapshots design's D1 |
| +chose "from an older ABI → discarded" deliberately. Splitting the version is |
| +a better answer to a case that design did not have — an ABI bump that leaves |
| +the state encoding untouched — but it is still a reversal, and belongs on the |
| +same list as the `fragment_text` decision in §4. |
| --- |
| @@ -353,7 +439,18 @@ in this slice — those marks exist for the toolbar, and inferring them from |
| Word's own font table is a fidelity project of its own. |
| **Markdown** (`markdown.go`) — the subset `internal/export/markdown.go` |
| -writes, which makes export→import a testable round trip. |
| +writes, which makes export→import a testable round trip; unescaping what |
| +that file's `escapeMarkdown` wrote is what makes it identity rather than |
| +merely close. |
| + |
| +But §1 promises `.md` from *anything*, and real Markdown carries more than |
| +this app's exporter emits. Every construct outside the subset has a defined |
| +answer, so no implementer has to invent one: **link** → text kept, target |
| +dropped, warn; **image** → dropped, warn; **fenced or indented code** → |
| +paragraphs carrying the `code` mark, warn (import's subset has no |
| +`code_block`); **table** → cell text as paragraphs, warn; **HTML block or |
| +inline** → its text kept, warn; **thematic break** → dropped, warn; **hard |
| +line break** → paragraph split, warn — the same answer `<w:br/>` gets (§3). |
| **Plain text** (`text.go`) — blank-line-separated paragraphs. **UTF-8 is |
| validated in Go**, with a BOM stripped and invalid bytes a refusal, before |
| @@ -422,8 +519,12 @@ Main spec §11.2 requires it: "a report naming exactly what it could not |
| bring in, and where". Revision 1 left its storage, lifetime and content |
| rules undefined, which was its largest under-specification. |
| -**Storage: a `doc_import_reports` row, keyed by document**, written in the |
| -same transaction as the import and rendered on `show.html`. A `303` to |
| +**Storage: a `doc_import_reports` row, keyed by document**, written after |
| +`Store.ApplyUpdate` returns, and rendered on `show.html`. Not "in the same |
| +transaction": no such transaction exists — `ApplyUpdate` opens and commits |
| +its own (`store.go:795`) and `Create` autocommits. A failed report write |
| +fails the import and triggers §5's cleanup, because a document that silently |
| +lost a table is the outcome main spec §11.2 forbids. A `303` to |
| `/d/{sqid}` carries nothing, and a flash cookie's 4 KB cannot hold a real |
| DOCX report. Dismissal is a `POST` that deletes the row; the `fetch` path |
| receives the same structure as JSON. |
| @@ -464,7 +565,11 @@ sniffing `Accept`. No-JS with several files selected redirects to the list |
| (not to any one document) with the report rows waiting on each; a single |
| file redirects to that document, matching `POST /documents` |
| (`handlers.go:255`). Mixed success and failure in the no-JS path lands on |
| -the list, where each result is visible. |
| +the list. A file that *fails* produces no document and therefore no report |
| +row, so those results cannot live where the reports do: the redirect carries |
| +a count, and the list renders one notice naming how many files did not |
| +import and why. Otherwise the only invisible results would be the ones that |
| +matter. |
| **Ownership is not novel here** — the owner comes from `h.owner(r)` like |
| every other handler, `Store.Create` takes it, and nothing in the multipart |
| @@ -503,8 +608,34 @@ that actually bind are three, and the budget is derived from them: |
| (`core.go:561`) and the row. |
| So: **the update is capped at 2 MB**, leaving headroom under 8 MB for a |
| -document's subsequent editing life, and the file caps are derived from that |
| -rather than chosen. A build whose snapshot exceeds the cap is refused at |
| +document's subsequent editing life. Seed data puts that in proportion: an |
| +`.update.bin` runs 1.05–1.18× its source text, so 2 MB of update is on the |
| +order of 1.7 MB of plain text — or perhaps 1 MB once heavily marked. Several |
| +hundred pages either way. |
| + |
| +The file caps protect that budget and are deliberately not the same number, |
| +because a 20 MB `.docx` can hold 100 KB of text with images making up the |
| +rest: |
| + |
| +| Cap | Value | |
| +|---|---| |
| +| Whole request body (`MaxBytesReader`) | 25 MB | |
| +| One file (per-part `LimitReader`) | 20 MB | |
| +| Uncompressed bytes read from one zip | 60 MB | |
| +| Zip entries | 512 | |
| +| The body part (`document.xml`) | 20 MB | |
| +| Tree nodes / tree depth | 200,000 / 32 | |
| +| Resulting update | 2 MB | |
| + |
| +A build whose snapshot exceeds 2 MB is refused at step 4 of §5 — before any |
| +row exists. |
| + |
| +**One thing here is still unmeasured**, and is a gate rather than a guess: |
| +that building, snapshotting and re-applying a 2 MB tree each fit inside the |
| +1-second guest-call cap. `find_position` walks from the start of an |
| +`XmlText`, so a block with *k* runs costs O(k²) — blocks are small, so this |
| +is probably fine, and "probably" is what a gate is for. Plan 1 measures it |
| +and asserts a ceiling (≤ 250 ms) on the CI runner. A build whose snapshot exceeds the cap is refused at |
| step 4 of §5 — before any row exists. |
| - **Stream the parts** via `r.MultipartReader()` with a per-part |
| @@ -533,8 +664,12 @@ In order, first that yields something: the document's own title metadata |
| (`docProps/core.xml`'s `dc:title`, PDF's `/Title`), then the first heading in |
| the content, then the filename with its extension stripped. Never empty. |
| -**A leading H1 is consumed into the title only when it is the first block |
| -and no metadata title exists.** This is what makes Markdown's |
| +**For Markdown only, a leading H1 is consumed into the title when it is the |
| +first block and no metadata title exists.** For DOCX it is `dc:title`, then |
| +the filename — never the first heading, and body content is never removed: |
| +Word files routinely carry an empty or stale `dc:title` ("Document1") above a |
| +genuine `Heading1`, and consuming it would delete a real section heading from |
| +the document. This is what makes Markdown's |
| export→import→export round trip identity: `internal/export/markdown.go:152` |
| writes the title as a leading `# H1`, so keeping it doubles the H1 on |
| re-export, and consuming it unconditionally loses a genuine content heading. |