fix: int32→int64 for BSON property values#71
Conversation
Property values must be int64 in BSON — Studio Pro's type cache matches by BSON type and crashes with "Sequence contains no matching element" on int32. Array markers remain int32 (verified against Studio Pro BSON). - Convert all property values from int32 to int64 across writer_*.go - Add safeInt64() helper for settings serialization - Remove debug logging left in writer_widgets_custom.go - Fix 4 comments that incorrectly referenced int64 for array markers - Add golden .mxunit files from Studio Pro for roundtrip testing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
AI Code ReviewReview SummaryThis PR fixes a critical compatibility issue where Studio Pro crashes due to BSON type mismatches. The changes are focused, well-verified, and follow the project's patterns. What Looks Good
Moderate Issues
Minor Issues
RecommendationApprove - This PR correctly addresses a critical compatibility issue with Studio Pro. The changes are:
The fix maintains the required distinction between property values (int64) and array markers (int32) as verified against Studio Pro output, resolving the "Sequence contains no matching element" crash. The one pending manual verification item (Studio Pro opening projects) is understandable for this type of fix and doesn't block approval given the strong evidence provided. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
ako
left a comment
There was a problem hiding this comment.
Review
This is a clean, well-scoped rework of PR #66. All the issues from my earlier review have been addressed:
| PR #66 Issue | Status |
|---|---|
Debug logging (log.Printf("SERIALIZE CHECK: ...")) |
Fixed — removed |
| Inconsistent array marker treatment | Fixed — all array markers consistently int32, only property values changed to int64 |
| Comment/code mismatch | Fixed — comments correctly reference int32 for array markers |
| Bundled unrelated changes (CREATE OR REPLACE PAGE) | Fixed — split out, single concern |
What looks good
- Consistent rule: property values →
int64, array markers →int32. Applied uniformly across all 14 writer files. - Single commit, single concern — exactly what the repo's commit checklist asks for.
- Test updated (
writer_rest_test.go) — the Timeout assertion correctly changed toint64. - Verified against Studio Pro BSON per the test plan.
Minor items (non-blocking)
-
safeInt64is a no-op —func safeInt64(v int) int64 { return int64(v) }. On 64-bit Go this is a widening cast that can never fail. The function name implies safety/clamping that no longer exists. Consider inliningint64(v)directly or renaming totoInt64. Very minor. -
Golden
.mxunitfiles have no test functions — the 4 testdata files are added but no Go test reads them yet. The PR description says they're "for roundtrip testing" — if a follow-up is planned, that's fine, but standalone they're unused. -
Studio Pro test unchecked — the test plan shows
[ ] Studio Pro opens project without crash. This is the most important validation for this fix. Would be good to confirm before merging.
Looks good overall. This is ready to merge once the Studio Pro test is confirmed.
Summary
bson.A) kept as int32 — verified against Studio Pro-generated.mxunitfilessafeInt64()helper for settings serializationwriter_widgets_custom.go.mxunitfiles from Studio Pro for roundtrip testingRoot Cause
Studio Pro crashes with
Sequence contains no matching elementwhen BSON property values are int32 — its type cache matches by BSON wire type (0x10 vs 0x12).Test plan
go buildpassesgo test ./sdk/mpr/passes🤖 Generated with Claude Code