fp: round-trippable serde with golden format tests#229
Merged
JoeyBF merged 3 commits intoSpectralSequences:masterfrom Apr 8, 2026
Merged
fp: round-trippable serde with golden format tests#229JoeyBF merged 3 commits intoSpectralSequences:masterfrom
JoeyBF merged 3 commits intoSpectralSequences:masterfrom
Conversation
Derive Serialize/Deserialize on Fp, FqVector, Matrix, Subspace, and
QuasiInverse, and rework FpVector's manual serde impl to route through
FqVector<Fp<ValidPrime>> so the prime is encoded in the wire format. This
gives a uniform representation across all primes that round-trips without
out-of-band context, which the zarr save system relies on.
The sseq_gui web frontend reads permanents / classes / decompositions as
flat JS arrays of u32 (interface/panels.js: `d[0].reduce(...)`,
`d[0].indexOf(1)`, `permanentClasses.map(rowToKaTeX)`), so SetClass now
carries those fields as Vec<u32> and converts from FpVector at the
message boundary. That keeps the JS format stable independently of
FpVector's own serde impl.
Add `tests/serde_format.rs`:
* round-trip proptests for FpVector, Matrix, Subspace, and QuasiInverse
over a broad spread of primes, dimensions, and limb counts (F_2
vectors up to 5 limbs);
* golden tests pinned via expect-test for p = 2, 3, 5, including F_2
vectors that span two and three limbs, so any future format drift
fails loudly and is only updatable via UPDATE_EXPECT=1.
Collaborator
Author
|
@coderabbitai review |
📝 WalkthroughWalkthroughAdds serde (Serialize/Deserialize) support across the fp crate types, enables Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ext/crates/fp/src/vector/fp_wrapper/mod.rs`:
- Around line 262-276: After deserializing the intermediate FqVector in the
Deserialize impl for FpVector, validate that the deserialized limbs length meets
the expected limb count before calling v.to_bytes to avoid panics; compute the
expected number of limbs from the field characteristic and vector length (use
v.fq().number(v.len()) or the library helper that returns the required limb
count), compare it to v.limbs().len() (or equivalent accessor), and return a
serde::de::Error::custom error if the limbs array is too short; only proceed to
v.to_bytes(...) and Self::from_bytes(...) when the length check passes.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: cf629cb4-518f-4318-915d-44b681e8de92
📒 Files selected for processing (10)
ext/crates/fp/Cargo.tomlext/crates/fp/src/field/fp.rsext/crates/fp/src/matrix/matrix_inner.rsext/crates/fp/src/matrix/quasi_inverse.rsext/crates/fp/src/matrix/subspace.rsext/crates/fp/src/vector/fp_wrapper/mod.rsext/crates/fp/src/vector/inner.rsext/crates/fp/tests/serde_format.rsweb_ext/sseq_gui/src/actions.rsweb_ext/sseq_gui/src/sseq.rs
The derived `Deserialize` impls for `FqVector` and `Matrix` accepted any combination of dimensions and limb data, so malformed input could build instances whose internal invariants (`limbs.len() == fq.number(len)`, `data.len() == physical_rows * stride`, etc.) didn't hold. Downstream accessors like `FqVector::entry`, `FqVector::to_bytes`, `Matrix::row`, and `Matrix::to_bytes` then panicked on bounds-checked slice indexing, which escapes the deserialize boundary instead of surfacing as a normal serde error. Replace both derived impls with manual ones that: * accept an inert `Raw` struct via the derive, * validate the invariants, and * return a `serde::de::Error::custom` on mismatch. The same validation protects `FpVector::deserialize`, which routes through `FqVector<Fp<ValidPrime>>`, so its caller-visible behavior is now "recoverable serde error on malformed input" instead of "panic on the first accessor that touches the bad limb data". Add regression tests covering both the FqVector limb-count check and the Matrix stride / data-length checks. Also switch the golden format tests to `serde_json::to_string_pretty` so the pinned JSON is readable in the test source. This only affects test source formatting, not the wire format.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ext/crates/fp/src/matrix/matrix_inner.rs`:
- Around line 73-81: The current validation computes raw.physical_rows *
raw.stride without checking for overflow; update the check in the
deserialization path that returns D::Error::custom to use checked_mul on
raw.physical_rows and raw.stride, return a custom error if checked_mul returns
None (overflow) and otherwise compare raw.data.len() with the computed product,
ensuring you reference the existing variables raw.data.len(), raw.physical_rows,
raw.stride and the same error pathway (D::Error::custom) so behavior and
messaging remain consistent.
In `@ext/crates/fp/tests/serde_format.rs`:
- Around line 155-164: Add a new unit test
`matrix_malformed_pivots_length_errors` in ext/crates/fp/tests/serde_format.rs
that deserializes a JSON where `columns` is 3 but `pivots` has length 2 (e.g.
pivots=[0,1]); assert that `serde_json::from_str::<Matrix>` returns an error and
that the error string contains the phrase "pivots length" to exercise the
`Matrix` deserialization validation for `pivots` (related to Matrix::deserialize
/ matrix_inner.rs pivots check).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 860da5ec-eb6e-4449-8c26-1db6c3e776d7
📒 Files selected for processing (3)
ext/crates/fp/src/matrix/matrix_inner.rsext/crates/fp/src/vector/inner.rsext/crates/fp/tests/serde_format.rs
github-actions bot
added a commit
that referenced
this pull request
Apr 8, 2026
* fp: round-trippable serde with golden format tests
Derive Serialize/Deserialize on Fp, FqVector, Matrix, Subspace, and
QuasiInverse, and rework FpVector's manual serde impl to route through
FqVector<Fp<ValidPrime>> so the prime is encoded in the wire format. This
gives a uniform representation across all primes that round-trips without
out-of-band context, which the zarr save system relies on.
The sseq_gui web frontend reads permanents / classes / decompositions as
flat JS arrays of u32 (interface/panels.js: `d[0].reduce(...)`,
`d[0].indexOf(1)`, `permanentClasses.map(rowToKaTeX)`), so SetClass now
carries those fields as Vec<u32> and converts from FpVector at the
message boundary. That keeps the JS format stable independently of
FpVector's own serde impl.
Add `tests/serde_format.rs`:
* round-trip proptests for FpVector, Matrix, Subspace, and QuasiInverse
over a broad spread of primes, dimensions, and limb counts (F_2
vectors up to 5 limbs);
* golden tests pinned via expect-test for p = 2, 3, 5, including F_2
vectors that span two and three limbs, so any future format drift
fails loudly and is only updatable via UPDATE_EXPECT=1.
* Fix lint
* fp: validate FqVector/Matrix invariants in Deserialize
The derived `Deserialize` impls for `FqVector` and `Matrix` accepted any
combination of dimensions and limb data, so malformed input could build
instances whose internal invariants (`limbs.len() == fq.number(len)`,
`data.len() == physical_rows * stride`, etc.) didn't hold. Downstream
accessors like `FqVector::entry`, `FqVector::to_bytes`, `Matrix::row`, and
`Matrix::to_bytes` then panicked on bounds-checked slice indexing, which
escapes the deserialize boundary instead of surfacing as a normal serde
error.
Replace both derived impls with manual ones that:
* accept an inert `Raw` struct via the derive,
* validate the invariants, and
* return a `serde::de::Error::custom` on mismatch.
The same validation protects `FpVector::deserialize`, which routes through
`FqVector<Fp<ValidPrime>>`, so its caller-visible behavior is now
"recoverable serde error on malformed input" instead of "panic on the
first accessor that touches the bad limb data".
Add regression tests covering both the FqVector limb-count check and the
Matrix stride / data-length checks.
Also switch the golden format tests to `serde_json::to_string_pretty` so
the pinned JSON is readable in the test source. This only affects test
source formatting, not the wire format.
github-actions bot
added a commit
to JoeyBF/sseq
that referenced
this pull request
Apr 8, 2026
…#229) * fp: round-trippable serde with golden format tests Derive Serialize/Deserialize on Fp, FqVector, Matrix, Subspace, and QuasiInverse, and rework FpVector's manual serde impl to route through FqVector<Fp<ValidPrime>> so the prime is encoded in the wire format. This gives a uniform representation across all primes that round-trips without out-of-band context, which the zarr save system relies on. The sseq_gui web frontend reads permanents / classes / decompositions as flat JS arrays of u32 (interface/panels.js: `d[0].reduce(...)`, `d[0].indexOf(1)`, `permanentClasses.map(rowToKaTeX)`), so SetClass now carries those fields as Vec<u32> and converts from FpVector at the message boundary. That keeps the JS format stable independently of FpVector's own serde impl. Add `tests/serde_format.rs`: * round-trip proptests for FpVector, Matrix, Subspace, and QuasiInverse over a broad spread of primes, dimensions, and limb counts (F_2 vectors up to 5 limbs); * golden tests pinned via expect-test for p = 2, 3, 5, including F_2 vectors that span two and three limbs, so any future format drift fails loudly and is only updatable via UPDATE_EXPECT=1. * Fix lint * fp: validate FqVector/Matrix invariants in Deserialize The derived `Deserialize` impls for `FqVector` and `Matrix` accepted any combination of dimensions and limb data, so malformed input could build instances whose internal invariants (`limbs.len() == fq.number(len)`, `data.len() == physical_rows * stride`, etc.) didn't hold. Downstream accessors like `FqVector::entry`, `FqVector::to_bytes`, `Matrix::row`, and `Matrix::to_bytes` then panicked on bounds-checked slice indexing, which escapes the deserialize boundary instead of surfacing as a normal serde error. Replace both derived impls with manual ones that: * accept an inert `Raw` struct via the derive, * validate the invariants, and * return a `serde::de::Error::custom` on mismatch. The same validation protects `FpVector::deserialize`, which routes through `FqVector<Fp<ValidPrime>>`, so its caller-visible behavior is now "recoverable serde error on malformed input" instead of "panic on the first accessor that touches the bad limb data". Add regression tests covering both the FqVector limb-count check and the Matrix stride / data-length checks. Also switch the golden format tests to `serde_json::to_string_pretty` so the pinned JSON is readable in the test source. This only affects test source formatting, not the wire format.
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
1 participant
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.
Summary
Prequel PR to an upcoming zarr save system rework. Extracted to minimize that PR's diff and land the
fpserde changes independently, with format tests.fpcrateSerialize/DeserializeonFp<P>,FqVector<F>,Matrix,Subspace,QuasiInverse. Enables downstream serializers to store these types directly without hand-rolled byte formats.FpVector's manual serde impl to route throughFqVector<Fp<ValidPrime>>so the prime is encoded in the wire format. The previous impl serialized as a bareVec<u32>and panicked on deserialize, so it couldn't survive a round-trip.aligned-vecgains theserdefeature (needed forMatrix::data: AVec<Limb>).Web frontend boundary fix (
web_ext/sseq_gui/)SetClass.permanents/.classes/.decompositionsas flat JS arrays of u32 (seeinterface/panels.js:d[0].reduce(...),d[0].indexOf(1),permanentClasses.map(rowToKaTeX)). IfFpVector's serde format becomes prime-tagged, those JS sites break.SetClassnow carries those fields asVec<Vec<u32>>(etc.) and converts fromFpVectorat the message boundary insseq.rs. The JS wire format is unchanged, andFpVector's own serde is free to carry prime metadata.Tests (
crates/fp/tests/serde_format.rs, 177 lines)FpVector/Matrix/Subspace/QuasiInverse, 128 cases each, F_2 vectors up to 5 limbs.expect-testforp ∈ {2, 3, 5}, including F_2 vectors spanning exactly two and three limbs — catches mistakes in the multi-limb encoding.UPDATE_EXPECT=1 cargo test -p fp --test serde_format.Test plan
cargo test -p fp --test serde_format— all 11 tests pass (4 proptests × 128 cases + 7 goldens)nix run ./ext#test— full CI pipeline (lint, tests, benchmarks, miri) green🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Safety
Tests
Chores