Skip to content

fix(safetensors): conform JSON unicode and whitespace - #79

Merged
rmems merged 1 commit into
mainfrom
codex/linear-mention-rm-1463-fixsafetensors-rfc-8259-json-u
Sep 24, 2026
Merged

rmems merged 1 commit into
mainfrom
codex/linear-mention-rm-1463-fixsafetensors-rfc-8259-json-u

Conversation

@rmems

@rmems rmems commented Sep 22, 2026

Copy link
Copy Markdown
Owner

User description

Motivation

  • Ensure the crate's zero-dependency Safetensors JSON parser conforms to RFC 8259 by decoding UTF-16 surrogate pairs in \uXXXX escapes and limiting structural whitespace to the four characters allowed by the spec.
  • Prevent incorrect acceptance of non-RFC Unicode whitespace as structural separators and ensure duplicate-key detection occurs after escape decoding so emoji/non-BMP keys and values round-trip correctly or fail cleanly.

Description

  • Restrict structural whitespace handling by replacing c.is_whitespace() with an explicit match for ' ' | '\t' | '\n' | '\r' in skip_whitespace to match RFC 8259.
  • Add parse_escaped_unicode_scalar to combine high/low UTF-16 surrogate pairs into the correct Unicode scalar and to reject lone, reversed, truncated, or malformed surrogate sequences.
  • Use the new surrogate-aware decoding path from parse_string so \u escapes produce proper non-BMP characters and malformed escapes surface the existing JSON parse error path.
  • Add unit tests covering surrogate decoding and compact round-trip, malformed surrogate/escape rejection, duplicate-key rejection after surrogate decoding, and acceptance/rejection of RFC structural whitespace versus Unicode whitespace inside strings.

Testing

  • Ran cargo fmt -- --check, which succeeded.
  • Ran cargo test --locked --features safetensors safetensors::json::tests, and the safetensors JSON tests passed.
  • Ran cargo test --locked --features safetensors, and the safetensors feature test suite passed.
  • Ran cargo test --locked --all-features, and the full test suite completed with all tests passing.

Codex Task


Summary by cubic

Fixes the safetensors JSON parser to conform to RFC 8259 by decoding UTF-16 surrogate pairs in \u escapes and limiting structural whitespace to the four characters allowed by the spec.

  • Restricts whitespace to ' ' | '\t' | '\n' | '\r' so Unicode whitespace no longer acts as a structural separator.
  • Decodes high/low surrogate pairs into proper Unicode scalars and rejects lone, reversed, truncated, or malformed escapes.
  • Runs duplicate-key detection after escape decoding so emoji and non-BMP keys and values round-trip correctly.
  • Adds tests covering surrogate decoding, malformed escapes, duplicate-key rejection, and RFC 8259 whitespace handling.

Written for commit 29a449a. Summary will update on new commits.

Review in cubic


CodeAnt-AI Description

Make Safetensors JSON parsing conform to RFC 8259 for Unicode escapes and whitespace

What Changed

  • Escaped UTF-16 surrogate pairs now decode into the correct non-BMP characters in JSON keys and values
  • Malformed, incomplete, reversed, or standalone surrogate escapes are rejected
  • Duplicate keys are detected after escape decoding, including keys that represent the same emoji
  • Structural whitespace is limited to spaces, tabs, line feeds, and carriage returns; other Unicode whitespace is rejected outside strings
  • Added coverage for valid round trips, invalid escapes, duplicate keys, and RFC-compliant whitespace

Impact

✅ Correct emoji and other non-BMP characters in parsed JSON
✅ Fewer malformed JSON values accepted
✅ Consistent duplicate-key detection

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

Decode UTF-16 surrogate pairs in escaped strings and restrict structural whitespace to the four characters allowed by RFC 8259. Add coverage for valid round trips and malformed input.

Co-authored-by: Codex <noreply@openai.com>
@codeant-ai

codeant-ai Bot commented Sep 22, 2026

Copy link
Copy Markdown

🤖 CodeAnt AI — Review Status

