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.
  • Suppressed error output 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

Suggested labels: area: networking

🚥 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 preserve null failures, return "list-failed", skip forward stop, and retain successful empty-list behavior required by issue #8522.
Out of Scope Changes check	✅ Passed	The implementation and regression tests are limited to failed forward-list handling, cleanup classification, polling, and related adapters.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: classify failed forward-list operations as "list-failed" during onboarding cleanup.

✨ 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 3592ca5 in the fix/forward-list-fai... branch remains at 96%, unchanged from commit 31cae59 in the main branch.

---

_{Updated August 11, 2026 09:57 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 — Informational

Advisor assessment: Informational / low confidence
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions
Status: PR review advisor skipped: OpenShell inference configuration failed or the advisor credential is unavailable

Model lanes

• GPT-5.6 Terra (primary): Skipped
• Nemotron 3 Ultra (second opinion): Failed after a partial review · low confidence · 2 blockers · 1 warning · 0 suggestions

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

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite for the commit under review.

Recommended E2E: None

Manual-only E2E: channels-add-remove, channels-stop-start, onboard-repair, onboard-resume, cloud-onboard
The manual PR workflow does not run these selectors for the commit under review. Run them from reviewed code on main.

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.

[cv]

Re-reviewed commit 38d56ae9c. The requested createSandboxForwardStopper regression is present. A null list result now produces list-failed and does not call forward stop. The shared forward-list type preserves null, and the added tests distinguish list failure from a successful empty list. I found no new correctness or security blocker.

I am not approving this commit because the required changes check is SKIPPED. CodeRabbit is also pending. Obtain a successful required-check result on this commit, then request approval.

[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.

[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]

Security review: PASS for the current branch.

Rechecked again after refreshing the branch with current main; the 11-file PR diff and assessment below remain unchanged. The five focused test files pass 54 tests, CLI and plugin type checks pass, and changed-file hooks pass.

Reviewed secrets and credentials, input validation, authorization and ownership, dependencies, logging, cryptography, configuration, tests, and failure modes. The forward-list boundary remains fail-closed: an unavailable ownership result stays distinct from a successful empty list, so cleanup does not stop a forward and start polling does not accept a live port when ownership is unknown. Raw credential handling, permissions, network policy, and destructive authority are unchanged.

The PR-specific change documents the existing null-versus-empty contract. The latest main refresh changes disjoint files. No security findings remain.

[apurvvkumaria]

Reviewed the completed forward-list failure classification and its adapter coverage. A failed list remains distinct from a successful empty result, and stop operations remain fail-closed. All current checks pass, no unresolved review threads remain, documentation and security receipts are current, and the previously requested tests are present. I approve the implementation.

[apurvvkumaria]

Implementation review is complete. The requested adapter regressions are present, no review threads remain unresolved, the current CI suite passes, and a maintainer approval is recorded. GitHub still reports changes requested from the earlier review, so this PR is not merge-ready. I will not dismiss or reroute that review. Main has advanced; after the reviewer updates the outstanding review state, the branch needs a routine refresh and another required-check run.

The latest PR commit tests a null forward-list result, returns list-failed, and proves that no stop command runs.

[apurvvkumaria]

Post-merge CI and review classification: repository lifecycle/infrastructure; no PR code regression found.

• All 12 CLI shards passed, the merged coverage report was generated, and the coverage ratchet passed. The aggregate failed only when GitHub rejected the coverage upload with HTTP 400 after the PR had merged; the final checks result reflects that upload failure.
• The GPT advisor could not configure its inference endpoint because of a transport failure. The Nemotron advisor stopped during its internal finding-ledger operation.
• I checked the three partial Nemotron findings against the merged branch. The forward cleanup and startup call sites no longer suppress ownership-enumeration failures, so failures throw and are classified safely. The dashboard now passes the list result through without collapsing empty success and failure. A later successful poll clears the prior transient error before the untracked-forward compatibility check. The partial findings therefore do not reproduce on the merged code.

The post-merge all-agent managed-runtime job is still running and remains under observation.