fix(cli): release sandbox forwards on full stop - #6395
Closed
chengjiew wants to merge 1 commit into
Closed
Conversation
Signed-off-by: Chengjie Wang <chengjiew@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds a ChangesGateway Port Release Fix
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as stopAll
participant Svc as stopSandboxForwards
participant OpenShell
participant GatewayStop as releaseGatewayPortForStop
CLI->>Svc: stopSandboxForwards(sandboxName)
Svc->>OpenShell: openshell forward list
OpenShell-->>Svc: list of forwards
Svc->>Svc: filter ports via getOccupiedPorts(sandboxName)
loop for each matching port
Svc->>OpenShell: openshell forward stop
end
Svc-->>CLI: log "Released N managed host forward(s)"
CLI->>GatewayStop: releaseGatewayPortForStop(...)
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/tunnel/services-sandbox.test.ts (2)
245-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest title lacks issue-number suffix.
If this fix is tracked by a local issue (the linked issue this PR addresses), the title should end with a
(#1234)-style reference per repo convention.As per coding guidelines,
**/*.test.ts: "Write behavior-oriented test titles, and put local issue references in a final(#1234)suffix."🤖 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 `@src/lib/tunnel/services-sandbox.test.ts` at line 245, Update the test title in services-sandbox.test.ts so it follows the repo’s test naming convention by ending with the local issue reference suffix in the form (`#1234`). Keep the title behavior-oriented, and apply the change to the existing it(...) block for the deprecated full stop case.Source: Coding guidelines
247-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock sequence is positionally coupled to unrelated upstream
spawnSynccalls.The list-output mock is the 3rd
mockReturnValueOnce, relying onstopSandboxChannelsViaKubectland the "sandbox exec" fallback each making exactly onespawnSynccall beforestopSandboxForwardsruns. If that call count/order shifts later, this mock would silently attach to the wrong call.Since "If the return value depends on the arguments passed to the mocked function, use mockImplementation()", keying the response off the command arguments (e.g.,
["forward", "list"]vs. others) would make this test robust to unrelated upstream call-order changes.🤖 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 `@src/lib/tunnel/services-sandbox.test.ts` around lines 247 - 259, The test’s spawnSyncSpy setup is brittle because the list-output response is tied to call order rather than the command being invoked. Update the mocking in services-sandbox.test.ts so the response for the forwarding list path is selected by inspecting the spawnSync arguments inside the spy implementation, and keep the existing behaviors for stopSandboxChannelsViaKubectl, the sandbox exec fallback, and stopSandboxForwards. This should be anchored around the spawnSyncSpy setup and the stopSandboxForwards-related expectations so unrelated upstream spawnSync calls cannot shift the mock sequence.
🤖 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 `@src/lib/tunnel/services.ts`:
- Around line 595-627: The success log in stopSandboxForwards currently counts
targeted ports rather than actual successful stops, so it can report releases
even when a forward stop fails. Update stopSandboxForwards to inspect each
spawnSync(openshell, ["forward", "stop", ...]) result, count only successful
stops, and log that count instead of ports.length. Keep the existing behavior
around resolveOpenshell, getOccupiedPorts, and the final info message, but base
the message on verified stop outcomes.
---
Nitpick comments:
In `@src/lib/tunnel/services-sandbox.test.ts`:
- Line 245: Update the test title in services-sandbox.test.ts so it follows the
repo’s test naming convention by ending with the local issue reference suffix in
the form (`#1234`). Keep the title behavior-oriented, and apply the change to the
existing it(...) block for the deprecated full stop case.
- Around line 247-259: The test’s spawnSyncSpy setup is brittle because the
list-output response is tied to call order rather than the command being
invoked. Update the mocking in services-sandbox.test.ts so the response for the
forwarding list path is selected by inspecting the spawnSync arguments inside
the spy implementation, and keep the existing behaviors for
stopSandboxChannelsViaKubectl, the sandbox exec fallback, and
stopSandboxForwards. This should be anchored around the spawnSyncSpy setup and
the stopSandboxForwards-related expectations so unrelated upstream spawnSync
calls cannot shift the mock sequence.
🪄 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: Enterprise
Run ID: 99cc10f1-2223-4d40-b8f2-fb748f231427
📒 Files selected for processing (2)
src/lib/tunnel/services-sandbox.test.tssrc/lib/tunnel/services.ts
Comment on lines
+595
to
+627
| function stopSandboxForwards(sandboxName: string): void { | ||
| const openshell = resolveOpenshell(); | ||
| if (!openshell) { | ||
| warn("openshell not found — cannot release managed host forwards."); | ||
| return; | ||
| } | ||
|
|
||
| const listResult = spawnSync(openshell, ["forward", "list"], { | ||
| encoding: "utf-8", | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| timeout: OPENSHELL_PROBE_TIMEOUT_MS, | ||
| }); | ||
| if (listResult.status !== 0) { | ||
| warn("Could not list OpenShell forwards — managed host forwards may still be running."); | ||
| return; | ||
| } | ||
|
|
||
| const output = listResult.stdout ?? ""; | ||
| const ports = [...getOccupiedPorts(output).entries()] | ||
| .filter(([, owner]) => owner === sandboxName) | ||
| .map(([port]) => port); | ||
| for (const port of ports) { | ||
| spawnSync(openshell, ["forward", "stop", port, sandboxName], { | ||
| encoding: "utf-8", | ||
| stdio: "ignore", | ||
| timeout: OPENSHELL_OPERATION_TIMEOUT_MS, | ||
| }); | ||
| } | ||
| if (ports.length > 0) { | ||
| info(`Released ${String(ports.length)} managed host forward(s) for sandbox ${sandboxName}.`); | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Success count doesn't verify actual forward stop outcome.
ports.length reflects how many ports were targeted, not how many stops actually succeeded — the spawnSync result for each forward stop call is discarded. If a stop fails, the log still reports Released N managed host forward(s), which is misleading precisely for the scenario this PR is fixing (confirming that a forward like port 8642 was actually released).
🐛 Proposed fix to track actual successes
- for (const port of ports) {
- spawnSync(openshell, ["forward", "stop", port, sandboxName], {
- encoding: "utf-8",
- stdio: "ignore",
- timeout: OPENSHELL_OPERATION_TIMEOUT_MS,
- });
- }
- if (ports.length > 0) {
- info(`Released ${String(ports.length)} managed host forward(s) for sandbox ${sandboxName}.`);
- }
+ let released = 0;
+ for (const port of ports) {
+ const stopResult = spawnSync(openshell, ["forward", "stop", port, sandboxName], {
+ encoding: "utf-8",
+ stdio: "ignore",
+ timeout: OPENSHELL_OPERATION_TIMEOUT_MS,
+ });
+ if (stopResult.status === 0) released += 1;
+ }
+ if (released > 0) {
+ info(`Released ${String(released)} managed host forward(s) for sandbox ${sandboxName}.`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function stopSandboxForwards(sandboxName: string): void { | |
| const openshell = resolveOpenshell(); | |
| if (!openshell) { | |
| warn("openshell not found — cannot release managed host forwards."); | |
| return; | |
| } | |
| const listResult = spawnSync(openshell, ["forward", "list"], { | |
| encoding: "utf-8", | |
| stdio: ["ignore", "pipe", "pipe"], | |
| timeout: OPENSHELL_PROBE_TIMEOUT_MS, | |
| }); | |
| if (listResult.status !== 0) { | |
| warn("Could not list OpenShell forwards — managed host forwards may still be running."); | |
| return; | |
| } | |
| const output = listResult.stdout ?? ""; | |
| const ports = [...getOccupiedPorts(output).entries()] | |
| .filter(([, owner]) => owner === sandboxName) | |
| .map(([port]) => port); | |
| for (const port of ports) { | |
| spawnSync(openshell, ["forward", "stop", port, sandboxName], { | |
| encoding: "utf-8", | |
| stdio: "ignore", | |
| timeout: OPENSHELL_OPERATION_TIMEOUT_MS, | |
| }); | |
| } | |
| if (ports.length > 0) { | |
| info(`Released ${String(ports.length)} managed host forward(s) for sandbox ${sandboxName}.`); | |
| } | |
| } | |
| function stopSandboxForwards(sandboxName: string): void { | |
| const openshell = resolveOpenshell(); | |
| if (!openshell) { | |
| warn("openshell not found — cannot release managed host forwards."); | |
| return; | |
| } | |
| const listResult = spawnSync(openshell, ["forward", "list"], { | |
| encoding: "utf-8", | |
| stdio: ["ignore", "pipe", "pipe"], | |
| timeout: OPENSHELL_PROBE_TIMEOUT_MS, | |
| }); | |
| if (listResult.status !== 0) { | |
| warn("Could not list OpenShell forwards — managed host forwards may still be running."); | |
| return; | |
| } | |
| const output = listResult.stdout ?? ""; | |
| const ports = [...getOccupiedPorts(output).entries()] | |
| .filter(([, owner]) => owner === sandboxName) | |
| .map(([port]) => port); | |
| let released = 0; | |
| for (const port of ports) { | |
| const stopResult = spawnSync(openshell, ["forward", "stop", port, sandboxName], { | |
| encoding: "utf-8", | |
| stdio: "ignore", | |
| timeout: OPENSHELL_OPERATION_TIMEOUT_MS, | |
| }); | |
| if (stopResult.status === 0) released += 1; | |
| } | |
| if (released > 0) { | |
| info(`Released ${String(released)} managed host forward(s) for sandbox ${sandboxName}.`); | |
| } | |
| } |
🤖 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 `@src/lib/tunnel/services.ts` around lines 595 - 627, The success log in
stopSandboxForwards currently counts targeted ports rather than actual
successful stops, so it can report releases even when a forward stop fails.
Update stopSandboxForwards to inspect each spawnSync(openshell, ["forward",
"stop", ...]) result, count only successful stops, and log that count instead of
ports.length. Keep the existing behavior around resolveOpenshell,
getOccupiedPorts, and the final info message, but base the message on verified
stop outcomes.
cv
requested changes
Jul 7, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
The forward cleanup currently reports every attempted stop as released even when openshell forward stop fails, times out, or cannot spawn. stopSandboxForwards discards each spawnSync result and logs the selected port count unconditionally, so the linked port-leak bug can persist behind a success message. Please inspect each stop result, warn with the affected sandbox/port on failure, count only confirmed successes, and add a failed-stop/timeout regression that proves the command does not claim the forward was released. The existing happy-path test defaults all stop calls to status 0 and does not exercise this behavior.
wscurran
added
v0.0.77
area: cli
Contributor
|
✨ Thanks for the fix, Chengjie. The targeted forward release and product-name cleanup look correct, and the regression coverage for port 8642 and multi-sandbox preservation gives good confidence. Ready for maintainer review. Related open issues: Related open issues: |
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
3 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
This PR makes deprecated full stop release OpenShell forwards owned by the selected sandbox before releasing the host gateway. It fixes
nemohermes stopleaving the Hermes API forward on port 8642 alive and makes the in-sandbox gateway stop message use the active agent product name.Related Issue
Fixes #6392
Changes
openshell forward listentries during legacy full stop and stop only forwards owned by the selected sandbox usingforward stop <port> <sandbox>.OpenClaw gatewaystop-starting copy with the active agent product name.8642and another owned forward while preserving another sandbox's forward.Type of Change
Quality Gates
openshell forward listownership and usesforward stop <port> <sandbox>to avoid collateral teardown.Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm test -- src/lib/tunnel/services-sandbox.test.ts src/lib/tunnel/services-gateway-ownership.test.ts src/lib/tunnel/service-command.test.tspassed, 3 files / 36 tests.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Chengjie Wang chengjiew@nvidia.com
Summary by CodeRabbit