Status Commit Started (UTC) Finished (UTC)
✅ Reviewed your PR 29a449a Sep 22, 2026 · 08:18 08:20

@codeant-ai

codeant-ai Bot commented Sep 22, 2026

Copy link
Copy Markdown

Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@linear-code

linear-code Bot commented Sep 22, 2026

Copy link
Copy Markdown

RM-1463

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 22, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-22T08:20:21.133231Z 29a449a PR opened
🔒 Security Review Completed 2026-09-22T08:22:31.744054Z 29a449a PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@codeant-ai codeant-ai Bot added the size:M This PR changes 30-99 lines, ignoring generated files label Sep 22, 2026
@coderabbitai

coderabbitai Bot commented Sep 22, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: e7b5e6d9-f595-4f44-b2fd-4ed7f9d49c08

📥 Commits

Reviewing files that changed from the base of the PR and between b735095 and 29a449a.

📒 Files selected for processing (1)
  • src/safetensors/json.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

📜 Recent review details
⏰ Context from checks skipped due to timeout. (3)
  • GitHub Check: Seer Code Review
  • GitHub Check: Build Docker Image (CPU-only)
  • GitHub Check: RustSec Audit
🧰 Additional context used
📓 Path-based instructions (3)
Do **not** add myelin (or CUDA) as a dependency of engram-parser — optional or not.

📄 CodeRabbit inference engine (REVIEW.md)

Files:

  • src/safetensors/json.rs
**No CUDA, dequant, mmap, or GGML compute** in this repo.

📄 CodeRabbit inference engine (REVIEW.md)

Files:

  • src/safetensors/json.rs
engram-parser never depends on either.

📄 CodeRabbit inference engine (REVIEW.md)

Files:

  • src/safetensors/json.rs
🔇 Additional comments (4)
src/safetensors/json.rs (4)

175-175: LGTM!


256-256: LGTM!


285-302: LGTM!


634-687: LGTM!


📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • JSON parsing now correctly decodes valid Unicode surrogate pairs.
    • Invalid or unmatched Unicode surrogates and malformed escapes are rejected.
    • Duplicate keys are detected consistently after Unicode decoding.
    • Whitespace handling now follows JSON standard rules, rejecting unsupported Unicode whitespace characters.

Walkthrough

The JSON parser now decodes valid UTF-16 surrogate pairs, rejects invalid Unicode escapes, limits structural whitespace to RFC 8259 characters, and adds tests for decoding, duplicate keys, round-tripping, and whitespace behavior.

Changes

JSON conformance

Layer / File(s) Summary
Unicode escape decoding
src/safetensors/json.rs
\u escapes now combine valid high and low surrogates into Unicode scalar values. Lone, reversed, truncated, and malformed surrogates are rejected. Tests cover decoding, duplicate keys, and compact round-tripping.
Structural whitespace enforcement
src/safetensors/json.rs
Structural parsing now accepts only space, tab, line feed, and carriage return. Unicode whitespace remains valid inside quoted strings and is rejected elsewhere.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 29a44

The parser now handles Unicode escapes and structural whitespace according to the stated JSON requirements, with no remaining merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main changes: RFC 8259 compliance for Safetensors JSON Unicode escapes and whitespace handling.
Description check ✅ Passed The description directly explains the parser changes, test coverage, motivation, and expected impact. It is related to the changeset.
Linked Issues check ✅ Passed RM-1463 coding requirements are met. parse_escaped_unicode_scalar combines valid high and low UTF-16 surrogates and rejects lone, reversed, truncated, and malformed escapes through the parser error …
Out of Scope Changes check ✅ Passed The reviewed change is limited to src/safetensors/json.rs. The implementation and tests directly support RM-1463 JSON conformance. No parser replacement, payload mmap work, shard identity work, rele…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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

A rabbit parsed the moonlit code
Surrogate pairs now share one road
Lone escapes hop away
Four JSON tests join the day
Whitespace keeps its proper abode
The parser rests, precise and bold

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

@rmems
rmems merged commit 74985fe into main Sep 24, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codex size:M This PR changes 30-99 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant