perf(test): narrow gateway drift preflight graph - #6286
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
📝 WalkthroughWalkthroughThis PR replaces the previous gateway drift preflight/result-issue detection and recovery-based sandbox list validation logic in Estimated code review effort: 3 (Moderate) | ~25 minutes ChangesGateway preflight-or-exit consolidation
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 the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
PR Review Advisor (Nemotron Ultra) — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Findings index
Review findings by urgency: 0 required fixes, 0 items to resolve/justify, 2 in-scope improvements
|
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
|
Advisor follow-up for the final head (
No advisor item requires a code change on this head. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/gateway-drift-preflight.test.ts (1)
231-237: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSuffix-only stripping is fragile.
nodeOptionsWithoutSourceLoaderonly removesSOURCE_REQUIRE_OPTIONwhen it appears at the exact tail ofNODE_OPTIONS(or equals it entirely). If any other flag is appended after the source loader (e.g., by another test-runner integration or future ambient env changes), the loader silently leaks into the compiled-dist child process, undermining the intent of "pins them to case-local OpenShell state" and removing the source loader from compiled children.♻️ More robust token-based stripping
function nodeOptionsWithoutSourceLoader(nodeOptions: string | undefined): string { - if (!nodeOptions || nodeOptions === SOURCE_REQUIRE_OPTION) return ""; - const sourceLoaderSuffix = ` ${SOURCE_REQUIRE_OPTION}`; - return nodeOptions.endsWith(sourceLoaderSuffix) - ? nodeOptions.slice(0, -sourceLoaderSuffix.length) - : nodeOptions; + if (!nodeOptions) return ""; + return nodeOptions + .split(/\s+/) + .filter((token) => token && token !== SOURCE_REQUIRE_OPTION) + .join(" "); }🤖 Prompt for 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. In `@test/gateway-drift-preflight.test.ts` around lines 231 - 237, `nodeOptionsWithoutSourceLoader` is too brittle because it only strips `SOURCE_REQUIRE_OPTION` when it is the exact suffix of `NODE_OPTIONS`; update this helper to remove that token by parsing `nodeOptions` into individual flags and filtering out the source-loader entry wherever it appears, then rejoin the remaining options. Keep the existing `SOURCE_REQUIRE_OPTION` symbol and adjust the logic used by the child-process setup in `test/gateway-drift-preflight.test.ts` so compiled-dist children never inherit the loader even when other `NODE_OPTIONS` flags are present.src/lib/openshell-sandbox-list.test.ts (1)
126-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an argument-based
detectResultIssuemock here.
mockReturnValueOnce(null).mockReturnValueOnce(issue)ties this test to the current two-call sequence and can leave queued state behind if that flow changes. A value-basedmockImplementationkeeps the assertion focused on the retry result instead of call order.♻️ Suggested fix
- mocks.detectResultIssue.mockReturnValueOnce(null).mockReturnValueOnce(issue); + mocks.detectResultIssue.mockImplementation((result: { output?: string }) => + result.output === issue.output ? issue : null, + );🤖 Prompt for 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. In `@src/lib/openshell-sandbox-list.test.ts` around lines 126 - 164, The test for captureSandboxListWithGatewayPreflightOrExit is relying on a queued detectResultIssue return sequence, which couples it to the current call order. Update the mock in the protobuf_mismatch case to use an argument-based mockImplementation that returns the issue only for the retry output and null otherwise, so the assertion stays focused on the observed result rather than the number of detectResultIssue calls. Keep the rest of the test behavior and expectations unchanged.Source: Path instructions
🤖 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.
Nitpick comments:
In `@src/lib/openshell-sandbox-list.test.ts`:
- Around line 126-164: The test for captureSandboxListWithGatewayPreflightOrExit
is relying on a queued detectResultIssue return sequence, which couples it to
the current call order. Update the mock in the protobuf_mismatch case to use an
argument-based mockImplementation that returns the issue only for the retry
output and null otherwise, so the assertion stays focused on the observed result
rather than the number of detectResultIssue calls. Keep the rest of the test
behavior and expectations unchanged.
In `@test/gateway-drift-preflight.test.ts`:
- Around line 231-237: `nodeOptionsWithoutSourceLoader` is too brittle because
it only strips `SOURCE_REQUIRE_OPTION` when it is the exact suffix of
`NODE_OPTIONS`; update this helper to remove that token by parsing `nodeOptions`
into individual flags and filtering out the source-loader entry wherever it
appears, then rejoin the remaining options. Keep the existing
`SOURCE_REQUIRE_OPTION` symbol and adjust the logic used by the child-process
setup in `test/gateway-drift-preflight.test.ts` so compiled-dist children never
inherit the loader even when other `NODE_OPTIONS` flags are present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3572c6d9-85c0-458f-bb54-be6f10096125
📒 Files selected for processing (9)
src/lib/actions/gateway-drift-preflight.test.tssrc/lib/actions/maintenance.test.tssrc/lib/actions/maintenance.tssrc/lib/actions/upgrade-sandboxes-preflight.test.tssrc/lib/actions/upgrade-sandboxes-recovery.test.tssrc/lib/actions/upgrade-sandboxes.tssrc/lib/openshell-sandbox-list.test.tssrc/lib/openshell-sandbox-list.tstest/gateway-drift-preflight.test.ts
💤 Files with no reviewable changes (1)
- src/lib/actions/gateway-drift-preflight.test.ts
<!-- markdownlint-disable MD041 --> ## Summary Run the integration project as a bounded four-worker phase during the canonical local `npm test`, while keeping CI, coverage, focused integration, and direct Vitest runs serialized. Isolate two onboarding fixtures from host-global dashboard ports so the parallel suite remains deterministic. This is the final cumulative #6245 step after the named onboarding conversions, representative process-contract work, and sequenced loader cleanup already merged; the final clean-build Node 22 suite passes in 3:52.03. ## Related Issue Closes #6245. ## Changes - Replace the dashboard-exhaustion fixture's real host listeners with a fake `lsof` while retaining the real CLI, preflight, diagnostic, and non-zero exit contract. - Give the restore-intent fixture an explicit existing dashboard forward so unrelated host port occupancy cannot divert the behavior under test. - Resolve integration scheduling from npm lifecycle, CI, coverage, and worker-cap inputs: local `npm test` uses at most four workers in group 1, while every safety-sensitive route stays serial. - Add a behavior matrix covering local, CI, coverage, focused, direct, and explicit worker-throttle modes. - Complete the cumulative #6245 acceptance path after #6276/#6336/#6383 converted the named onboarding hotspots, #6285/#6417 retained representative process contracts, and #6286/#6299/#6388/#6415 sequenced loader cleanup after process removal. - Record the final host-specific timings, hotspot disposition, and retained process-contract inventory in `test/README.md` as an advisory acceptance snapshot rather than a permanent CI budget. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [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: Test fixtures and local test-runner scheduling changed; NemoClaw commands, configuration, runtime behavior, and CI/coverage workflows are unchanged. - [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 final-diff review confirmed that the fake `lsof` preserves the real CLI/preflight/exit contract, the restore-intent assertions remain intact, and resolved CI/coverage configurations remain serialized. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed 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: Real CLI exhaustion contract passed; restore-intent passed with all 11 dashboard ports deliberately occupied; scheduling matrix passed 14/14 through the lifecycle-triggered config; `npm run test:projects:check` reported 1,327 files disjoint across 8 projects. - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Clean-build Node 22 `npm test -- --reporter=blob` under the normal `umask 022` passed 1,251 files and 13,879 tests with 39 skipped, 1 todo, and zero failures in 3:52.03, down 73% from the issue's 14:19.65 baseline despite a larger suite. The matching diff-scoped routine pre-commit stage passed in 13.95s. #6270 separately removed full coverage from routine pre-commit while preserving manual and authoritative CI gates. - [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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Integration test runs now use adaptive scheduling to speed up local execution while keeping CI/focused runs serialized. * **Bug Fixes** * Improved reliability of onboarding regression coverage by simulating dashboard port exhaustion in a hermetic way. * Updated onboarding-related fixtures to better match the intended readiness/exit behavior. * **Tests** * Added coverage for integration scheduling behavior (local caps, invalid inputs, and CI/coverage scenarios). * **Documentation** * Added test-suite documentation with a local performance snapshot and key test hotspots. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
<!-- markdownlint-disable MD041 --> ## Summary Narrows gateway-drift preflight coverage so fail-closed behavior no longer loads the 632-module maintenance and upgrade graph inside a timed hook. This addresses the hottest source-loader path behind NVIDIA#6237 while preserving the distinct process-level drift contracts. ## Related Issue Refs NVIDIA#6237 ## Changes - Centralize sandbox-list preflight, one-shot recovery, result classification, and generic failure handling in a leaf helper shared by `backup-all` and `upgrade-sandboxes`. - Replace the timed broad-graph preflight suite with leaf behavior tests and small caller-adapter tests, reducing the measured import graph from 632 modules to 68 (89.2%). - Retain five distinct compiled-CLI drift sentinels, pin them to case-local OpenShell state, and remove only Vitest's appended TypeScript loader from those compiled children. - Verify 36 focused tests in 3.13s locally, including the real process contract in 2.23s; the latest upstream baseline for the removed source test was 9.18s. ## 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 <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [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: internal test-boundary and shared-helper refactor with no user-facing behavior change; documentation review found no update needed. - [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 fail-closed/security and performance reviews found no actionable findings; focused coverage verifies preflight ordering, one-shot recovery, retry classification, and generic failure status preservation. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed 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 --project cli --project integration src/lib/openshell-sandbox-list.test.ts src/lib/actions/maintenance.test.ts src/lib/actions/upgrade-sandboxes-preflight.test.ts src/lib/actions/upgrade-sandboxes-recovery.test.ts src/lib/actions/sandbox/rebuild-gateway-drift.test.ts test/gateway-drift-preflight.test.ts --reporter=verbose` — 6 files and 36 tests passed in 3.13s. - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: final-head CI passed all five CLI coverage shards and the merged `cli-tests` coverage gate; shard wall times were 5m20s–7m32s with zero infrastructure hook timeouts. - [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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Run the integration project as a bounded four-worker phase during the canonical local `npm test`, while keeping CI, coverage, focused integration, and direct Vitest runs serialized. Isolate two onboarding fixtures from host-global dashboard ports so the parallel suite remains deterministic. This is the final cumulative NVIDIA#6245 step after the named onboarding conversions, representative process-contract work, and sequenced loader cleanup already merged; the final clean-build Node 22 suite passes in 3:52.03. ## Related Issue Closes NVIDIA#6245. ## Changes - Replace the dashboard-exhaustion fixture's real host listeners with a fake `lsof` while retaining the real CLI, preflight, diagnostic, and non-zero exit contract. - Give the restore-intent fixture an explicit existing dashboard forward so unrelated host port occupancy cannot divert the behavior under test. - Resolve integration scheduling from npm lifecycle, CI, coverage, and worker-cap inputs: local `npm test` uses at most four workers in group 1, while every safety-sensitive route stays serial. - Add a behavior matrix covering local, CI, coverage, focused, direct, and explicit worker-throttle modes. - Complete the cumulative NVIDIA#6245 acceptance path after NVIDIA#6276/NVIDIA#6336/NVIDIA#6383 converted the named onboarding hotspots, NVIDIA#6285/NVIDIA#6417 retained representative process contracts, and NVIDIA#6286/NVIDIA#6299/NVIDIA#6388/NVIDIA#6415 sequenced loader cleanup after process removal. - Record the final host-specific timings, hotspot disposition, and retained process-contract inventory in `test/README.md` as an advisory acceptance snapshot rather than a permanent CI budget. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [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: Test fixtures and local test-runner scheduling changed; NemoClaw commands, configuration, runtime behavior, and CI/coverage workflows are unchanged. - [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 final-diff review confirmed that the fake `lsof` preserves the real CLI/preflight/exit contract, the restore-intent assertions remain intact, and resolved CI/coverage configurations remain serialized. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed 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: Real CLI exhaustion contract passed; restore-intent passed with all 11 dashboard ports deliberately occupied; scheduling matrix passed 14/14 through the lifecycle-triggered config; `npm run test:projects:check` reported 1,327 files disjoint across 8 projects. - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Clean-build Node 22 `npm test -- --reporter=blob` under the normal `umask 022` passed 1,251 files and 13,879 tests with 39 skipped, 1 todo, and zero failures in 3:52.03, down 73% from the issue's 14:19.65 baseline despite a larger suite. The matching diff-scoped routine pre-commit stage passed in 13.95s. NVIDIA#6270 separately removed full coverage from routine pre-commit while preserving manual and authoritative CI gates. - [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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Integration test runs now use adaptive scheduling to speed up local execution while keeping CI/focused runs serialized. * **Bug Fixes** * Improved reliability of onboarding regression coverage by simulating dashboard port exhaustion in a hermetic way. * Updated onboarding-related fixtures to better match the intended readiness/exit behavior. * **Tests** * Added coverage for integration scheduling behavior (local caps, invalid inputs, and CI/coverage scenarios). * **Documentation** * Added test-suite documentation with a local performance snapshot and key test hotspots. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
Narrows gateway-drift preflight coverage so fail-closed behavior no longer loads the 632-module maintenance and upgrade graph inside a timed hook. This addresses the hottest source-loader path behind #6237 while preserving the distinct process-level drift contracts.
Related Issue
Refs #6237
Changes
backup-allandupgrade-sandboxes.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project cli --project integration src/lib/openshell-sandbox-list.test.ts src/lib/actions/maintenance.test.ts src/lib/actions/upgrade-sandboxes-preflight.test.ts src/lib/actions/upgrade-sandboxes-recovery.test.ts src/lib/actions/sandbox/rebuild-gateway-drift.test.ts test/gateway-drift-preflight.test.ts --reporter=verbose— 6 files and 36 tests passed in 3.13s.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: final-head CI passed all five CLI coverage shards and the mergedcli-testscoverage gate; shard wall times were 5m20s–7m32s with zero infrastructure hook timeouts.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com