perf(test): retire rebuild CommonJS loader seams - #6388
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR extracts sandbox helper logic into standalone modules, routes upgrade-sandboxes through an injectable dependency wrapper, updates related tests to static ES imports and new spies, adds a package-contract boundary test, and revises the createRequire budget allowlist. ChangesSandbox rebuild and upgrade module refactor
Estimated code review effort: 3 (Moderate) | ~25 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
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) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/actions/sandbox/rebuild-flow-helpers.test.ts (1)
45-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDouble-cast (
as unknown as X) masks a fixture/type mismatch.
makeSandboxEntry's return is force-cast throughunknown, which fully bypasses structural type-checking againstParameters<typeof backupSandboxStateForRebuild>[1]. If that parameter type later gains new required fields, this fixture will silently stay incomplete without a compiler error. Consider building a fixture that actually satisfies the type (e.g., viasatisfiesplus filling in the missing required fields) or usingPartial<...>explicitly if a partial fixture is intentional.🤖 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/actions/sandbox/rebuild-flow-helpers.test.ts` around lines 45 - 54, The double-cast in makeSandboxEntry is hiding a fixture that may no longer match Parameters<typeof backupSandboxStateForRebuild>[1]. Replace the as unknown as X pattern with a real value that structurally satisfies the target type, or make the intent explicit by using Partial<...> if a partial fixture is required. Use makeSandboxEntry and backupSandboxStateForRebuild as the key references when updating the test fixture.
🤖 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/actions/sandbox/rebuild-config-hash-command.ts`:
- Around line 9-23: The shell command built by rebuild-config-hash-command
currently masks failures from writing the new hash because the `sha256sum` step
is followed by a permissive `chmod ... || true` in a `;`-joined chain. Update
the command returned by the config hash builder so the write to `.config-hash`
must succeed before any later step runs, using a fail-fast chain or an explicit
exit path, and keep the existing symlink/ownership checks intact. Make sure
`refreshMutableOpenClawConfigHashAfterPostRestoreWrites` can detect the failure
via `mutableConfigHashRefreshUnverified` rather than seeing a false success.
---
Nitpick comments:
In `@src/lib/actions/sandbox/rebuild-flow-helpers.test.ts`:
- Around line 45-54: The double-cast in makeSandboxEntry is hiding a fixture
that may no longer match Parameters<typeof backupSandboxStateForRebuild>[1].
Replace the as unknown as X pattern with a real value that structurally
satisfies the target type, or make the intent explicit by using Partial<...> if
a partial fixture is required. Use makeSandboxEntry and
backupSandboxStateForRebuild as the key references when updating the test
fixture.
🪄 Autofix (Beta)
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: e4471059-1f24-4670-b509-1f01ca66bc09
📒 Files selected for processing (11)
scripts/checks/test-create-require-budget.tssrc/lib/actions/sandbox/rebuild-config-hash-command.tssrc/lib/actions/sandbox/rebuild-config-hash.test.tssrc/lib/actions/sandbox/rebuild-config-hash.tssrc/lib/actions/sandbox/rebuild-flow-helpers.test.tssrc/lib/actions/sandbox/rebuild-messaging-phase.tssrc/lib/actions/sandbox/rebuild-messaging-stage.test.tssrc/lib/actions/sandbox/rebuild-messaging-stage.tssrc/lib/actions/upgrade-sandboxes-preflight.test.tssrc/lib/actions/upgrade-sandboxes-recovery.test.tssrc/lib/actions/upgrade-sandboxes.ts
💤 Files with no reviewable changes (1)
- scripts/checks/test-create-require-budget.ts
Collaborator
Author
|
Automated-review follow-up on final head
No docs update is needed: the CLI already reports and remediates the existing incomplete config-hash state, and no user-facing command/config contract changed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/package-contract/rebuild-loader-boundary.test.ts (2)
62-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate path literal instead of reusing
rebuildPath.Test 1 already computes
rebuildPathviapath.join(repoRoot, ...); test 2 hardcodes"../../dist/lib/actions/sandbox/rebuild.js"instead of reusing a shared path derivation, risking drift if the dist layout changes.♻️ Proposed fix
it("preserves the public rebuild facade exports (`#6245`)", () => { - const rebuild = require("../../dist/lib/actions/sandbox/rebuild.js") as { + const rebuildPath = path.join(repoRoot, "dist", "lib", "actions", "sandbox", "rebuild.js"); + const rebuild = require(rebuildPath) as { buildRefreshMutableOpenClawConfigHashCommand?: (configDir?: string) => string; stageMessagingManifestPlanForRebuild?: (...args: unknown[]) => Promise<unknown>; };🤖 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/package-contract/rebuild-loader-boundary.test.ts` at line 62, The rebuild boundary test hardcodes the dist module path in the second test instead of reusing the already computed rebuildPath, which can drift if the layout changes. Update the require in rebuild-loader-boundary.test.ts to use the shared rebuildPath (or the same path derivation logic used in the first test) so both tests resolve the same target consistently.
22-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid tying this test to
Module._loadinternals. The lazy-load assertion depends on an undocumented Node loader hook, so it’s brittle across Node upgrades. Prefer a public-boundary check, or isolate the loader seam behind an injectable helper if this coverage must stay.🤖 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/package-contract/rebuild-loader-boundary.test.ts` around lines 22 - 35, The test is coupled to Node’s internal Module._load loader hook, which is brittle across Node versions. Update rebuild-loader-boundary.test.ts to verify the lazy-load behavior through a public boundary instead, or extract the loading behavior into an injectable helper and test that seam directly. Keep the assertion centered on the upgradePath/rebuildSandbox flow and remove the direct Module._load interception.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.
Inline comments:
In `@src/lib/actions/sandbox/rebuild-config-hash.test.ts`:
- Around line 71-93: Guard the failing hash-command test in
rebuild-config-hash.test.ts against root execution, since
buildRefreshMutableOpenClawConfigHashCommand can short-circuit with a 0 exit
when the config directory owner is root and bypass the injected sha256sum
failure. Update the test around runRefresh and the “reports hash command
failures instead of masking them” case to skip or assert the non-root assumption
when process.getuid?.() === 0 so the expected status 14 is only checked where
the failure path is reachable.
---
Nitpick comments:
In `@test/package-contract/rebuild-loader-boundary.test.ts`:
- Line 62: The rebuild boundary test hardcodes the dist module path in the
second test instead of reusing the already computed rebuildPath, which can drift
if the layout changes. Update the require in rebuild-loader-boundary.test.ts to
use the shared rebuildPath (or the same path derivation logic used in the first
test) so both tests resolve the same target consistently.
- Around line 22-35: The test is coupled to Node’s internal Module._load loader
hook, which is brittle across Node versions. Update
rebuild-loader-boundary.test.ts to verify the lazy-load behavior through a
public boundary instead, or extract the loading behavior into an injectable
helper and test that seam directly. Keep the assertion centered on the
upgradePath/rebuildSandbox flow and remove the direct Module._load interception.
🪄 Autofix (Beta)
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: 4d324da9-32ca-44ef-b215-09c69804821e
📒 Files selected for processing (4)
src/lib/actions/sandbox/rebuild-config-hash-command.tssrc/lib/actions/sandbox/rebuild-config-hash.test.tssrc/lib/actions/sandbox/rebuild-flow-helpers.test.tstest/package-contract/rebuild-loader-boundary.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/actions/sandbox/rebuild-config-hash-command.ts
Contributor
E2E Target Results — ❌ Some jobs failedRun: 28857240747
|
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28857578895
|
Collaborator
Author
|
Final-head validation settled:
No unresolved actionable automated-review findings remain. |
Collaborator
Author
|
Follow-up |
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28858495296
|
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28858774350
|
Collaborator
Author
|
Final-head follow-up on
No further code changes are needed for this batch. |
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
<!-- markdownlint-disable MD041 --> ## Summary Reduce cold collection overhead in four rebuild-focused CLI test files by replacing CommonJS source-loader/cache-invalidation seams with native source imports and dependency-light production boundaries. This preserves the existing behavioral coverage while removing incidental loading of the full rebuild graph. ## Related Issue Refs NVIDIA#6245 Refs NVIDIA#6237 ## Changes - Extract messaging-plan staging and config-hash command construction into dependency-light leaf modules while preserving the public rebuild facade exports. - Defer loading `sandbox/rebuild` from `upgrade-sandboxes` until a sandbox actually needs rebuilding, with an explicit dependency seam for focused tests. - Convert four CLI suites from `createRequire`, cache deletion, and loader warmups to native imports and typed spies. - Tighten the exact-path `createRequire` ratchet from 32 to 28 CLI test files. - Preserve all 43 existing assertions in the optimized suites and the real Bash/filesystem, manifest planner, recovery, and gateway-classification contracts. - Add a compiled package-contract test for lazy rebuild loading and facade exports, and make config-hash refresh propagate `sha256sum` failures instead of masking them behind best-effort permission repair. Matched CI evidence: the previous merged head reported 28.806s of aggregate collection time for these four files. Final-head NVIDIA#6388 CI reports 3.041s, a reduction of 25.765s (89.44%; 9.47× faster), while preserving the original assertions and adding the hash-failure regression. Aggregate collection work overlaps across Vitest shards, so this is not a claim of equivalent one-for-one shard-wall savings. ## 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: internal module-boundary and test-loader refactor only; commands, flags, defaults, configuration, protocols, and user-visible behavior 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: two independent source reviews found no blocking findings; focused rebuild, recovery, preflight, messaging, config-hash, and compiled package-contract coverage passed. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [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 — 126/126 focused CLI tests passed across the optimized suites, upgrade preflight, and the broader rebuild flow; 2/2 compiled package-contract tests and the ratchet's 8/8 integration tests also passed. - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — five-shard final-head `CI / Pull Request` and coverage merge passed; all 40 PR checks are green. - [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) Additional local verification: - `npm run typecheck:cli` - `npm run build:cli` - compiled package-contract coverage proves importing `dist/lib/actions/upgrade-sandboxes.js` does not eagerly load the rebuild module, forwards the lazy call exactly, and preserves both extracted rebuild facade exports - `npx tsx scripts/checks/test-create-require-budget.ts` (28 CLI files, 8 support files) - `npm run test:projects:check` - required live E2E passed: `rebuild-openclaw`, `rebuild-hermes`, `sandbox-rebuild`, `upgrade-stale-sandbox`, and `channels-add-remove`, and `messaging-providers` (OpenClaw passed on retry after an initial npm `ECONNRESET` during fixture setup) - `git diff --check` --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added rebuild messaging “recreate contract” planning based on built-in channel manifests. * Introduced a safer sandbox config-hash refresh command with stricter pre-checks before updating the mutable OpenClaw config hash. * **Bug Fixes** * Improved restore/recovery and rebuild messaging preparation, including safer skip conditions when messaging support is unavailable. * Ensured config-hash refresh failures surface correctly, with stronger protections against symlinked/mismatched config inputs. * **Tests** * Updated and expanded package-contract and rebuild flow tests to validate loader laziness and expected rebuild entrypoints. <!-- 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
<!-- 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 -->
wscurran
added
area: cli
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 cold collection overhead in four rebuild-focused CLI test files by replacing CommonJS source-loader/cache-invalidation seams with native source imports and dependency-light production boundaries. This preserves the existing behavioral coverage while removing incidental loading of the full rebuild graph.
Related Issue
Refs #6245
Refs #6237
Changes
sandbox/rebuildfromupgrade-sandboxesuntil a sandbox actually needs rebuilding, with an explicit dependency seam for focused tests.createRequire, cache deletion, and loader warmups to native imports and typed spies.createRequireratchet from 32 to 28 CLI test files.sha256sumfailures instead of masking them behind best-effort permission repair.Matched CI evidence: the previous merged head reported 28.806s of aggregate collection time for these four files. Final-head #6388 CI reports 3.041s, a reduction of 25.765s (89.44%; 9.47× faster), while preserving the original assertions and adding the hash-failure regression. Aggregate collection work overlaps across Vitest shards, so this is not a claim of equivalent one-for-one shard-wall savings.
Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — five-shard final-headCI / Pull Requestand coverage merge passed; all 40 PR checks are green.npm run docsbuilds without warnings (doc changes only)Additional local verification:
npm run typecheck:clinpm run build:clidist/lib/actions/upgrade-sandboxes.jsdoes not eagerly load the rebuild module, forwards the lazy call exactly, and preserves both extracted rebuild facade exportsnpx tsx scripts/checks/test-create-require-budget.ts(28 CLI files, 8 support files)npm run test:projects:checkrebuild-openclaw,rebuild-hermes,sandbox-rebuild,upgrade-stale-sandbox, andchannels-add-remove, andmessaging-providers(OpenClaw passed on retry after an initial npmECONNRESETduring fixture setup)git diff --checkSigned-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests