Add disable_cross_region_sharing param

[SozinM]

📝 Summary

Needed for flowproxy

💡 Motivation and Context

---

✅ I have completed the following steps:

• Run make lint
• Run make test
• Added tests (if applicable)

[claude]

Claude finished @SozinM's task in 1m 54s —— View job

---

Claude Code Review

• Read changed files
• Analyze logic, edge cases, and best practices
• Post review feedback

Findings

Issues that should be addressed:

1. InMemorySize::size() not updated — crates/rbuilder-primitives/src/lib.rs:115-121 does not include the new disable_cross_region_sharing field. Memory accounting will under-report the size of Metadata. Inline comment posted with a suggested fix.

2. Misleading doc comment — crates/rbuilder-primitives/src/lib.rs:54 says "Used for encode_no_blobs conversion." This describes a downstream usage rather than what the field represents. The wire-format counterpart at serialize.rs:156 already has the correct semantic doc ("Disable multiplexing bundle to other region builders") — recommend matching it. Inline suggestion posted.

3. Option<bool> modelling (minor) — Storing a "disable" flag as Option<bool> exposes three states where only two are meaningful (None and Some(false) are semantically identical but produce different serialized output). Worth considering whether the internal Metadata should hold a plain bool while only RawBundleMetadata keeps the Option<bool> for wire-format compatibility. Inline note posted.

Other observations (no action requested):

• The new field is currently only stored — no consumer code in this PR reads it. Presumably the consumer lives in flowproxy per the PR description; just flagging that this PR is purely plumbing.
• V1 validation correctly rejects disable_cross_region_sharing (serialize.rs:237-242), and the field is gated to V2+. Good.
• Bundle.metadata is correctly excluded from PartialEq/Hash via #[derivative(... = "ignore")], so adding this field doesn't affect bundle hash/uuid stability or existing roundtrip tests.
  • Branch

[claude]

Minor API note: the new field semantically represents "disable" (a bool flag), and modelling it as Option<bool> invites three states — None, Some(false), Some(true) — where only two are meaningful. If the goal is to round-trip the wire-format Option<bool> faithfully, this is fine, but consider if the internal Metadata could store a plain bool (defaulting to false) and only the wire RawBundleMetadata keeps the Option. As written, None and Some(false) are indistinguishable in behaviour but produce different serialized output, which can lead to roundtrip surprises.

[SozinM]

Option is used to have correct RawBuilder -> Bundle -> RawBundle conversion

[claude]

Claude finished @SozinM's task in 2m 6s —— View job

---

Claude Code Review (re-review after Fixes commit)

• Read changed files
• Diff against develop, compare to prior review feedback
• Analyze logic, edge cases, best practices
• Post review feedback

Status of prior feedback

1. InMemorySize::size() — ✅ Fixed in 0a0c273. Option<bool> is now included in Metadata::size() (lib.rs:120).
2. Doc comment on disable_cross_region_sharing — ⚠️ Updated in 0a0c273 to "RawBundle field, round-tripped through Bundle. Not consumed by rbuilder." (lib.rs:54). This is an improvement (no longer claims it's for encode_no_blobs), but it describes the field's plumbing rather than its meaning. The wire-format counterpart at serialize.rs:156 has the semantic doc ("Disable multiplexing bundle to other region builders."). Non-blocking — your call whether to align them.
3. Option<bool> modelling — ❌ Not addressed. As noted previously, None and Some(false) are behaviourally identical but produce different serialized output. Minor / non-blocking if intentional for wire-format faithfulness.

New finding

V1 rejection of disable_cross_region_sharing is not covered by tests. validate_fields rejects the field on V1 at serialize.rs:237-242, but test_error_bundle_decoding_invalid_fields_v1 (serialize.rs:1014) does not include "disableCrossRegionSharing" in its extra_invalid_fields array. A regression that drops this rejection would pass CI silently. Inline suggestion posted. (Same gap exists for the pre-existing delayedRefund field — out of scope for this PR.)

Other observations

• Round-trip wiring (decode at serialize.rs:389-390 ↔ encode_no_blobs at serialize.rs:430) looks correct and symmetric.
• Field is correctly gated to V2+ via validate_fields.
• All Metadata construction sites have been updated (only one struct-literal site exists at synthetic_orders.rs:97; everything else uses Metadata::new / Metadata::default, both of which initialise the new field).
• No consumer of the field exists in this PR — purely plumbing for flowproxy, as stated in the PR description.
  • Branch

[claude]

The V1 rejection added here isn't covered by test_error_bundle_decoding_invalid_fields_v1 (serialize.rs:1014). Consider adding r#" "disableCrossRegionSharing": true "# to the extra_invalid_fields array so this branch is exercised — otherwise a future change that drops/breaks this check won't be caught.

[claude]

Claude finished @SozinM's task in 1m 15s —— View job

---

Claude Code Review (re-review after f9f3226)

• Read changed files at current HEAD
• Diff against develop and compare to prior review feedback
• Analyze new changes for issues
• Post review feedback

Status of prior feedback

1. InMemorySize::size() — ✅ Fixed in 0a0c273 (lib.rs:120).
2. Doc comment on disable_cross_region_sharing — ⚠️ Updated in 0a0c273 to "RawBundle field, round-tripped through Bundle. Not consumed by rbuilder." (lib.rs:54). Still describes the field's plumbing rather than its meaning (the wire-format counterpart at serialize.rs:156 carries the semantic doc). Non-blocking — your call.
3. Option<bool> modelling — ✅ Resolved. Author confirmed it's intentional for RawBundle → Bundle → RawBundle round-trip fidelity.
4. V1 rejection test coverage — ✅ Fixed in f9f3226 (serialize.rs:1032-1033). extra_invalid_fields now covers both disableCrossRegionSharing and the previously-uncovered delayedRefund. Nice catch including the latter.

New findings

None. The changes since the last review are limited to the two test additions and they look correct.

Other observations

• All Metadata construction sites carry the new field (struct-literal sites in synthetic_orders.rs:101 and store.rs:747; everywhere else uses Metadata::new/Metadata::default).
• Round-trip wiring (decode ↔ encode_no_blobs at serialize.rs:389-390 / serialize.rs:430) is symmetric.
• Field is correctly gated to V2+ via validate_fields (serialize.rs:237-242).
• The V1 test asserts on BundleVersion::V1 only (not the rejected field name), so a regression that mis-attributes the field name in the error would still pass — consistent with how the other entries are tested, so not a regression introduced by this PR.
  • Branch