fix(llm-tools): deletion guard on tracked rewrite + actionable oneOf validation errors#3584
Open
andrii-harbour wants to merge 1 commit into
Open
fix(llm-tools): deletion guard on tracked rewrite + actionable oneOf validation errors#3584andrii-harbour wants to merge 1 commit into
andrii-harbour wants to merge 1 commit into
Conversation
…validation errors (SD-3287) Closes the two highest-volume LLM-tooling error classes from production traffic. **executor.ts — empty text nodes on tracked rewrite (1001/wk).** The word-diff prefix/suffix trim optimization (#2817/#2832) could reduce a non-empty replacement to an empty delta (e.g. "best endeavours to:" → "endeavours to:" leaves trimmedNew === ""), then the single-change branch called schema.text(""), which ProseMirror rejects with `RangeError: Empty text nodes are not allowed`. Add an `else if (trimmedNew.length === 0)` branch that uses `tr.delete(trimmedFrom, trimmedTo)` instead — same semantics as the existing tracked-review-state guard. Regression tests in tracked-rewrite.integration.test.ts cover both layers (direct executor and end-to-end doc.replace in tracked mode). The remap-length empty-replacement test was asserting the buggy `tr.replaceWith(5, 12, <empty node>)` call — the mocked schema masked the crash — corrected to assert `tr.delete(5, 12)` and `tr.replaceWith` not called. **operation-args.ts — opaque oneOf validation errors (2000+/wk cluster).** `validateValueAgainstTypeSpec`'s oneOf branch returned the unactionable `X must match one of the allowed schema variants.` for every object-union failure. LLM agents looped on the same malformed shape because the error named no specific issue. Make the branch discriminator-aware: * When object variants share a const discriminator (kind/op/type) and the value carries it matching exactly one variant, re-validate against that variant and surface its specific failure (e.g. `at.target is required.`, `target.nodeType is required.`). * When the value carries the discriminator but matches no variant, list the allowed tag values and echo what was sent. * When no discriminator is shared (or the value isn't an object), enumerate the accepted variant shapes via `describeVariant` and report the received value via `describeReceived`. Behavior is message-only: identical pass/fail across every input. Same fix benefits every oneOf-validated parameter (target/at/within/steps[]) across insert/create/lists/styles/format/query/mutations at once. Defensive details: * `return;` after the inner re-validate in the discriminator-match path (unreachable in practice — the matched variant already failed in the outer loop — but pins the invariant against future fast-path refactors). * `details.selectedError` set consistently across all three error paths (matched-variant, unmatched-tag, generic fallback). * `truncateForError` caps serialized values at 64 chars so an accidentally- large payload can't blow up logs or LLM context. Matches the same truncation pattern used by REPAIR_BLOCKED. New tests in validate-type-spec.test.ts cover real contract schemas (doc.create.paragraph:at and doc.insert:target) plus a non-discriminated repeated-error case that pins the `selectRepeatedActionableOneOfError` fallback (which the new discriminator path would otherwise mask). The existing "oneOf with mixed schemas" generic-fallback assertion was updated to the enumerated message. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
No issues found across 5 files
Linked issue analysis
Linked issue: SD-3287: Empty text nodes are not allowed
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | When a replacement trims to an empty delta, perform a deletion (tr.delete) instead of creating an empty text node. | The executor now detects trimmedNew.length === 0 and calls tr.delete(trimmedFrom, trimmedTo) rather than building schema.text(''), preventing creation of empty text nodes. |
| ✅ | Unit test verifies that an empty replacement collapses to a pure deletion (expects tr.delete called, replaceWith not called). | The remap-length unit test was updated to assert tr.delete was called with the expected range and that replaceWith was not called for the empty-replacement case. |
| ✅ | Integration tests reproduce the customer-reported crash scenario and assert the document state and tracked deletions are correct after a replacement that trims to empty. | New integration tests exercise both the executor & tracked-replace flow: they assert executeTextRewrite returns changed, the doc textContent is correct, accepted text parts are preserved, and removed prefix is present as a tracked deletion. |
Tip: cubic could auto-approve low-risk PRs like this, if it thinks it's safe to merge. Learn more
Re-trigger cubic
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Closes the two highest-volume LLM-tooling error classes from production traffic.