fix(onboard): classify a failed forward list as list-failed

[laitingsheng]

Summary

bestEffortForwardStopForSandbox inferred a failed openshell forward list only from a thrown exception, so the messaging adapter's null result was coerced to an empty string, parsed as an empty ownership map, and reclassified as no-entry. Before this change a failed list ran the sandbox-scoped forward stop precisely when ownership could not be established; after it, the helper returns list-failed and skips the stop. The same failure signal is now preserved during forward-start polling, so the live-port compatibility fallback cannot succeed unless ownership enumeration actually succeeded.

Related Issue

Fixes #8522

Changes

• src/lib/onboard/forward-cleanup.ts: ForwardListRunner returns string | null, and bestEffortForwardStopForSandbox returns list-failed for null before it reaches getOccupiedPorts. The stale comment stating that runners must throw is rewritten to describe both failure signals.
• src/lib/onboard/agent-fixed-forward.ts: both the stop and start-polling adapters preserve null as a failed ownership probe instead of converting it to a successful empty result.
• src/lib/onboard/dashboard-forward-control.ts: the adapter-local throw on null becomes a pass-through. Classification now lives at the shared boundary, so a future adapter cannot reintroduce the defect by forgetting to throw.
• src/lib/onboard/dashboard-forward-control.test.ts (new): exercises the dashboard stopper when the ownership probe returns null, verifies list-failed, and verifies that no stop command runs.
• src/lib/actions/sandbox/messaging-host-forward-lifecycle.ts: captureOpenShellOutput forces ignoreError so a timed-out probe returns null. Without it the OpenShell adapter treats the ETIMEDOUT spawn error as fatal and exits the process, so the classification above is never reached. The existing caller already passed ignoreError, so only the stop-adapter path changes.
• src/lib/actions/sandbox/messaging-host-forward-lifecycle.test.ts (new): exercises the concrete rebuild adapter when both forward-list probes fail, verifies that the captures ignore the command error, and verifies that no stop command runs.
• src/lib/onboard/agent-fixed-forward.test.ts (new) and src/lib/onboard/forward-cleanup.test.ts: cover the failed-list and genuinely-empty-list outcomes at both the helper and the ensureAgentFixedForward entry point, and assert that each case reaches the forward list probe before its fail-closed result.
• src/lib/onboard/forward-start.ts, src/lib/onboard/dashboard.ts, and src/lib/onboard/forward-start.test.ts: preserve null through start polling, distinguish it from a genuine empty list, and prove that a live listener is not accepted when ownership enumeration failed.

Type of Change

• Code change (feature, bug fix, or refactor)
• Code change with doc updates
• Doc only (prose changes, no code sample modifications)
• Doc only (includes code sample changes)

Quality Gates

• Tests added or updated for changed behavior
• Existing tests cover changed behavior — justification:
• Tests not applicable — justification:
• Docs updated for user-facing behavior changes
• Docs not applicable — justification: no command, flag, output contract, or documented behavior changes. The change corrects an internal ownership classification so a cleanup step is skipped when it cannot be proven safe.
• Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
• Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: security review and validation: PASS with no findings.
• Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

• Documentation writer subagent reviewed the completed changes
• Result: no-docs-needed
• Evidence: No documentation paths changed. The reviewed revision preserves failed OpenShell forward-list results at the existing cleanup and start-polling ownership boundaries. It adds no command, flag, configuration, output, or operator procedure. Current documentation already requires ownership reconciliation before accepting or cleaning up a forward.
• Agent: Codex Desktop

DGX Station Hardware Evidence

• Tested on DGX Station
• Tested commit:
• Station profile/scenario:
• Result:
• Supporting evidence:

Verification

• PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
• Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
• Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: affected CLI coverage passed after the current main refresh (5 files, 54 tests), as did the dashboard integration coverage (1 file, 16 tests). npm run typecheck:cli, npm run test:titles:check, npm run test:projects:check, source-shape, test-size, Biome, and diff checks passed.
• Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
• Quality Gates section completed with required justifications or waivers
• No secrets, API keys, or credentials committed
• npm run 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)

---

Signed-off-by: Tinson Lai tinsonl@nvidia.com

Summary by CodeRabbit

