fix: resolve additional patch rountrip issues - #293
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new retry path in patch() returns the retried output without validating it round-trips, which can still silently produce an incorrect/invalid result in the failure scenario it is meant to guard.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR tightens TOML patch round-trip correctness by adding validation/retry logic and improving array/multiline-container change planning to avoid offset/index corruption discovered by fuzzing.
Changes:
- Add a round-trip validation step in
patch()and retry patching with multiline-container transactions when the initial output does not match the requested JS object. - Improve array-diff/remove semantics by tagging “source-coordinate” removes and teaching
reorder()to keep them ahead of same-array adds. - Add distilled fuzz regressions, a seed distillation utility script, and documentation summarizing the fuzz sweep and fixes.
File summaries
| File | Description |
|---|---|
| src/patch.ts | Adds output validation/retry; introduces multiline-container transactional planning; updates change reordering to respect source-coordinate removes. |
| src/diff.ts | Adds internal Remove.coordinate metadata and expands “layout-sensitive” array handling to reduce unsafe remove/move sequences. |
| src/tests/patch.fuzz.test.ts | Adds regression tests for multiple fuzz seeds and updates existing fuzz regression notes. |
| scripts/distill-seed.ts | New utility to reduce failing fuzz seeds into minimal deterministic regressions. |
| docs/bug-notes/fuzz-sweep-0-3000000-roundtrip-fixes.md | Documents the sweep, the fixes applied per seed group, and validation steps. |
| docs/bug-notes/fuzz-error-seeds-0-3000000-rerun.md | Captures the rerun seed list and failure summaries. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
DecimalTurn
force-pushed
the
dev-fuzz-bug-fixes
branch
from
August 23, 2026 08:28
e7e1c5c to
dfadf5f
Compare
DecimalTurn
marked this pull request as ready for review
August 23, 2026 08:39
patch() re-parses its own output and checks that it round-trips to the
updated object on every call. Measured against latest, that costs ~35% on
a small config and ~19% on a 45KB document, and it accounts for the whole
regression: with validation removed the numbers return to baseline.
Validation stays on by default so the guarantee holds. Callers for whom
patching is hot can pass { validate: false }, which returns the first
attempt directly and reaches baseline (0.999x small, 1.008x big). Opting
out also disables the retry and the throw, so a patch that would have been
repaired returns malformed TOML instead. The tests assert both outcomes
exactly so the tradeoff is visible rather than implied.
Also drops two redundant walks from the default path: hasTemporal() ran
once inside patchCst() and again in every comparison, and
normalizePatchComparison() was applied to the updated object twice. The
function is idempotent, so neither removal changes behavior.
Finally, the multiline transaction planner only feeds the retry pass, but
it walked the whole document plus every change on the fine-grained pass as
well and discarded the result. It now runs only when
useMultilineTransactions is set.
TomlDocument.patch() called patchCst() directly, so the round-trip
guarantee in patch() never applied to it. On the distilled seed 771152
input it silently stored TOML that does not parse, and every later
toJsObject()/toTomlString() call read from that broken state. This
reproduces on latest.
TomlDocument.patch() now validates and retries the way patch() does, and
accepts the same { validate: false } opt-out.
Rollback is required rather than optional here, because patchCst() mutates
the CST nodes it is handed: by the time the first attempt is known to be
bad, this._cst is already spent. So the retry starts from a fresh parse of
the pre-patch source, and if neither attempt round-trips the document is
restored from that source before throwing. A caught failure therefore
leaves the document byte-identical and still patchable.
The comparison honours the document's own integersAsBigInt. That exposed a
gap in the shared comparison: TOML has one integer type, so a document
read as bigint legitimately accepts a plain number assigned back into it,
but 2 and 2n stringified differently and failed validation on a correct
patch. Integers are now canonicalised to a decimal string, which keeps
that working while still catching genuine precision loss, where the two
decimal strings differ.
patchResultMatches(), normalizePatchComparison() and hasTemporal() move to
patch-validate.ts so both entry points share one implementation. That
extraction was verified behaviour-neutral before any of the above.
multilineChangeCounts mirrored multilineChanges exactly: both were written in the same block with no branch between them, so the count was always multilineChanges.get(container).length. Reading it from the list drops the parallel map and the `?? []` fallbacks that only existed because the counter was the thing being iterated. Container insertion order is unchanged, since the first write for a given container landed in both maps in the same iteration, so transactionalPaths comes out in the same order.
…ents bc3e844 ("fix: harden multiline patch planning") also deleted two inline rationales and two blank lines from coalesceStructuralReplacements, a function it did not otherwise touch. The comments explained why each guard exists, which is not obvious from the conditions alone: parentPath.length === 0 -> never coalesce at the document root tryFindByPath(original, path) -> parent still exists literally The function is now byte-identical to its pre-bc3e844 state.
The round-trip fixes note attributed every fix to `aa60a53` and named `3a593d0` as the distillation control. Neither SHA is reachable from this branch any more: both were rebased away, so a reader after merge cannot resolve either one. Commits are now named by subject, which survives a rebase, with a note saying why. The one SHA still quoted, `82d6bca`, is verified reachable and identified by its relationship to the fix commit rather than standing alone. Also drops the closing claim that the suite still contains legacy exact-format failures. That described an intermediate state; the suite is green.
normalizePatchFormat() lived in patch.ts, so only patch() validated
newLine. Every other entry point took the raw value straight to the writer:
stringify(value, { newLine: 'LF' }) -> "a = 9LFb = 2LF"
stringify(value, { newLine: '\r' }) -> invalid TOML, silently
new TomlFormat('LF') -> literal "LF" between lines
The normalization moves into validateFormatObject(), which every plain
format object already passes through, and into the TomlFormat constructor,
which is the only place that can normalize what an instance carries because
resolveTomlFormat() returns instances untouched. patch() no longer needs its
own copy.
Aliases (LF, CRLF, unix, DOS and the two-character \n / \r\n spellings) are
accepted case-insensitively; anything that denotes neither LF nor CRLF
throws. The alias table is a Map so a key like `constructor` cannot resolve
through Object.prototype.
Ordering is deliberate: normalization runs after the schema type checks, so
a non-string newLine is still reported as a type error rather than an
unsupported value. A null or absent newLine still falls back to the default,
as `?? DEFAULT_NEWLINE` did.
Two existing tests pinned the old behaviour and are updated:
- TomlFormat('') asserted that an empty newLine was accepted, which runs
every line together into TOML that cannot parse.
- The js stringify test asserted that newLine: "\n" was emitted verbatim,
checking the output ended with a literal backslash-n. It now resolves to a
real LF.
The three tests added for patch validation were copied verbatim from fuzz seed 771152, so they carried the seed's random identifiers (b_3cmsbhh, al1erl4-9, us.xl."/", k51, QhvX_vl aKj9dsQ0r7). That hid what the cases actually exercise. What reproduces the bug is the document structure: a multiline inline array holding multiline strings, with several writes landing in one container. The key spellings are irrelevant, which I confirmed by checking that the renamed fixture still needs the transactional retry and still produces unparseable TOML when validation is skipped. Renamed to build.artifacts / meta.notes / width / label, and the numeric noise reduced to values that read as deliberate. The assertions stay exact.
Every distilled regression asserted only expect(parse(result)).toEqual(obj), which ignores formatting by construction. Most of these cases round-trip only because patch() validates its first attempt and retries transactionally, and that retry rewrites whole multiline inline containers: multiline strings collapse to basic strings, non-decimal integer bases are normalized, dotted keys expand into nested inline tables. None of that was visible to the suite, so the formatting cost of the retry could grow unnoticed. All 16 now assert the exact TOML as well. That includes the 2591153 case, which carried a commented-out placeholder and a TODO asking for an exact assertion once the intended formatting was specified; recording what is actually produced today is more useful than leaving it unasserted. Inline snapshots rather than hand-written strings: these fixtures embed multiline basic and literal strings, and quoting them by hand invites an assertion that is subtly wrong. The values are generated, so they record current output including the known losses above. A change that recovers any original formatting now surfaces as a reviewable diff instead of passing silently. Regenerate with `vitest -u` once a change is confirmed to be an improvement.
toJsObject() converts the internal TOML date classes to plain Date objects so callers get the standard Date contract. But the writer derives a value's TOML text from toISOString(), and those classes override it to return the original spelling, so a plain Date handed back in was written as a full offset date-time. Reading a document, changing one unrelated value and patching it back rewrote every date in the affected container: stamps = [ 1986-03-02, 07:32:00, 1979-05-27T07:32:00Z, "tail" ] -> [ 1986-03-02T00:00:00.000Z, 0000-01-01T07:32:00.000Z, ... ] Fixed where the loss happens rather than at the getter. Removing the conversion would have been smaller, but toJsObject() would then expose objects whose toISOString() returns the TOML spelling instead of the standard form, which is a documented part of its contract and is covered by an existing test. toISOString() cannot be corrected on the classes either, since generate.ts uses it to produce the TOML text. TomlDocument.patch() now pairs the incoming object against the pre-patch values and puts the original typed instance back wherever the caller left a date alone: still a plain Date, same instant. A date whose instant changed is left to widen, because a plain Date cannot say whether a new instant is still meant to be a local date, and the exported date classes are how the caller says so. Instances of those classes are never touched, and the caller's object is not mutated (a node is copied only when something beneath it changed). The pairing is positional, so a date that moves within an array no longer lines up with its original node and still widens. That boundary is pinned by a test rather than left to be discovered. Cost is negligible: a cheap hasPlainDate() scan gates the work, so a document with no dates skips it entirely.
The distilled regressions carried the raw output of the fuzzer: keys like b_3cmsbhh.al1erl4-9 and "AKy:}nV@", random string payloads, and in the case of seed 2591153 a 110-line document that was never actually reduced. Nothing in that is load-bearing. What reproduces these bugs is the shape: multiline strings inside multiline inline containers, and how many writes land in one container. All 16 are renamed to readable keys and values, and 2591153 is reduced from 110 lines to 9. Titles now say what each case exercises instead of only quoting a seed number, and each carries a comment explaining the mechanism. Verified rather than assumed. Every case was run against a build of 82d6bca, the commit before `fix: harden multiline patch planning`, where 13 of the 16 still fail. The same 13 fail there after the rewrite, with the same failure modes, so no reproduction was lost. The other three already passed at that commit, so the control cannot speak to them; for those the diff() change-set shape was checked to be unchanged. Two cases needed care. Minimising 1286183 too far removed the trailing multiline strings and it stopped reproducing, so that structure is restored. 2824408 stopped reproducing when the renamed strings came out shorter, since the bug turns on offsets, so its line lengths now match the original. The seeds themselves remain covered byte-for-byte by the historical harness test, which is what makes reducing these safe.
The round-trip verification is what fixes the multiline offset bugs on this branch, but the way it was exposed made the branch a minor release: a new `options` argument on patch() and TomlDocument.patch(), a new exported PatchOptions type, and a new throw when neither attempt round-trips. None of that is needed for the fixes to work, so it comes out: - the `options` parameter and the PatchOptions type are gone; verification and the transactional retry stay as implementation details - an unsatisfiable patch now returns the fine-grained result instead of throwing, so nothing fails where earlier versions succeeded, and TomlDocument commits that result with its CST re-derived from it rather than rolling back The generated .d.ts is now byte-identical to origin/latest, so this branch carries no public API change at all. Verification cannot simply be dropped instead. With it disabled, 13 seeds in the historical harness and 11 distilled regressions fail. Running the transactional planner unconditionally fixes those but breaks 21 formatting tests, because the transaction rewrites whole containers and is only correct as a fallback. Fixing the underlying stale-offset problem needs the CST to carry explicit trivia instead of encoding formatting as coordinates, which is not a patch-sized change. The `validate` option, the fail-fast throw and the TomlDocument rollback are all worth having; they belong on a follow-up branch that can be a minor release. The tests covering them are removed here, with the behaviour they protected kept as assertions that do not reference the option.
Verification ran on every call, and removing the `validate` option took away the only way to avoid it. Measured against origin/latest that was about 1.35x on a small config and 1.19x on a 45KB document, paid unconditionally. The retry only differs from the first attempt when the transactional planner finds a multiline inline container that holds a multiline string and carries no comment. Without such a container the retry reproduces the first attempt byte for byte, verification cannot change what is returned, and the whole re-parse and structural comparison is dead work. Gated on exactly that, in two steps: an indexOf for a multiline delimiter, then a walk of the CST that is already parsed. Both entry points decide before patchCst() runs, since it mutates the nodes the scan needs to see -- computing it afterwards reads a collapsed string and silently skips verification, which cost 5 seed regressions on the first attempt. This is not a trade against correctness. Where the gate skips, the output is identical by construction, which is only true because an unsatisfiable patch now returns the fine-grained result rather than throwing. Against origin/latest: small config, no multiline string 1.00x small config, multiline string outside inline containers 0.99x small config, multiline string inside one (verifies) 1.38x big doc 45KB, no multiline string 0.98x / 1.01x So baseline everywhere except documents that actually contain the construct these fixes exist for.
patchNeedsVerification() only looked for a multiline string delimiter anywhere in the source, but its name and comment claimed it decided whether verification was needed. It does not: it is one half of hasMultilineStringDelimiter(source) && hasTransactionCandidate(cst) where the second half is what checks the condition that matters, a multiline string inside a multiline inline container. Renamed to say what it looks at, and its comment now points at the function that makes the decision. The comment also records why the loose form is the safe one. It is sound as a pre-filter because TOML has no other way to spell a string spanning lines: a raw newline is rejected inside single- and double-quoted strings, and a line-ending backslash is only legal within `"""`. A false positive costs one tree walk; a false negative would silently skip verification. hasTransactionCandidate() now also states that it deliberately ignores two of the planner's conditions (containers holding a comment, and whether a change lands inside one), since erring wide only wastes a verification while erring narrow would skip a needed one.
A `"""` or `'''` can sit inside a basic string or a comment without the document containing any multiline string. hasMultilineStringDelimiter() cannot tell the difference and lets those through, and hasTransactionCandidate() then finds no multiline inline container, so verification is skipped. That is the harmless direction -- a false positive costs one walk of an already-parsed CST, measured at 0.98x against origin/latest on a 45KB document, which is inside the noise. But nothing pinned the part that actually matters, which is that the output is correct either way. Three cases now do: a basic string holding three apostrophes, a comment holding three apostrophes, and a multiline literal string holding the two consecutive apostrophes TOML permits, which is genuinely multiline yet still not a candidate because it sits at the top level rather than inside an inline container.
Adds the `"""` counterparts to the delimiter-in-content cases: a comment holding three quotes, a multiline literal string holding three quotes at the top level (pre-filter passes, no transaction candidate), and the same inside a multiline inline table, which is a candidate and so exercises the full verification path. Writing those turned up a parse bug. A single-line literal string cannot hold an odd number of double quotes: a = '"' throws a = '""' ok a = '"""' throws a = '""""' ok Literal strings have no escaping, so everything between the delimiters is taken verbatim and all of these are valid TOML. The alternating pass/fail says the tokenizer is pairing double quotes while scanning a context where they carry no meaning. Multiline literal strings are unaffected, so it is specific to the single-quote scanner. It predates this work, reproduces on origin/latest and on the published 3.0.3, and the official toml-test suite passes, so that suite does not cover it. Recorded as test.fails with the two neighbouring cases that do pass, so the boundary is written down rather than rediscovered.
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
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.