fix(onboard): classify a failed forward list as list-failed - #8529
fix(onboard): classify a failed forward list as list-failed#8529laitingsheng wants to merge 9 commits into
Conversation
The cleanup helper inferred list failure 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. That ran the sandbox-scoped forward stop precisely when ownership could not be established. ForwardListRunner now returns string or null, and the helper treats null as list-failed before parsing, so the safety contract sits at the shared boundary instead of relying on each adapter to throw. The dashboard adapter's throw guard becomes a pass-through. The messaging capture seam also forces ignoreError so that a timed-out probe returns null. Without it the OpenShell adapter reports a spawn error and exits the process, so the classification is never reached. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change preserves ChangesForward cleanup failure handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 237a9ab in the TypeScript / code-coverage/cliThe overall coverage in commit 237a9ab in the Show a code coverage summary of the most impacted files.
Updated |
laitingsheng
added
area: onboarding
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
📒 Files selected for processing (6)
src/lib/actions/sandbox/messaging-host-forward-lifecycle.tssrc/lib/onboard/agent-fixed-forward.test.tssrc/lib/onboard/agent-fixed-forward.tssrc/lib/onboard/dashboard-forward-control.tssrc/lib/onboard/forward-cleanup.test.tssrc/lib/onboard/forward-cleanup.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
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.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the formal change request on branch revision
Validation after the main refresh:
Documentation writer review remains Security reviewVerdict: 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.
Fresh required checks are running for the refreshed branch. |
|
CI disposition after the branch refresh:
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
left a comment
cv
left a comment
There was a problem hiding this comment.
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.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the remaining dashboard-forward-stopper change request in branch revision
Security reviewVerdict: PASS. No security findings were found. The new test closes the remaining caller-level coverage gap without changing production behavior.
|
3 participants
Summary
bestEffortForwardStopForSandboxinferred a failedopenshell forward listonly from a thrown exception, so the messaging adapter'snullresult was coerced to an empty string, parsed as an empty ownership map, and reclassified asno-entry. Before this change a failed list ran the sandbox-scopedforward stopprecisely when ownership could not be established; after it, the helper returnslist-failedand skips the stop.Related Issue
Fixes #8522
Changes
src/lib/onboard/forward-cleanup.ts:ForwardListRunnerreturnsstring | null, andbestEffortForwardStopForSandboxreturnslist-failedfornullbefore it reachesgetOccupiedPorts. The stale comment stating that runners must throw is rewritten to describe both failure signals.src/lib/onboard/agent-fixed-forward.ts: the stop adapter no longer coercesnullto"". The start-polling runner keeps its coercion, since an empty poll result is not an ownership decision.src/lib/onboard/dashboard-forward-control.ts: the adapter-local throw onnullbecomes 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 returnsnull, verifieslist-failed, and verifies that no stop command runs.src/lib/actions/sandbox/messaging-host-forward-lifecycle.ts:captureOpenShellOutputforcesignoreErrorso a timed-out probe returnsnull. Without it the OpenShell adapter treats theETIMEDOUTspawn error as fatal and exits the process, so the classification above is never reached. The existing caller already passedignoreError, 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) andsrc/lib/onboard/forward-cleanup.test.ts: cover the failed-list and genuinely-empty-list outcomes at both the helper and theensureAgentFixedForwardentry point, and assert that each case reaches theforward listprobe before its fail-closed result.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project cli src/lib/actions/sandbox/messaging-host-forward-lifecycle.test.ts src/lib/onboard/agent-fixed-forward.test.ts src/lib/onboard/dashboard-forward-control.test.ts src/lib/onboard/forward-cleanup.test.ts— 4 files and 11 tests passed after the currentmainrefresh.npm run typecheck:cli,npm run test:titles:check, andnpm run test:projects:checkpassed.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: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests