fix(forward): stop default-port forward when custom dashboard port is set

[Sanjays2402]

Summary

When NEMOCLAW_DASHBOARD_PORT is set to a non-default value (e.g. 19003), openshell forward list shows two forwards: one on the custom port and one on the default 18789.

Root cause: openshell sandbox create auto-forwards the default port 18789 (baked into the Docker image). ensureDashboardForward() then creates the correct custom-port forward but never cleans up the stale default-port forward.

Related Issue

Fixes #2007

Changes

• src/lib/onboard.ts — in ensureDashboardForward(), when the resolved port differs from the hardcoded default (18789), explicitly stop the default-port forward before starting the custom one.

Type of Change

• Code change (feature, bug fix, or refactor)

Verification

• npx prek run --all-files passes
• npm test passes
• Tests added or updated for new or changed behavior
• No secrets, API keys, or credentials committed
• Docs updated for user-facing behavior changes
• make docs builds without warnings (doc changes only)
• Doc pages follow the style guide (doc changes only)
• New doc pages include SPDX header and frontmatter (new pages only)

Manual repro: set NEMOCLAW_DASHBOARD_PORT=19003, run onboarding, confirm openshell forward list shows only the custom-port forward.

AI Disclosure

• AI-assisted — tool: OpenClaw (Claude Opus 4.7 via GitHub Copilot)

Summary by CodeRabbit

• Refactor

  • Improved internal code organization by making port configuration constants publicly accessible.

• Tests

  • Added regression test to verify dashboard port handling consistency and prevent configuration drift.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 7218476e-b129-41e7-a39d-703cedb4b1d7

📥 Commits

Reviewing files that changed from the base of the PR and between 3c54141 and 8b5640e.

📒 Files selected for processing (1)

• src/lib/ports.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• src/lib/ports.ts

---

📝 Walkthrough

Walkthrough

A port constant is exported to enable consistent usage across the codebase, and a regression test is added to validate that dashboard cleanup logic derives the forward-stop port from the shared exported constant rather than a hardcoded value.

Changes

Cohort / File(s)	Summary
Port Constant Export src/lib/ports.ts	SANDBOX_DASHBOARD_PORT changed from private to exported constant, enabling reuse across modules for centralized port configuration.
Regression Test test/onboard.test.ts	Added test validating that dashboard-forward cleanup logic uses the exported SANDBOX_DASHBOARD_PORT constant rather than hardcoded port "18789", preventing duplicate forwards on default port when custom port is specified.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A constant exported, no more hiding in the dark,
No hardcoded numbers to leave their telltale mark,
One forward true, not two—what harmony we seek!
When ports align as one, our sandbox code is sleek. 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly describes the main change: stopping the default-port forward when a custom dashboard port is set, which is the core fix for the reported issue.
Linked Issues check	✅ Passed	The PR implements the key fix: exporting SANDBOX_DASHBOARD_PORT from ports.ts, using it in onboard.ts instead of a hardcoded literal, and adding logic to stop the default-port forward when a custom port is set.
Out of Scope Changes check	✅ Passed	All changes directly address the linked issue #2007: exporting a constant, refactoring to use it, adding regression tests, and implementing the forward-cleanup logic as required.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Review rate limit: 9/10 reviews remaining, refill in 6 minutes.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[wscurran]

✨ Thanks for submitting this PR that proposes a fix for the default-port forward when a custom dashboard port is set, which could help improve the reliability of the dashboard forwarding.

---

Possibly related open issues:

• #2007 openshell sandbox forward list still pointing to 18789 for additional sandbox

[brandonpelfrey]

Hi @Sanjays2402 !

1. Please merge latest from main.
2. Rather than using DEFAULT_SANDBOX_PORT, please use the existing SANDBOX_DASHBOARD_PORT.
3. Also please add appropriate tests for this cleanup behavior and indicate such in your PR description.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[Sanjays2402]

Thanks @brandonpelfrey — addressed all three points:

1. Merged latest main: rebased on 4e6508d (latest main at time of push).

2. Use SANDBOX_DASHBOARD_PORT: exported the constant from src/lib/ports.ts (it was file-private), imported it in src/lib/onboard.ts, and replaced the inline "18789" literal with String(SANDBOX_DASHBOARD_PORT). Now there's a single source of truth.

3. Tests added: new regression test in test/onboard.test.ts ("cleans up the sandbox-default dashboard forward via SANDBOX_DASHBOARD_PORT (no hardcoded 18789)") asserts:

   • ports.ts exports SANDBOX_DASHBOARD_PORT
   • onboard.ts uses String(SANDBOX_DASHBOARD_PORT) for the cleanup branch
   • the old const DEFAULT_SANDBOX_PORT = "18789" literal is gone (regression guard)

   All commits are DCO-signed. Force-pushed to fix/forward-duplicate-default-port.

