fix(onboard): suppress 'No active forward found' from best-effort forward stop#3997
Merged
Conversation
…ward stop Onboard issues best-effort `openshell forward stop` calls before every `forward start` to clear stale forwards. On a clean first onboard the target port has never been forwarded, so openshell emits a yellow '! No active forward found for port N' warning that surfaces between the 'Dashboard is live' and 'Sandbox created' lines. Centralise the cleanup through a single `bestEffortForwardStop` helper that passes `suppressOutput: true` alongside the existing `ignoreError: true` — real failures are still surfaced by the following `forward start`, only the unsolicited cleanup chatter is hidden. Fixes #3971 Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds a new helper ChangesDashboard Forward Stop Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Comment |
laitingsheng
added
the
enhancement: ui
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
PR Review AdvisorRecommendation: blocked This is an automated advisory review. A human maintainer must make the final merge decision. Limitations: This advisory review used the provided deterministic context and diff; it did not execute tests, package-manager commands, PR scripts, or E2E workflows.; The E2E pass evidence in issue comments targets 30ae093, not the requested head SHA 624f236.; Several CI contexts were still IN_PROGRESS/QUEUED/PENDING in the trusted status rollup for the current head SHA.; Issue #3971 has no comments; acceptance mapping is based on the issue body only.; PR-provided title/body/checklist claims were treated as untrusted evidence only. Full advisor summaryPR Review AdvisorBase: The helper extraction is small and aligned with #3971, but merge is blocked by pending CI/blocked merge state and required E2E evidence is not available for the current head SHA. Gate status
🔴 Blockers
🟡 Warnings
🔵 Suggestions
Acceptance coverage
Security review
Test / E2E status
✅ What looks good
Review completeness
|
![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 (1)
src/lib/onboard.ts (1)
8561-8566: Run the onboarding E2E matrix for this core-flow change.Since this touches core onboarding cleanup behavior, I recommend running the listed onboarding-focused nightly jobs (at least
cloud-e2e,sandbox-operations-e2e,rebuild-openclaw-e2e, andchannels-stop-start-e2e) before merge.As per coding guidelines, "
src/lib/onboard.ts: This file contains core onboarding logic. Changes here affect the full sandbox creation and configuration flow." and the provided E2E test recommendation list.Also applies to: 8571-8573
🤖 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/onboard.ts` around lines 8561 - 8566, This change modifies core onboarding cleanup behavior in src/lib/onboard.ts (notably the bestEffortForwardStop function and the nearby cleanup calls around lines ~8571-8573), so before merging run the onboarding-focused E2E matrix: at minimum execute cloud-e2e, sandbox-operations-e2e, rebuild-openclaw-e2e, and channels-stop-start-e2e to validate sandbox creation/cleanup and port-forward teardown; if any failures occur, capture logs from bestEffortForwardStop/runOpenshell invocations and iterate until the cleanup flow is stable across these jobs.
🤖 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/onboard.ts`:
- Around line 8561-8566: The new helper function bestEffortForwardStop increases
onboard.ts size and breaks the file-size budget; either remove or compress this
change or update the budget policy in the same PR: either delete or inline the
logic where bestEffortForwardStop is used (replace calls with
runOpenshell([...], {ignoreError:true, suppressOutput:true} ) to avoid adding
lines) or add a corresponding adjustment to the size/budget policy configuration
in this PR so CI accepts the net growth.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 8561-8566: This change modifies core onboarding cleanup behavior
in src/lib/onboard.ts (notably the bestEffortForwardStop function and the nearby
cleanup calls around lines ~8571-8573), so before merging run the
onboarding-focused E2E matrix: at minimum execute cloud-e2e,
sandbox-operations-e2e, rebuild-openclaw-e2e, and channels-stop-start-e2e to
validate sandbox creation/cleanup and port-forward teardown; if any failures
occur, capture logs from bestEffortForwardStop/runOpenshell invocations and
iterate until the cleanup flow is stable across these jobs.
🪄 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: 1d9e9662-d9e2-45a6-8e9e-69cb0da35de8
📒 Files selected for processing (2)
src/lib/onboard.tstest/onboard-forward-stop-quiet.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26220671624
|
…ypoint budget Move bestEffortForwardStop into src/lib/onboard/forward-cleanup.ts with runOpenshell injected as a parameter so the helper has no implicit dependency on the onboard.ts entry point. Keeps onboard.ts net-neutral under the entrypoint budget while preserving the single quiet-stop call path across all 12 best-effort cleanup sites. Also suppress the same noisy 'No active forward found' warning on the sandbox recovery code path (forward stop before re-establish) by forwarding stdio=ignore through adapters/openshell runOpenshell. Rewrite the regression test to import the helper directly from the sub-module with a callback runner stub, dropping the spawnSync subprocess pattern (which depended on a real openshell binary being on PATH and failed in unit-vitest-linux CI). Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard.ts (1)
3559-3622: Run the onboarding forward-lifecycle E2Es on this helper fan-out.These replacements now sit on reuse, rollback, and retry cleanup paths inside core onboarding, so I'd validate at least
cloud-e2e,sandbox-operations-e2e, andchannels-stop-start-e2ebefore merging.As per coding guidelines,
src/lib/onboard.ts: This file contains core onboarding logic. Changes here affect the full sandbox creation and configuration flow.Also applies to: 8562-8690, 9441-9480
🤖 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/onboard.ts` around lines 3559 - 3622, Run the onboarding forward-lifecycle E2E suites against the helper fan-out that touches src/lib/onboard.ts; specifically exercise the reuse/rollback/retry cleanup paths that call waitForGatewayHttpReady, bestEffortForwardStop, destroyGatewayForReuse, getGatewayLocalEndpoint, getGatewayClusterImageDrift and retireLegacyGatewayForDockerDriverUpgrade. Validate at minimum cloud-e2e, sandbox-operations-e2e, and channels-stop-start-e2e (and the regions covering the other affected ranges noted: 8562-8690 and 9441-9480) to ensure the replacements behave correctly during reuse, abort, recreate, and cleanup flows before merging.
🤖 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 `@src/lib/onboard.ts`:
- Around line 3559-3622: Run the onboarding forward-lifecycle E2E suites against
the helper fan-out that touches src/lib/onboard.ts; specifically exercise the
reuse/rollback/retry cleanup paths that call waitForGatewayHttpReady,
bestEffortForwardStop, destroyGatewayForReuse, getGatewayLocalEndpoint,
getGatewayClusterImageDrift and retireLegacyGatewayForDockerDriverUpgrade.
Validate at minimum cloud-e2e, sandbox-operations-e2e, and
channels-stop-start-e2e (and the regions covering the other affected ranges
noted: 8562-8690 and 9441-9480) to ensure the replacements behave correctly
during reuse, abort, recreate, and cleanup flows before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8344a758-8a5d-4c50-aed0-2327a4c6349a
📒 Files selected for processing (4)
src/lib/actions/sandbox/process-recovery.tssrc/lib/onboard.tssrc/lib/onboard/forward-cleanup.tstest/onboard-forward-stop-quiet.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/actions/sandbox/process-recovery.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26224683198
|
ericksoa
added
bug
ericksoa
approved these changes
May 21, 2026
Contributor
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Reviewed for v0.0.49 feature freeze: scoped WSL/onboard forward cleanup bugfix, CodeRabbit finding addressed, checks and selective E2E green.
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
Onboard prints a stray
! No active forward found for port 18789warning between the✓ Dashboard is liveand✓ Sandbox '...' createdlines on a clean first run. Route the dozen unconditionalopenshell forward stopcleanup calls through a single quiet-stop helper so unsolicited chatter from a stop with no active forward is hidden, while real failures still surface from the followingforward start.Related Issue
Fixes #3971
Changes
src/lib/onboard/forward-cleanup.ts(new): exportsbestEffortForwardStop(runOpenshell, port)— wrapsrunOpenshell(["forward", "stop", port], { ignoreError: true, suppressOutput: true }). Lives under the onboard sub-module dir so the entrypoint-budget check stays net-neutral onsrc/lib/onboard.ts.src/lib/onboard.ts: import the helper and route all 12 best-effort cleanup call sites through it (was the unconditional stop betweenDashboard is liveandSandbox createdlines).src/lib/actions/sandbox/process-recovery.ts: same noise class on the sandbox-recoveryforward stoppath — passstdio: "ignore"so the! No active forward foundwarning does not surface throughnemoclaw <name> connectrecovery either.test/onboard-forward-stop-quiet.test.ts: regression test — imports the helper directly with a callback runner stub and asserts the args,ignoreError: true, andsuppressOutput: trueare passed through. Plus port-coercion assertion for numeric and string inputs.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
Refactor
Bug Fix
Tests