fix(build): repair #715 runner build break and org test mock#734
Merged
Conversation
- sdks/go: add WithPort so apps/runner compiles. #715 added boxlite.WithPort() call sites without the SDK function; the C ABI, Rust FFI, and runtime port-forwarding all already exist, only the Go layer was missing. Mirrors the WithVolume pattern; arg order (guest, host) matches the C ABI. - api test: fix OrganizationService mock argument order in the default-org-membership spec so configService lands in the right constructor slot (was crashing on configService.getOrThrow). Pre-existing on main; verified by reproducing on a clean checkout.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFixes a test helper constructor call and adds Go SDK port-forwarding: new port types, cfg.ports field, exported WithPort(host,guest), C-layer wiring, tests, and runtime caller updates. ChangesTest Helper Constructor Fix
Go SDK Port Forwarding Feature
Sequence Diagram(s)sequenceDiagram
participant Runner
participant BoxSDK
participant CLayer
Runner->>BoxSDK: call WithPort(host, guest)
BoxSDK->>BoxSDK: append portSpec{host, guest, protocol, host_ip}
BoxSDK->>CLayer: boxlite_options_add_port(guest_port, host_port)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@sdks/go/options.go`:
- Around line 170-176: WithPort currently appends raw ints into boxConfig.ports
(portEntry) without range checks which can wrap/truncate when crossing the FFI
boundary; add explicit validation inside WithPort: ensure guestPort is in
1..65535 and hostPort is in 0..65535 and reject invalid inputs immediately
(e.g., return a clear error by panicking or logging and not appending) so no
invalid port values are stored; apply the same validation logic to the other
port-accepting code paths referenced (the code that converts ports to u16 at the
FFI boundary) so invalid values are rejected before conversion.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 01937f31-7d2f-4d2d-98ae-062033a049cb
📒 Files selected for processing (2)
apps/api/src/organization/services/default-organization-membership.spec.tssdks/go/options.go
a7e4d4a to
be64a6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
sdks/go/options.go (1)
179-183:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winValidate port ranges before storing and crossing the FFI boundary.
WithPortaccepts unconstrainedintvalues forhostandguestthat are later narrowed tou16at the C/Rust boundary (Line 306). Negative or out-of-range values (>65535) will wrap/truncate, silently misconfiguring port forwarding. This matches the previously flagged issue.Guest ports must be in range 1-65535 (explicitly addressable inside the sandbox). Host ports must be in range 0-65535 (0 allows dynamic assignment).
🛡️ Proposed validation fix
// WithPort forwards a host port to a guest port. A host of 0 lets the runtime // assign one dynamically. func WithPort(host, guest int) BoxOption { + if guest < 1 || guest > 65535 { + panic(fmt.Sprintf("boxlite: guest port must be in range 1-65535, got %d", guest)) + } + if host < 0 || host > 65535 { + panic(fmt.Sprintf("boxlite: host port must be in range 0-65535, got %d", host)) + } return WithPortSpec(PortSpec{Host: host, Guest: guest}) }🤖 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/go/options.go` around lines 179 - 183, Validate port ranges in WithPort before constructing PortSpec: check guest is within 1..65535 and host is within 0..65535, and if any value is out of range return a clear failure (e.g., panic/log.Fatalf or otherwise surface an error) instead of letting the invalid int propagate across the FFI; update the WithPort function to perform these checks and only call WithPortSpec(PortSpec{Host: host, Guest: guest}) when the inputs are valid, referencing the WithPort function, WithPortSpec helper, and PortSpec struct.
🧹 Nitpick comments (1)
sdks/go/options.go (1)
239-361: ⚖️ Poor tradeoffConsider refactoring buildCOptions to stay within the 100-line guideline.
The
buildCOptionsfunction now spans 122 lines, exceeding the 100-line limit specified in the coding guidelines. While most of this function pre-dates this PR, the addition of port-forwarding logic (Lines 305-307) increases the length further. Consider extracting configuration-setting blocks into focused helper functions (e.g.,applyVolumeOptions,applyPortOptions,applyNetworkOptions) to improve cohesion and readability.Source: Coding guidelines
🤖 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 `@sdks/go/options.go`:
- Around line 84-89: Add a comprehensive doc comment for the exported PortSpec
struct describing its purpose as a container for port mapping configuration
(host and container/guest port), the meaning and units of each field (Host,
Guest as port numbers, Protocol as PortProtocol enum, HostIP as optional host
interface/address to bind), expected valid values and defaults (e.g., 0 meaning
"ephemeral"/unspecified), and any behavior/validation assumptions used by code
that consumes PortSpec (e.g., whether HostIP can be empty to bind all
interfaces). Place the comment immediately above the `type PortSpec struct { ...
}` declaration and reference the fields Host, Guest, Protocol, and HostIP by
name in the docstring.
- Around line 77-82: The exported PortProtocol type and its constants are
missing documentation; add Go doc comments above the type declaration for
PortProtocol explaining what it represents (protocol for container/network
ports), and add doc comments above each constant PortProtocolTCP and
PortProtocolUDP describing their meanings ("TCP protocol" and "UDP protocol")
and any usage constraints or examples; ensure comments follow Go doc style
(complete sentences starting with the symbol name) and place them immediately
above the type and constant declarations.
- Around line 305-307: The loop over cfg.ports in the function that calls
C.boxlite_options_add_port currently only passes port.Guest and port.Host and
silently drops PortSpec.Protocol and PortSpec.HostIP; update the code by adding
a clear comment adjacent to the loop (and referencing cfg.ports, PortSpec, and
C.boxlite_options_add_port) stating that the C/Rust layer currently ignores
Protocol and HostIP (protocol is hardcoded to TCP and host_ip to None) and that
this is a known limitation until sdks/c/src/options.rs is extended; do not
change behavior, just document the limitation so callers and tests are aware.
- Around line 185-194: WithPortSpec currently sets PortSpec.Protocol and HostIP
into cfg.ports but those fields are never forwarded to the runtime
(buildCOptions calls C.boxlite_options_add_port with only Guest and Host), and
options_add_port in the C/Rust layer drops Protocol/HostIP and unsafely casts
ints to u16; fix by plumbing Protocol and HostIP through the FFI (update
buildCOptions to call a new C function C.boxlite_options_add_port_ex that
accepts protocol and host_ip, implement corresponding C shim and Rust handler in
sdks/c/src/options.rs to map protocol and Option<InetAddr> properly) or
explicitly document they’re unsupported, and add validation in WithPortSpec
(and/or the C shim) to enforce guest in 1..=65535 and host in 0..=65535 before
converting to u16 to avoid truncation (adjust tests to exercise UDP/HostIP
forwarding or the documented limitation).
---
Duplicate comments:
In `@sdks/go/options.go`:
- Around line 179-183: Validate port ranges in WithPort before constructing
PortSpec: check guest is within 1..65535 and host is within 0..65535, and if any
value is out of range return a clear failure (e.g., panic/log.Fatalf or
otherwise surface an error) instead of letting the invalid int propagate across
the FFI; update the WithPort function to perform these checks and only call
WithPortSpec(PortSpec{Host: host, Guest: guest}) when the inputs are valid,
referencing the WithPort function, WithPortSpec helper, and PortSpec struct.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 69d56217-ebe9-4b5e-83e3-c4282b79cb2c
📒 Files selected for processing (5)
apps/api/src/organization/services/default-organization-membership.spec.tsapps/runner/pkg/boxlite/client.goapps/runner/pkg/boxlite/stubs.gosdks/go/boxlite_test.gosdks/go/options.go
🚧 Files skipped from review as they are similar to previous changes (4)
- sdks/go/boxlite_test.go
- apps/runner/pkg/boxlite/stubs.go
- apps/runner/pkg/boxlite/client.go
- apps/api/src/organization/services/default-organization-membership.spec.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
sdks/go/options.go (1)
179-189: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExpand docstring to meet comprehensive documentation guidelines.
The current docstring is too brief for an exported function. Per coding guidelines, comprehensive docstrings are required for all public functions. The docstring should document:
- Parameter meanings and valid ranges (guest: 1-65535, host: 0-65535 where 0=dynamic)
- Protocol behavior (currently hardcoded to TCP)
- Return value semantics (returns a BoxOption)
- Usage example
📝 Suggested comprehensive docstring
-// WithPort forwards a host port to a guest port. A host of 0 lets the runtime -// assign one dynamically. +// WithPort publishes a guest (container) port on a host port. +// +// Parameters: +// - host: the port on the host machine (0 for dynamic assignment, range 0-65535) +// - guest: the port inside the sandbox (range 1-65535) +// +// The protocol is currently hardcoded to TCP. UDP and host IP binding are not yet supported. +// +// Example: +// WithPort(8080, 3000) // Publish container port 3000 on host port 8080 +// WithPort(0, 8080) // Publish container port 8080 on a dynamically assigned host port func WithPort(host, guest int) BoxOption {As per coding guidelines: "Write comprehensive docstrings for all public functions and classes."
Validation concern from previous review still applies.
The previous review correctly identified that port range validation is missing. Guest and host ports are not validated before crossing the FFI boundary, which can cause
u16truncation in the Rust layer. This is still a valid concern.🤖 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/go/options.go` around lines 179 - 189, Add a comprehensive docstring for the exported function WithPort describing parameters (guest must be 1–65535, host must be 0–65535 where 0 means dynamic), the hardcoded TCP protocol behavior (portProtocolTCP), the return value (BoxOption that mutates *boxConfig by appending a portSpec), and a short usage example; additionally, add input validation in WithPort to reject invalid guest/host values before mutating boxConfig (return or log an error/ panic according to project conventions) to prevent u16 truncation across the FFI boundary—refer to the WithPort function, portSpec, boxConfig, and portProtocolTCP symbols when making changes.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@sdks/go/options.go`:
- Around line 179-189: Add a comprehensive docstring for the exported function
WithPort describing parameters (guest must be 1–65535, host must be 0–65535
where 0 means dynamic), the hardcoded TCP protocol behavior (portProtocolTCP),
the return value (BoxOption that mutates *boxConfig by appending a portSpec),
and a short usage example; additionally, add input validation in WithPort to
reject invalid guest/host values before mutating boxConfig (return or log an
error/ panic according to project conventions) to prevent u16 truncation across
the FFI boundary—refer to the WithPort function, portSpec, boxConfig, and
portProtocolTCP symbols when making changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7960a931-beae-4220-a0fb-593b67205d2d
📒 Files selected for processing (5)
apps/api/src/organization/services/default-organization-membership.spec.tsapps/runner/pkg/boxlite/client.goapps/runner/pkg/boxlite/stubs.gosdks/go/boxlite_test.gosdks/go/options.go
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/runner/pkg/boxlite/stubs.go
- apps/runner/pkg/boxlite/client.go
be64a6d to
8471545
Compare
Keep WithPort as the public Go entry point, but mirror the Python SDK's internal port shape: host, guest, protocol, and host_ip. Convert that shape through toCSpec as host_port, guest_port, protocol, and host_ip before calling the existing C ABI.
8471545 to
a40b2b6
Compare
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
2 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.
Summary
WithPortoption required by the runner build path after Converge A2 + MVP box journey into main (super PR — review & split) #715Verification
make test:changedwas triggered by the local hook and failed in the existing macOS Go/native link test path before push--no-verifyper handoff flow; CI is the source of truth for this PRSummary by CodeRabbit
New Features
Bug Fixes
Tests