Add the document change digest compute service and its REST route #200

Activity

1. HMarzban

   added

   enhancementNew feature or requestNew feature or request

   Feature

   tiptap

   on Aug 31, 2026
2. HMarzban commented on Sep 1, 2026

   

   HMarzban

   on Sep 1, 2026

   Last edited by HMarzban

   CollaboratorAuthor

   More actions

   Architecture review findings — 2026-09-01

   A two-team architecture review of the Part D and Part E design filed 5 finding(s) on this issue. Each one survived adversarial verification; 19 of 30 ranked findings were refuted and are not listed. Full report and the refuted list: REPORT-partDE-architecture-review.md.

   Apply these before writing the code they touch.

   ALG-1 — The positional title rule makes one deleted heading read as two contradictory rows

   Severity: high. Line 394 says "title pairs positionally regardless of toc-id". Line 393 says the title is the first heading. So the baseline's first heading is always paired with the head's first heading, whatever their toc-ids say. Delete the title and every following section shifts up by one. The API then reports the second heading as both modified and removed, and never mentions the heading that actually went. Insert a heading above the title and the same duplicate appears. This is not a large-document problem. It is wrong at three sections, and Part C's PATCH replace can produce exactly this head document. The toc-ids needed to get it right are present and are simply overruled.

   Fix. Drop the positional title exemption from pairSections. The webapp's exemption exists for its search UI, not for a diff, and toc-id pairing already covers the title. Keep the positional fallback only for the preamble, which has no toc-id and no name. Add both cases above to the D5 unit list at line 410: title deleted, and a heading inserted above the title. If the maintainer wants the exemption kept, gate it on both first headings carrying the same toc-id, so it can never overrule stable identity.

   D3-1 — changed is byte-derived and never recomputed, so the email prints "0 sections changed"

   Severity: high. Part D sets changed from the byte compare. Nothing re-derives it from the section statuses. The canonicalizer then strips toc-id, so a window that only carries the webapp's first-open stamping pass yields bytes that differ and a diff where every section is unchanged. The response is changed: true with a zeroed summary. D-4(b) attaches the block on changed === true, and Part E's zero-content skip only fires when the block is absent. So the reader gets an email whose whole body reads "0 sections changed since your last visit · +0 / −0 words". This is the exact path the canonicalizer was built for, so it is common, not exotic. The already-found self-healing guard makes it worse: a mark-only edit reclassifies to unchanged and lands in the same empty email.

   Fix. CUT output. Derive changed from the section statuses after the diff, not from the byte compare, and drop the content_changes block when no section survives as non-unchanged. One boolean, no new machinery. Keep the byte compare as the zero-decode fast path only.

   API-1 — The route ships no OpenAPI paths file, so it is invisible in /docs and nothing catches it

   Severity: medium. Every other REST module owns a file under src/modules/openapi/domain/paths/ and is spread into buildOpenApiDocument. Part D's docs plan (D-6) names API.md, AGENTS.md, ENV.md, Readme.md, documents.http, email-templates and one comment fix. It never names the openapi module. D-5 step D5 lists only schema/controller/router plus module.ts and index.ts. No test asserts that a mounted route appears in the spec, so the omission would ship silently. The same gap drops the 429 the shipped paths files all publish, even though D-1 states the global limiter applies.

   Fix. Add three edits to D-5 and D-6. (1) New file src/modules/openapi/domain/paths/documentChanges.ts exporting documentChangesPaths: OpenApiPaths, built from the live zod schemas with toParameters(documentIdParamSchema, 'path', {...}) and toParameters(changesQuerySchema, 'query', {...}), a 200 from dataEnvelope, and the shared error refs ValidationError / Unauthorized / NotFound / InternalError plus rateLimitedRef. Omit 503 and 413 and say so in the description, the way the diff route does. (2) Import and spread documentChangesPaths in document.ts. (3) Add a Document changes entry to the TAGS array in document.ts.

   ALG-3 — The (level, text) fallback misaligns by one, turning one insertion into three modified sections

   Severity: low. Line 394's fallback pairs leftovers by (level, normalizedHeadingText) in document order, exact match only. When several headings share a level and a name and carry no toc-id, that rule pairs them positionally by accident. Insert a new one at the front and every later section pairs with its predecessor. The digest then says three sections were modified and one was added, when one was added and nothing was modified. Line 394 documents a related caveat, that a renamed null-toc-id heading reads removed plus added, but it does not document this one, which is worse: it reports edits to sections nobody touched.

   Fix. Run the null-toc-id leftovers through matchBlocks instead of the name equality rule, keying each leftover on its canonicalized section JSON rather than its heading text. LCS anchors on the unchanged bodies, so a front insertion comes out as one added and three unchanged. It also emits the merged order that ALG-5's splice rule is trying to rebuild. Note the honest limit: LCS cannot follow a moved section, so keep toc-id pairing first and use LCS only on what is left.

   D5-1 — The plain-text digest part cannot escape anything, and Part D feeds it stranger-written document text

   Severity: low. Part D puts raw document body text into a digest email that real inboxes trust. The HTML part is safe, because Eta auto-escapes. The plain-text MIME part is not. It is built by raw string concatenation, and text/plain has no escaping. A public, non-read-only document accepts anonymous writable WebSocket connections, so any stranger with the link can type the payload. A 140-character excerpt that contains newlines forges a second document entry, plus a link line, inside a DKIM-signed email. The section heading text and the breadcrumb carry no length cap at all, so the forgery surface is larger than the excerpt. This is the first path in the system that carries unauthenticated stranger text into an outbound email; chat previews at least require a signed-in writer.

   Fix. Sanitise at the source, not in the renderer. In diffSections, collapse every whitespace run (newlines included) to one space and strip control characters before writing excerpt and the section text, and cap text the same way the excerpt is capped. One helper, called in one place, protects both MIME parts. Delete the sentence "escaping is the renderer's job" from D-3: a plain-text renderer has no escaping to offer.

   Acceptance

   • ALG-1 applied — The positional title rule makes one deleted heading read as two contradictory rows
   • D3-1 applied — changed is byte-derived and never recomputed, so the email prints "0 sections changed"
   • API-1 applied — The route ships no OpenAPI paths file, so it is invisible in /docs and nothing catches it
   • ALG-3 applied — The (level, text) fallback misaligns by one, turning one insertion into three modified sections
   • D5-1 applied — The plain-text digest part cannot escape anything, and Part D feeds it stranger-written document text
3. HMarzban commented on Sep 2, 2026

   

   HMarzban

   on Sep 2, 2026

   CollaboratorAuthor

   More actions

   Built, reviewed and verified against the real corpus

   Landed on main in 21dd38844, which carries Closes #200. The issue will close when that commit is pushed.

   The five architecture findings

   Each was applied before the code it touches was written.

   • ALG-1 — the positional title rule is gone. Only the preamble still pairs by position, because it carries no toc-id and no name.
   • D3-1 — changed is derived from the section statuses. It is now derived inside one respond helper from the summary counts the response itself reports, so changed: true with a zeroed summary is unrepresentable rather than merely avoided.
   • API-1 — the route is in the OpenAPI document, and a new test refuses any mounted module route that is not published.
   • ALG-3 — leftovers pair through LCS, not a name match.
   • D5-1 — text is sanitised at the source. tocId was the one string that bypassed it, caught in review and fixed.

   Acceptance

   All ten boxes pass. bun run test in apps/hocuspocus.server is green at 655, and neither package.json nor backend-ci.yml was edited — a bare bun test finds the new __tests__ directories.

   Verified against the real database, over HTTP

   The service ran against the local corpus of 93 version rows across 29 documents.

   A window over the demo document resolved version 27 to version 29 and reported one section modified, +24 / -0 words, across 2 versions. An independent path decoded both snapshots and word-counted the whole document as plain text: 4,288 against 4,312. The difference is 24, which the route produced through a completely different mechanism.

   401 with no bearer, 400 on a malformed id, 404 on an unknown document, 400 on a reversed window, and both zero-decode fast paths were each exercised live.

   Two limits recorded rather than fixed

   A pure section reorder reports changed: false. Sections that only moved are each unchanged. The obvious fix re-creates this issue's own D3-1 symptom: changed: true beside a zeroed summary produces an email whose body reads "0 sections changed". A correct fix needs a moved status the summary can count, which is new machinery outside this issue.

   A caller can defeat section pairing by rotating every toc-id. encodeContent keeps a caller-supplied toc-id verbatim, so patching content with one set of ids and then patching back leaves nothing to match on. The result is coarser — removed plus added where modified is ideal — never false, and no edit is attributed to an untouched section.

   What is next

   #202 closes this out with the end-to-end script, the CI step and the docs. #203 is the SQL carrier that #201 depends on.
4. HMarzban

   closed this as completedin 21dd388on Sep 2, 2026
5. HMarzban commented on Sep 2, 2026

   

   HMarzban

   on Sep 2, 2026

   CollaboratorAuthor

   More actions

   Both acceptance lists are ticked, each re-verified against HEAD rather than against the earlier run.

   • The three formatting fixtures and the toc-id fixture: 9 tests pass in document-changes/__tests__/unit/diffSections.test.ts.
   • The six route boxes: 14 tests pass in document-changes/__tests__/integration/.
   • The whole backend suite: 655 pass, 0 fail, across 51 files.

   One box needs a footnote rather than a silent tick. It reads "with no edit to package.json or backend-ci.yml". backend-ci.yml is untouched and "test": "bun test" is unchanged, so no test path list was added anywhere — which is what the box exists to protect.

   package.json did gain one line, and it is unrelated to test wiring. A later change in the same session moved the media storage pick from a call-time process.env read to validated config. That config freezes at first import, so scripts/e2e-duplicate-media.ts could no longer force local storage from its own module scope, and its guard against deleting from the live bucket had gone quiet. The flag now rides on the package script, where it lands before the process starts.