refactor(onboard): centralize readiness authority wiring - #8909
Conversation
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
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. |
|
Sensitive-path security review (exact head 864f434): PASS with no findings. Reviewed the complete diff across all nine required categories: (1) input validation and trust boundaries—the same gateway name, port, owner, options, and readiness inputs are passed through typed bindings; (2) credentials and secrets—no credential source, value, storage, or logging changes; (3) network exposure and SSRF—the same local gateway readiness and port-conflict functions execute with dynamic current values; (4) command execution and injection—the same OpenShell install dependency set and process-exit callback are used, with no new command or interpolation; (5) permissions and privilege boundaries—no privilege or ownership rule changes; (6) sandbox isolation—the same fail-closed gateway authority and reuse lifecycle remains; (7) injection classes—no shell, URL, parser, SQL, HTML, or template sink changes; (8) dependencies and supply chain—no package, image, pin, or artifact changes; (9) data exposure and logging—no output or persisted data changes. Focused tests explicitly prove lazy install dependencies, trusted-owner persistence, dynamic gateway values, and the preserved install → checkpoint → readiness → port-check → reuse order. |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit a1dad11 in the TypeScript / code-coverage/cliThe overall coverage in commit a1dad11 in the Show a code coverage summary of the most impacted files.
Updated |
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughOpenShell installation now uses factory-created bindings with gateway-owner persistence. Gateway-authority preflight is composed once and reused by runtime, rebuild, and initial readiness flows. Namespaced checkpoint APIs and focused factory tests were added. ChangesOnboarding gateway authority
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OnboardFlow
participant OnboardPreflightGatewayAuthority
participant ReadinessCollector
participant GatewayAuthorityPreparation
OnboardFlow->>OnboardPreflightGatewayAuthority: request gateway readiness
OnboardPreflightGatewayAuthority->>ReadinessCollector: collect readiness
OnboardPreflightGatewayAuthority->>GatewayAuthorityPreparation: prepare gateway authority
GatewayAuthorityPreparation-->>OnboardFlow: return prepared authority
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/machine/preflight-gateway-authority.ts`:
- Around line 76-82: Extend onboard/machine/preflight-gateway-authority.ts in
runRuntimePreflight to accept and forward a narrow exitProcess override to
fatalRuntimePreflight.runReadinessGatedRuntimePreflight. In src/lib/onboard.ts
lines 3686-3701, replace the direct fatal-runtime-preflight invocation with
onboardPreflightGatewayAuthority.runRuntimePreflight so rebuild uses the
authoritative path. In
src/lib/onboard/machine/preflight-gateway-authority.test.ts lines 19-131, add
coverage confirming the override reaches the runtime-preflight context.
🪄 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: 4eb97322-75d0-4efc-911c-0475a80efa7a
📒 Files selected for processing (5)
src/lib/onboard.tssrc/lib/onboard/machine/preflight-gateway-authority.test.tssrc/lib/onboard/machine/preflight-gateway-authority.tssrc/lib/onboard/openshell-install.test.tssrc/lib/onboard/openshell-install.ts
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
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. 1 semantic terminology decisionTerminology 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: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
Sensitive-path security review (exact head 54eb045): PASS with no findings. Reviewed the complete diff across all nine required categories: (1) input validation and trust boundaries—the same gateway name, port, owner, options, readiness inputs, and collector implementation cross the typed binding; (2) credentials and secrets—no credential source, value, storage, or logging changes; (3) network exposure and SSRF—the same local gateway readiness and port-conflict functions execute with dynamic current values; (4) command execution and injection—the same OpenShell install dependency set and process-exit callback are used, with no new command or interpolation; (5) permissions and privilege boundaries—no privilege or ownership rule changes; (6) sandbox isolation—the same fail-closed gateway authority and reuse lifecycle remains; (7) injection classes—no shell, URL, parser, SQL, HTML, or template sink changes; (8) dependencies and supply chain—no package, image, pin, or artifact changes; (9) data exposure and logging—no output or persisted data changes. The follow-up explicitly injects the pre-existing readiness collector so tests and callers retain the same override seam; focused tests cover lazy dependency resolution, trusted-owner persistence, dynamic gateway values, and the preserved install → checkpoint → readiness → port-check → reuse order. |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
Sensitive-path security review (exact head 5d8243e): PASS with no findings. The only delta since the prior full nine-category PASS at 54eb045 is a focused unit-test assertion that runtime preflight receives the current interaction mode and the exact factory readiness collector. It changes no production code, input boundary, credential or secret handling, network or SSRF behavior, command execution, privilege model, sandbox isolation, injection sink, dependency, persisted data, or logging. The complete PR therefore retains the prior PASS across all nine required categories, with stronger coverage for the fail-closed readiness binding. |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
Sensitive-path security review (exact head a1dad11): PASS with no findings. Independent nine-category review covered the complete five-file diff plus adjacent fatal-runtime-preflight and authoritative-rebuild enforcement. No credential, input, dependency, crypto, persisted-data, or logging boundary changes. The authoritative rebuild path sets NON_INTERACTIVE=true before invoking the central authority; runRuntimePreflight resolves that value at invocation time, uses the canonical readiness collector, and forwards the exact throwing exit handler into runReadinessGatedRuntimePreflight. Failed readiness therefore rejects the rebuild and reaches the enclosing finally restoration; fatal assertions retain post-exit throw backstops. Focused verification passed 49 assertions across four CLI test files plus CLI TypeScript, and commit a1dad11 has a good ED25519 signature. |
2 participants
Summary
This refactor removes five formatter exemptions that survived the entrypoint-budget work in #8738. It moves the default OpenShell installation and readiness/gateway-authority dependency assembly into the existing modules that own those contracts.
The install, trusted authority checkpoint, readiness, port-conflict, and gateway-reuse order is unchanged.
src/lib/onboard.tsbecomes one line smaller, its measured fan-out falls from 211 to 210, and no configuration or supported behavior changes.Changes
onboard.tswiring with formatted calls and remove five growth-guardrail formatter suppressions.The current consumers are the standard onboarding preflight and authoritative rebuild readiness paths. Existing owner modules now assemble their canonical dependencies; no new extension or configuration surface is introduced.
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 build:cli,npm run typecheck:cli, source architecture, source shape, and test-size checks passed. The existing macOS integration test advances past the readiness-collector regression and then encounters an unrelated host Homebrew trust probe; the Linux CI rerun is authoritative.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable to this focused internal dependency-wiring refactor.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Charan Jagwani cjagwani@nvidia.com