fix(platform-value): reject lossy CBOR integer conversions#4146
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesCBOR integer validation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
…undaries Round-trip I128 boundary values through to_cbor_buffer and back to pin the serialized encoding at the edges of the native CBOR integer range, and move the u64::MAX accept case into the u128 in-range test so the reject test only covers rejection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
✅ Final review complete — no blockers (commit 9ce1e94) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #4146 +/- ##
============================================
+ Coverage 87.43% 87.44% +0.01%
============================================
Files 2646 2648 +2
Lines 333981 334222 +241
============================================
+ Hits 292023 292273 +250
+ Misses 41958 41949 -9
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR fixes a real correctness bug where U128/I128 platform values outside ciborium's native CBOR integer range were silently truncated via as u64/as i64 casts, causing distinct values to collide or flip sign on decode. The fix replaces these with checked Integer::try_from conversions that I verified against ciborium 0.2.2's actual TryFrom/TryFrom source match the native range exactly, failing closed with the crate's existing Error::IntegerSizeError pattern. All 38 relevant tests pass, clippy is clean with warnings denied, and the new boundary/rejection/nested-container/round-trip tests correctly exercise the fix. Only a minor test-code duplication nitpick was found; no blocking issues.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— security-auditor (completed),claude-sonnet-5— rust-quality (completed)
💬 1 nitpick(s)
| @@ -535,6 +593,23 @@ mod tests { | |||
| assert!(!buf.is_empty()); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn to_cbor_buffer_roundtrips_native_cbor_integer_boundaries() { | |||
| const MIN_NATIVE_CBOR_INTEGER: i128 = -(u64::MAX as i128) - 1; | |||
There was a problem hiding this comment.
💬 Nitpick: MIN_NATIVE_CBOR_INTEGER boundary constant duplicated across three test functions
const MIN_NATIVE_CBOR_INTEGER: i128 = -(u64::MAX as i128) - 1; is independently redefined inside i128_preserves_the_full_native_cbor_integer_range (line 400), i128_rejects_values_outside_the_native_cbor_integer_range (line 418), and to_cbor_buffer_roundtrips_native_cbor_integer_boundaries (line 598). This is the exact boundary value the whole PR is about, so keeping it in three places means a future edit could update it in one test but not the others, silently weakening coverage of the edge case this PR was written to protect.
| // at module scope, e.g. just inside `mod tests {` | |
| const MIN_NATIVE_CBOR_INTEGER: i128 = -(u64::MAX as i128) - 1; |
source: ['claude']
There was a problem hiding this comment.
Resolved in 9ce1e94 — MIN_NATIVE_CBOR_INTEGER boundary constant duplicated across three test functions no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…e scope Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This delta is a pure test-only cleanup: it hoists the previously triplicated MIN_NATIVE_CBOR_INTEGER constant to module scope in packages/rs-platform-value/src/converter/ciborium.rs and updates the three boundary/rejection/round-trip tests to reference it. No production code, validation logic, or consensus-relevant behavior changed. Both Codex and Claude reviewer lanes independently confirm the prior duplication finding is fixed and report no new defects.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— general (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— general (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— general (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— general (completed),claude-sonnet-5— rust-quality (completed)
2 participants
What
Why
Distinct Platform Value integers could previously serialize to the same CBOR integer, and some native CBOR values could change sign during a decode/re-encode cycle. The conversion now fails closed with the existing IntegerSizeError.
This covers PLATFORM-DS-CAND-366 and PLATFORM-DS-CAND-367.
Checks
The complete package command also ran all unit and integration tests successfully; its unrelated pre-existing doctest example fails because serde_json is not available to that doctest target.
Summary by CodeRabbit