• Bug Fixes

  • Improved forwarding cleanup when status checks fail or return no result.
  • Prevented failed checks from being treated as empty port lists, avoiding unintended stop operations.
  • Forward-start and forward-stop actions now handle unavailable command output gracefully.
  • Improved retry handling for failed forwarding-list checks.
  • Error output is suppressed during best-effort cleanup to avoid disrupting onboarding flows.

• Tests

  • Added coverage for failed, empty, and unavailable forwarding-list scenarios, including retries and cleanup behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

The change preserves null from failed forward-list commands and classifies it as "list-failed". Cleanup skips forward stop after failed probes. Forward-start polling records failed fetches. Tests cover failed probes and successful empty-list cleanup.

Changes

Forward cleanup failure handling

Layer / File(s)	Summary
Preserve nullable forward-list results src/lib/actions/sandbox/messaging-host-forward-lifecycle.ts, src/lib/onboard/agent-fixed-forward.ts, src/lib/onboard/dashboard-forward-control.ts, src/lib/onboard/*forward*.test.ts	Forward-list adapters preserve null results. Messaging capture forces ignoreError: true. Tests cover failed probes, retries, skipped cleanup, and successful empty-list cleanup.
Fail-closed cleanup classification src/lib/onboard/forward-cleanup.ts, src/lib/onboard/forward-cleanup.test.ts, src/lib/onboard/dashboard-forward-control.test.ts	ForwardListRunner accepts null. Cleanup returns "list-failed" for null or thrown results and skips forward stop. Tests verify null-result behavior and capture timeout handling.
Nullable forward-start polling src/lib/onboard/forward-start.ts, src/lib/onboard/forward-start.test.ts	Forward-start polling distinguishes null results from empty list snapshots, records a fetch diagnostic, and avoids live-port fallback when ownership is unknown.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• NVIDIA/NemoClaw#8526: Extends nullable forward-list failure handling across related adapters, polling, cleanup, and regression tests.
• NVIDIA/NemoClaw#8577: Shares forward-list probing and cleanup behavior while modifying different functions.

Suggested labels: area: sandbox

Suggested reviewers: cv, cjagwani

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.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
Linked Issues check	✅ Passed	The changes satisfy issue #8522 by preserving null failures, returning list-failed, skipping forward stop, and retaining empty-list cleanup behavior.
Out of Scope Changes check	✅ Passed	All changes support issue #8522 through implementation updates and focused regression tests; no unrelated scope is present.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: classifying failed forward-list operations as "list-failed".

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/forward-list-failure-classification

---

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

[github-code-quality]

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 38d56ae in the fix/forward-list-fai... branch remains at 96%, unchanged from commit fbc7a9d in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit 38d56ae in the fix/forward-list-fai... branch remains at 81%, unchanged from commit fbc7a9d in the main branch.

Show a code coverage summary of the most impacted files.

File	main fbc7a9d	fix/forward-list-fai... 38d56ae	+/-
src/lib/messagi...ate-resolver.ts	100%	87%	-13%
src/lib/onboard/dashboard.ts	71%	72%	+1%
src/lib/actions...air-approval.ts	89%	90%	+1%
src/lib/security/redact-url.ts	97%	98%	+1%
src/lib/securit...ntial-filter.ts	94%	95%	+1%
src/lib/onboard...oute-handoff.ts	92%	95%	+3%
src/lib/messagi...ate-resolver.ts	95%	100%	+5%
src/lib/onboard...ixed-forward.ts	69%	77%	+8%
src/lib/onboard...ward-control.ts	86%	100%	+14%
src/lib/actions...rd-lifecycle.ts	29%	67%	+38%

---

_{Updated August 07, 2026 19:51 UTC}

[coderabbitai]

Actionable comments posted: 2

🤖 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/agent-fixed-forward.test.ts`:
- Around line 29-48: Update both tests in
src/lib/onboard/agent-fixed-forward.test.ts lines 29-48 to assert that
deps.runCaptureOpenshell receives ["forward", "list"] in each scenario. Also
update src/lib/onboard/forward-cleanup.test.ts lines 98-106 to assert that fetch
receives ["forward", "list"] before checking the fail-closed result; no other
behavioral assertions need changing.

In `@src/lib/onboard/forward-cleanup.ts`:
- Around line 67-72: Update the contract comment near the forward-cleanup runner
handling to allow adapters such as captureOpenShellOutput to use ignoreError:
true, provided ignored command failures are converted to null. Clarify that
runners must not convert failures into an empty string, so getOccupiedPorts
receives the preserved failure signal.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8ae965a1-24f9-434f-83f8-6c69ab67e37e

📥 Commits

Reviewing files that changed from the base of the PR and between e3a824d and 91efcde.

📒 Files selected for processing (6)

• src/lib/actions/sandbox/messaging-host-forward-lifecycle.ts
• src/lib/onboard/agent-fixed-forward.test.ts
• src/lib/onboard/agent-fixed-forward.ts
• src/lib/onboard/dashboard-forward-control.ts
• src/lib/onboard/forward-cleanup.test.ts
• src/lib/onboard/forward-cleanup.ts

[github-actions]

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

• GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Model comparison: normalized findings match; normalized terminology decisions differ; normalized E2E selections differ; severity counts match.

2 additional E2E selections from the second opinion

Advisory only. The primary lane did not select these E2E jobs or targets.

• full-e2e: The completed second-opinion lane identified E2E coverage that the primary lane omitted.
• sandbox-survival: The completed second-opinion lane identified E2E coverage that the primary lane omitted.

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

3 semantic terminology decisions

Terminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.

• established — list-failed at src/lib/onboard/forward-cleanup.ts:68: Keep `list-failed` for this failed-enumeration outcome across forwarding lifecycle paths.
• justified — ownership evidence at src/lib/onboard/forward-cleanup.ts:71: Keep the modifier because the presence of ownership evidence determines whether cleanup may stop a forward.
• define — forward list result at src/lib/onboard/forward-start.ts:383: Keep the phrase with its existing local diagnostic context, which distinguishes a null result from empty output.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite against this exact revision.

Recommended E2E: channels-add-remove, channels-stop-start, onboard-repair, onboard-resume, cloud-onboard

1 optional E2E recommendation

• rebuild-openclaw

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

[cv]

Please cover the changed concrete messaging adapter before approval. Add a focused test through ensureMessagingHostForwardAfterRebuild that makes forward list fail, verifies the capture uses ignoreError: true, and verifies no forward stop runs. The shared helper and fixed-forward tests do not execute that adapter seam. Refresh onto the current main commit and rerun the required checks after adding the test.

[apurvvkumaria]

Addressed the formal change request on branch revision 8a79bb965.

• Added messaging-host-forward-lifecycle.test.ts to exercise ensureMessagingHostForwardAfterRebuild through the concrete OpenShell adapter.
• The test makes both forward list probes fail, verifies that both captures use ignoreError: true, and verifies that no forward stop command runs.
• Updated the adjacent test-contract comment so it permits either a thrown failure or a null result, but not an empty-string conversion.
• Merged current main at cc4b816c7. The merge was conflict-free and did not overlap the forwarding files.

Validation after the main refresh:

• Focused CLI tests: 3 files, 10 tests passed.
• CLI type-check: passed.
• Test-title and Vitest-project checks: passed.
• Pre-commit and pre-push hooks: passed.
• Both maintainer commits are signed, include DCO trailers, and GitHub reports them as Verified.

Documentation writer review remains no-docs-needed. The change affects internal failure classification and does not change commands, flags, configuration, output, setup, migration, or a supported product surface.

Security review

Verdict: PASS. No security findings were found. The change improves the fail-closed cleanup boundary by preserving unknown ownership and retaining the sandbox-scoped stop form.

Category	Result	Evidence
Secrets and credentials	PASS	No credential material or logging changes. The secret scan passed.
Input validation and sanitization	PASS	The shared boundary distinguishes null failure from a genuinely empty list before parsing.
Authentication and authorization	PASS	No authentication surface changed. Cleanup still requires sandbox ownership evidence.
Dependencies	PASS	No dependency or lockfile changes.
Error handling and logging	PASS	Ignored OpenShell failures become null; they do not terminate the process or trigger cleanup.
Cryptography and data protection	PASS	Not applicable. No cryptographic or data-storage behavior changed.
Configuration and security defaults	PASS	No configuration changed. Unknown ownership skips the stop operation.
Security testing	PASS	Helper, caller, and concrete adapter failure paths now have regression coverage.
Holistic posture	PASS	The sandbox-scoped stop retains the time-of-check/time-of-use protection and avoids cross-sandbox cleanup without ownership evidence.

Fresh required checks are running for the refreshed branch.

[apurvvkumaria]

CI disposition after the branch refresh:

• The first shard 5 attempt timed out in test/managed-bootstrap-trampoline.test.ts, outside this PR's forwarding and cleanup changes.
• The same shard passed on neighboring PR runs using the same base revision, and it had passed on this branch before the refresh. I classified the timeout as transient runner/load sensitivity and reran the failed lane without changing the branch.
• The rerun passed cli-test-shards (5), the aggregate cli-tests job, and the required checks gate: https://github.com/NVIDIA/NemoClaw/actions/runs/31161870273
• All required CI gates are green. The optional Nemotron advisor lane is still running and is not a merge gate.

The concrete messaging-adapter regression requested in the formal review is present, both review threads are resolved, and the branch is now waiting only for independent re-review.

The requested messaging adapter test was added. A current review records the remaining dashboard adapter coverage gap separately.

[cv]

The messaging adapter test is now present. One equivalent changed boundary remains uncovered: add a focused createSandboxForwardStopper test where runCaptureOpenshell returns null, assert list-failed, and verify no forward stop command runs.

[apurvvkumaria]

Addressed the dashboard-forward-stopper change request and completed a refreshed security review for branch revision 38d56ae9c.

• The focused createSandboxForwardStopper test makes runCaptureOpenshell return null, verifies list-failed, and verifies that no forward stop command runs.
• The branch is refreshed onto the current main.
• The review also found and fixed an adjacent start-polling gap: both callers now preserve null as a failed ownership probe, so a live port cannot be accepted through the compatibility fallback unless ownership enumeration succeeded.
• A focused regression supplies the listener diagnostic plus a live-port signal, then verifies failure, no live-port probe, and cleanup of the detached child.
• Post-refresh validation passed: 5 affected CLI files / 54 tests, dashboard integration 1 file / 16 tests, CLI type-check, Biome, title, project-membership, source-shape, test-size, diff, commit, and push gates.
• The documentation writer review returned no-docs-needed: this changes an internal ownership-evidence classification without changing commands, configuration, output contracts, or operator procedures.

Security review

Verdict: PASS. No remaining findings.

Category	Result	Evidence
Secrets and credentials	PASS	No credential flow, token handling, or sensitive logging changed; secret scanning passed.
Input validation and sanitization	PASS	Sandbox and port parsing are unchanged; failed ownership output remains distinguishable from a genuine empty list.
Authentication and authorization	PASS	Unknown ownership can no longer satisfy either cleanup or listener-acceptance paths.
Dependencies and third-party libraries	PASS	No dependency or lockfile changes.
Error handling and logging	PASS	Thrown and null failures remain failures; genuine empty output remains a successful empty enumeration; diagnostics stay bounded.
Cryptography and data protection	PASS	No cryptographic, storage, or data-protection behavior changed.
Configuration and security defaults	PASS	The sandbox-scoped stop form and existing compatibility contract remain unchanged.
Security testing	PASS	Shared classification plus agent, messaging, dashboard, thrown-failure, null-failure, and successful-empty-list paths have focused coverage.
Holistic security posture	PASS	Ownership-sensitive cleanup and live-listener acceptance now both fail closed when OpenShell cannot provide ownership evidence.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[apurvvkumaria]

Addressed the remaining change request and refreshed the branch onto current main.

• The requested createSandboxForwardStopper coverage is in src/lib/onboard/dashboard-forward-control.test.ts: a null result produces list-failed and no forward stop call.
• The current revision keeps that test and also closes the adjacent start-polling path found during follow-up security review: null ownership probes can no longer be converted to successful empty results before the live-port fallback.
• Post-refresh validation passed: 54 affected CLI tests, 16 dashboard integration tests, CLI type-check, formatting/lint, title, project-membership, source-shape, test-size, diff, commit, and push gates.
• Both new commits are signed, carry DCO sign-off, and appear as Verified on GitHub.
• Documentation review: no-docs-needed. Security review: PASS with no remaining findings.

Fresh CI is fully green for branch revision 38d56ae9c: all eight CLI shards and aggregate checks, managed-image activation, self-hosted images/smokes, CodeQL and ShellCheck, Code Quality, DCO, documentation receipt, commit lint, installer hash, CLI parity, growth guardrails, CodeRabbit, and both Advisor lanes passed. The PR is now waiting only for human rereview of the addressed request.