refactor(onboard): centralize scoped sandbox creation - #8902
Conversation
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change extracts dashboard-port reservation and sandbox setup into a shared factory. Onboarding uses the factory for standard and temporary managed-runtime sandbox creation. Tests verify contexts, compute plans, reservation scopes, flags, names, invocation counts, ordering, and error propagation. ChangesSandbox entry-point reservation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Onboarding
participant DashboardPortFactory
participant ReservationScope
participant SandboxCreator
Onboarding->>DashboardPortFactory: create sandbox entry points
DashboardPortFactory->>ReservationScope: open dashboard-port reservation
DashboardPortFactory->>SandboxCreator: create sandbox with scope and runtime flag
SandboxCreator-->>Onboarding: return sandbox
Possibly related PRs
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 91a19e1 in the TypeScript / code-coverage/cliThe overall coverage in commit 91a19e1 in the Show a code coverage summary of the most impacted files.
Updated |
VerdictPASS. The review found no security findings. The refactor preserves the existing dashboard-port reservation lifecycle and exact sandbox-creation arguments without widening a trust boundary or changing a security control. Findings TableThe review found no findings. Detailed Analysis
Files Reviewed
Reviewed commit: |
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 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 for the commit under review. Recommended E2E: Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
Follow-up sensitive-path review for exact head 2e9f31e: PASS with no findings. The delta restores the original compute-plan/base-image-context evaluation order and adds an event-order assertion. Rechecked all nine categories: no input boundary, credential, network/SSRF, command execution, permission, sandbox-isolation, injection, dependency, or data-exposure behavior changes. The original full nine-category review remains applicable to the complete PR diff. |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
Sensitive-path security review (exact head 796ea2c): PASS with no findings. The complete diff was rechecked across all nine required categories. The new delta only expands an existing accepted-recreate branch: it preserves the same condition, optional transaction completion, awaited dashboard restoration, and return order while removing an inherited formatter suppression. It changes no input or trust boundary, credential or secret handling, network or SSRF behavior, command execution, privilege model, sandbox isolation, injection sink, dependency, persisted data, or logging. The original factory extraction still preserves fresh per-call reservation scopes, compute-plan-before-scope ordering, base-image context creation, fixed nullable arguments, and standard versus temporary runtime selection. |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Exact-head sensitive-path reviewVerdict: PASS for signed head The final delta addresses PR Review Advisor finding PRA-1 by restoring the original rejected-Promise contract when All nine categories passed: secrets/credentials, input validation, authentication/authorization, dependencies, error handling/logging, cryptography/data protection, configuration, security testing, and system security. Verification:
The earlier full-PR nine-category review remains applicable to the complete refactor. |
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed the current head, compute-plan factory semantics, required CI, and exact-head advisor/security feedback. Required checks are green; ordering, per-call scope, and rejection behavior are preserved. Auxiliary base-image/rerun-artifact failures remain for maintainer disposition.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Summary
This refactor removes all three line-budget evasions attributable to #8863: its two formatter-suppressed sandbox-creation calls and its awaited dashboard-recovery addition to a block suppressed by #7788. The duplicated creation calls move into one typed composition helper in the existing dashboard-port owner, while the recovery branch is expanded normally.
It preserves dashboard-port reservation scope, compute-plan evaluation before scope creation, base-image context creation inside the scope, temporary-runtime selection, nullable arguments, forwarded sandbox arguments, Promise rejection for synchronous setup failures, accepted-target completion, awaited dashboard restoration, and return order.
src/lib/onboard.tsis net two lines smaller.Changes
src/lib/onboard.tsand remove their two growth-guardrail formatter suppressions.The current consumer is the standard and temporary managed-runtime creation paths in
src/lib/onboard.ts.A direct readable expansion would grow the line-budgeted monolith, while the existing
dashboard-port.tsmodule already owns reservation scope creation and cleanup.The new dashboard-port tests protect this composition contract, and the existing reservation-failure test protects cleanup.
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 unavailablenpm run typecheck:cli, source architecture, source shape, test-size, normal commit hooks, and pre-push CLI typecheck passed. Earlier full-PR validation passed 31 dashboard-port tests and 21 onboarding dashboard/rollback integrations.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable to this focused internal refactor.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Charan Jagwani cjagwani@nvidia.com