oficina / docs Public

<task-notification> <task-id>a0ffd8eacbfc26de9</task-id> <tool-use-id>toolu_01TUbcyBBdGnNqfodBgiCMJS</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-docs/f51dc426-0d05-413b-a979-c81d95ac5994/tasks/a0ffd8eacbfc26de9.output</output-file> <status>completed</status> <summary>Agent "Fable spec review" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>**VERDICT: not ready.** The core mechanism (Go parses → one new Rust builder → existing snapshot pair → `Store.ApplyUpdate`) is sound and most of the spec's checkable claims hold. But one claim at the centre is wrong in a way that would make roughly 999 of every 1000 imported documents unopenable in the editor, two of the four "spike-proven" findings are misdescribed, and the round-trip gate as written does not prove what the spec says it proves. Smallest set of changes to make it ready: 1. §5 step 2: do **not** build under the row's `ClientID`. Draw a fresh id in `[0, 2^32)` for the build, state why (lib0 decodes client ids as JS numbers and throws past 2^53), and make the §12 cross-implementation fixture use the production id path. 2. Correct F4 (the editor writes `level` as a **number**; import must too) and F3's explanation (yrs already unsets omitted marks on `insert_with_attributes`; the inheritance is on plain `insert`). 3. Define the round-trip over a **canonical** tree, compared structurally, and add ProseMirror content-model validation to the builder — otherwise the gate passes trees the editor cannot open. 4. Replace "25 MB" with budgets derived from the three real limits (8 MB `maxLogBytes`, 1 s guest call, 512 MB box), plus a concurrency cap and no temp files. 5. Specify the report's storage, lifetime, dismissal and content rules. 6. Reorder §5 so no row exists until there is a verified update to write; specify no-JS multi-file and how fetch vs form is distinguished. 7. State the ABI-bump consequence (every existing snapshot discarded) or decouple the snapshot version from the ABI. 8. Split into three plans (steps 1–4; DOCX; PDF). Also a process note: `core/src/fragment.rs` and `core/tests/fragment.rs` are already modified and uncommitted in this worktree (the `fragment_text` fix from §4). Implementation has started ahead of the review AGENTS.md requires. --- ## Defects **1. holes-the-centre — Building under the row's `ClientID` produces updates the browser cannot decode.** Spec §5 steps 1–2: "a row, with its app-assigned GUID and client id … open a scratch core document on that same GUID and client id." Evidence: `internal/docs/store.go:267` `clientID := uint64(mrand.Int64())` — uniform in `[0, 2^63)`. yrs encodes it as a u64 varint (`yrs-0.23.5/src/block.rs:76`, `src/updates/encoder.rs:141-143`). The browser decodes it with `lib0@0.2.117/decoding.js:245-264` `readVarUint`, which accumulates into a JS double and throws `Integer out of Range` once the partial value exceeds `Number.MAX_SAFE_INTEGER`; `yjs@13.6.27/src/utils/UpdateDecoder.js:53-55` `readClient()` goes through it, and the editor's load path is exactly that decoder (`fixtures/editor-entry.js:120-126`, `:388`). Yjs's own ids are `random.uint32` (`yjs/src/utils/Doc.js:23`). I reproduced it: a doc with `clientID = 2**62 + 2**55` encoded by yjs and re-applied throws "Integer out of Range". For a random 63-bit id, roughly 7/8 throw outright; most of the rest decode to a *rounded* id (precision loss), so a later browser delete-set referencing the importer's blocks would name the wrong client. Only ids `&lt; 2^53` (probability ~2^-10) work. Why nothing has caught this: the server has never created a block, so its id has never been on the wire; every wire fixture uses `clientID = 1/2/3` (`fixtures/generate.mjs:17`, `generate-fragment.mjs:13`, `generate-seed.mjs:122`). `generate-seed.mjs:117-121` even records the wrong belief that real edits are "attributed to" the row's ClientID — they are attributed to the browser's `new Y.Doc()` id. Correction: the builder takes a client id drawn fresh per import in Go, in `[0, 2^32)` (mirror Yjs), and the spec says the row's `ClientID` is never used to author blocks. Separately flag `Store.Create`'s 63-bit range as a latent trap for any future server-side write. The §12 cross-implementation gate must be built with the same id-generation code the handler uses, not a literal `1`. **2. material — F4 is wrong about what the editor writes; import "writes the string" would make imported headings the odd ones out.** Spec F4: "every document the editor has ever written stores `level: "2"`, because that is what y-prosemirror produces." Evidence: y-prosemirror passes the ProseMirror attr straight through (`y-prosemirror@1.3.7/src/plugins/sync-plugin.js:874-881` `type.setAttribute(key, val)`), and this app's toolbar supplies a number (`fixtures/toolbar.mjs:167` `Number(value.split('-')[1])`). Probe: a heading built that way round-trips through yjs with `typeof level === 'number'`. What *is* a string is the repo's own fixture: `fixtures/generate-fragment.mjs:50` `el('heading', { level: '2' }, …)`, so `nested.bin` decodes to `"2"`. The Rust reader hides the difference — `xml.rs:1026-1030` `get_attribute` returns `value.to_string()` and `any.rs:224` prints `2.0` as `2` — which is presumably how the spike concluded "string". The `node.check()` remark is therefore inverted: today's editor-written headings pass; imported string-level headings would be the ones `validate: "number"` rejects. Correction: the builder writes `level` as a numeric `Any`; F4 and its footnote are rewritten; fix `generate-fragment.mjs:50` to a number so the fixture represents the editor. **3. minor — F3's mechanism is misdescribed (the prescription is harmless but the reasoning is wrong).** Spec F3: "each run is inserted with the full mark set and `Any::Null` for every mark that should not apply." Evidence: `yrs-0.23.5/src/types/text.rs:707` `pos.unset_missing(&amp;mut attributes)` → `block.rs:952-962` inserts `Any::Null` for every currently-active attribute the caller omitted — the same step as Yjs's `insertText` (`YText.js:255-257`). The inheritance F3 observed comes from plain `insert` (`text.rs:212-227`, no attribute handling) or `XmlTextPrelim::new`. Correction: "unmarked runs are inserted with `insert_with_attributes(…, Attrs::new())`, never `insert`; yrs unsets the rest." Explicit Nulls stay legal, but the spec should not teach a false model of Y.Text. **4. material — The round-trip gate does not hold as stated, and does not prove editor-openability.** Spec §3/§12: "for any tree the importers can produce, `fragment_json(build(tree))` must equal `tree`." Asymmetries in `core/src/fragment.rs`: (a) adjacent runs with identical marks are merged by `diff` (`text.rs` `pack_str`, ~:685) — a DOCX importer will emit adjacent identical `w:r` runs constantly; (b) empty text runs are dropped (`fragment.rs:302-304`); (c) marks are re-ordered into `MARKS` order and deduplicated (`:277-291`); (d) `attrs` is emitted only for `heading` (`:343-355`) and `content:[]` always; (e) the output is a hand-written string — a Go `encoding/json` encoding of the same tree differs (escapes `&lt;&gt;&amp;`, key order), so "equal" must mean structural. And the property is silent on *structure*: `fragment_json` happily emits `text` directly under `doc`, a `list_item` outside a list, or a heading inside a paragraph, and the builder as specified would build them — the F2 gate is per-fixture, so an importer bug producing a schema-invalid tree passes the round-trip and breaks the editor (F2's exact fear). Correction: define a canonical tree (adjacent equal-mark runs merged, no empty text, marks in `MARKS` order, no duplicates, `attrs` only on `heading`), require importers to emit it, compare via `serde_json::Value`, and have `build_fragment` enforce the content model: `doc → block+`, `paragraph|heading → text*`, `bullet_list|ordered_list → list_item+`, `list_item → paragraph block*` (`fixtures/editor-entry.js:38`), `blockquote → block+`; anything else is a refusal. **5. material — The Go side of the interchange is unspecified and, if `export.Node` is reused, wrong.** Spec §2/§6: `Import(r, size) (Tree, Report, error)` — `Tree` undefined. `internal/export/document.go:25-36` `Node.Attrs` is a value struct, so encoding it emits `"attrs":{"level":0}` on every paragraph, plus `"text":""`, `"marks":null`, `"content":null` — none of which `fragment_json` produces and all of which a parser that "refuses everything else" (§5) would reject. Correction: define the Go tree type and its exact JSON (or state the builder is fed canonical JSON per defect 4), with `omitempty`/pointers pinned by a golden test against `fragment_json`. **6. material — Mark and attribute *values* on the wire are unpinned.** §3 lists mark names only. y-prosemirror writes each mark as `{ name: mark.attrs }` = `{}` for attribute-free marks (`sync-plugin.js:856-866`, `attributesToMarks` at `:837`); the fixtures agree (`generate-fragment.mjs:43,70` `{ strong: {} }`). A builder writing `Any::Bool(true)` renders fine but differs from what the browser writes, so y-prosemirror's attribute comparison rewrites the run on first edit. Correction: pin `Any::Map(empty)` for marks and numeric for `level` (defect 2). **7. material — Size limits are disconnected from the limits that actually bind.** Spec §10: "25 MB per file … capped in node count and depth." Evidence: the imported update is served to every browser load by `updatesGet`, which 500s above `maxLogBytes = 8 &lt;&lt; 20` (`internal/docs/handlers.go:393, :509-513`) — an import bigger than that yields a document that renders server-side and can never be edited. Every guest call is capped at 1 s with no override wired in this app (`internal/core/core.go:100`; no `WithGuestCallTimeout` caller under `internal/docs` or `cmd`), and a timeout is treated as a trap — so `docs_core_build`, the snapshot encode, and `ApplyUpdate`'s re-apply each have 1 s under wazero. Memory: main spec §5.2 sizes the box at 512 MB; one 25 MB file exists simultaneously as the body, the parsed XML, the JSON string, its copy in wasm linear memory, the yrs doc, the snapshot, `Handle.applied`'s copy (`core.go:561-563`) and the row. Correction: cap the *update* (e.g. ≤ 2 MB, leaving headroom under 8 MB), derive file caps from that, put a semaphore on concurrent imports, stream parts via `r.MultipartReader()` with a per-part `LimitReader` and a total-body `MaxBytesReader` (25 MB "per file" via `MaxBytesReader` is a total cap, not per-file), never `ParseMultipartForm` (spills plaintext to temp files). Also enforce zip sizes on the actual read, not only the declared `UncompressedSize64`. **8. material — The report has no storage, lifetime or content rules.** §8 "shown after import, on the document, and dismissible"; §9 "the page inserts the new rows". A 303 to `/d/{sqid}` (`handlers.go:255` precedent) carries nothing, so the report must live somewhere between POST and GET: a table keyed by doc (with a `POST` dismiss route), a flash cookie (4 KB — too small for a real DOCX report), or a query string. Unspecified, along with what the fetch path shows and whether catalog rules apply. Content rule: `Where`/`Detail` must not quote document text or link targets (main spec §2.1 items 4–5 — titles are plaintext by ruling; reports are not), and `Kind`/`Detail` as free strings violate "every user-visible string is a catalog key" (AGENTS.md). Correction: a `doc_import_reports` row (structured: kind enum → catalog key, locator, bounded non-content detail), rendered on `show.html`, dismissed by POST; the JSON response returns the same structure. **9. material — Step order leaves an empty document behind on failure, contradicting §7.** §5 creates the row first; §7: a text-free PDF "creates no document." `build`, `Snapshot` and `ApplyUpdate` (`store.go:709-855`) can all fail after `Store.Create` (`:262-295`); `seed.go:85-99` has the same shape but tolerates it. Correction: parse → build on the scratch doc → snapshot → verify → *then* `Create` and `ApplyUpdate`, deleting the row if `ApplyUpdate` fails. (Only possible once the build id is decoupled from the row — defect 1.) **10. material — The ABI bump discards every existing snapshot; the spec does not say so.** §5 "ABI 4 → 5". `internal/docs/snapshots.go:161-166` filters `abi_version = coreABIVersion`; after the bump every document replays its full log until its next write, and one past 4096 rows re-seeds on every read with a Warn per read (`store.go:470-471`). The state encoding does not change. Correction: state and accept the cost, or split a `snapshotEncodingVersion` from the ABI number. **11. material — PDF's in-process posture is not the containment the spec claims.** §7 "Extraction is sandboxed the same as everything else (§9)" — §9 is the HTTP section; §10 is meant, and §10's containment (`internal/core` traps) covers only wasm. A Go PDF library runs native: `recover()` catches panics, not stack exhaustion or OOM from a recursive object graph, and main spec §5.2 called the wasm sandbox "load-bearing for security" for exactly this class of input. Correction: decide the posture explicitly (accept native with depth/size pre-checks and a fuzz corpus; or subprocess; or a wasm-compiled extractor), and fix the cross-reference. **12. material — Markdown export→import→export cannot be identity as specified.** `internal/export/markdown.go:152-158` writes the title as a leading `# H1`; §11 takes "the first heading in the imported content" as the title without saying whether it is consumed. Kept → re-export doubles the H1; consumed → a genuine content H1 is lost. Correction: consume a leading H1 into the title *only* when it is the first block and the file supplies no metadata title; state it. **13. minor — "The vocabulary is what the editor's schema already speaks, no more" is inaccurate, and the gap matters for DOCX.** `fixtures/editor-entry.js:37-39` builds from `prosemirror-schema-basic` + `addListNodes`, so the editor also speaks `hard_break`, `code_block`, `horizontal_rule`, `image`, `link`. The importer's set is a strict subset of the editor's, not equal. `&lt;w:br/&gt;` (a line break inside a paragraph — ubiquitous in Word and Google Docs) has no target; specify: split the paragraph, or collapse to a space, and warn. **14. minor — Ambiguities that can be built two ways.** (a) How the server tells the fetch path from the form (Accept header? a field?) and what a no-JS post of *multiple* files (`&lt;input multiple&gt;`) redirects to, and mixed results in no-JS. (b) `docs_core_build` on a non-empty root: refuse or append. (c) `.txt` encoding: UTF-8 only? BOM? Windows-1252? Invalid bytes replaced or refused (Go's importers can and should validate UTF-8 before it reaches the hand-written Rust parser). (d) DOCX `Title` style → title metadata, H1, or dropped; whether DOCX fonts/sizes ever map to `font_*`/`size_*` (§3 says the marks are in scope; §6 mentions only `code`). (e) Which fonts count as "monospace by name". (f) `word/document.xml` located by fixed path vs `_rels/.rels` officeDocument relationship — the §12 malformed-input gate assumes the fixed path. **15. minor — Cross-references and small factual slips.** §1 "§2 of the main spec rules that out for conversion APIs" — it is main §11.2 (`2026-08-30-docs-design.md:919-921`) that rejects them, citing §2. §7 "(§9)" should be §10. §14 step 2 says "four-step sequence in §5"; §5 lists five. **16. minor — UI constraints not acknowledged.** `list.html:150-158` documents the header already at its 320 px reflow limit with five controls; a file input plus button is two more. New strings (labels, the scan-PDF error, warning kinds) are catalog keys and go through copy review; the drop-zone script is a new module under main §12's 300-line cap, served the way `jumpToScriptPath()` is. **C. Scope.** Not one plan. Steps 1–4 (core, `Store.Import`, text+Markdown, HTTP+form+drop+report) are a coherent, shippable plan. DOCX (step 5: `numbering.xml`, style resolution, three real-world fixture families) is a second. PDF (step 6: new third-party dependency, licence, undecided containment posture) is a third and is correctly last. The §14 order is right; the split is what is missing. --- **Checked and holds** (so the caller knows what was verified rather than assumed): `snapshot_bytes` is `encode_state_as_update_v1(&amp;StateVector::default())` (`core/src/snapshot.rs:169-172`) — whole state including pending queue, no state vector prefix, right call for this use; ABI is 4 on both sides (`core/src/lib.rs:101`, `internal/core/core.go:79`); the node/mark vocabulary matches `MARKS` (`fragment.rs:62-71`) and `node_html`/`node_json` exactly; `Store.Text` has no non-test caller (only `store.go:568` inside itself); no seed carries a mark (`generate-seed.mjs:93-104`, plain `insert`); `fragment_text`'s markup leak is real (`yrs xml.rs:548-567` writes `&lt;key&gt;` tags); `Store.Create`/`ApplyUpdate` have the assumed shape and the snapshot cadence (`store.go:744`) has no bad interaction with a whole-state first update; CSRF is fine — `csrf.Protect` passes `Sec-Fetch-Site: same-origin` first (`rastrillo@v0.22.0/csrf/csrf.go:26-27`), which a same-origin `fetch` multipart POST sends; the "same GUID" is harmless (the GUID is not in the update); Go's `encoding/xml` claim is right.</result> <usage><subagent_tokens>241901</subagent_tokens><tool_uses>50</tool_uses><duration_ms>820656</duration_ms></usage> </task-notification>