Cloonar/deckle

Fork 0

items owners: one canonical row layout owned by a single split/compose, so load, save and translation hash cannot drift #232

New issue

Closed

opened 2026-10-12 18:05:19 +02:00 by dominik.polakovics · 0 comments

dominik.polakovics commented

2026-10-12 18:05:19 +02:00

Owner

This was generated by AI during triage.

Summary

An items owner's children (its item rows plus its body region) get one canonical layout, body first and then one row per line, owned by a single split/compose pair that every reader and writer of those children goes through. Today every editor save of a hand-authored text-media/figures element rewrites it into glued rows on one line with the body moved behind them, the layout the sidecar comments and AGENTS.md teach is one the engine never writes, and the "merge the text nodes a removal exposes" rule exists four times.

Current behavior

Confirmed by reproduction on main (ea11f78), a figures element hand-authored in the documented shape:

{{< figures uid="FFFFFFFFFFFF" >}}
Body.
{{< img alt="A" src="a.jpg" uid="AAAAAAAAAAAA" />}}
{{< img alt="B" src="b.jpg" uid="BBBBBBBBBBBB" />}}
{{< /figures >}}

GET /api/element/body serves "Body.\n\n": the two separator newlines leak into the body the editor sees.
The form always sends the items collection when the element owns one, even for an Appearance-only edit. The save (elemops.ApplyEdit) composes rows ++ rest with no separator, so the first save of any hand-authored items owner rewrites the file to
```
{{< figures uid="FFFFFFFFFFFF" >}}
{{< img alt="A" src="a.jpg" uid="AAAAAAAAAAAA" />}}{{< img alt="B" src="b.jpg" uid="BBBBBBBBBBBB" />}}Body.
{{< /figures >}}
```
Rows glued, body moved behind the rows, a structure-mangling diff for a change that touched neither. Adding, removing or reordering an image afterwards is a diff on one long line. Rendering is unaffected (img pushes onto the parent's Scratch; the body comes from Inner).
The translation hash (l10n.Hash, via bodyContent in internal/l10n/hash.go) hashes the separator whitespace with the body: the served "Body.\n\n" and the RTE's normalized "Body." hash differently, so a touch-and-undo body edit flags every connected translation as "source changed". (An untouched body is not re-sent by the form, so a save that never opened the body keeps the hash; the leak needs an actual body edit.)
All three layouts, body-first pretty-printed, rows-first pretty-printed and glued, round-trip byte-identical through content.Parse/content.Serialize, so harness check calls every one canonical: "canonical" today means "round-trips", not "what the editor writes". AGENTS.md "Editing content by hand" promises that saving an element without changing anything leaves its file byte-identical; for items owners that is false.
The item/body split has one owner (elemops.PartitionItems), but compose is a hand-written switch in ApplyEdit that is not its inverse, and "is this child an item of its parent" is evaluated again in l10n.isItem and l10n.holdsElement (uid-gated in bodyContent/elementChildren).
"Merge the adjacent text nodes a removal exposes" is implemented four times: content.removeAt (internal/content/mutate.go), elemops.coalesceText (internal/elemops/save.go), l10n.bodyContent (internal/l10n/hash.go), l10n.spliceNodes (internal/l10n/l10n.go). CLAUDE.md names internal/content as the one owner of that rule.

Desired behavior

One grammar. The canonical layout of an items owner's children is: the body region, then a newline, then the item rows, one per line, in order. An owner with no body (body: none, or an empty rte body) holds the rows alone, one per line; an owner with no rows holds the body region alone. The documented shape above is therefore already canonical: the editor's save of it is byte-identical.

One owner. A split/compose pair in elemops replaces the raw PartitionItems, and they are exact inverses on canonical input:

split takes the children and the owner's schema and returns the rows (in order) and the body region, with the layout's separator newlines removed. The body region is the leftover text and inline nodes with the newline runs at its two edges stripped (they are the layout's separators, non-semantic in markdown exactly as the parser's block-body strip rules already treat the newline after a block tag); interior bytes are byte-exact. Split accepts any layout the parser accepts (glued rows, rows before the body, text between rows, trailing text): rows come out in order, the rest is merged into a serializable body region.
compose takes rows and a body region and writes the canonical layout.
Every reader and writer of an items owner's children goes through the pair and nobody slices or concatenates children by hand: GET /api/element/body (internal/server/body.go), the save composition and WouldClobberBody (internal/elemops/save.go), the form's row list (internal/server/forms.go), the translated-items refusal (internal/server/translation.go), the hash's body content (internal/l10n/hash.go), and the translation structure repair that adds, removes or reorders rows in a connected translation (internal/l10n/structure.go: spliceNodes/insertNode/placeIndex on an items owner's rows). A row inserted into a translation by repair lands in canonical layout too.
"Is this child an item of its parent" is one function; l10n.isItem and the items branch of l10n.holdsElement call it.

