Reverse Port Forwarding (ssh -R) [Spec + Impl] - #47
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds ChangesReverse port forwarding SDK
Project metadata updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
==========================================
+ Coverage 89.41% 89.43% +0.02%
==========================================
Files 78 78
Lines 4901 4932 +31
==========================================
+ Hits 4382 4411 +29
Misses 352 352
- Partials 167 169 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
openshell/v1/internal/grpc/conn_test.go (1)
15-74: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the transport before asserting successful creation.
grpc.NewClientcreates an idle client and connects later, so these tests currently only validate argument parsing and construction.Call
conn.Connect()and wait forconnectivity.Readybefore closing. Add an in-process listener/RPC for the TLS and insecure cases, and capture server-side metadata inTestNewConnectionHTTPWithTokenAuthto verify theauthorizationheader is sent.🤖 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 `@openshell/v1/internal/grpc/conn_test.go` around lines 15 - 74, Update the connection tests around NewConnection to exercise the transport by calling conn.Connect() and waiting for connectivity.Ready before closing. Replace unreachable TLS and insecure endpoints with in-process listeners and RPC-capable servers configured for the relevant transport, and in TestNewConnectionHTTPWithTokenAuth capture server metadata and assert the authorization header contains the expected token.openshell/v1/grpc_errors.go (1)
13-20: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCheck wrapped context context sentinels before mapping them to
ErrorInternal.
contextErroronly compares the returned error value directly, sofmt.Errorf("wait failed: %w", context.DeadlineExceeded)maps toErrorInternal. Useerrors.Isfor bothcontext.DeadlineExceededandcontext.Canceled, add tests for wrapped context errors, and updateWaitReadyto use this helper instead of producing anErrorInternalmessage.Proposed fix
-import "context" +import ( + "context" + "errors" +) ... -switch err { -case context.DeadlineExceeded: +switch { +case errors.Is(err, context.DeadlineExceeded): ... -case context.Canceled: +case errors.Is(err, context.Canceled):🤖 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 `@openshell/v1/grpc_errors.go` around lines 13 - 20, Update contextError to use errors.Is when matching context.DeadlineExceeded and context.Canceled so wrapped sentinels map to their existing deadline and cancellation codes; add coverage for wrapped errors, and change WaitReady to route failures through contextError instead of constructing an ErrorInternal result directly.openshell/v1/fake/tcp_test.go (1)
210-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert option values directly.
This test only proves that the options do not change the returned error. Because
fakeTCPClient.RemoteListenignores options and returnsErrorUnimplemented, the test passes even if either option stops populating its field. Add a package-level test inopenshell/v1/tcp_client_test.gothat applies both options toremoteListenConfigand assertsbindAddressandserviceID.The implementation plan requires option tests to verify the populated configuration.
🤖 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 `@openshell/v1/fake/tcp_test.go` around lines 210 - 218, Add a package-level test in tcp_client_test.go that applies WithRemoteBindAddress and WithRemoteListenServiceID to a remoteListenConfig, then directly assert that bindAddress and serviceID contain the expected values. Keep TestFakeTCP_RemoteListen_WithOptions focused on the unimplemented error behavior rather than using it to validate option population.
🤖 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 `@openshell/v1/doc.go`:
- Around line 245-264: The RemoteListen documentation currently presents
blocking tunnel behavior even though the real and fake implementations return
ErrorUnimplemented. Update the “Reverse Port Forwarding” section around
RemoteListen to explicitly state that the method remains unavailable until
upstream proto support lands, or clearly label the example as future behavior.
In `@openshell/v1/internal/converter/coverage_test.go`:
- Around line 6-42: Update assertAllFieldsCovered to use Testify assertions
instead of t.Errorf for both uncovered fields and stale handled-field checks,
adding the appropriate testify import while retaining the existing test helper
behavior.
- Around line 16-42: Update assertAllFieldsCovered to validate every name in
skipped against msg.Fields(), mirroring the existing stale-entry validation for
handled. Report skipped names that no longer exist in the proto descriptor while
preserving the current coverage checks.
In `@openshell/v1/internal/converter/network_policy.go`:
- Around line 314-331: Update mcpOptionsFromProto and mcpOptionsToProto to
deep-copy each non-nil boolean pointer instead of reusing the source pointers,
while preserving nil values and existing nil-object handling. Add tests that
mutate either converted MCP options object and verify the original policy
object's boolean fields remain unchanged in both conversion directions.
In `@openshell/v1/internal/grpc/conn_test.go`:
- Around line 6-13: Update the tests in conn_test.go to use testify assertions
consistently: replace each t.Fatalf error check with require.NoError or
assert.NoError as appropriate, adding the testify assertion import and
preserving the existing failure behavior.
In `@openshell/v1/sandbox_client.go`:
- Line 241: Update the asynchronous receive-error path in Watch so the recvErr
assigned to EventError.Err is passed through converter.FromGRPCError, matching
the initial stream.Recv error handling; add coverage for a post-event
codes.Unavailable error and assert the published event satisfies IsUnavailable.
In `@openshell/v1/tcp_client.go`:
- Around line 88-104: Update tcpClient.RemoteListen to check the client
lifecycle state after validating inputs and return the established Unavailable
status when t is closed, before the existing Unimplemented fallback. Add a
regression test using a real client that closes it, invokes RemoteListen with
valid arguments, and verifies the Unavailable result.
In `@specs/025-reverse-port-forwarding/REVIEW-CODE.md`:
- Line 180: Update the fenced test-output block at the referenced Markdown
section to include the text language tag, preserving its existing command-output
contents.
---
Nitpick comments:
In `@openshell/v1/fake/tcp_test.go`:
- Around line 210-218: Add a package-level test in tcp_client_test.go that
applies WithRemoteBindAddress and WithRemoteListenServiceID to a
remoteListenConfig, then directly assert that bindAddress and serviceID contain
the expected values. Keep TestFakeTCP_RemoteListen_WithOptions focused on the
unimplemented error behavior rather than using it to validate option population.
In `@openshell/v1/grpc_errors.go`:
- Around line 13-20: Update contextError to use errors.Is when matching
context.DeadlineExceeded and context.Canceled so wrapped sentinels map to their
existing deadline and cancellation codes; add coverage for wrapped errors, and
change WaitReady to route failures through contextError instead of constructing
an ErrorInternal result directly.
In `@openshell/v1/internal/grpc/conn_test.go`:
- Around line 15-74: Update the connection tests around NewConnection to
exercise the transport by calling conn.Connect() and waiting for
connectivity.Ready before closing. Replace unreachable TLS and insecure
endpoints with in-process listeners and RPC-capable servers configured for the
relevant transport, and in TestNewConnectionHTTPWithTokenAuth capture server
metadata and assert the authorization header contains the expected token.
🪄 Autofix
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: 62d441d2-2248-4431-8786-4e58693e3c0f
📒 Files selected for processing (28)
.gitignoreCLAUDE.mdopenshell/v1/config_client_test.goopenshell/v1/doc.goopenshell/v1/errors_test.goopenshell/v1/fake/tcp.goopenshell/v1/fake/tcp_test.goopenshell/v1/grpc_errors.goopenshell/v1/internal/converter/coverage_test.goopenshell/v1/internal/converter/errors.goopenshell/v1/internal/converter/network_policy.goopenshell/v1/internal/grpc/conn.goopenshell/v1/internal/grpc/conn_test.goopenshell/v1/sandbox_client.goopenshell/v1/tcp.goopenshell/v1/tcp_client.goopenshell/v1/tcp_client_test.goopenshell/v1/types/errors.goopenshell/v1/types/network_policy.goopenshell/v1/types/types.goopenshell/v1/types/watch.gospecs/025-reverse-port-forwarding/REVIEW-CODE.mdspecs/025-reverse-port-forwarding/checklists/requirements.mdspecs/025-reverse-port-forwarding/data-model.mdspecs/025-reverse-port-forwarding/plan.mdspecs/025-reverse-port-forwarding/research.mdspecs/025-reverse-port-forwarding/spec.mdspecs/025-reverse-port-forwarding/tasks.md
| import ( | ||
| "testing" | ||
|
|
||
| pb "github.com/rhuss/openshell-sdk-go/proto/openshellv1" | ||
| sbv1 "github.com/rhuss/openshell-sdk-go/proto/sandboxv1" | ||
| "google.golang.org/protobuf/reflect/protoreflect" | ||
| ) | ||
|
|
||
| type fieldSet map[string]bool | ||
|
|
||
| func assertAllFieldsCovered(t *testing.T, msg protoreflect.MessageDescriptor, handled, skipped fieldSet) { | ||
| t.Helper() | ||
|
|
||
| fields := msg.Fields() | ||
| for i := 0; i < fields.Len(); i++ { | ||
| name := string(fields.Get(i).Name()) | ||
| if handled[name] { | ||
| continue | ||
| } | ||
| if skipped[name] { | ||
| continue | ||
| } | ||
| t.Errorf("proto field %s.%s is not handled by the converter", msg.FullName(), name) | ||
| } | ||
|
|
||
| for name := range handled { | ||
| found := false | ||
| for i := 0; i < fields.Len(); i++ { | ||
| if string(fields.Get(i).Name()) == name { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| t.Errorf("handled field %q does not exist in proto %s (stale entry)", name, msg.FullName()) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use Testify assertions in this test.
This file only uses t.Errorf. Update assertAllFieldsCovered to use assert or require.
As per coding guidelines, “Go tests must use the Go testing package with testify assertions (assert/require).”
🤖 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 `@openshell/v1/internal/converter/coverage_test.go` around lines 6 - 42, Update
assertAllFieldsCovered to use Testify assertions instead of t.Errorf for both
uncovered fields and stale handled-field checks, adding the appropriate testify
import while retaining the existing test helper behavior.
Source: Coding guidelines
There was a problem hiding this comment.
Valid. The helper uses t.Errorf while the project convention requires testify assertions. Will update to use assert/require.
There was a problem hiding this comment.
@rhuss, acknowledged. The finding remains open until the helper uses Testify assertions.
You are interacting with an AI system.
| func assertAllFieldsCovered(t *testing.T, msg protoreflect.MessageDescriptor, handled, skipped fieldSet) { | ||
| t.Helper() | ||
|
|
||
| fields := msg.Fields() | ||
| for i := 0; i < fields.Len(); i++ { | ||
| name := string(fields.Get(i).Name()) | ||
| if handled[name] { | ||
| continue | ||
| } | ||
| if skipped[name] { | ||
| continue | ||
| } | ||
| t.Errorf("proto field %s.%s is not handled by the converter", msg.FullName(), name) | ||
| } | ||
|
|
||
| for name := range handled { | ||
| found := false | ||
| for i := 0; i < fields.Len(); i++ { | ||
| if string(fields.Get(i).Name()) == name { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| t.Errorf("handled field %q does not exist in proto %s (stale entry)", name, msg.FullName()) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Validate stale skipped entries.
The helper validates stale names only in handled. A removed or renamed skipped proto field remains silently accepted. Validate every skipped name against the descriptor too.
🤖 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 `@openshell/v1/internal/converter/coverage_test.go` around lines 16 - 42,
Update assertAllFieldsCovered to validate every name in skipped against
msg.Fields(), mirroring the existing stale-entry validation for handled. Report
skipped names that no longer exist in the proto descriptor while preserving the
current coverage checks.
There was a problem hiding this comment.
Valid. Stale entries in the skipped list are silently accepted. Will add validation against the proto descriptor.
There was a problem hiding this comment.
@rhuss, acknowledged. Validating skipped names against msg.Fields() will prevent stale exclusions from hiding removed or renamed proto fields.
You are interacting with an AI system.
| func (t *tcpClient) RemoteListen(_ context.Context, _, sandboxName string, remotePort uint32, localTarget string, _ ...RemoteListenOption) error { | ||
| if sandboxName == "" { | ||
| return &StatusError{Code: ErrorInvalidArgument, Message: "sandbox name must not be empty"} | ||
| } | ||
| if remotePort == 0 || remotePort > 65535 { | ||
| return &StatusError{ | ||
| Code: ErrorInvalidArgument, | ||
| Message: fmt.Sprintf("port must be in range 1-65535, got %d", remotePort), | ||
| } | ||
| } | ||
| if _, _, err := net.SplitHostPort(localTarget); err != nil { | ||
| return &StatusError{ | ||
| Code: ErrorInvalidArgument, | ||
| Message: fmt.Sprintf("invalid localTarget %q: %v", localTarget, err), | ||
| } | ||
| } | ||
| return &StatusError{Code: ErrorUnimplemented, Message: "RemoteListen requires upstream proto extension (not yet available)"} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Return Unavailable after the real client closes.
RemoteListen does not inspect t or issue an RPC. A closed client with valid input therefore always returns Unimplemented. This violates the specified closed-client contract and breaks real/fake validation parity.
Use the client lifecycle state before returning Unimplemented. Add a real-client regression test for the closed state.
🤖 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 `@openshell/v1/tcp_client.go` around lines 88 - 104, Update
tcpClient.RemoteListen to check the client lifecycle state after validating
inputs and return the established Unavailable status when t is closed, before
the existing Unimplemented fallback. Add a regression test using a real client
that closes it, invokes RemoteListen with valid arguments, and verifies the
Unavailable result.
There was a problem hiding this comment.
Valid. RemoteListen should check the client lifecycle state and return Unavailable when closed, before the Unimplemented fallback. This maintains fake-real parity per project invariants.
There was a problem hiding this comment.
@rhuss, acknowledged. The finding remains valid.
RemoteListen must check the client lifecycle state after input validation. If the client is closed, it must return Unavailable before the Unimplemented fallback. The regression test must verify this real-client behavior.
🐇
✏️ Learnings added
Learnt from: rhuss
Repo: rhuss/openshell-sdk-go PR: 47
File: openshell/v1/tcp_client.go:88-104
Timestamp: 2026-08-08T05:37:54.808Z
Learning: In the OpenShell Go SDK, `tcpClient.RemoteListen` must preserve fake/real client parity: after valid argument validation, it must return the established `Unavailable` status when the client is closed, before it returns `ErrorUnimplemented` for unavailable upstream protocol support.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
|
|
||
| ### Post-Fix Verification | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the fenced test-output block.
markdownlint-cli2 reports MD040 at Line [180] because the fenced block has no language. Use text for this command-output block.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 180-180: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@specs/025-reverse-port-forwarding/REVIEW-CODE.md` at line 180, Update the
fenced test-output block at the referenced Markdown section to include the text
language tag, preserving its existing command-output contents.
Source: Linters/SAST tools
There was a problem hiding this comment.
Valid. Will add the text language tag.
There was a problem hiding this comment.
@rhuss, acknowledged. Adding the text language tag will resolve MD040 for the command-output block.
You are interacting with an AI system.
6695de4 to
9f537f6
Compare
There was a problem hiding this comment.
Pull request overview
Adds the SDK API surface for reverse port forwarding while upstream protocol support remains unavailable.
Changes:
- Adds
RemoteListen, options, validation, and real/fake stubs. - Adds unit tests and usage documentation.
- Adds feature specifications, plans, and review artifacts.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
.gitignore |
Ignores local MCP configuration. |
CLAUDE.md |
Points to the current feature plan. |
openshell/v1/tcp.go |
Defines the new API and options. |
openshell/v1/tcp_client.go |
Adds the real-client stub and validation. |
openshell/v1/tcp_client_test.go |
Tests real-client validation and stub behavior. |
openshell/v1/fake/tcp.go |
Adds fake-client validation and stub behavior. |
openshell/v1/fake/tcp_test.go |
Tests fake-client behavior and validation. |
openshell/v1/doc.go |
Documents reverse forwarding usage. |
specs/025-reverse-port-forwarding/spec.md |
Defines requirements and scenarios. |
specs/025-reverse-port-forwarding/plan.md |
Describes implementation strategy. |
specs/025-reverse-port-forwarding/tasks.md |
Tracks implementation tasks. |
specs/025-reverse-port-forwarding/research.md |
Records design decisions. |
specs/025-reverse-port-forwarding/data-model.md |
Documents API entities and relationships. |
specs/025-reverse-port-forwarding/REVIEW-CODE.md |
Records the prior compliance review. |
specs/025-reverse-port-forwarding/checklists/requirements.md |
Tracks specification readiness. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Fix fake Forward parity: add sandboxName validation (was missing, violating fake-real parity invariant) - Name context parameter in real RemoteListen stub (prevents future implementation from discarding it) - Add error message content assertions to RemoteListen tests - Add bare IPv6 malformed target case to fake parity test Assisted-By: 🤖 Claude Code
Adds reverse port forwarding review findings (fake Forward parity, context parameter, bindAddress default, Close error surfacing, RemoteListen return type design, closed-client detection pattern) to the upstream contribution checklist. Assisted-By: 🤖 Claude Code
Add RemoteListen method to TCPInterface enabling reverse port forwarding from sandbox to client (ssh -R equivalent). Includes functional options (WithRemoteBindAddress, WithRemoteListenServiceID), input validation, fake client support with Unimplemented error, and comprehensive tests. Real gRPC implementation deferred pending upstream proto extension.
9c276d2 to
36fc410
Compare
Summary
Implementation of Reverse Port Forwarding (ssh -R) for the OpenShell SDK.
Adds
RemoteListenmethod toTCPInterfacethat sets up reverse port forwardingfrom a sandbox back to the client. Since the upstream proto extension does not yet
exist, this covers the SDK API surface: interface method, functional options, input
validation, real client stub, and fake client with validation parity.
Changes
openshell/v1/tcp.go: AddedRemoteListentoTCPInterface,RemoteListenOption,WithRemoteBindAddress,WithRemoteListenServiceIDopenshell/v1/tcp_client.go: Real client stub (validates inputs, returns Unimplemented)openshell/v1/fake/tcp.go: Fake client with validation parityopenshell/v1/tcp_client_test.go: Comprehensive tests for real clientopenshell/v1/fake/tcp_test.go: Comprehensive tests for fake clientopenshell/v1/doc.go: RemoteListen usage examplesArtifacts
specs/025-reverse-port-forwarding/spec.mdspecs/025-reverse-port-forwarding/plan.mdspecs/025-reverse-port-forwarding/tasks.mdSummary by CodeRabbit