Align CBM serialized form with declared schema#1065
Open
Conversation
`ConsecutiveBlockMinimization`'s `ProblemSchemaEntry::fields` declares
`matrix` and `bound`, but its serialized form also wrote `num_rows` and
`num_cols`, and its `try_from` rejected JSON that lacked them. This made
the schema-driven `pred create` path fail with "missing field `num_rows`"
for any caller that built JSON from the declared schema fields.
`num_rows` and `num_cols` are fully derived from `matrix` (just
`matrix.len()` and `matrix[0].len()`), so they don't belong in the wire
format. Switch to symmetric `serde(try_from / into = Def)` so both
serialize and deserialize round-trip through the minimal `{matrix, bound}`
form, and let `try_new` recompute the cached dimensions on the way in.
The cached fields stay on the in-memory struct for fast getter access.
Replace the now-unreachable inconsistent-dimensions test with one that
pins the minimal wire format and one that exercises ragged-matrix
rejection — the actual remaining input validation.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The CI Test job for #1065 failed because cli_tests.rs:445 still asserted the old "num_cols must match matrix column count" error. Under the new serde definition, extra num_rows/num_cols fields in the JSON are silently ignored (serde's default behavior for unknown fields when not using deny_unknown_fields), so the previous JSON now deserializes successfully and the CLI no longer rejects it. Switch the test to exercise the actual remaining input validation — ragged-matrix rejection — which is what `validate_matrix_dimensions` guards against and what the in-tree unit test now also covers. Rename the test accordingly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1065 +/- ##
=======================================
Coverage 97.92% 97.92%
=======================================
Files 966 966
Lines 100043 100044 +1
=======================================
+ Hits 97967 97971 +4
+ Misses 2076 2073 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
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
ConsecutiveBlockMinimization'sProblemSchemaEntry::fieldsdeclares onlymatrixandbound, but the model's serde was inconsistent with that declaration:Serializewrote 4 fields (matrix,num_rows,num_cols,bound) via the default field-by-field deriveDeserialize(viatry_from = ConsecutiveBlockMinimizationDef) requirednum_rowsandnum_colsto be present, even though they're fully derivable frommatrixThat inconsistency broke the schema-driven
pred createpath: it builds JSON from the declared schema fields (matrix + bound), then validates by deserializing — which failed withSchema-driven factory rejected generated data for ConsecutiveBlockMinimization: missing field 'num_rows'. Reproducer:Fix
num_rowsandnum_colsare justmatrix.len()andmatrix[0].len()— they're cached for fast getter access but they're not independent data. Drop them from the wire format entirely by addingserde(into = ConsecutiveBlockMinimizationDef)so both directions round-trip through the minimal{matrix, bound}form.try_newalready recomputes the cached dimensions on construction.The previous validation that "if the JSON contains a num_rows that disagrees with the matrix, reject" was guarding against malformed input we shouldn't have been producing in the first place; the real input validation that survives is ragged-matrix rejection (in
validate_matrix_dimensions), which is now what the test suite exercises.Verification
Test plan
cargo test --lib— 4958 passed (9 CBM-specific tests pass)cargo test --test main— 75 integration tests passedcargo fmt --all -- --checkcargo clippy --all-targets --workspace -- -D warningspred create CBM ... | pred solvereturnsOr(true)with witness[0, 2, 1]Related
🤖 Generated with Claude Code