refactor(onboard): extract gateway lifecycle ownership - #9178
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (16)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 625971a in the TypeScript / code-coverage/cliThe overall coverage in commit 625971a in the Show a code coverage summary of the most impacted files.
Updated |
| removeDockerDriverGatewayRegistration, | ||
| retireLegacyGatewayForDockerDriverUpgrade, | ||
| runQuietOpenshell, | ||
| stopDockerDriverGatewayProcess, |
| }); | ||
|
|
||
| const { | ||
| attachGatewayMetadataIfNeeded, |
| const { | ||
| attachGatewayMetadataIfNeeded, | ||
| recoverGatewayRuntime, | ||
| registerDockerDriverGatewayEndpoint, |
| startDockerDriverGateway, | ||
| startGateway, | ||
| startGatewayForRecovery, | ||
| startGatewayWithOptions, |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
4 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
3 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
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 for the commit under review. Recommended E2E: Manual-only E2E: 1 optional E2E recommendation
1 warning · 0 suggestionsWarningsWarnings do not block.
|
| removeDockerDriverGatewayRegistration, | ||
| retireLegacyGatewayForDockerDriverUpgrade, | ||
| runQuietOpenshell, | ||
| stopDockerDriverGatewayProcess, |
| }); | ||
|
|
||
| const { | ||
| attachGatewayMetadataIfNeeded, |
| const { | ||
| attachGatewayMetadataIfNeeded, | ||
| recoverGatewayRuntime, | ||
| registerDockerDriverGatewayEndpoint, |
| startDockerDriverGateway, | ||
| startGateway, | ||
| startGatewayForRecovery, | ||
| startGatewayWithOptions, |
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed exact head 625971a. No blocking findings. Required checks are green. Non-blocking follow-up: add focused coverage for the standalone Docker-driver launch failure path.
<!-- markdownlint-disable MD041 --> ## Summary Close static TypeScript coverage gaps in the onboarding entry-composition ratchet after #9178. The check now records existing lifecycle decisions that the prior scan missed and fails closed when new decisions exceed the merge-base ceiling. ## Related Issue Advances #9172. ## Changes - Scan declaration headers, callable scopes, initializers, module statements, and nested callable bodies. - Normalize the static lifecycle and recovery call forms covered by the regression matrix. - Record existing gateway, messaging, policy, and provider decisions. The work in #9169, #9170, and #9172 will remove these allowances. - Reject budget increases above the decisions present at the merge base. - Add regression tests for the decision and recovery syntax that the check detects. ## Type of Change - [x] 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 - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This change affects an internal repository check only. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent Codex Desktop review passed all nine security categories for commit under review `9072818a6220c97bcd02a2b20b0a34915902df6d`. The PR-owned files are unchanged by the additive merge of `a8ceeb1a6e5ca4e517a4bc0c9767f8a7cc52916d`. That merge adds Google Gemini docs and provider-catalog tests outside this check. The review confirmed fixed-argument Git execution, fail-closed merge-base errors, lexical alias isolation, and positive and negative tests. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: The PR changes an internal repository check, its allowance data, and tests. The additive merge of `a8ceeb1a6e5ca4e517a4bc0c9767f8a7cc52916d` changes only Google Gemini docs and provider-catalog tests. The merge does not alter the three PR-owned blobs or their reviewed behavior. This change does not alter runtime, CLI, API, configuration, defaults, policy schema, or supported product behavior. The prior focused tests passed 179/179. I did not rerun them because the PR-owned blobs are unchanged. - Agent: Codex Desktop <!-- docs-review-head-sha: 9072818 --> <!-- docs-review-agents-blob-sha: e30afb2 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] 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 - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run test/onboard-entry-composition.test.ts test/checks-runner.test.ts` passed, 179/179 tests. - [ ] Applicable broad gate passed — `npm run check` was attempted. Host-specific failures include an unsafe writable parent, a foreign user gateway service, overlong Unix socket paths, and missing plugin-local `json5`. GitHub CI provides the clean broad result. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved detection of gateway and onboarding decisions across complex code patterns, including callbacks, nested calls, recovery actions, and lifecycle logic. * Added safeguards to prevent composition budgets from expanding beyond approved limits. * Improved validation and reporting when comparison baselines cannot be determined. * **Tests** * Expanded coverage for decision detection, budget enforcement, duplicate allowances, and category or declaration changes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
Summary
Move onboarding gateway lifecycle decisions from
src/lib/onboard.tsinto focused gateway application modules. Add a ratcheted entry-boundary check so gateway decisions cannot return and later phase work must decrease its recorded allowances.Related Issue
Fixes #9171
Advances #9172. Provider, messaging, and policy allowances remain assigned to #9169, #9170, and #9172.
Changes
src/lib/onboard/gateway/.src/lib/onboard.tsresponsible for sequencing and dependency wiring. Its line count decreases from 4,274 to 3,902, and fan-out decreases from 210 to 202.The focused modules are required by #9171. A direct move into one facade kept the original coupling, so the lifecycle is split by current responsibility. The focused tests and the architecture check protect these contracts.
Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededsrc/lib/onboard/gateway/, and the architecture check affects contributors only. The feat(runtime): preserve host-local inference lifecycle #9123 host-local lifecycle files and fix(gateway): name a gateway start command that exists #9151 guidance remain unchanged. Extracted paths preserve gateway recovery guidance, lifecycle authority, and user-visible behavior.DGX 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 test:changedpassed 1,271 tests in 107 files. Focused gateway, onboarding, and architecture suites passed 154 tests.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: Carlos Villela cvillela@nvidia.com