Skip to content

Batch mutations into one snapshot, and a DocxDiff-scored puzzle eval for the agent surface - #475

Open
JSv4 wants to merge 3 commits into
mainfrom
claude/docx-arcade-assessment-7mzumo
Open

Batch mutations into one snapshot, and a DocxDiff-scored puzzle eval for the agent surface#475
JSv4 wants to merge 3 commits into
mainfrom
claude/docx-arcade-assessment-7mzumo

Conversation

@JSv4

@JSv4 JSv4 commented Aug 14, 2026

Copy link
Copy Markdown
Owner

Three commits, from an assessment of what the arcade actually proves.

The arcade proves the addressing model: anchors survive save/reopen, and the reconciler stays incremental while a host mutates the session behind it. It proves nothing about the surface we ask agents to drive, because it drives raw.replaceXml — the escape hatch for callers who want to hand-author OOXML. That gap is what these two changes close from opposite ends.

1. DocxSession.Batch — one snapshot, one undo step

Every mutation records a pre-op snapshot, and a snapshot deep-clones every projected part, so its cost scales with the document, not the edit. Measured on TestFiles/NVCA-Model-COI.docx (144 KB on disk), one snapshot retains ≈7.5 MB — so a forty-edit sequence paid forty of those and consumed forty entries of a twenty-deep ring. The sequence a caller had just applied was already only half reversible by the time it finished. Both costs are properties of the loop, not of the work. Agent callers hit it hardest, because a plan is naturally a list of edits.

  • Batch(steps, options) for callers who can express steps as Func<EditResult> closures; BeginBatch/EndBatch for those who cannot — a JSON dispatcher running its own switch, which is exactly the MCP case. Batch is implemented on the pair, so there is one mechanism.
  • 49 _history.RecordPreOp(TakeSnapshot()) sites now route through RecordPreOpSnapshot(), which suppresses inside a batch. The suppression has to live at the call site, not in UndoRingTakeSnapshot() is evaluated before the ring ever sees the entry, so dropping it inside the ring would still pay for the clone.
  • Failure policy cannot escape the guarantee Roll back partially-applied mutations when a DocxSession op throws #459 established. Atomic (default) reverses on the first failure. Best-effort tolerates only failures that provably did not touch the document; a step that threw partway, or one the validator rejected after it had already written, reverses the whole batch regardless — inside a batch there is no per-step snapshot left to unpick that step alone.
  • The two raw-op validation paths that hand-rolled their own pop-and-restore now use RollbackFailedOp(), removing the last non-uniform history call site.
  • Batch() holds the mutation gate from Add optimistic mutation preconditions (#447) #476 for the whole sequence rather than per step: releasing it between steps could interleave with a concurrent mutation and then reverse that mutation's work during rollback. BeginBatch/EndBatch cannot offer this — holding a lock across a JSON-RPC round trip waits on a caller that may never close its batch — and the asymmetry is documented rather than papered over.

docxodus_mutations becomes genuinely atomic on the back of this, with no wire schema change. A step that mutated before failing now reports rolledBack: true with editsApplied: 0 rather than describing a half-applied document as a partial success.

23 tests (DS430–DS443) cover undo grain, both failure policies, damage overriding policy, nesting, the scope form, and the retained-memory shape.

2. The puzzle eval — measuring the surface agents actually get

A level is a pair of documents and a budget: transform start into target in at most par calls. A level is solved when DocxDiff returns zero revisions against the target, so the scoring function is the comparison engine itself — exact, unarguable, and the differentiator no demo surface touches.

  • Levels declare both documents as paragraph lists rather than shipping binaries: they diff in review, and one builder constructs both sides, so a scoring difference can only come from the player's edits.
  • Reference solutions address blocks by content through FindAllByText — the op behind docxodus_search — so a reference cannot accidentally be easier than the task by starting with ids nobody has yet.
  • CI keeps the levels honest rather than testing the session: the reference solves the level (PE001), within par (PE002), and the start does not already score as solved (PE003). PE003 is load-bearing — a mis-built target would otherwise let an empty solution pass every level and report the pack at 100%. PE004 pins that the scorer rejects a partial solve; PE005 applies a whole level as one batch.

L01-clause-order ships first and already shows the surface something: a two-paragraph clause costs two MoveBlock calls because there is no move-the-section op. That is the kind of affordance gap the pack exists to surface.

3. Merge of main (#476, #478, #483)

Main moved three PRs ahead while this was open, touching the same surfaces. Resolutions are detailed in the merge commit; the two that matter:

  • docxodus_mutations preview. Main independently fixed the under-revert by counting version deltas per step instead of steps. The batch supersedes the counting entirely — there is exactly one snapshot to restore however much each step cost — so the delta accounting and the undo loop are gone. Main's entry precondition check is kept, and so is RestorePreviewVersion: the batch restores the document but not the version counter, and a caller holding a version describing no state the document was ever in would fail its next precondition check against a mutation that never happened. Version is now restored on any reversal, not just preview.
  • Main's two new mutating ops are batch-aware, because the call-site rewrite was re-applied over the merged file rather than assumed to still hold.

Also

The arcade's og:/twitter: copy read as a live WAD parser, where tools/wad2cart.mjs rasterizes E1M1's geometry at build time and ships static data. freedoom-e1m1.js was already accurate; only the social meta overstated it.

Not done here

The batch primitive is core + facade + MCP only. The WASM bridge, npm/TS, and the stdio host + Python client do not yet expose it, so the ripple checklist is deliberately incomplete — flagging rather than hiding it. The MCP surface was the one that mattered for the eval, and it needed no new wire schema.

Testing

  • Merged suite: 3518 passed, 1 failed, 3 skipped.
  • The one failure is HCO081_RenderBlocksHtml_MatchesFullRenderFragments, which fails identically on clean main at 7349259, whose own CI run is already red. It is a base-branch failure this branch inherits, not one it introduces — see the comment below for the evidence.
  • All 14 checks were green on the pre-merge head (5637383), including the .NET suite and Playwright.
  • Library 115 warnings and test project 618 — both unchanged from the documented baseline.
  • Not run in this environment: the Playwright suite (no browser deps installed) and the weekly LibreOffice parity job.

claude added 3 commits August 14, 2026 05:29
Every DocxSession mutation records a pre-op snapshot, and a snapshot deep
clones every projected part — so its cost scales with the DOCUMENT, not
with the edit. Measured on TestFiles/NVCA-Model-COI.docx (144 KB on disk),
one snapshot retains ~7.5 MB, so a forty-edit sequence paid forty of those
AND consumed forty entries of a twenty-deep ring. The sequence a caller had
just applied was therefore already only half reversible by the time it
finished. Both costs are properties of the loop, not of the work: N edits
that form one intent deserve one snapshot and one undo step.

Adds Batch(steps, options) for callers who can express steps as
Func<EditResult> closures, and BeginBatch/EndBatch for those who cannot —
a JSON dispatcher running its own switch, which is exactly the MCP case.
Batch is implemented on top of the pair, so there is one mechanism.

The 47 `_history.RecordPreOp(TakeSnapshot())` sites now route through
RecordPreOpSnapshot(), which suppresses inside a batch. The suppression has
to live at the call site rather than in UndoRing: TakeSnapshot() is
evaluated before the ring ever sees the entry, so dropping it inside the
ring would still pay for the clone.

Failure policy cannot be used to escape the guarantee PR #459 established.
Atomic (default) reverses on the first failure. Best-effort tolerates only
failures that provably did not touch the document; a step that threw
partway, or one the validator rejected after it had already written,
reverses the whole batch regardless — inside a batch there is no per-step
snapshot left to unpick that step alone. The two raw-op validation paths
that hand-rolled their own pop-and-restore now use RollbackFailedOp() too,
which removes the last non-uniform history call site.

docxodus_mutations becomes genuinely atomic on the back of this, with no
wire schema change. Its preview mode restores one snapshot instead of
issuing N Undo() calls with N tracked by hand — that loop under-reverted
whenever a step consumed more than one ring entry, making "nothing is left
changed" a promise it could not keep.

20 tests (DS430-DS443) cover undo grain, both failure policies, damage
overriding policy, nesting, the scope form, and the retained-memory shape.
Full suite: 3476 passed, 0 failed.
The arcade proved the addressing model — anchors survive save/reopen, and
the reconciler stays incremental while a host mutates the session behind
it. It proved nothing about the surface we ask agents to drive, because it
drives raw.replaceXml: the escape hatch for callers who want to hand-author
OOXML, which is the opposite of what the tool surface is for.

A level is a pair of documents and a budget: transform start into target in
at most `par` calls, using only the grouped tools and anchor addressing. A
level is solved when DocxDiff between the player's document and the target
returns zero revisions, so the scoring function is the comparison engine
itself — exact, unarguable, and the differentiator no demo surface touches.

Levels declare both documents as paragraph lists rather than shipping
binaries: they diff in review, and one builder constructs both sides so a
scoring difference can only come from the player's edits. Reference
solutions address blocks by content through FindAllByText — the op behind
docxodus_search — so a reference cannot accidentally be easier than the
task by starting with ids nobody has yet.

CI keeps the LEVELS honest rather than testing the session: the reference
solves the level (PE001), does so within par (PE002), and the start does
not already score as solved (PE003). PE003 is the load-bearing one — a
mis-built target would otherwise let an empty solution pass every level and
report the pack at 100%. PE004 pins that the scorer rejects a partial
solve; PE005 applies a whole level as one batch, since a plan is exactly
what batching exists for.

L01-clause-order ships first, and already shows the surface something: a
two-paragraph clause costs two MoveBlock calls because there is no
move-the-section op, which is the kind of affordance gap this pack exists
to surface.

Also corrects the arcade's og:/twitter: copy, which read as a live WAD
parser where wad2cart.mjs rasterizes E1M1 at build time. freedoom-e1m1.js
was already accurate; only the social meta overstated it.

Full suite: 3481 passed, 0 failed. Library 115 warnings, tests 618 — both
unchanged from baseline.
Main gained three PRs that touch the same surfaces: #476 (mutation
preconditions + ExecuteMutation and a mutation gate), #478 (canonical table
addressing), and #483 (page citations).

Resolutions:

- DocxSession.cs, 8 conflicts, all the same shape — main added code around
  a snapshot call site. Took main's side wholesale, then re-applied the
  call-site rewrite over the merged file so main's two NEW mutating ops are
  batch-aware too. 49 sites now route through RecordPreOpSnapshot(), up
  from 47.

- Dispatcher.cs, docxodus_mutations. Main independently fixed the preview
  under-revert by counting version deltas per step instead of steps, then
  rewinding that many entries. The batch supersedes the counting entirely —
  there is exactly one snapshot to restore however much each step cost — so
  the delta accounting and the undo loop are gone. Main's entry
  precondition check is kept as-is, and so is RestorePreviewVersion: the
  batch restores the DOCUMENT but not the version counter, and a caller
  holding a version that describes no state the document was ever in would
  fail its next precondition check against a mutation that never happened.
  Version is now restored on any reversal, not just preview.

- Batch() takes the mutation gate main introduced, for the whole sequence
  rather than per step: a batch that released it between steps could
  interleave with a concurrent mutation and then reverse that mutation's
  work as part of its own rollback. BeginBatch/EndBatch cannot offer this,
  since holding a lock across a JSON-RPC round trip waits on a caller that
  may never close its batch. The asymmetry is documented rather than
  papered over.

- CHANGELOG.md: additive on both sides, kept both.

Merged suite: 3518 passed, 1 failed. The failure is
HCO081_RenderBlocksHtml_MatchesFullRenderFragments, which fails identically
on clean origin/main at 7349259 and whose CI run on main is already red. It
is a base-branch failure, not one this branch introduces.

JSv4 commented Aug 14, 2026

Copy link
Copy Markdown
Owner Author

Merged main in to clear the conflict (#476 preconditions, #478 table addressing, #483 page citations all landed while this was open). mergeable_state is back to blocked rather than dirty.

One heads-up on CI, since this branch now inherits a failure it did not cause.

HCO081_RenderBlocksHtml_MatchesFullRenderFragments fails on main itself. I ran it in a clean worktree at origin/main (7349259), untouched by this branch:

Failed!  - Failed: 1, Passed: 0, Skipped: 0, Total: 1

The assertion is that batch block rendering matches the full render, and the two paths disagree on attribute emission:

Expected: ···"70f9b72168781faec5" data-source-anchor-id"···
Actual:   ···"70f9b72168781faec5" style="--docx-auto-li"···

Main's own CI run for 7349259 is already red (CI and Playwright Tests both failing), so this predates the merge. Nothing in this PR touches WmlToHtmlConverter or HtmlConversionOps.

I have not fixed it — it is outside what this PR is for, and picking a side between the two render paths wants whoever owns the page-citation / data-source-anchor-id work. Happy to take it in a follow-up if you'd like, but it should not gate this branch.

Everything else on the merged tree is green: 3518 passed, 1 failed, 3 skipped, with library warnings at 115 and test warnings at 618 — both at the documented baseline. The pre-merge head (5637383) had all 14 checks green, including Playwright.


Generated by Claude Code

JSv4 commented Aug 14, 2026

Copy link
Copy Markdown
Owner Author

Both failing checks on ca080c6 are inherited from main. Evidence for the second one (test / Playwright), to go with the build-and-test note above.

Same four specs fail on main at 7349259, with identical totals:

this PR (ca080c6) main (7349259)
failed 4 4
skipped 9 9
passed 499 499

Same four, in both runs:

[chromium] › tests/docx-session.spec.ts:415:3 › DocxSession (WASM bridge) › mergeCells / unmergeCells across the bridge (#340)
[chromium] › tests/pagination-footnote-geometry.spec.ts:157:3 › notes stack with no renderer-invented spacing between them
[chromium] › tests/pagination-keep-with-next.spec.ts:97:3 › starts a new keep chain after page-break-before with a footnote continuation
[chromium] › tests/pagination-keep-with-next.spec.ts:125:3 › does not force a chain onto a fresh page without room for its pending continuation

…failing on the same assertion, an extra empty page in the paginated result:

  Array [ "body" ],
+ Array [],

When it broke. Playwright on main was green at ebfc6fc1 — this branch's merge base — and at 3223daaf. It went red at 356ee2b6 (#478) and has stayed red through bb4d130c (#476) and 73492596 (#483):

main SHA Playwright
ebfc6fc (branch point)
3223daa
356ee2b (#478)
bb4d130 (#476)
7349259 (#483)

This branch's own pre-merge head (5637383) had all 14 checks green, Playwright included. The red only appeared once main was merged in.

Summary of the two blockers, neither of which this PR touches:

  1. build-and-testHCO081_RenderBlocksHtml_MatchesFullRenderFragments, reproduced in a clean origin/main worktree (comment above).
  2. test — the four specs above, cell-merge across the WASM bridge (Canonicalize table addressing across APIs (#450) #478's area) and footnote pagination geometry (Add portable page citation maps #483's area).

I've left both alone deliberately: fixing table addressing and pagination geometry inside a batching PR would make this unreviewable, and picking a side wants whoever owns that work. Happy to take either as a follow-up — just say which.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants