perf(test): reduce rebuild and provider-selection process isolation - #6383
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe rebuild flow test harness adds a reconciled sandbox gateway override, stale-recovery tests are rewritten around the harness and a simplified fixture, gateway-drift coverage is expanded, one default plugin-entry expectation changes, and a CI budget entry is updated. ChangesRebuild Stale-Recovery Test Refactor
OpenClaw Default Plugin Entry Expectation
CI File-Size Budget Update
Estimated code review effort: 3 (Moderate) | ~20 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 |
Contributor
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 |
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
E2E Target RecommendationRequired E2E targets: None Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
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. |
Contributor
PR Review Advisor (Nemotron Ultra) — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Collaborator
Author
|
Maintainer pass note: I cannot submit an independent review on my own PR. One blocker remains: the wrong-gateway stale-recovery tests now inject terminal wrong_gateway_active state, so they no longer exercise the integrated classifier-to-destructive-guard path. Add at least one test using the real getReconciledSandboxGatewayState flow and prove that a detected wrong gateway triggers neither delete nor onboard. The separate Nemotron claim about losing a successful-recovery happy path is not blocking; the old fixture never completed recreation. |
cv
changed the title
Collaborator
Author
|
Final-head advisor resolution:
The GPT-5.5 advisor reports no blocking findings, CodeRabbit reports no actionable comments, all commits are GitHub-verified, and the PR body records the #6245 benchmark and targeted-test evidence. |
Collaborator
Author
|
Final Nemotron re-indexed finding resolution for head
GPT-5.5 reports merge-as-is. Runtime follow-ups requested there are already complete: provider selection 66/66 twice, stale recovery 5/5, gateway drift 6/6, and the combined rebuild-flow/gateway set 85/85. CodeRabbit reports no actionable comments. |
Collaborator
Author
|
Final-head advisor resolution for
No additional code change is warranted for these non-binding findings. |
21 tasks
cv
added a commit
that referenced
this pull request
Jul 7, 2026
<!-- 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 -->
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
…VIDIA#6383) <!-- markdownlint-disable MD041 --> ## Summary Reduce process isolation in two high-cost integration targets without changing production behavior. Against the final `main` base, stale rebuild recovery drops from 14.94–15.01 seconds to approximately 3.50 seconds (76.6% faster), while provider selection drops from 17.60–18.07 seconds to 9.95–10.23 seconds (43.4% faster). ## Related Issue Part of NVIDIA#6245. ## Changes - replace five redundant full-CLI stale-rebuild launches with the existing direct flow harness while retaining one real CLI/HOME rollback contract - retain a focused 65ms harness case for successful stale recovery, including backup skip, recreate handoff, and finalization assertions - exercise unit-shaped onboarding cases through typed source seams while preserving real-process coverage for module-wiring, environment parsing, and subprocess boundaries - batch nine provider credential back/exit scenarios through their real `setupNim` call sites in one isolated child; full Node boots fall from 50 to 26 with all 66 provider-selection behaviors retained - add a real-classifier rebuild contract for both recorded/active gateway mismatch directions instead of injecting the terminal `wrong_gateway_active` state - ratchet the legacy `onboard-selection.test.ts` size budget from 5,624 to 4,834 lines - sync the latest `main` and correct its stale plugin-entry assertion so the test preserves the intentional omission of `acpx` ## Wrong-Gateway Source-of-Truth Review - **Invalid state:** an empty active-gateway sandbox list is ambiguous when the registry's recorded gateway differs from the currently selected gateway; it must not authorize stale destruction - **Source boundary:** `gateway-state.ts#getReconciledSandboxGatewayState` and its named-gateway reconciliation path own the classification - **Source constraint:** gateway selection is mutable, so a `NotFound` response by itself cannot prove the recorded sandbox is stale on its own gateway - **Regression coverage:** `rebuild-gateway-drift.test.ts` now drives the real classifier for `nemoclaw`/`other-gw` and `nemoclaw-9000`/`nemoclaw`, then verifies no backup, delete, registry removal, or onboard occurs - **Removal condition:** retain this contract until gateway reconciliation and the rebuild mutation decision become one typed, atomic boundary ## 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: test-isolation and regression-coverage changes only; production behavior, CLI output, configuration, defaults, and public APIs 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 reviews verified the real wrong-gateway reconciliation path, destructive-boundary assertions, provider call-site wiring, environment cleanup, and retained process boundaries; no actionable findings remain - [ ] 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 that 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 — `test/onboard-selection.test.ts` passed 66/66 at final head; `test/rebuild-stale-recovery.test.ts` passed 5/5 in 3.49 seconds wall; `rebuild-flow.test.ts` plus `rebuild-gateway-drift.test.ts` passed 85/85; the final `main` sync's OpenClaw config test plus the unrelated Hermes stress file passed 33/33 with 2 environment skips - [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](https://github.com/NVIDIA/NemoClaw/actions/runs/28853151732) passed all five CLI coverage shards, the merged coverage gate, static/type checks, package lanes, and sandbox smoke tests - [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>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- 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 -->
This was referenced Jul 19, 2026
wscurran
added
the
area: ci
wscurran
added
the
area: e2e
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reduce process isolation in two high-cost integration targets without changing production behavior. Against the final
mainbase, stale rebuild recovery drops from 14.94–15.01 seconds to approximately 3.50 seconds (76.6% faster), while provider selection drops from 17.60–18.07 seconds to 9.95–10.23 seconds (43.4% faster).Related Issue
Part of #6245.
Changes
setupNimcall sites in one isolated child; full Node boots fall from 50 to 26 with all 66 provider-selection behaviors retainedwrong_gateway_activestateonboard-selection.test.tssize budget from 5,624 to 4,834 linesmainand correct its stale plugin-entry assertion so the test preserves the intentional omission ofacpxWrong-Gateway Source-of-Truth Review
gateway-state.ts#getReconciledSandboxGatewayStateand its named-gateway reconciliation path own the classificationNotFoundresponse by itself cannot prove the recorded sandbox is stale on its own gatewayrebuild-gateway-drift.test.tsnow drives the real classifier fornemoclaw/other-gwandnemoclaw-9000/nemoclaw, then verifies no backup, delete, registry removal, or onboard occursType of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailabletest/onboard-selection.test.tspassed 66/66 at final head;test/rebuild-stale-recovery.test.tspassed 5/5 in 3.49 seconds wall;rebuild-flow.test.tsplusrebuild-gateway-drift.test.tspassed 85/85; the finalmainsync's OpenClaw config test plus the unrelated Hermes stress file passed 33/33 with 2 environment skipsnpm 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, the merged coverage gate, static/type checks, package lanes, and sandbox smoke testsnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com