fix(sdk): make boxlite_options_add_port carry the full PortSpec#741
Merged
Conversation
The C function took (guest_port, host_port) as two ints — reversed relative to core PortSpec, the Python/Node/Go SDKs, the CLI -p flag, Docker's host:guest convention, and add_volume in the same header — and could not express protocol or host_ip, capping every binding built on the C ABI below what the core model carries. C SDK: - New signature: (opts, host_port, guest_port, protocol, host_ip), host-first, field-for-field the core PortSpec order. Old 3-arg form removed outright; the arity change turns stale callers into compile errors instead of silently flipped forwarding. - Ports are uint16_t; returns BoxliteErrorCode instead of void (InvalidArgument on NULL opts, guest_port 0, or non-UTF-8 host_ip). - New BoxlitePortProtocol enum, following the BoxliteRegistryTransport pattern. host_ip NULL or "" = all interfaces. - The C layer stores protocol/host_ip verbatim (same as Node/Python); capability enforcement for values the runtime cannot honor yet stays a core concern. Go SDK: - WithPortSpec merged into WithPort(spec PortSpec); the two-int form is gone (Go has no overloading), matching the WithNetwork/WithSecret struct-option style. Named fields replace positional host/guest. - Zero-value Protocol now normalizes to TCP in toCSpec, matching the core default; UDP and HostIP are still rejected until the runtime honors them, with errors that blame the runtime, not the bridge. - Bridge passes protocol and host_ip through the new FFI signature and checks the returned error code. Also corrects the stale "None/0 => dynamically assigned" comment on core PortSpec.host_port (actual behavior mirrors guest_port) and the same false claim in the Go docs, and updates both apps/runner call sites.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR redesigns the port-forwarding API across C, Rust FFI, Go SDK, and runner clients. The C API gains a protocol enum, host IP binding support, and error reporting; Rust FFI implements the new contract with validation; Go SDK refactors ChangesPort-Forwarding API and Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
|
||
| assert_eq!(zero_guest_code, BoxliteErrorCode::InvalidArgument); | ||
| assert_eq!(null_opts_code, BoxliteErrorCode::InvalidArgument); | ||
| let ports = unsafe { &(*opts).options.ports }; |
…inter' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: dorianzheng <8065637+DorianZheng@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
sdks/c/README.md (1)
835-837: ⚡ Quick winShow return-code handling in the migration “After” snippet.
The new API now returns
BoxliteErrorCode; the snippet should show checkingcode != Okso migrated callers don’t accidentally ignore failures.🤖 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 `@sdks/c/README.md` around lines 835 - 837, The example call to boxlite_options_add_port now returns a BoxliteErrorCode but the snippet doesn’t check it; update the migration “After” snippet to capture the return value from boxlite_options_add_port (e.g. BoxliteErrorCode code = boxlite_options_add_port(opts, 8080, 80, BoxlitePortProtocolTcp, NULL);) and then check if code != Ok and handle/log/return the error accordingly so callers don’t ignore failures.sdks/c/src/tests.rs (1)
483-510: ⚡ Quick winAdd a regression test for invalid UTF-8
host_ip.This suite covers null handle and zero guest port, but the documented
host_ipUTF-8 rejection path is still untested. Add a case with non-UTF-8 bytes and assertInvalidArgumentplus no port insertion.🤖 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 `@sdks/c/src/tests.rs` around lines 483 - 510, Add a new assertion in the test add_port_rejects_zero_guest_port_and_null_options (or a new test) that exercises the host_ip UTF-8 rejection: call boxlite_options_add_port with a valid opts, valid host_port/guest_port, BoxlitePortProtocol::BoxlitePortProtocolTcp and a pointer to a C string containing non-UTF-8 bytes (e.g., a byte slice with invalid UTF-8 cast to *const c_char), assert the return is BoxliteErrorCode::InvalidArgument, verify (&(*opts).options.ports) is still empty, and free opts with boxlite_options_free; reference new_test_options, boxlite_options_add_port, BoxliteErrorCode, and boxlite_options_free to locate where to add the check.
🤖 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.
Nitpick comments:
In `@sdks/c/README.md`:
- Around line 835-837: The example call to boxlite_options_add_port now returns
a BoxliteErrorCode but the snippet doesn’t check it; update the migration
“After” snippet to capture the return value from boxlite_options_add_port (e.g.
BoxliteErrorCode code = boxlite_options_add_port(opts, 8080, 80,
BoxlitePortProtocolTcp, NULL);) and then check if code != Ok and
handle/log/return the error accordingly so callers don’t ignore failures.
In `@sdks/c/src/tests.rs`:
- Around line 483-510: Add a new assertion in the test
add_port_rejects_zero_guest_port_and_null_options (or a new test) that exercises
the host_ip UTF-8 rejection: call boxlite_options_add_port with a valid opts,
valid host_port/guest_port, BoxlitePortProtocol::BoxlitePortProtocolTcp and a
pointer to a C string containing non-UTF-8 bytes (e.g., a byte slice with
invalid UTF-8 cast to *const c_char), assert the return is
BoxliteErrorCode::InvalidArgument, verify (&(*opts).options.ports) is still
empty, and free opts with boxlite_options_free; reference new_test_options,
boxlite_options_add_port, BoxliteErrorCode, and boxlite_options_free to locate
where to add the check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eb5d9b44-9c5e-4b17-87d1-5edb73362ddc
📒 Files selected for processing (9)
apps/runner/pkg/boxlite/client.goapps/runner/pkg/boxlite/stubs.gosdks/c/README.mdsdks/c/include/boxlite.hsdks/c/src/options.rssdks/c/src/tests.rssdks/go/boxlite_test.gosdks/go/options.gosrc/boxlite/src/runtime/options.rs
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: dorianzheng <8065637+DorianZheng@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdks/c/src/tests.rs (1)
453-480:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTest doesn't verify the default values it's named for.
The test name
add_port_zero_host_and_empty_ip_mean_defaultspromises validation of default semantics, but the test only checks that both calls returnOk(lines 477-478) without asserting the storedPortSpecfield values.According to the implementation (context snippet 1) and C header documentation (context snippet 3):
host_port=0should map toNone(meaning "use guest_port")NULLor emptyhost_ipshould map toNone(meaning "all interfaces")Add assertions after line 478 to verify:
let ports = unsafe { &(*opts).options.ports }; assert_eq!(ports.len(), 2); // First call: host_port=0, host_ip=NULL assert_eq!(ports[0].host_port, None, "host_port=0 should map to None"); assert_eq!(ports[0].guest_port, 80); assert_eq!(ports[0].host_ip, None, "NULL host_ip should map to None"); // Second call: host_port=443, host_ip="" assert_eq!(ports[1].host_port, Some(443)); assert_eq!(ports[1].guest_port, 443); assert_eq!(ports[1].host_ip, None, "empty host_ip should map to None");Without these checks, a regression in default handling would go undetected.
🤖 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 `@sdks/c/src/tests.rs` around lines 453 - 480, The test add_port_zero_host_and_empty_ip_mean_defaults currently only checks return codes; update it to inspect the stored PortSpec entries on opts to ensure defaults are applied: after the two boxlite_options_add_port calls and before boxlite_options_free, read &(*opts).options.ports, assert len==2, then verify for the first PortSpec that host_port is None, guest_port==80, host_ip is None (NULL case), and for the second PortSpec that host_port is Some(443), guest_port==443, and host_ip is None (empty string case); reference the test function name, opts, options.ports, and PortSpec fields when making these assertions.
🤖 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.
Outside diff comments:
In `@sdks/c/src/tests.rs`:
- Around line 453-480: The test add_port_zero_host_and_empty_ip_mean_defaults
currently only checks return codes; update it to inspect the stored PortSpec
entries on opts to ensure defaults are applied: after the two
boxlite_options_add_port calls and before boxlite_options_free, read
&(*opts).options.ports, assert len==2, then verify for the first PortSpec that
host_port is None, guest_port==80, host_ip is None (NULL case), and for the
second PortSpec that host_port is Some(443), guest_port==443, and host_ip is
None (empty string case); reference the test function name, opts, options.ports,
and PortSpec fields when making these assertions.
cgo maps the `enum BoxlitePortProtocol` parameter to its underlying integer type rather than the typedef's named type, so cPortProtocol now returns plain uint32 with explicit constant conversions — the same shape as cRegistryTransport. Also wraps the CodeQL-suggested assert in the C SDK test helper to satisfy rustfmt.
G4614
added a commit
that referenced
this pull request
Jun 12, 2026
… PortSpec impl merging upstream/main into the PR brought in #741's PortSpec-based WithPort but the session's earlier stub (added to unblock the runner build before #741 landed) remained in place, causing duplicate declarations: ports redeclared in struct boxConfig WithPort redeclared in this block cannot use portMapping{} as PortSpec value in append Remove the stub block: stub's ports field, portMapping struct, and stub WithPort func. #741's PortSpec + WithPort + ports []PortSpec are the canonical definitions.
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
4 participants
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.
The C function took (guest_port, host_port) as two ints — reversed relative to core PortSpec, the Python/Node/Go SDKs, the CLI -p flag, Docker's host:guest convention, and add_volume in the same header — and could not express protocol or host_ip, capping every binding built on the C ABI below what the core model carries.
C SDK:
Go SDK:
Also corrects the stale "None/0 => dynamically assigned" comment on core PortSpec.host_port (actual behavior mirrors guest_port) and the same false claim in the Go docs, and updates both apps/runner call sites.
Summary by CodeRabbit
Breaking Changes
New Features
Documentation
Tests