fix(plan-engine): runtime block-identity self-heal on compile #3585
Closed
andrii-harbour wants to merge 3 commits into
Closed
fix(plan-engine): runtime block-identity self-heal on compile #3585andrii-harbour wants to merge 3 commits into
andrii-harbour wants to merge 3 commits into
Conversation
Contributor
|
I wasn't able to get the ecma-spec MCP tools authorized in this session (each call returned a pending-permission error), so I verified the OOXML-relevant claims against my own knowledge of ECMA-376 rather than live spec lookups. Flagging that up front for transparency. Status: PASS The two files in scope — On the
No incorrect defaults, no fabricated attributes, no missing-required-attribute issues at the OOXML layer. The refactor is a pure extract-and-share of the existing normalizer logic — identity semantics are unchanged — and the surrounding runtime-repair/revision-tracker changes don't touch OOXML serialization. One caveat worth noting (not a spec violation): if you'd like me to confirm the |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
cubic analysis
No issues found across 15 files
Linked issue analysis
Linked issue: SD-3283: Document API mutations fail with DOCUMENT_IDENTITY_CONFLICT on duplicate DOCX paragraph identities
| Status | Acceptance criteria | Notes |
|---|---|---|
| Add a regression test with two paragraph-like blocks sharing the same imported `paraId` but having unique `sdBlockId`s. | New unit and regression tests exercise duplicated paraId scenarios and the runtime repair pass, but the added tests create duplicate `paraId` cases and assert `paraId` renames; I don't see an explicit test that sets distinct `sdBlockId` values while sharing the same imported `paraId` (the grouping logic supports multi-attr groups). | |
| ✅ | Mutations no longer fail with `DOCUMENT_IDENTITY_CONFLICT` for duplicate imported paragraph IDs when a unique runtime identity can be assigned safely. | compilePlan now runs a repair pass before asserting duplicates; repair dispatches metadata-only AttrSteps and tests assert compile/execute succeed and index is deduped afterward. |
| Truly ambiguous/duplicate non-repairable identities still fail with a clear error. | The repair code detects when a transaction filter blocks the repair and throws a `REPAIR_BLOCKED` PlanError with a remediation message; however, I don't see a focused unit/test that constructs an unrecoverable/ambiguous identity and asserts the exact error path in this PR (the code implements the behavior). | |
| The customer workaround of Word re-open/re-upload is no longer required for affected precedents/documents. | The runtime repair reproduces import-time dedup semantics and tests show in-session duplicates are healed so mutations succeed without re-import, which addresses the workaround in typical cases. Full proof against production precedents is out of scope for unit/CI tests, so this is implemented but not proven at scale here. | |
| ✅ | Error remediation text matches an actually available API/workflow. | Error messages and `remediation` payload were updated to point to `doc.open` re-import, and tests were updated to assert the new message and to preserve colliding IDs in the message for SDK consumers. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9dfcf81a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
DOCUMENT_IDENTITY_CONFLICT was blocking every mutation on documents whose
in-session ProseMirror state already carried duplicate paraId / sdBlockId
values. The dominant production path is a Yjs collab restore: the import-
time normalizer (`normalizeDuplicateBlockIdentitiesInContent`) runs only on
fresh DOCX imports, so any duplicate that landed in the persisted Yjs
state (whether copy-paste-cloned paraIds from Word's exporter, or older
runtime splits before keepOnSplit:false shipped) survived every later
hydration. The only recovery in production was "open in Word, re-save,
re-upload" — and the legacy error message even told callers to invoke a
`document.repair()` op that did not exist.
This change makes `compilePlan` self-heal a corrupted in-session state
before `assertNoDuplicateBlockIds` fires, using the same rename semantics
the importer already applies.
What landed:
* New `plan-engine/repair-block-identities.ts` walks the PM doc, dispatches
one transaction with per-attribute `tr.setNodeAttribute` renames (AttrStep,
no from/to — explicitly skipped by structured-content lock and permission-
ranges filters), then post-dispatch verifies the attrs actually applied;
throws REPAIR_BLOCKED with offending IDs in the message string when a
filter rejects it (Python SDK strips `details`).
* `BlockIndex` carries an optional `explicitIdentities` side-channel populated
during the existing `buildBlockIndex` descendants walk — the planner
consumes it for O(1) clean-doc early-exit, no second walk.
* `compilePlan` gains `CompilePlanOptions { skipIdentityRepair }`; `previewPlan`
passes it so preview stays non-mutating and surfaces the conflict as a
PreviewFailure via the existing PlanError catch.
* The repair transaction is meta-tagged with `superdoc/block-identity-repair`.
Both `trackRevisions` and the story-runtime host-store sync listener
meta-skip it (placed before `docChanged` so a future metadata-only repair
also stays revision-invisible), and `trackedTransaction` bypasses
track-changes wrapping at an explicit check rather than the implicit
disallowed-meta fall-through.
* `executePlan` now calls `checkRevision` BEFORE `compilePlan` so a stale
optimistic-concurrency request rejects without first letting the repair
rewrite identity attrs. Regression test pins that the doc identity is
unchanged and revision unmoved across a stale-revision rejection.
* `assertNoDuplicateBlockIds` error message embeds the colliding IDs in the
string (with per-ID + total-count truncation) and points remediation at
`doc.open`, removing the dead `document.repair()` reference.
* Importer normalizer (`normalizeDuplicateBlockIdentitiesInContent`) refactored
to consume `getExplicitIdentityEntries` + `groupIdentityEntriesByValue`
from the shared `block-identity-renaming.js` module; the repair module
imports the same primitives. Single source of truth for "which attrs
carry identity" across import-time and runtime.
Coverage:
* Unit: `repair-block-identities.test.ts` pins the fast-path early-exit,
per-attribute setNodeAttribute, deterministic allocator ordering, and
the REPAIR_BLOCKED throw on filter-rejected dispatch.
* Regression: `tests/regression/duplicate-block-identities.test.js` exercises
end-to-end recovery from a Yjs-restore-shaped state through compilePlan +
executeCompiledPlan, previewPlan non-mutation, and stale-revision rejection
before any repair mutation.
* Track-changes: `revision-tracker.test.ts` pins the meta-skip ordering
invariant (skip applies even for hypothetical metadata-only repair txs).
* All affected suites green (180/180 in touched files; full super-editor
suite ran clean during cross-review iterations).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ision check Addresses the automated review findings on the runtime identity-repair PR. **STALE_REF guard (review P1).** Refs minted against a duplicate-laden doc carry only a blockId — no occurrence index. When the compile-time repair renames the later duplicate(s), a pre-repair ref naming the duplicated id is ambiguous: it may have meant the renamed occurrence, and resolving it against the surviving block would silently mutate the wrong content. `compilePlan` now rejects such refs with `STALE_REF` (re-query remediation in the message) in the same compile that performed the repair. Covered shapes: decoded V3/V4 ref segments, V4 node-scope ids, raw block refs, `by: 'block'` addresses, and SelectionTarget text/nodeEdge points. Selector-based wheres re-resolve against the repaired doc and pass through untouched. The one-compile guard closes the entire wrong-block window: the repair is revision-invisible, but any successful mutation bumps the revision, so refs from earlier calls already die in the existing `rev` checks of `resolveV3TextRef` / `resolveV4TextRef`. **Selection-wrapper revision ordering (review P2).** `selectionMutationWrapper` checked the caller's `expectedRevision` only after `compilePlan` returned — on a duplicate-laden doc a stale selection mutation (or dry run) would rewrite block identities before throwing `REVISION_MISMATCH`. The guard now runs before compile, mirroring `executePlan`; the redundant post-compile check is removed (dry-run is covered by the earlier check). **Test gaps from review.** * Distinct `sdBlockId`s sharing one imported `paraId`: rename targets only the colliding attr group; the sdBlockIds survive as aliases. * `REPAIR_BLOCKED`: observe-but-drop dispatch wrapper pins the post-dispatch verification path — error code, bounded id preview, `doc.open` remediation. * New `stale-refs-after-identity-repair.test.js` regression file: ambiguous ref rejected in the repairing compile; same ref resolves cleanly on the next compile; refs to unaffected blocks resolve during the repairing compile. * `selection-mutation-wrapper.test.ts` pins the ordering invariant: stale revision throws before `compilePlan` is ever invoked. * Existing duplicate-identity regression tests switched from by-ref to selector-based wheres — their old form now correctly trips the STALE_REF guard, which the new regression file covers explicitly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
andrii-harbour
force-pushed
the
andrii/sd-3283-document-api-mutations-fail-with-document_identity_conflict
branch
from
June 11, 2026 01:09
f9dfcf8 to
6c497f3
Compare
…f guard Two gaps in `collectReferencedBlockIds` found during cross-review: * `SelectWhere.within` / `RefWhere.within` carry a pre-minted `BlockNodeAddress` — a selector scoped within a renamed duplicate would silently re-scope to the surviving block. The collector now picks up `within.nodeId` for every where shape that supports it. * nodeEdge SelectionPoints nest the block id at `point.node.nodeId` (SelectionEdgeNodeAddress), not top-level — a pre-repair edge-anchored target could bypass the guard entirely. The collector now reads through the nested shape; text points keep their top-level `blockId` read. New regression: a nodeEdge SelectionTarget anchored on the duplicated id throws STALE_REF in the repairing compile. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
This pull request landed on |
Contributor
|
🎉 This PR is included in superdoc-cli v0.17.0 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in superdoc-sdk v1.16.0 |
Contributor
|
🎉 This PR is included in @superdoc-dev/mcp v0.12.0 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in superdoc v1.40.0 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in @superdoc-dev/react v1.11.0 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in vscode-ext v2.12.0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.