fix(contracts): enforce integer bounds on the Rust serialize path, not only on parse - #3537
Merged
Merged
Conversation
…t only on parse The generator emitted bounds checks when deserializing and none when serializing, so a value that could never be ACCEPTED could still be EMITTED. That is the one-sided form of a cross-language gate: this runtime can produce a payload its siblings will reject, which surfaces as a different wrong answer rather than a clean failure. Rust now generates serializer hooks from the same schema maximum the parse path already reads, so the two directions cannot drift. TypeScript and C# were checked for the same asymmetry and did not have it — the outbound assertion seam and the generated bounded setters were already symmetric. Neither needed a generator change, and neither is claimed as fixed. The generator also now rejects a scalar, null or array additionalProperties before writing anything, following the existing convention that a rejection test asserts both a non-zero exit and an empty output directory. Proven by mutation toward a different wrong answer: with the serializer hooks suppressed, the round trip emits the out-of-range integer rather than refusing it, and the new Rust test fails on exactly that. The shared fixture covers the case in all three languages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change adds serializer-side numeric range validation to generated Rust protocol bindings. It updates code generation, validates ChangesNumeric validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ProtocolFixture
participant OutboundSerializer
participant RangeValidator
ProtocolFixture->>OutboundSerializer: serialize unsafe SyncCadence interval
OutboundSerializer->>RangeValidator: check roundIntervalSeconds maximum
RangeValidator-->>OutboundSerializer: return range error
OutboundSerializer-->>ProtocolFixture: reject outbound payload
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
|
thought (non-blocking): Accessibility audit (advisory)The sharded axe audit is report-only while the baseline and runtime budget mature.
Shard 1 reportShard 2 reportShard 3 report |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/contracts/rust/src/lib.rs`:
- Around line 186-189: The invalid sync-cadence test data must come from the
shared fixture instead of duplicated literals. In
packages/contracts/rust/src/lib.rs lines 186-189, read
sync-cadence-above-safe-integer.json and derive next_round_at and
round_interval_seconds before constructing SyncCadence; in
packages/contracts/tests/ProtocolFixtureTests.cs lines 45-49, likewise read the
fixture and derive NextRoundAt and RoundIntervalSeconds before constructing the
invalid model.
In `@tooling/carrier-contract-codegen/generate.mjs`:
- Around line 255-258: Update rustDefinition to preserve schema-valued
additionalProperties by emitting a serde-flattened BTreeMap<String,
rustType(definition.additionalProperties)> field, while retaining the existing
handling for boolean values. Add a successful round-trip test covering
schema-valued maps and review the generated Rust output for architectural
consistency and required public API documentation.
- Around line 1131-1135: Update rustNumeric() so the .0 suffix is added only
when a number-valued bound is an integer; preserve fractional values unchanged
while continuing to stringify non-number types as before.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ea85b34-2cdc-4321-9096-2d77263245e5
📒 Files selected for processing (8)
packages/contracts/protocol/fixtures/manifest.jsonpackages/contracts/protocol/fixtures/sync-cadence-above-safe-integer.jsonpackages/contracts/rust/src/generated.rspackages/contracts/rust/src/lib.rspackages/contracts/src/__tests__/protocol-fixtures.test.tspackages/contracts/tests/ProtocolFixtureTests.cstooling/carrier-contract-codegen/generate.mjstooling/carrier-contract-codegen/tests/generate.test.mjs
Comment on lines
+186
to
+189
| let cadence = SyncCadence { | ||
| next_round_at: Some("2026-08-02T12:00:00Z".to_owned()), | ||
| round_interval_seconds: 9_007_199_254_740_992, | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
suggestion [blocking]: Use the shared unsafe fixture as the test-data source.
The Rust and C# tests copy the same invalid payload that TypeScript reads from sync-cadence-above-safe-integer.json. Read the fixture in each test and extract its values before constructing the invalid model.
packages/contracts/rust/src/lib.rs#L186-L189: derivenext_round_atandround_interval_secondsfromsync-cadence-above-safe-integer.json.packages/contracts/tests/ProtocolFixtureTests.cs#L45-L49: deriveNextRoundAtandRoundIntervalSecondsfromsync-cadence-above-safe-integer.json.
As per path instructions, “avoid hand-parallel duplicate copies of single-source things (A4).”
📍 Affects 2 files
packages/contracts/rust/src/lib.rs#L186-L189(this comment)packages/contracts/tests/ProtocolFixtureTests.cs#L45-L49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/contracts/rust/src/lib.rs` around lines 186 - 189, The invalid
sync-cadence test data must come from the shared fixture instead of duplicated
literals. In packages/contracts/rust/src/lib.rs lines 186-189, read
sync-cadence-above-safe-integer.json and derive next_round_at and
round_interval_seconds before constructing SyncCadence; in
packages/contracts/tests/ProtocolFixtureTests.cs lines 45-49, likewise read the
fixture and derive NextRoundAt and RoundIntervalSeconds before constructing the
invalid model.
Source: Path instructions
Comment on lines
+255
to
+258
| if (Object.hasOwn(value, 'additionalProperties') | ||
| && typeof value.additionalProperties !== 'boolean' | ||
| && (typeof value.additionalProperties !== 'object' || value.additionalProperties === null || Array.isArray(value.additionalProperties))) { | ||
| throw new Error(`${path}: additionalProperties must be a boolean or schema object for consistent TypeScript, C#, and Rust projections`) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
issue [blocking]: Preserve schema-valued additional properties in Rust.
Line 255 permits a schema object for additionalProperties. rustDefinition only emits a flattened map when the value is true. A schema-valued map is therefore ignored by Rust deserialization and lost during serialization.
Emit BTreeMap<String, ${rustType(definition.additionalProperties)}> with #[serde(flatten)] for this case, or reject it until all projections support it. Add a successful schema-valued-map round-trip test.
As per path instructions, “Review generated Rust output for architectural consistency and public API documentation requirements.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tooling/carrier-contract-codegen/generate.mjs` around lines 255 - 258, Update
rustDefinition to preserve schema-valued additionalProperties by emitting a
serde-flattened BTreeMap<String, rustType(definition.additionalProperties)>
field, while retaining the existing handling for boolean values. Add a
successful round-trip test covering schema-valued maps and review the generated
Rust output for architectural consistency and required public API documentation.
Source: Path instructions
Comment on lines
+1131
to
+1135
| function rustNumeric(value, property) { | ||
| const type = Array.isArray(property.type) | ||
| ? property.type.find((item) => item !== 'null') | ||
| : property.type | ||
| return type === 'number' ? `${value}.0` : String(value) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pnpm kb query "What command compiles generated Rust contract bindings, and what Rust toolchain does CI use?"
rg -n -C 3 '"type"[[:space:]]*:[[:space:]]*"number"|minimum|maximum' \
packages/contracts/protocol/schemas \
tooling/carrier-contract-codegen \
2>/dev/null || trueRepository: Harborline-Software/shipyard
Length of output: 23066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Relevant generate.mjs sections =="
sed -n '1000,1145p' tooling/carrier-contract-codegen/generate.mjs
echo
echo "== Numerical bounds in schemas =="
python3 - <<'PY'
import json
from pathlib import Path
p=Path('packages/contracts/protocol/schemas/carrier-protocol.schema.json')
s=json.loads(p.read_text())
for path, schema in [(None,s), *(('/'+'/'.join(k),sub) for sub in s.get('definitions',{}).items())] + [('/',*kv) for kv in (s.get('$defs') or {}).items() if isinstance(kv[1],dict)]:
def walk(node, p='`#/`'):
if isinstance(node, dict):
if node.get('type')=='number' and ('minimum' in node or 'maximum' in node):
print(path + p, node)
for k,v in node.items():
walk(v, f"{p}{k}/")
walk(schema, path+'/')
PY
echo
echo "== Rust tests for generated Rust binding compilation =="
rg -rn 'cargo[ -]|rustc|--release|generated|Carriers|Protocol|carrier|contract.*rust|rust|generate' tooling packages .github 2>/dev/null | head -200 || true
echo
echo "== Read-only behavior probe for rustNumeric =="
node - <<'JS'
function rustNumeric(value, property) {
const type = Array.isArray(property.type)
? property.type.find((item) => item !== 'null')
: property.type
return type === 'number' ? `${value}.0` : String(value)
}
for (const value of [0, 5, -5, 2.5, -2.5, 1e3, 1e-2, 2.9e1]) {
const s = String(value);
const out = rustNumeric(value, {type: 'number'});
console.log(`${s}`.padEnd(8), '=>', out, out.includes('.0') ? (s.includes('.') || s.includes('e') || s.includes('E') ? 'bad-decimal' : 'integer-suffix') : 'ok')
}
JSRepository: Harborline-Software/shipyard
Length of output: 7276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Progress bound definitions =="
python3 - <<'PY'
import json
from pathlib import Path
schema = json.loads(Path('packages/contracts/protocol/schemas/carrier-protocol.schema.json').read_text())
for section_name, section in schema.get('definitions', {}).items():
if not isinstance(section, dict):
continue
props = section.get('properties') or {}
for prop_name, prop in props.items():
if prop.get('type') == 'number' and ('minimum' in prop or 'maximum' in prop):
print(section_name, prop_name, json.dumps({'type': prop.get('type'), 'minimum': prop.get('minimum'), 'maximum': prop.get('maximum')}, separators=(',', ':')))
PY
echo
echo "== Rust literal probe =="
node - <<'JS'
function rustNumeric(value, property) {
const type = Array.isArray(property.type)
? property.type.find((item) => item !== 'null')
: property.type
return type === 'number' ? `${value}.0` : String(value)
}
for (const value of [0, 5, -5, 2.5, -2.5, 1e3, 1e-2, 2.9e1]) {
const out = rustNumeric(value, {type: 'number'});
console.log(`${String(value)} => ${out}`);
}
JS
echo
echo "== Search for Rust codegen tests or generated Rust compilation =="
rg -n --glob '!node_modules' --glob '!dist' --glob '!build' 'generate.*Rust|cargo[ -]|rustc|--release|generated.*rust|rust.*binding|carrier.*rust|target[\\/]rust|\\.rs' .github tooling packages 2>/dev/null || trueRepository: Harborline-Software/shipyard
Length of output: 4910
issue [blocking]: Generate valid Rust literals for fractional bounds.
rustNumeric() appends .0 to every number bound, so fractional values such as 2.5 emit 2.5.0, which Rust rejects. Generate the suffix only for integer-looking numeric bounds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tooling/carrier-contract-codegen/generate.mjs` around lines 1131 - 1135,
Update rustNumeric() so the .0 suffix is added only when a number-valued bound
is an integer; preserve fractional values unchanged while continuing to
stringify non-number types as before.
Source: Coding guidelines
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.
fix(contracts): enforce integer bounds on the Rust serialize path, not only on parse
The generator emitted bounds checks when deserializing and none when serializing, so a value that
could never be ACCEPTED could still be EMITTED. That is the one-sided form of a cross-language gate:
this runtime can produce a payload its siblings will reject, which surfaces as a different wrong
answer rather than a clean failure.
Rust now generates serializer hooks from the same schema maximum the parse path already reads, so
the two directions cannot drift.
TypeScript and C# were checked for the same asymmetry and did not have it — the outbound assertion
seam and the generated bounded setters were already symmetric. Neither needed a generator change,
and neither is claimed as fixed.
The generator also now rejects a scalar, null or array additionalProperties before writing anything,
following the existing convention that a rejection test asserts both a non-zero exit and an empty
output directory.
Proven by mutation toward a different wrong answer: with the serializer hooks suppressed, the round
trip emits the out-of-range integer rather than refusing it, and the new Rust test fails on exactly
that. The shared fixture covers the case in all three languages.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
Summary by CodeRabbit
Bug Fixes
Tests