One text merge. internal/content exports one helper that merges adjacent text nodes (fresh copies, inputs never mutated); removeAt, the split, bodyContent and spliceNodes all use it. internal/removecmd/strip.go is intentionally different (it repairs whitespace seams and re-verifies the round trip) and stays as it is.

Hash. The owner's translation hash is computed over the split's body region, so the layout's separators never enter it: a body edit that ends in the same text leaves every connected translation untouched.

Migration. A file the editor already wrote in the glued form is reformatted into the canonical layout once, on its next save of that element (one row per line, body first). harness check stays silent about a non-canonical row layout. The pull request says so in its description, as a release note.

Docs and skills (CLAUDE.md rules): the layout is documented once and the skills point at it.

Decisions

Body first, then one row per line, no blank line between them — the maintainer's call: it minimises git noise (adding, removing or reordering an image is a one-line diff; a body edit never touches the row lines), and it is the shape the text-media.yaml/image.yaml comments, AGENTS.md and hand authors already use. The current "rows lead" order in ApplyEdit was never documented and is dropped.
The body region's edge newline runs are the layout's separators and are stripped by split, not kept byte-exact — a trailing newline count carries no meaning in markdown (the engine already normalises the document-final run the same way), and keeping it is what leaks hand-authored whitespace into the GET body and the hash.
Split is lenient, compose is strict — any parseable layout must still open in the editor (#230), and one save must reach the fixed point.
content.Serialize keeps refusing adjacent text nodes — the invariant is what the tests and the merge helpers rely on, and relaxing it would not fix the layout anyway. Rejected deliberately (second time, see the original report).
harness check does not warn on a non-canonical row layout — every layout round-trips, and a warning would fire on every existing editor-written site until each element is resaved.
The split/compose pair lives in elemops — l10n and server already import it, and it is where PartitionItems lives today; the text merge lives in content, the owner CLAUDE.md already names.
One issue, not two — the text-merge consolidation is small and the split is its main caller; landing them together means one reformat, one release note.

Key interfaces

elemops.PartitionItems(children, itemElement) — becomes the split half of an exact split/compose pair taking the owner's schema; the compose half is new. Starting hint: internal/elemops/save.go.
elemops.ApplyEdit — the children-composition switch becomes "split, replace the axes the request sent, compose". The absent-versus-present semantics of items and body (table in its comment) are unchanged.
content — one exported adjacent-text merge helper; removeAt uses it. Starting hint: internal/content/mutate.go.
l10n.bodyContent, l10n.spliceNodes, l10n.insertNode, l10n.isItem, l10n.holdsElement — call the owners instead of re-deriving. Starting hints: internal/l10n/hash.go, internal/l10n/l10n.go, internal/l10n/structure.go.
handleElementBody — serves the split's body region. Starting hint: internal/server/body.go.
scaffold/site/AGENTS.md "Editing content by hand" (the "Bodied elements use paired tags … Items rows follow the same rule" passage and the byte-identical-save promise) and the "Item collections" paragraph; scaffold/editor/elements/text-media.yaml and image.yaml comments (the stored shape); scaffold/site/.claude/skills/edit-content/SKILL.md (the "Items children" bullet: where rows go and one per line, pointing at AGENTS.md), migrate-site/SKILL.md (what harness check reports and what the editor canonicalises on first save); redo-theme/references/showcase-page.md already places rows one per line — confirm it parses as canonical layout; the package comments in internal/elemops/save.go and internal/server/body.go that currently describe rows leading.

Acceptance criteria

Saving the documented hand-authored shape (body, then one img row per line) with its rows and body unchanged leaves the file byte-identical.
GET /api/element/body on that element serves "Body." (no separator newlines), and the same after a save.
A round-trip table test over hand-authored shapes — glued rows, one row per line, body before rows, body after rows, text between rows, trailing text, rows only, body only, both empty — shows: the GET succeeds; GET then save reaches the canonical layout; the second save is byte-identical to the first; harness check accepts the result.
An editor-written glued file ({{< img … />}}{{< img … />}}Body.) is rewritten to the canonical layout on its next save of that element, and never touched by a save of another element on the page.
The translation hash of an items owner is unchanged across a save whose body region text is unchanged, whatever the layout before the save; a real body text change still changes it.
A translation repair that adds, removes or reorders a row in a connected translation's items owner leaves that owner in canonical layout.
WouldClobberBody, the form's row list, the translated-items refusal and the hash's body content behave as before on canonical input (existing tests), now via the split.
Exactly one adjacent-text merge in the engine, exported from internal/content; coalesceText, the merge in bodyContent and the merge in spliceNodes are gone; removecmd/strip.go is untouched.
Exactly one "is this child an item of its parent" predicate; l10n.isItem and l10n.holdsElement call it.
scaffold/site/AGENTS.md, the two sidecar comments and the skills named above describe the canonical layout once and point at it, and the drift tests (internal/element/agentsdoc_test.go, scaffold/skills_test.go) pass.
The existing tests that pin the glued form (/>}}{{< in internal/server/items_test.go, element_test.go, l10n/*_test.go and others, 27 occurrences) are updated to the canonical layout, not deleted.
The PR description carries the release note: editor-written items owners get a one-time reformat on their next save.

How to verify

go test ./... and make check (or the repo's lint target) green.
The reproduction: write the hand-authored figures file above into an itemsSite test site, GET /api/element/body, then POST /api/page/save with the two rows (uids echoed) and no body; before the change the file comes back glued and the body served is "Body.\n\n".
cd web && npm test still green: the client's body handling does not change, only what the server serves.

Assumptions to check

The RTE's markdown serializer never emits a trailing newline run, so the split's edge-stripping makes the served body and the RTE's own output agree for an unedited body; verify with web/src/rte/markdown.ts before relying on the hash criterion.
No theme partial depends on the rows preceding the body in source order (img pushes onto the parent's Scratch and the body comes from Inner, so order should not matter); confirm in themes/deckle before changing the order.
placeIndex in internal/l10n/structure.go orders by source position among all elements of a parent; once rows are a separate list, confirm reordering a translation's rows still follows the source order.

Out of scope

Relaxing content.Serialize to merge adjacent text.
A harness check warning or a harness reformat command for non-canonical row layouts.
Changing the form's "items always sent" behaviour or the save request's absent/present semantics.
internal/removecmd/strip.go.
Any change to how rows render.

Blocked by

None - can start immediately

Original report

Follow-up to #230 / PR #231, which only made the body GET tolerate the row layout the save never produced. The underlying asymmetry is still there.

Problem

An items owner's children are read and written by code that each knows the children grammar on its own:

GET /api/element/body (internal/server/body.go) strips the rows and serves the rest.
elemops.ApplyEdit composes rows ++ body with no separator between rows, so every save writes {{< img … />}}{{< img … />}}Body. on one line and moves the body behind the rows. Rendering is unaffected (img pushes onto the parent's Scratch; the body comes from Inner), but:
- every save of a hand-authored element produces a noisy, structure-mangling diff (reporter's "minor" point);
- the inter-row whitespace of the original file leaks into the translation hash (internal/l10n/hash.go bodyContent hashes Body.\n\n, the saved form hashes Body.), so the first no-op editor save of a hand-authored items owner flags every connected translation as "source changed";
- the file shape the shipped text-media.yaml comment and AGENTS.md teach (one row per line) is not the shape the engine writes, so docs and engine disagree about what canonical looks like.
elemops.WouldClobberBody, internal/server/forms.go (row list) and internal/server/translation.go (item sync) each call the split for one half.

Proposal

One grammar. Define the canonical layout of an items owner's body once, e.g. one row per line, then one newline, then the body region. Decide and document whether the body precedes or follows the rows (the sidecar comment says body first; the engine writes rows first). Rendering does not care; hand-editing and docs do.
One owner. Replace the raw PartitionItems with a split/compose pair in elemops that are exact inverses: split returns (rows, body) with the separator newlines removed, compose writes them back. GET body, ApplyEdit, WouldClobberBody, the forms row list, the translation item sync and l10n.bodyContent all go through it. Nobody slices children by hand.
Round-trip table test over hand-authored shapes (glued rows, one row per line, body before rows, body after rows, text between rows, trailing text): GET succeeds; GET then save is a fixed point (second save byte-identical to the first); a no-op save leaves the translation hash unchanged; harness check accepts the result.
Collapse the duplicated text-merge. "Merge the adjacent text nodes a removal exposes" exists in content/mutate.go (removeAt), elemops/save.go (coalesceText), l10n/hash.go (bodyContent), l10n/l10n.go (spliceNodes). One exported helper in content, used by all four. (removecmd/strip.go is intentionally different: it repairs whitespace seams and re-verifies the round trip; leave it.)

Migration

Every element the editor already wrote in the glued form gets a one-time reformat diff on its next save. Acceptable, but say so in the release notes. Consider whether harness check should report non-canonical row layout, or stay silent since parse/serialize round-trips either way.

Surface to update (CLAUDE.md rules)

scaffold/site/AGENTS.md: "Bodied elements use paired tags … Items rows follow the same rule" passage and the content canonicalization rules; the item collections paragraph.
scaffold/editor/elements/image.yaml, text-media.yaml comments (the stored shape).
Skills: edit-content-by-hand (how to write rows by hand), migrate-site (what the engine canonicalizes), redo-theme/references/showcase-page.md if it shows rows.
internal/server package README (frozen save contract) if it spells out child composition.

Open questions for triage

Body before or after rows?
Should the GET also strip (and compose re-add) exactly one blank line between rows and body, or keep the body region byte-exact?
Alternative with a much smaller blast radius: make content.Serialize merge adjacent text instead of refusing it (the dangerous merge, one forming a shortcode opener, is still caught by checkTextRaw). It kills the class of bug but gives up an invariant tests rely on. Rejected in the first pass; worth a deliberate decision.

> *This was generated by AI during triage.* ## Summary An items owner's children (its item rows plus its body region) get one canonical layout, body first and then one row per line, owned by a single split/compose pair that every reader and writer of those children goes through. Today every editor save of a hand-authored `text-media`/`figures` element rewrites it into glued rows on one line with the body moved behind them, the layout the sidecar comments and AGENTS.md teach is one the engine never writes, and the "merge the text nodes a removal exposes" rule exists four times. ## Current behavior Confirmed by reproduction on `main` (ea11f78), a `figures` element hand-authored in the documented shape: ``` {{< figures uid="FFFFFFFFFFFF" >}} Body. {{< img alt="A" src="a.jpg" uid="AAAAAAAAAAAA" />}} {{< img alt="B" src="b.jpg" uid="BBBBBBBBBBBB" />}} {{< /figures >}} ``` - `GET /api/element/body` serves `"Body.\n\n"`: the two separator newlines leak into the body the editor sees. - The form always sends the items collection when the element owns one, even for an Appearance-only edit. The save (`elemops.ApplyEdit`) composes `rows ++ rest` with no separator, so the first save of any hand-authored items owner rewrites the file to ``` {{< figures uid="FFFFFFFFFFFF" >}} {{< img alt="A" src="a.jpg" uid="AAAAAAAAAAAA" />}}{{< img alt="B" src="b.jpg" uid="BBBBBBBBBBBB" />}}Body. {{< /figures >}} ``` Rows glued, body moved behind the rows, a structure-mangling diff for a change that touched neither. Adding, removing or reordering an image afterwards is a diff on one long line. Rendering is unaffected (`img` pushes onto the parent's Scratch; the body comes from Inner). - The translation hash (`l10n.Hash`, via `bodyContent` in `internal/l10n/hash.go`) hashes the separator whitespace with the body: the served `"Body.\n\n"` and the RTE's normalized `"Body."` hash differently, so a touch-and-undo body edit flags every connected translation as "source changed". (An untouched body is not re-sent by the form, so a save that never opened the body keeps the hash; the leak needs an actual body edit.) - All three layouts, body-first pretty-printed, rows-first pretty-printed and glued, round-trip byte-identical through `content.Parse`/`content.Serialize`, so `harness check` calls every one canonical: "canonical" today means "round-trips", not "what the editor writes". AGENTS.md "Editing content by hand" promises that saving an element without changing anything leaves its file byte-identical; for items owners that is false. - The item/body split has one owner (`elemops.PartitionItems`), but compose is a hand-written `switch` in `ApplyEdit` that is not its inverse, and "is this child an item of its parent" is evaluated again in `l10n.isItem` and `l10n.holdsElement` (uid-gated in `bodyContent`/`elementChildren`). - "Merge the adjacent text nodes a removal exposes" is implemented four times: `content.removeAt` (`internal/content/mutate.go`), `elemops.coalesceText` (`internal/elemops/save.go`), `l10n.bodyContent` (`internal/l10n/hash.go`), `l10n.spliceNodes` (`internal/l10n/l10n.go`). CLAUDE.md names `internal/content` as the one owner of that rule. ## Desired behavior **One grammar.** The canonical layout of an items owner's children is: the body region, then a newline, then the item rows, one per line, in order. An owner with no body (`body: none`, or an empty rte body) holds the rows alone, one per line; an owner with no rows holds the body region alone. The documented shape above is therefore already canonical: the editor's save of it is byte-identical. **One owner.** A split/compose pair in `elemops` replaces the raw `PartitionItems`, and they are exact inverses on canonical input: - split takes the children and the owner's schema and returns the rows (in order) and the body region, with the layout's separator newlines removed. The body region is the leftover text and inline nodes with the newline runs at its two edges stripped (they are the layout's separators, non-semantic in markdown exactly as the parser's block-body strip rules already treat the newline after a block tag); interior bytes are byte-exact. Split accepts any layout the parser accepts (glued rows, rows before the body, text between rows, trailing text): rows come out in order, the rest is merged into a serializable body region. - compose takes rows and a body region and writes the canonical layout. - Every reader and writer of an items owner's children goes through the pair and nobody slices or concatenates children by hand: `GET /api/element/body` (`internal/server/body.go`), the save composition and `WouldClobberBody` (`internal/elemops/save.go`), the form's row list (`internal/server/forms.go`), the translated-items refusal (`internal/server/translation.go`), the hash's body content (`internal/l10n/hash.go`), and the translation structure repair that adds, removes or reorders rows in a connected translation (`internal/l10n/structure.go`: `spliceNodes`/`insertNode`/`placeIndex` on an items owner's rows). A row inserted into a translation by repair lands in canonical layout too. - "Is this child an item of its parent" is one function; `l10n.isItem` and the items branch of `l10n.holdsElement` call it. **One text merge.** `internal/content` exports one helper that merges adjacent text nodes (fresh copies, inputs never mutated); `removeAt`, the split, `bodyContent` and `spliceNodes` all use it. `internal/removecmd/strip.go` is intentionally different (it repairs whitespace seams and re-verifies the round trip) and stays as it is. **Hash.** The owner's translation hash is computed over the split's body region, so the layout's separators never enter it: a body edit that ends in the same text leaves every connected translation untouched. **Migration.** A file the editor already wrote in the glued form is reformatted into the canonical layout once, on its next save of that element (one row per line, body first). `harness check` stays silent about a non-canonical row layout. The pull request says so in its description, as a release note. **Docs and skills** (CLAUDE.md rules): the layout is documented once and the skills point at it. ## Decisions - Body first, then one row per line, no blank line between them — the maintainer's call: it minimises git noise (adding, removing or reordering an image is a one-line diff; a body edit never touches the row lines), and it is the shape the `text-media.yaml`/`image.yaml` comments, AGENTS.md and hand authors already use. The current "rows lead" order in `ApplyEdit` was never documented and is dropped. - The body region's edge newline runs are the layout's separators and are stripped by split, not kept byte-exact — a trailing newline count carries no meaning in markdown (the engine already normalises the document-final run the same way), and keeping it is what leaks hand-authored whitespace into the GET body and the hash. - Split is lenient, compose is strict — any parseable layout must still open in the editor (#230), and one save must reach the fixed point. - `content.Serialize` keeps refusing adjacent text nodes — the invariant is what the tests and the merge helpers rely on, and relaxing it would not fix the layout anyway. Rejected deliberately (second time, see the original report). - `harness check` does not warn on a non-canonical row layout — every layout round-trips, and a warning would fire on every existing editor-written site until each element is resaved. - The split/compose pair lives in `elemops` — `l10n` and `server` already import it, and it is where `PartitionItems` lives today; the text merge lives in `content`, the owner CLAUDE.md already names. - One issue, not two — the text-merge consolidation is small and the split is its main caller; landing them together means one reformat, one release note. ## Key interfaces - `elemops.PartitionItems(children, itemElement)` — becomes the split half of an exact split/compose pair taking the owner's schema; the compose half is new. Starting hint: `internal/elemops/save.go`. - `elemops.ApplyEdit` — the children-composition `switch` becomes "split, replace the axes the request sent, compose". The absent-versus-present semantics of `items` and `body` (table in its comment) are unchanged. - `content` — one exported adjacent-text merge helper; `removeAt` uses it. Starting hint: `internal/content/mutate.go`. - `l10n.bodyContent`, `l10n.spliceNodes`, `l10n.insertNode`, `l10n.isItem`, `l10n.holdsElement` — call the owners instead of re-deriving. Starting hints: `internal/l10n/hash.go`, `internal/l10n/l10n.go`, `internal/l10n/structure.go`. - `handleElementBody` — serves the split's body region. Starting hint: `internal/server/body.go`. - `scaffold/site/AGENTS.md` "Editing content by hand" (the "Bodied elements use paired tags … Items rows follow the same rule" passage and the byte-identical-save promise) and the "Item collections" paragraph; `scaffold/editor/elements/text-media.yaml` and `image.yaml` comments (the stored shape); `scaffold/site/.claude/skills/edit-content/SKILL.md` (the "Items children" bullet: where rows go and one per line, pointing at AGENTS.md), `migrate-site/SKILL.md` (what `harness check` reports and what the editor canonicalises on first save); `redo-theme/references/showcase-page.md` already places rows one per line — confirm it parses as canonical layout; the package comments in `internal/elemops/save.go` and `internal/server/body.go` that currently describe rows leading. ## Acceptance criteria - [ ] Saving the documented hand-authored shape (body, then one `img` row per line) with its rows and body unchanged leaves the file byte-identical. - [ ] `GET /api/element/body` on that element serves `"Body."` (no separator newlines), and the same after a save. - [ ] A round-trip table test over hand-authored shapes — glued rows, one row per line, body before rows, body after rows, text between rows, trailing text, rows only, body only, both empty — shows: the GET succeeds; GET then save reaches the canonical layout; the second save is byte-identical to the first; `harness check` accepts the result. - [ ] An editor-written glued file (`{{< img … />}}{{< img … />}}Body.`) is rewritten to the canonical layout on its next save of that element, and never touched by a save of another element on the page. - [ ] The translation hash of an items owner is unchanged across a save whose body region text is unchanged, whatever the layout before the save; a real body text change still changes it. - [ ] A translation repair that adds, removes or reorders a row in a connected translation's items owner leaves that owner in canonical layout. - [ ] `WouldClobberBody`, the form's row list, the translated-items refusal and the hash's body content behave as before on canonical input (existing tests), now via the split. - [ ] Exactly one adjacent-text merge in the engine, exported from `internal/content`; `coalesceText`, the merge in `bodyContent` and the merge in `spliceNodes` are gone; `removecmd/strip.go` is untouched. - [ ] Exactly one "is this child an item of its parent" predicate; `l10n.isItem` and `l10n.holdsElement` call it. - [ ] `scaffold/site/AGENTS.md`, the two sidecar comments and the skills named above describe the canonical layout once and point at it, and the drift tests (`internal/element/agentsdoc_test.go`, `scaffold/skills_test.go`) pass. - [ ] The existing tests that pin the glued form (`/>}}{{<` in `internal/server/items_test.go`, `element_test.go`, `l10n/*_test.go` and others, 27 occurrences) are updated to the canonical layout, not deleted. - [ ] The PR description carries the release note: editor-written items owners get a one-time reformat on their next save. ## How to verify - `go test ./...` and `make check` (or the repo's lint target) green. - The reproduction: write the hand-authored `figures` file above into an `itemsSite` test site, `GET /api/element/body`, then `POST /api/page/save` with the two rows (uids echoed) and no body; before the change the file comes back glued and the body served is `"Body.\n\n"`. - `cd web && npm test` still green: the client's body handling does not change, only what the server serves. ## Assumptions to check - The RTE's markdown serializer never emits a trailing newline run, so the split's edge-stripping makes the served body and the RTE's own output agree for an unedited body; verify with `web/src/rte/markdown.ts` before relying on the hash criterion. - No theme partial depends on the rows preceding the body in source order (`img` pushes onto the parent's Scratch and the body comes from Inner, so order should not matter); confirm in `themes/deckle` before changing the order. - `placeIndex` in `internal/l10n/structure.go` orders by source position among all elements of a parent; once rows are a separate list, confirm reordering a translation's rows still follows the source order. ## Out of scope - Relaxing `content.Serialize` to merge adjacent text. - A `harness check` warning or a `harness` reformat command for non-canonical row layouts. - Changing the form's "items always sent" behaviour or the save request's absent/present semantics. - `internal/removecmd/strip.go`. - Any change to how rows render. ## Blocked by None - can start immediately ## Original report Follow-up to #230 / PR #231, which only made the body GET tolerate the row layout the save never produced. The underlying asymmetry is still there. ## Problem An items owner's children are read and written by code that each knows the children grammar on its own: - `GET /api/element/body` (internal/server/body.go) strips the rows and serves the rest. - `elemops.ApplyEdit` composes `rows ++ body` with no separator between rows, so every save writes `{{< img … />}}{{< img … />}}Body.` on one line and moves the body behind the rows. Rendering is unaffected (img pushes onto the parent's Scratch; the body comes from Inner), but: - every save of a hand-authored element produces a noisy, structure-mangling diff (reporter's "minor" point); - the inter-row whitespace of the original file leaks into the translation hash (`internal/l10n/hash.go` `bodyContent` hashes `Body.\n\n`, the saved form hashes `Body.`), so the first no-op editor save of a hand-authored items owner flags every connected translation as "source changed"; - the file shape the shipped `text-media.yaml` comment and AGENTS.md teach (one row per line) is not the shape the engine writes, so docs and engine disagree about what canonical looks like. - `elemops.WouldClobberBody`, `internal/server/forms.go` (row list) and `internal/server/translation.go` (item sync) each call the split for one half. ## Proposal 1. **One grammar.** Define the canonical layout of an items owner's body once, e.g. one row per line, then one newline, then the body region. Decide and document whether the body precedes or follows the rows (the sidecar comment says body first; the engine writes rows first). Rendering does not care; hand-editing and docs do. 2. **One owner.** Replace the raw `PartitionItems` with a split/compose pair in `elemops` that are exact inverses: split returns `(rows, body)` with the separator newlines removed, compose writes them back. GET body, ApplyEdit, WouldClobberBody, the forms row list, the translation item sync and `l10n.bodyContent` all go through it. Nobody slices children by hand. 3. **Round-trip table test** over hand-authored shapes (glued rows, one row per line, body before rows, body after rows, text between rows, trailing text): GET succeeds; GET then save is a fixed point (second save byte-identical to the first); a no-op save leaves the translation hash unchanged; `harness check` accepts the result. 4. **Collapse the duplicated text-merge.** "Merge the adjacent text nodes a removal exposes" exists in `content/mutate.go` (`removeAt`), `elemops/save.go` (`coalesceText`), `l10n/hash.go` (`bodyContent`), `l10n/l10n.go` (`spliceNodes`). One exported helper in `content`, used by all four. (`removecmd/strip.go` is intentionally different: it repairs whitespace seams and re-verifies the round trip; leave it.) ## Migration Every element the editor already wrote in the glued form gets a one-time reformat diff on its next save. Acceptable, but say so in the release notes. Consider whether `harness check` should report non-canonical row layout, or stay silent since parse/serialize round-trips either way. ## Surface to update (CLAUDE.md rules) - `scaffold/site/AGENTS.md`: "Bodied elements use paired tags … Items rows follow the same rule" passage and the content canonicalization rules; the item collections paragraph. - `scaffold/editor/elements/image.yaml`, `text-media.yaml` comments (the stored shape). - Skills: `edit-content-by-hand` (how to write rows by hand), `migrate-site` (what the engine canonicalizes), `redo-theme/references/showcase-page.md` if it shows rows. - `internal/server` package README (frozen save contract) if it spells out child composition. ## Open questions for triage - Body before or after rows? - Should the GET also strip (and compose re-add) exactly one blank line between rows and body, or keep the body region byte-exact? - Alternative with a much smaller blast radius: make `content.Serialize` merge adjacent text instead of refusing it (the dangerous merge, one forming a shortcode opener, is still caught by `checkTextRaw`). It kills the class of bug but gives up an invariant tests rely on. Rejected in the first pass; worth a deliberate decision.

dominik.polakovics added the

needs-triage

label

2026-10-12 18:05:19 +02:00

dominik.polakovics referenced this issue

2026-10-12 18:31:54 +02:00

Drift audit: rules implemented twice across engine, server, client and theme #234

dominik.polakovics added

bug

ready-for-agent

and removed

needs-triage