Tolerate lone UTF-16 surrogates in renderer JSON (#4710) by AbanoubGhadban · Pull Request #4745 · shakacode/react_on_rails

AbanoubGhadban · 2026-07-18T12:55:29Z

Problem

When the JavaScript renderer produces a string containing a lone UTF-16 surrogate, server rendering crashes. This happens most commonly from truncating text mid-emoji — e.g. an excerpt/teaser cut with slice/substring that lands inside a surrogate pair, leaving the high half orphaned. JavaScript holds this happily, and JSON.stringify serializes it as the escape \ud83d, but Ruby's JSON.parse rejects that escape:

JSON.parse('{"html":"Hello \uD83D world"}')
# => JSON::ParserError: incomplete surrogate pair

One bad character takes down the entire render. Fixes #4710.

Fix

New ReactOnRails::LenientJson parses normally and only on failure repairs lone-surrogate escapes to the Unicode replacement character (�, U+FFFD) and retries:

def parse(json)
  JSON.parse(json)          # clean payloads pay nothing
rescue JSON::ParserError => e
  repaired = repair_lone_surrogates(json)
  raise e if repaired == json   # genuinely malformed -> surface the real error
  JSON.parse(repaired)
end

Zero cost on the happy path — Ruby's rescue costs nothing until an exception is raised, and the repair scan/allocation happen only on the failure path.
Valid surrogate pairs and literal \\ud83d text are preserved (the repair is pair-aware and backslash-parity-aware).
Genuinely malformed JSON still raises its true error — the repair is a no-op when there is no surrogate to fix.
Replacing with � matches what a browser renders for broken text anyway — passing content through instead of failing, which is the behavior the issue asks for.

Wired into the three JSON.parse sites that consume renderer output: metadata and object payloads in LengthPrefixedParser, and the legacy path in ruby_embedded_java_script.

Why the json floor moves to `>= 2.17`

json < 2.17 (including 2.7.2, previously bundled) silently mangles a lone high surrogate inside the C parser without raising — so the rescue could never fire. json >= 2.17 raises incomplete surrogate pair, which is exactly what makes the repair reachable. Confirmed empirically across json 2.7.2 / 2.17.1 / 2.20.0.

Known limitation (documented in the code)

A lone low surrogate does not make JSON.parse raise on any json version — Ruby accepts it and returns invalid UTF-8 — so the rescue does not fire for it. That case comes from the rarer prefix-drop path; the common source (end-truncation) yields a lone high surrogate, which raises and is handled. Catching the low case would require inspecting every successful parse result, a per-request cost this deliberately avoids.

Testing

New lenient_json_spec.rb: clean JSON unchanged, valid astral char untouched, lone high / lone-in-key / reversed-pair repaired, valid escape-pair preserved, literal backslash text untouched, malformed JSON re-raises.
Integration cases in length_prefixed_parser_spec.rb: lone surrogate in metadata and in an object payload no longer fail the render.
All 53 affected specs pass; RuboCop clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Fixed a server-side rendering failure when renderer output contains invalid lone UTF-16 surrogate characters.
- Renderer output is now automatically repaired by replacing the problematic characters with the Unicode replacement character, letting rendering continue.
- Improved behavior applies to both standard and length-prefixed rendering responses, with encoding preserved.
Tests
- Added coverage for lone-surrogate cases in both chunk metadata and JSON payload parsing, including new lenient parsing/repair scenarios.

coderabbitai · 2026-07-18T12:56:03Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d774590f-8d85-4153-85c9-d0e884713ff6

📥 Commits

Reviewing files that changed from the base of the PR and between 7f387a9 and 008e931.

📒 Files selected for processing (2)

react_on_rails/lib/react_on_rails/lenient_json.rb
react_on_rails/spec/lib/react_on_rails/lenient_json_spec.rb

💤 Files with no reviewable changes (1)

react_on_rails/spec/lib/react_on_rails/lenient_json_spec.rb

Walkthrough

Adds LenientJson repair-and-retry parsing for lone UTF-16 surrogate escapes and uses it across legacy and length-prefixed server-rendering JSON paths. Tests cover surrogate repair, valid JSON preservation, malformed input, and UTF-8 encoding.

Changes

Surrogate-tolerant server rendering

Layer / File(s)	Summary
Lenient JSON parser and coverage `react_on_rails/lib/react_on_rails/lenient_json.rb`, `react_on_rails/spec/lib/react_on_rails/lenient_json_spec.rb`	Adds surrogate detection, repair, and retry behavior while preserving valid pairs, escaped text, raw astral characters, and genuine parse errors.
Render payload parsing integration `react_on_rails/lib/react_on_rails/length_prefixed_parser.rb`, `react_on_rails/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb`, `react_on_rails/spec/lib/react_on_rails/length_prefixed_parser_spec.rb`, `CHANGELOG.md`	Applies lenient parsing to metadata, object payloads, and legacy render results; adds integration coverage and documents the fix.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant JavaScriptRenderer
  participant RubyEmbeddedJavaScript
  participant LengthPrefixedParser
  participant LenientJson
  JavaScriptRenderer->>RubyEmbeddedJavaScript: emit legacy render result JSON
  RubyEmbeddedJavaScript->>LenientJson: parse render result
  JavaScriptRenderer->>LengthPrefixedParser: emit length-prefixed chunks
  LengthPrefixedParser->>LenientJson: parse metadata or object payload
  LenientJson-->>RubyEmbeddedJavaScript: return repaired parsed result
  LenientJson-->>LengthPrefixedParser: return repaired parsed payload

Possibly related PRs

shakacode/react_on_rails#4726: Updates the same SSR JSON parsing paths for lone UTF-16 surrogate escapes.

Suggested labels: ready-for-hosted-ci

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately summarizes the main change to tolerate lone UTF-16 surrogates in renderer JSON.
Linked Issues check	✅ Passed	The parser changes cover the known metadata, object payload, and legacy fallback JSON.parse paths and repair the surrogate cases described.
Out of Scope Changes check	✅ Passed	The added module, parser updates, tests, and changelog entry all support the surrogate-repair fix; no unrelated changes are evident.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch 4710-investigate-json-parse-failures-from-javascript

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

greptile-apps · 2026-07-18T12:57:43Z

Greptile Summary

This PR makes renderer JSON parsing tolerant of lone UTF-16 surrogate escapes. The main changes are:

Adds a lenient JSON parser that repairs surrogate escapes after parse failures.
Uses the parser for metadata, object payloads, and legacy renderer results.
Raises the minimum json gem version to 2.17.
Adds unit and integration coverage for high-surrogate inputs.

Confidence Score: 4/5

Low-surrogate renderer output and stale Pro lockfiles need fixes before merging.

Lone low surrogates can still propagate invalid UTF-8 into rendering and serialization.
Two checked-in Pro dummy lockfiles conflict with the new json dependency floor.
High-surrogate repair, backslash handling, and the production load paths are otherwise consistent.

react_on_rails/lib/react_on_rails/lenient_json.rb and the Pro dummy lockfiles

Important Files Changed

Filename	Overview
react_on_rails/lib/react_on_rails/lenient_json.rb	Adds surrogate-aware retry parsing, but accepted lone low surrogates still escape as invalid UTF-8.
react_on_rails/lib/react_on_rails/length_prefixed_parser.rb	Routes metadata and object payload parsing through LenientJson while preserving ParseError translation.
react_on_rails/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb	Routes legacy renderer results through LenientJson with an explicit load dependency.
react_on_rails/react_on_rails.gemspec	Adds the json 2.17 dependency floor without updating all checked-in consumer lockfiles.
react_on_rails/spec/lib/react_on_rails/lenient_json_spec.rb	Covers high surrogates, valid and reversed pairs, escaped text, and malformed JSON, but not accepted low surrogates.

_{Reviews (1): Last reviewed commit: "Tolerate lone UTF-16 surrogates in rende..." | Re-trigger Greptile}

coderabbitai

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CHANGELOG.md`:
- Around line 40-48: Update the changelog entry for Issue 4710 to append the
repository-standard PR link and author attribution in the required “[PR …] by
[author]” format, preserving the existing description and issue link.

In `@react_on_rails/lib/react_on_rails/lenient_json.rb`:
- Around line 30-38: Update the successful-parse path in LenientJSON to inspect
inputs containing "\u" escapes for standalone low surrogates, repair them to
U+FFFD, and reparse only when the repaired input differs from the original.
Preserve existing handling for valid pairs and lone high surrogates, and add a
regression test covering a standalone low-surrogate escape.
- Around line 69-75: Update the rescue flow around repair_lone_surrogates so
JSON.parse(repaired) errors are rescued and the original JSON::ParserError is
re-raised, preserving its message and backtrace when repaired JSON remains
malformed. Extend the malformed-surrogate spec to assert the original error
message and backtrace in addition to its class.

In `@react_on_rails/spec/lib/react_on_rails/lenient_json_spec.rb`:
- Around line 3-6: Move the LenientJson unit suite from
react_on_rails/spec/lib/react_on_rails/lenient_json_spec.rb to
react_on_rails/spec/react_on_rails/, preserving its existing coverage and
requires. Relocate the LengthPrefixedParser unit coverage at
react_on_rails/spec/lib/react_on_rails/length_prefixed_parser_spec.rb:132-151 to
a suite under react_on_rails/spec/react_on_rails/, moving the existing suite if
appropriate; both sites require relocation only, with no behavioral changes.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7e1592bb-bf0a-45df-817f-718454f4b214

📥 Commits

Reviewing files that changed from the base of the PR and between 5f6e66d and d92d685.

⛔ Files ignored due to path filters (2)

react_on_rails/Gemfile.lock is excluded by !**/*.lock
react_on_rails/spec/dummy/Gemfile.lock is excluded by !**/*.lock

📒 Files selected for processing (7)

CHANGELOG.md
react_on_rails/lib/react_on_rails/length_prefixed_parser.rb
react_on_rails/lib/react_on_rails/lenient_json.rb
react_on_rails/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb
react_on_rails/react_on_rails.gemspec
react_on_rails/spec/lib/react_on_rails/length_prefixed_parser_spec.rb
react_on_rails/spec/lib/react_on_rails/lenient_json_spec.rb

The JS renderer can emit strings containing a lone UTF-16 surrogate -- most commonly from truncating text mid-emoji (e.g. an excerpt cut with slice/substring, which leaves the high half of a surrogate pair). JSON serializes it as a \uXXXX escape that Ruby's JSON.parse can reject with "incomplete surrogate pair", failing the entire server render over one bad character. Add ReactOnRails::LenientJson, which parses normally and only on failure repairs lone-surrogate escapes to the Unicode replacement character (U+FFFD) and retries. Clean payloads pay nothing -- the rescue costs nothing until an exception is raised, and the repair scan/allocation happen only on the failure path. Valid surrogate pairs and literal backslash text are preserved. Wire it into the three JSON.parse sites that consume renderer output: metadata and object payloads in LengthPrefixedParser, and the legacy JSON path in ruby_embedded_java_script. The repair engages wherever JSON.parse raises. Two cases it does not cover, both documented in LenientJson: a lone *low* surrogate never raises on any json version, and on json < 2.17 a lone *high* surrogate may be silently degraded by the C parser instead of raising -- but neither case crashes, so nothing regresses on older json. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

AbanoubGhadban · 2026-07-18T13:35:56Z

Thanks for the review. Triage of the four comments:

CHANGELOG PR link + author attribution — ✅ Applied (added [PR 4745] by [AbanoubGhadban] in the standard format).
Repair lone low surrogates on the success path — Skipping intentionally. A lone low surrogate never makes JSON.parse raise, so catching it requires inspecting every successful parse result, which imposes a per-request cost on clean payloads. That cost was explicitly considered and declined for this fix; the limitation is documented in LenientJson. The common real-world source (end-truncation) yields a lone high surrogate, which does raise and is handled.
Re-raise the original error when repaired JSON is still malformed — Skipping. When the repaired JSON still fails, the failure is about something other than the surrogate we already fixed; propagating that second error describes the actual remaining problem, whereas re-raising the original "incomplete surrogate pair" would be misleading (we did handle the surrogate). Both call sites wrap this error anyway (ParseError / JsonParseError with the original json attached), so the inner message is not surfaced raw.
Move the spec directory — Skipping. The new lenient_json_spec.rb sits beside the existing length_prefixed_parser_spec.rb under spec/lib/react_on_rails/, matching the file it extends. Relocating pre-existing specs is out of scope for this fix.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aee33d4c41

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

claude · 2026-07-18T13:40:53Z

Review Summary

What this PR does: Adds ReactOnRails::LenientJson, a wrapper around JSON.parse that catches JSON::ParserError, repairs lone UTF-16 surrogate escapes (e.g. \ud83d with no matching low half — typically from slice/substring truncating a JS string mid-emoji) by replacing them with U+FFFD, and retries. Wired into the three call sites that parse renderer output: LengthPrefixedParser's metadata/object-payload parsing, and the legacy JSON path in ruby_embedded_java_script.rb.

Strengths

Good root-cause fix for Investigate JSON.parse failures from JavaScript/Ruby Unicode handling differences #4710, with a genuinely zero-cost happy path (rescue only pays when an exception fires).
The repair regex is thoughtful: backslash-parity lookbehind correctly avoids mangling literal \\ud83d text, and it preserves genuine valid surrogate pairs written as escapes. I traced it through several edge cases (reversed pairs, pairs split across positions, literal double-backslash text) and it behaves correctly.
Solid, focused unit tests for .repair_lone_surrogates that assert exact output — these do verify the regex logic.
Docs in the module are unusually thorough about the known limitations (lone low surrogates, and json-gem-version dependence).
Wrapping call sites already had rescue JSON::ParserError / rescue StandardError, so even if the second, non-rescued JSON.parse(repaired) call re-raises (genuinely malformed JSON that also happens to contain a surrogate escape), it's still caught correctly upstream.

Main issue: the stated `json >= 2.17` floor isn't enforced, and CI doesn't exercise the new path

Posted as an inline comment on CHANGELOG.md, but summarizing here since it affects how much confidence to place in this PR's test coverage: neither the gemspec nor any lockfile is updated to require json >= 2.17, and react_on_rails/Gemfile.lock — the lockfile used when running this gem's own spec suite — still pins json (2.7.2). By the PR description's own account, json < 2.17 doesn't raise on a lone high surrogate (it silently degrades instead), so:

The .parse/integration-level specs (lenient_json_spec.rb's .parse block and the two new length_prefixed_parser_spec.rb cases) likely pass in this repo's actual CI regardless of whether the new rescue-and-repair logic ever fires, since the underlying JSON.parse probably doesn't raise in the first place under json 2.7.2.
End users who don't independently bump json past 2.17 get no crash protection from this fix, with no gemspec constraint steering them there.

Recommend adding an explicit json dependency floor to the gemspec (and bumping the relevant lockfiles) so the fix is both enforced for consumers and meaningfully exercised in CI.

Minor

repair_lone_surrogates scans the entire payload on every JSON::ParserError, not just surrogate-related ones, and the backtracking in (?:\\\\)* combined with gsub's per-position retry is O(n²) worst case for pathological backslash runs. Likely fine given this is an already-rare error path with normally-bounded payloads, but flagged in case very large SSR payloads can reach here. (Inline comment on lenient_json.rb.)

Overall this is a well-reasoned, well-documented fix for a real crash; the main thing blocking full confidence is that the version floor it depends on isn't actually wired up anywhere in the repo.

…review) Address review feedback: rescuing JSON::ParserError alone missed three real cases where JSON.parse succeeds but returns wrong/invalid text: - a lone LOW surrogate is accepted and returns invalid UTF-8 (greptile, CodeRabbit) - a lone HIGH surrogate before a non-low escape (e.g. an escaped control char) is silently mis-decoded into a bogus astral character (Codex) - on json < 2.17, a lone HIGH surrogate is silently degraded to "?" Repair now runs on the success path too, gated by a single cheap backslash-u-d substring check so clean payloads pay only ~170ns (metadata) and nothing scans the large HTML string-payload, which is never JSON-parsed. Results are now consistent across json 2.7.2 and 2.21: every lone surrogate becomes U+FFFD, valid pairs kept. Also document the valid-UTF-8 input assumption (input is JSON.stringify output, which is well-formed UTF-16 and always encodes to valid UTF-8), and add regression tests for the lone-low, escaped-control, and no-surrogate fast-path cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

AbanoubGhadban · 2026-07-18T14:20:28Z

Review addressed (commit `2baaa8e`)

Fixed (real issues):

Lone low surrogates & mis-decoded escapes (greptile, CodeRabbit, Codex): rescuing JSON::ParserError alone missed three cases where JSON.parse succeeds but returns wrong/invalid text — a lone low surrogate (invalid UTF-8), a lone high before a non-low escape such as an escaped control char (silently mis-decoded to a bogus astral character), and older-json lone-high degraded to ?. Repair now runs on the success path too, gated by a single cheap \ud substring check. All lone surrogates now normalize to U+FFFD consistently across json 2.7.2 and 2.21.1. Clean payloads pay ~170 ns (metadata); the large HTML string-payload is never JSON-parsed, so it isn't scanned.
CHANGELOG attribution (CodeRabbit): added [PR 4745] by [AbanoubGhadban].
Wasted gsub on non-surrogate malformed JSON (claude): both paths now short-circuit via the \ud gate before any repair scan.
Input assumption documented: repair_lone_surrogates is safe because its input is JSON.stringify output (well-formed UTF-16 → always valid UTF-8); the raw-invalid-bytes → ArgumentError case is out of scope by construction and now documented.

Stale (resolved by the earlier json-floor revert):

greptile "Pro Lockfiles Violate Floor" and claude "json floor isn't enforced" — the json >= 2.17 floor was reverted; gemspec/lockfiles are back to prior state and the fix works without a floor.

Kept with rationale (replied inline):

Re-raise original error when repaired JSON is still malformed (CodeRabbit) — propagating the actual remaining error is more accurate, and callers wrap it anyway.
Move specs to spec/react_on_rails/ (CodeRabbit) — placed beside the sibling length_prefixed_parser_spec.rb; relocating existing specs is out of scope.
O(n²) backtracking (claude) — not reproduced; repair is linear (50k backslashes = 0.07 ms), only on the failure path.

Tests pass on json 2.7.2 and 2.21.1; RuboCop clean.

claude · 2026-07-18T14:26:46Z

Code Review

Overview

This PR fixes server-render crashes caused by lone UTF-16 surrogates in JS-renderer JSON output (#4710). It adds ReactOnRails::LenientJson, wired into the three JSON.parse call sites that consume renderer output, and repairs lone-surrogate escapes to U+FFFD before re-parsing.

Strengths

The core regex (LONE_SURROGATE_ESCAPE) is carefully constructed — backslash-parity-aware (distinguishes a real \uXXXX escape from literal text like \\ud83d), and pair-aware (preserves valid high+low surrogate pairs written as escapes). I traced several tricky cases by hand (escaped-backslash-then-escape, reversed pairs, consecutive lone highs, high-before-non-low-escape) and the logic holds up.
Good test coverage in lenient_json_spec.rb for the regex itself and the integration behavior, plus new integration cases in length_prefixed_parser_spec.rb.
Correctly scoped: only wired into the 3 actual renderer-output parse sites, not a global monkeypatch.
The implementation is actually more robust than earlier JSON.parse-only designs would be, because it also inspects the raw text on the success path (surrogate_escape? check) to catch cases that don't raise at all (lone low surrogate, lone high before a non-low escape, older-json silent mangling) — see below though, this isn't reflected in the docs.

Issues found (left as inline comments)

PR description / CHANGELOG don't match the shipped implementation (on CHANGELOG.md): the CHANGELOG says "the repair only runs after a parse error" and that the fix "applies on json gem versions ... >= 2.17" with lone-low-surrogates as a documented unhandled limitation. None of that matches the actual code — LenientJson.parse also repairs on the success path (confirmed by the passing "lone low (parses without raising)" spec), and no json-gem floor was actually added anywhere (no gemspec/Gemfile change; react_on_rails/Gemfile.lock still has json (2.7.2)). This looks like stale text from an earlier design iteration — worth reconciling before merge so it doesn't mislead future readers about version requirements or coverage.
Possible ArgumentError leak on invalid-UTF-8 input (on length_prefixed_parser.rb): repair_lone_surrogates does a UTF-8 regex scan whenever the raw text contains \ud. If the original JSON::ParserError was actually caused by invalid encoding (not just the surrogate issue) and the payload also contains a literal \ud substring, the regex scan can raise ArgumentError instead — a class not covered by the rescue JSON::ParserError at the call sites in length_prefixed_parser.rb, and not covered at all in the Pro streaming path (react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb#process_response_chunks, which has no broad rescue around chunk parsing). This narrows an existing safety net in a corner case. Worth rescuing ArgumentError alongside JSON::ParserError in LenientJson.parse, or validating encoding before the regex scan.

Minor / non-blocking

The "valid pair" branch in repair_lone_surrogates reconstructs matched text by hand even though it's provably identical to the original substring — harmless, just a bit of unnecessary allocation on that path.
gh pr diff (and presumably GitHub's diff view) renders lenient_json.rb as a binary file rather than text, which is why I couldn't leave an inline comment directly on it — likely due to some byte sequence in the file (possibly related to the literal Unicode characters used in comments/constants). Worth a quick sanity check that this isn't causing encoding issues in other tooling (e.g. git diff/blame/syntax highlighting) — not something I could fully verify without invasive commands in this sandbox.

Overall this is a well-designed, well-tested fix for a real crash. The main ask is reconciling the CHANGELOG/PR description with what's actually implemented, plus a look at the ArgumentError edge case.

…d 2) Address claude[bot] review of the success-path change: - repair_lone_surrogates now scans a binary copy and restores the original encoding, so a genuinely invalid-UTF-8 payload can no longer turn the repair into an ArgumentError that a caller's `rescue JSON::ParserError` would miss (the Pro streaming caller has no broad rescue). Detection already used a byte-safe `include?`. Invalid bytes pass through and resurface as an ordinary JSON::ParserError. Out of scope for JSON.stringify output (always valid UTF-8), but cheap and strictly safer; added a regression test. - Correct the CHANGELOG entry: it described the earlier "repair only runs after a parse error" / ">= 2.17 floor" design. The shipped code repairs on the success path too and works the same across json versions; the entry now says so. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

claude · 2026-07-18T14:40:59Z

Review: Tolerate lone UTF-16 surrogates in renderer JSON (#4710)

Overall this is a well-designed, carefully-reasoned fix — I traced the LONE_SURROGATE_ESCAPE regex and the parse/repair_lone_surrogates logic against every documented edge case (valid pairs, reversed pairs, escaped-backslash-then-literal-text, malformed non-surrogate escapes, keys vs. values) and the logic holds up correctly in each case, including ones not in the test suite (e.g. \udev, 3/4-backslash prefixes).

🔴 Confirmed bug: literal NUL byte embedded in `lenient_json.rb`

react_on_rails/lib/react_on_rails/lenient_json.rb line 25 contains a raw NUL byte (0x00) inside a comment, right before the closing quote in "\ud83d "):

$ grep -naP '\x00' react_on_rails/lib/react_on_rails/lenient_json.rb
25:  #   * a lone HIGH followed by a non-low escape (e.g. "\ud83d ") is *silently mis-decoded*

$ sed -n '25p' react_on_rails/lib/react_on_rails/lenient_json.rb | od -An -tx1 | tail -2
 20 28 65 2e 67 2e 20 22 5c 75 64 38 33 64 00 22
 29 20 69 73 20 2a 73 69 6c 65 6e 74 6c 79 20 6d

That 00 where a space was clearly intended is why gh pr diff 4745 shows this entire new file as Binary files /dev/null and b/.../lenient_json.rb differ instead of a normal text diff — Git/GitHub is treating the file as binary because of the embedded NUL. Practically this means:

The file's diff isn't reviewable in the normal GitHub PR UI (no line-by-line view, no inline comments possible on it — confirmed: the inline-comment API rejects any line in this file with "could not be resolved").
Any other NUL-sensitive tooling (blame, some editors/linters) may also mishandle the file.

Ruby will still load the file fine (NUL is legal inside a comment), so tests pass and RuboCop likely doesn't flag it, but this should be fixed before merge — just remove the stray byte and retype the space.

🟡 PR description appears stale relative to the shipped code

Two claims in the PR description don't match the diff/implementation:

"Why the json floor moves to >= 2.17" — the diff contains no gemspec or Gemfile.lock change. react_on_rails.gemspec has no json dependency constraint at all, and Gemfile.lock still pins json (2.7.2). If the intent was really to bump the floor, that commit is missing from this PR.
Relatedly, the "Known limitation" section says lone low surrogates aren't handled ("Catching the low case would require inspecting every successful parse result, a per-request cost this deliberately avoids"). But the actual shipped code in lenient_json.rb#parse does re-scan every successful-parse result via the cheap surrogate_escape? gate and repairs lone low surrogates too — this is exactly what the "lone low (parses without raising)" case in lenient_json_spec.rb tests and expects to pass.

It looks like the description was written against an earlier revision (rescue-only) and not updated after the success-path surrogate_escape? gate was added, which (nicely) makes the fix version-independent and removes the need for the json floor bump. Worth reconciling the PR body/CHANGELOG-adjacent claims with what actually shipped so reviewers/maintainers aren't misled about the version requirement or the limitation.

Minor nit

LenientJson.parse's success path (JSON.parse succeeds, then re-check/repair) and rescue path duplicate the same "check → repair → bail-if-unchanged → reparse" sequence. Not a bug, but could be factored into one helper to avoid future drift between the two branches.

Security / correctness

No injection risk: the regex only ever swaps a well-formed 4-hex-digit lone-surrogate escape for a single literal U+FFFD character, which can't introduce an unescaped quote/backslash or otherwise break out of the JSON string. Malformed-but-surrogate-looking escapes (e.g. \udev, too few hex digits) correctly fail the regex and fall through to re-raising the original JSON::ParserError, matching the PR's stated guarantee.

Test coverage

Thorough — clean payloads, valid pairs, reversed pairs, backslash-parity, keys, invalid-UTF-8 defensive cases, and both length-prefixed integration paths (metadata + object payload) are all covered.

…4710) The binary-copy scan guarded an ArgumentError that cannot occur: input to these parse sites is always JSON.stringify output, which is well-formed UTF-16 and thus always valid UTF-8, so the repair regex never sees invalid bytes. The defensive conversion added complexity for an unreachable case, so revert to the plain gsub and keep the documented input assumption. All real fixes from this round (lone-low, escaped-control mis-decode, CHANGELOG correction) are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

chatgpt-codex-connector

💡 Codex Review

react_on_rails/react_on_rails/lib/react_on_rails/lenient_json.rb

Line 25 in 008e931

    
           #   * a lone HIGH followed by a non-low escape (e.g. "\ud83d") is *silently mis-decoded*

Remove the embedded NUL from the source comment

This example contains a literal NUL byte rather than the printable sequence \u0000, causing Git to classify the entire new Ruby file as binary in normal diffs. Future changes to this parser will therefore be hidden from ordinary textual diffs, inline review, and text-oriented tooling; spell the example as \u0000 instead.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

claude · 2026-07-18T15:03:16Z

Review Summary

This PR adds ReactOnRails::LenientJson, a wrapper around JSON.parse that repairs lone UTF-16 surrogate escapes in renderer output, replacing them with U+FFFD instead of crashing the render. It is wired into the three JSON.parse call sites that consume Node-renderer output (length_prefixed_parser.rb metadata + object payload, and the legacy ruby_embedded_java_script.rb path). Good test coverage in lenient_json_spec.rb and length_prefixed_parser_spec.rb.

Bug: stray raw NUL byte in the new file

react_on_rails/lib/react_on_rails/lenient_json.rb contains a literal NUL (0x00) byte on line 25, inside the doc comment describing the "silently mis-decoded" case, right before the closing quote of the example escape text shown there (it appears to have replaced what was meant to be a space character).

I confirmed this at the byte level with git cat-file blob piped through a NUL-searching regex, and by dumping the raw bytes around that offset with od -c: the byte sequence there is literally an open-paren, "e.g.", a quote, the six characters of the escape example, then a raw NUL byte, then the closing quote and paren.

The practical impact: file classifies this file as "data" (binary) rather than text, and as a direct consequence gh pr diff renders this entire file as "Binary files differ" instead of showing the actual diff -- the added code is invisible in the PR diff view. git blame/grep need the -a flag to treat it as text, and it may render oddly in some editors or syntax highlighters. GitHub's review-comment API also refused to let me anchor an inline comment to this file for the same reason.

It sits inside a comment, so Ruby itself parses the file fine and the specs pass -- but this is clearly an unintended artifact and should be removed before merge. Leaving it in permanently marks this file as binary in git history and tooling, and hides its diffs from reviewers on every future PR that touches it.

Description vs. implementation mismatch

The PR description's "Why the json floor moves to >= 2.17" and "Known limitation" sections do not quite match the shipped code:

The description says a lone low surrogate never makes JSON.parse raise, so "the rescue does not fire for it," and frames this as a known, deliberately-accepted gap to avoid a per-request cost.
But the actual LenientJson.parse implementation re-checks for a surrogate escape on the successful-parse path too, not only inside the rescue block -- so it already repairs lone-low-surrogate cases today (this is exactly what the "lone low (parses without raising)" spec exercises and expects to pass). The per-request cost the description says is avoided is in fact paid on every parse, success or failure.
Relatedly, no Gemfile.lock or gemspec change in this diff actually raises a json gem floor to >= 2.17 -- the "floor moves" framing in the title and description is not backed by any dependency constraint change in this PR. Since the code already repairs successful-but-silently-corrupt parses regardless of json gem version, it is worth double-checking whether that framing is still accurate and updating the PR description and CHANGELOG wording if it is stale relative to the final implementation.

Minor / non-blocking

LenientJson.parse has near-duplicate repair-or-raise logic between the success path and the rescue branch. Could be factored into a small private helper to keep the two paths from drifting, though it is easy enough to follow as-is.
The cheap gate that decides whether to attempt a repair only checks for the lowercase escape prefix (documented as safe given JSON.stringify always lowercases hex), while the repair regex still matches uppercase hex surrogate escapes. That branch of the regex is effectively dead code under the current gate -- harmless, but slightly confusing for future maintainers if LenientJson is ever reused for a non-JSON.stringify input source.

What looks solid

Zero-cost happy path is real: JSON.parse is tried first, and the cheap substring gate means clean payloads pay only a linear scan, no regex or allocation.
The backslash-parity-aware repair regex (distinguishing a real unicode escape from literal backslash-then-letters text) is subtle but well tested -- the "literal backslash text" and "valid pair preserved" specs specifically pin this down.
Error semantics are preserved: genuinely malformed JSON still raises through to the existing ParseError/JsonParseError wrapping at each call site, so no behavioral regression there.
Good integration test coverage at the length_prefixed_parser_spec.rb level (metadata and object-payload lone-surrogate cases), in addition to the unit-level lenient_json_spec.rb.

AbanoubGhadban · 2026-07-18T15:06:04Z

Review loop converged (commit `008e931`)

All review threads addressed and resolved. Latest re-review raised no new comments.

Round-2 outcome:

Fixed the CHANGELOG to match the shipped implementation (repair runs on the success path too; works the same across json versions — the >= 2.17 framing was from an earlier design).
Reverted the byte-safe (binary-copy) repair: it guarded an ArgumentError that is unreachable here, since input is always JSON.stringify output (well-formed UTF-16 → valid UTF-8, verified across all lone-surrogate configurations). Kept the code minimal with the valid-UTF-8 assumption documented in LenientJson.

Real fixes retained from this PR: lone-high (raise), lone-low (silently invalid UTF-8), lone-high-before-non-low-escape (silently mis-decoded), and older-json lone-high (silent ?) — all now normalize to U+FFFD consistently across json 2.7.2 and 2.21.1, gated by a single cheap substring check so clean payloads pay ~170 ns.

All checks green except required-pr-gate, which needs a maintainer +ci-run-hosted to run the hosted suite. Tests pass on json 2.7.2 and 2.21.1; RuboCop and Prettier clean.

greptile-apps Bot reviewed Jul 18, 2026

View reviewed changes

Comment thread react_on_rails/lib/react_on_rails/lenient_json.rb Outdated

Comment thread react_on_rails/react_on_rails.gemspec Outdated

coderabbitai Bot requested changes Jul 18, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

Comment thread react_on_rails/lib/react_on_rails/lenient_json.rb Outdated

Comment thread react_on_rails/lib/react_on_rails/lenient_json.rb

Comment thread react_on_rails/spec/lib/react_on_rails/lenient_json_spec.rb

AbanoubGhadban force-pushed the 4710-investigate-json-parse-failures-from-javascript branch from d92d685 to aee33d4 Compare July 18, 2026 13:35

Uh oh!

Uh oh!

Conversation

AbanoubGhadban commented Jul 18, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Why the json floor moves to >= 2.17

Known limitation (documented in the code)

Testing

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

greptile-apps Bot commented Jul 18, 2026

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AbanoubGhadban commented Jul 18, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Jul 18, 2026

Review Summary

Strengths

Main issue: the stated json >= 2.17 floor isn't enforced, and CI doesn't exercise the new path

Minor

Uh oh!

AbanoubGhadban commented Jul 18, 2026

Review addressed (commit 2baaa8e)

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Jul 18, 2026

Code Review

Overview

Strengths

Issues found (left as inline comments)

Minor / non-blocking

Uh oh!

claude Bot commented Jul 18, 2026

Review: Tolerate lone UTF-16 surrogates in renderer JSON (#4710)

🔴 Confirmed bug: literal NUL byte embedded in lenient_json.rb

🟡 PR description appears stale relative to the shipped code

Minor nit

Security / correctness

Test coverage

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

claude Bot commented Jul 18, 2026

Review Summary

Bug: stray raw NUL byte in the new file

Description vs. implementation mismatch

Minor / non-blocking

What looks solid

Uh oh!

AbanoubGhadban commented Jul 18, 2026 •

edited by coderabbitai Bot

Loading

Why the json floor moves to `>= 2.17`

coderabbitai Bot commented Jul 18, 2026 •

edited

Loading

Main issue: the stated `json >= 2.17` floor isn't enforced, and CI doesn't exercise the new path

Review addressed (commit `2baaa8e`)

🔴 Confirmed bug: literal NUL byte embedded in `lenient_json.rb`

Review loop converged (commit `008e931`)