[coderabbitai]

🧹 Nitpick comments (1)

test/onboard.test.ts (1)

996-1017: This regression test is structural, not behavioral.

Line [996]–Line [1017] only validate source-text patterns, so runtime cleanup behavior can still regress while this test passes. Please add an execution-level test that asserts actual command behavior for both cases: custom dashboard port (must stop default + start custom) and default dashboard port (must not stop default).

Suggested test-shape diff

+  it("stops stale default forward when custom dashboard port is used, and skips stop on default port", async () => {
+    // Arrange: mock runner/openshell command capture
+    // Case 1: NEMOCLAW_DASHBOARD_PORT=19003
+    //   assert command stream includes:
+    //   - forward stop <String(SANDBOX_DASHBOARD_PORT)>
+    //   - forward start ... 19003
+    // Case 2: default port
+    //   assert command stream does NOT include forward stop <default>
+    //   and includes forward start ... 18789
+  });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/onboard.test.ts` around lines 996 - 1017, Replace the
pure-text/source-regex test with an execution-level test that spawns the
onboarding logic and asserts actual commands run for both scenarios: when a
custom dashboard port is provided the test for onboard.ts should observe calls
to runOpenshell (or the CLI wrapper used) that first stop the default port
(matching defaultPortStr derived from SANDBOX_DASHBOARD_PORT) and then start the
custom port; and when no custom port is provided the test should observe that no
stop command for the default port is issued. Locate symbols
SANDBOX_DASHBOARD_PORT, defaultPortStr, and runOpenshell (or the onboarding
entrypoint that triggers it) to hook/mock the command invocations, simulate both
env/arg cases (custom port vs default), and assert the sequence and
absence/presence of ["forward","stop", defaultPortStr] and ["forward","start",
customPortStr] accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@test/onboard.test.ts`:
- Around line 996-1017: Replace the pure-text/source-regex test with an
execution-level test that spawns the onboarding logic and asserts actual
commands run for both scenarios: when a custom dashboard port is provided the
test for onboard.ts should observe calls to runOpenshell (or the CLI wrapper
used) that first stop the default port (matching defaultPortStr derived from
SANDBOX_DASHBOARD_PORT) and then start the custom port; and when no custom port
is provided the test should observe that no stop command for the default port is
issued. Locate symbols SANDBOX_DASHBOARD_PORT, defaultPortStr, and runOpenshell
(or the onboarding entrypoint that triggers it) to hook/mock the command
invocations, simulate both env/arg cases (custom port vs default), and assert
the sequence and absence/presence of ["forward","stop", defaultPortStr] and
["forward","start", customPortStr] accordingly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: d3272ff6-f1dc-4206-bb4c-af45306ceb14

📥 Commits

Reviewing files that changed from the base of the PR and between 8d55eb7 and 3c54141.

📒 Files selected for processing (3)

• src/lib/onboard.ts
• src/lib/ports.ts
• test/onboard.test.ts

[brandonpelfrey]

@Sanjays2402 there are some new conflicts. please take a look. I locally made a fix to this, but I cannot push to your fork because it's protected. If you can unprotect your fork so I can push commits or otherwise perform the below:

• src/lib/ports.ts — exported SANDBOX_DASHBOARD_PORT (was file-private)
• src/lib/onboard.ts — added SANDBOX_DASHBOARD_PORT to the imports
• test/onboard.test.ts — updated the regression test to assert against main's generic stale-forward cleanup loop pattern instead of the old defaultPortStr pattern

[Sanjays2402]

@brandonpelfrey Thanks — sorry about the protected fork. Just merged upstream/main into the branch and resolved the conflicts (3c54141..8b5640e).

Looking at the conflict, the original default-port-only cleanup is fully subsumed by the new generic stale-forward loop in ensureDashboardForward() (iterates getOccupiedPorts(existingForwards) and stops any forward owned by this sandbox on a port other than actualPort). So I kept upstream's onboard.ts as-is — no new SANDBOX_DASHBOARD_PORT import needed since the generic loop already covers the 18789 case from #2007.

ports.ts already exports SANDBOX_DASHBOARD_PORT on main, so left untouched.

For the regression test in test/onboard.test.ts, I swapped the defaultPortStr regex for one that asserts against the new generic cleanup pattern (getOccupiedPorts + owner === sandboxName + Number(port) !== actualPort + forward stop). Kept the DEFAULT_SANDBOX_PORT = "18789" literal guard and the SANDBOX_DASHBOARD_PORT export check.

Let me know if you'd prefer a different approach (e.g., still keep an explicit constant import for clarity).