Skip to content

Structural diff fails for a file that contains U+2028 or U+2029 #624

Closed

Description

@simon-lentz

Structural diff fails for a file that contains U+2028 or U+2029

What happens

The structural diff stops with Malformed diffr protocol record. when a changed file contains U+2028 (LINE SEPARATOR) or U+2029 (PARAGRAPH SEPARATOR). The whole comparison stops, not only the file that contains the character.

Cause

diffr writes its output as NDJSON, one JSON record per line. In NDJSON, only the byte 0x0A ends a record. JSON lets U+2028 and U+2029 appear unescaped inside strings. diffr 0.1.3 writes them unescaped.

structuralDiff in packages/review/src/server/structural-diff.ts splits the diffr output into records with node:readline. Since Node 24.0.0, readline ends a line at U+2028 and at U+2029, and also at \n. As a result, one record becomes several fragments, and JSON.parse rejects the first fragment.

Node 22.17.1 and Node 23.11.1 do not end a line at these two characters. The repository requires Node 24 (engines.node is >=24 <25). Node 24.0.0, Node 24.18.0, and Node 24.21.0 all have the fault.

Reproduce

In a Git repository, commit a file that contains U+2028 inside a string. Then open the structural diff for that commit.

You can see the unescaped character in the diffr output without a commit. In a checkout, pnpm --filter @dev.fast/review ensure:diffr puts diffr at packages/review/bin/diffr. Put diffr and Node on your PATH, and run these commands in an empty directory:

printf 'export const s = [];\n' > a.ts
node -e 'require("fs").writeFileSync("b.ts", "export const s = [\"a\u2028b\"];\n")'
diffr --no-index --format ndjson --stream-annotations a.ts b.ts | grep -c $'\xe2\x80\xa8'

The last command prints 1: one record holds the UTF-8 bytes of U+2028. If diffr escapes the character as \u2028, the command prints 0.

This script shows the fault in readline alone. Run it with Node 24, because Node 23 and Node 22 print 1:

node --input-type=module -e '
import { Readable } from "node:stream";
import { createInterface } from "node:readline";
const record = JSON.stringify({ text: "a\u2028b\u2029c" });
const lines = [];
for await (const line of createInterface({ input: Readable.from([record + "\n"]), crlfDelay: Infinity })) lines.push(line);
console.log(lines.length);'

On Node 24, the script prints 3. The correct result is 1.

Proposed fix

  • Add an NdjsonFramer to @dev.fast/review-protocol. It splits the byte stream at 0x0A only. It decodes each record after the split, so a UTF-8 character that crosses two chunks stays whole. It keeps the 64 MiB limit for each record.
  • Use NdjsonFramer in structuralDiff in place of readline.
  • Use NdjsonFramer in ReviewApiClient.watch and in the desktop StructuralDiffClient too. These two clients split at \n after they decode, so they do not have this fault. The change replaces three copies of the framing code with one.
  • Add ndjson.ts to MODULE_PATHS in packages/review-protocol/scripts/generate-native-source.mjs. The Code OSS source of the desktop app does not import @dev.fast/review-protocol. It gets NdjsonFramer from the generated file apps/review-desktop/code-oss/src/vs/review/common/reviewProtocol.ts, which Git ignores. After the change, run pnpm --filter @dev.fast/review-desktop protocol:sync to regenerate this file.

Environment

  • Node 24.21.0
  • diffr 0.1.3 (aarch64-apple-darwin)

Activity

  1. simon-lentz commented on Oct 2, 2026

    @simon-lentz
    Author

    Still reproduces on main (2f96836, 2 October)

    • diffr: main now bundles diffr 0.1.8 (Bump bundled diffr to 0.1.8 #818). diffr 0.1.8 still writes U+2028 and U+2029 unescaped.
    • Node: on Node 24.21.0, the readline script above still prints 3.
    • Host: structuralDiff still splits the diffr output with node:readline.

    Result: with real diffr 0.1.8 output for a file that contains either character, structuralDiff stops after the start event with Malformed diffr protocol record. The same run for a file without these characters completes.

    #632 is rebased onto 2f96836 and corrects this.

  2. sidkmenon commented on Oct 7, 2026

    @sidkmenon
    Contributor

    hey, this should be fixed on the latest release (devdotfast/diffr@d49a3ea + #965)

    thank you for the issue! closing for now; feel free to reopen if you notice something else :)

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

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions