perf(test): reduce CommonJS loader churn - #6299
Merged
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a createRequire budget check, shared NODE_OPTIONS helpers, injectable runtime seams for sandbox and gateway actions, and test migrations from CommonJS loading to direct ESM imports. ChangesCreateRequire budget and ESM migration
Estimated code review effort: 4 (Complex) | ~60 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 (Nemotron Ultra) — InformationalMerge posture: Informational / low confidence Action checklist
Findings index
Review findings by urgency: 0 required fixes, 1 item to resolve/justify, 0 in-scope improvements
|
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
|
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 2 items to resolve/justify, 0 in-scope improvements
|
![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 (4)
scripts/checks/test-create-require-budget.ts (1)
119-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate collector logic between production and test-support collectors.
collectProductionCreateRequireSourcesandcollectTestSupportCreateRequireSourcesare byte-identical apart from their defaultrootargument and name. As per path instructions forscripts/checks/**, "Derive inventories and limits from a canonical source where possible; flag duplicated lists that can silently drift" — extracting a shared helper avoids the two implementations drifting apart under future edits.♻️ Proposed refactor: extract shared helper
+function collectNonTestCreateRequireSources(root: string): string[] { + return [...walkTypeScriptFiles(root)] + .filter((absolutePath) => !TEST_FILE_PATTERN.test(absolutePath)) + .filter((absolutePath) => + containsCreateRequireIdentifier(readFileSync(absolutePath, "utf8"), absolutePath), + ) + .map((absolutePath) => path.relative(REPO_ROOT, absolutePath).split(path.sep).join("/")) + .sort(); +} + export function collectProductionCreateRequireSources(root = CLI_TEST_ROOT): string[] { - return [...walkTypeScriptFiles(root)] - .filter((absolutePath) => !TEST_FILE_PATTERN.test(absolutePath)) - .filter((absolutePath) => - containsCreateRequireIdentifier(readFileSync(absolutePath, "utf8"), absolutePath), - ) - .map((absolutePath) => path.relative(REPO_ROOT, absolutePath).split(path.sep).join("/")) - .sort(); + return collectNonTestCreateRequireSources(root); } export function collectTestSupportCreateRequireSources(root = TEST_SUPPORT_ROOT): string[] { - return [...walkTypeScriptFiles(root)] - .filter((absolutePath) => !TEST_FILE_PATTERN.test(absolutePath)) - .filter((absolutePath) => - containsCreateRequireIdentifier(readFileSync(absolutePath, "utf8"), absolutePath), - ) - .map((absolutePath) => path.relative(REPO_ROOT, absolutePath).split(path.sep).join("/")) - .sort(); + return collectNonTestCreateRequireSources(root); }🤖 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 `@scripts/checks/test-create-require-budget.ts` around lines 119 - 137, The two collectors, collectProductionCreateRequireSources and collectTestSupportCreateRequireSources, duplicate the same walk/filter/map/sort pipeline and can drift over time. Extract the shared logic into a single helper that accepts the root and reuse it from both functions, keeping only the differing default roots and exported names.Source: Path instructions
test/test-create-require-budget.test.ts (1)
50-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for real JSX content in
.tsxfixtures.This scan test only writes plain
importstatements into.mts/.cts/.tsxfixtures. It doesn't exercise a.tsxfile containing actual JSX syntax alongsidecreateRequire, so a scriptKind-related parsing regression (see companion comment onscripts/checks/test-create-require-budget.tslines 83-107) would go undetected here.🤖 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/test-create-require-budget.test.ts` around lines 50 - 72, The scan test currently covers only plain import statements, so it misses JSX parsing behavior in .tsx files. Update the test around collectProductionCreateRequireSources and collectTestCreateRequireSources to write a real .tsx fixture containing actual JSX syntax plus createRequire, and keep the existing .mts/.cts assertions so scriptKind-related regressions are exercised.Source: Path instructions
src/lib/actions/sandbox/policy-channel-agent-gate.test.ts (1)
19-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated test fixture helpers to a shared module.
agentFixtureandsuccessfulOpenshellResultare duplicated verbatim inpolicy-channel-cleanup.test.tsandpolicy-channel-conflict.test.ts. This PR already establishes the pattern of extracting shared test scaffolding (seetest/helpers/source-loader-options.ts); doing the same here (e.g. asandbox/test-helpers.ts) would prevent the three copies from silently diverging.Also note
agentFixturecasts{ name } as defs.AgentDefinition, bypassing the type checker for the many required fields onAgentDefinition— fine as long as tests only touch.name, but worth keeping in mind if a future test exercises another field via this fixture.🤖 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/policy-channel-agent-gate.test.ts` around lines 19 - 32, The duplicated test fixture helpers should be moved into a shared sandbox test helper module so the copies in these policy-channel tests do not drift apart. Extract `agentFixture` and `successfulOpenshellResult` into a common helper (for example alongside the existing test scaffolding pattern used by `test/helpers/source-loader-options.ts`), then update `policy-channel-agent-gate.test.ts`, `policy-channel-cleanup.test.ts`, and `policy-channel-conflict.test.ts` to import and use those shared helpers. Keep the `agentFixture` shape intentional and confined to the `.name` usage in these tests.src/lib/actions/sandbox/policy-channel.ts (1)
106-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLift the non-interactive check into one shared helper The same
NEMOCLAW_NON_INTERACTIVEcheck is repeated indestroy.tsand other boundary modules; centralize it if these paths need to stay aligned.🤖 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/policy-channel.ts` around lines 106 - 108, The NEMOCLAW_NON_INTERACTIVE environment check is duplicated across multiple boundary modules, so centralize it into one shared helper and reuse it everywhere instead of keeping separate copies. Move the logic currently in isNonInteractive in policy-channel.ts into a common utility, then update destroy.ts and any other callers to use that shared helper so the behavior stays aligned in one place.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 `@test/test-create-require-budget.test.ts`:
- Around line 25-90: The test suite in containsCreateRequireIdentifier,
collectProductionCreateRequireSources, collectTestSupportCreateRequireSources,
and createRequireBudgetFailure uses behavior-oriented titles but is missing the
required local issue-reference suffix. Update each it(...) title in this file to
end with a matching `(`#1234`)`-style suffix per the test naming guideline,
keeping the existing behavior-focused wording and adding the appropriate issue
ref for each case.
---
Nitpick comments:
In `@scripts/checks/test-create-require-budget.ts`:
- Around line 119-137: The two collectors, collectProductionCreateRequireSources
and collectTestSupportCreateRequireSources, duplicate the same
walk/filter/map/sort pipeline and can drift over time. Extract the shared logic
into a single helper that accepts the root and reuse it from both functions,
keeping only the differing default roots and exported names.
In `@src/lib/actions/sandbox/policy-channel-agent-gate.test.ts`:
- Around line 19-32: The duplicated test fixture helpers should be moved into a
shared sandbox test helper module so the copies in these policy-channel tests do
not drift apart. Extract `agentFixture` and `successfulOpenshellResult` into a
common helper (for example alongside the existing test scaffolding pattern used
by `test/helpers/source-loader-options.ts`), then update
`policy-channel-agent-gate.test.ts`, `policy-channel-cleanup.test.ts`, and
`policy-channel-conflict.test.ts` to import and use those shared helpers. Keep
the `agentFixture` shape intentional and confined to the `.name` usage in these
tests.
In `@src/lib/actions/sandbox/policy-channel.ts`:
- Around line 106-108: The NEMOCLAW_NON_INTERACTIVE environment check is
duplicated across multiple boundary modules, so centralize it into one shared
helper and reuse it everywhere instead of keeping separate copies. Move the
logic currently in isNonInteractive in policy-channel.ts into a common utility,
then update destroy.ts and any other callers to use that shared helper so the
behavior stays aligned in one place.
In `@test/test-create-require-budget.test.ts`:
- Around line 50-72: The scan test currently covers only plain import
statements, so it misses JSX parsing behavior in .tsx files. Update the test
around collectProductionCreateRequireSources and collectTestCreateRequireSources
to write a real .tsx fixture containing actual JSX syntax plus createRequire,
and keep the existing .mts/.cts assertions so scriptKind-related regressions are
exercised.
🪄 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: 14d76473-927c-4d95-a778-efa750ee46f3
📒 Files selected for processing (17)
.pre-commit-config.yamlscripts/checks/run.tsscripts/checks/test-create-require-budget.tssrc/lib/actions/sandbox/policy-channel-agent-gate.test.tssrc/lib/actions/sandbox/policy-channel-cleanup.test.tssrc/lib/actions/sandbox/policy-channel-conflict.test.tssrc/lib/actions/sandbox/policy-channel-policy.test.tssrc/lib/actions/sandbox/policy-channel-refresh.test.tssrc/lib/actions/sandbox/policy-channel-remove-flow.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/runner-argv.test.tssrc/lib/status-command-deps.test.tstest/cli/helpers.test.tstest/gateway-drift-preflight.test.tstest/helpers/source-loader-options.tstest/test-create-require-budget.test.tsvitest.config.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/test-create-require-budget.test.ts (1)
87-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant assertion doesn't exercise the production/test-support distinction.
Both
collectProductionCreateRequireSourcesandcollectTestSupportCreateRequireSourcesare called with the samedirectoryand checked against the identical expected array. Per the graph context, these functions only differ in their defaultroot(CLI_TEST_ROOTvsTEST_SUPPORT_ROOT), which isn't exercised here since both calls override the root explicitly. As written, the second assertion re-verifies the same shared filtering/traversal logic rather than the distinguishing behavior the test title implies ("production and non-test support files").Consider dropping the duplicate assertion (or asserting on the respective default roots) to keep the test focused on what it claims to cover.
🤖 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/test-create-require-budget.test.ts` around lines 87 - 93, The test is redundantly asserting the same behavior for both collectProductionCreateRequireSources and collectTestSupportCreateRequireSources by passing the same directory and expecting the same result, so it does not verify their different default roots. Update the test to either keep only one assertion if the shared traversal/filtering is what’s being covered, or call each function in a way that exercises its default root behavior (CLI_TEST_ROOT vs TEST_SUPPORT_ROOT). Keep the focus on the production/create-require source distinction in test/test-create-require-budget.test.ts.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 `@test/test-create-require-budget.test.ts`:
- Around line 87-93: The test is redundantly asserting the same behavior for
both collectProductionCreateRequireSources and
collectTestSupportCreateRequireSources by passing the same directory and
expecting the same result, so it does not verify their different default roots.
Update the test to either keep only one assertion if the shared
traversal/filtering is what’s being covered, or call each function in a way that
exercises its default root behavior (CLI_TEST_ROOT vs TEST_SUPPORT_ROOT). Keep
the focus on the production/create-require source distinction in
test/test-create-require-budget.test.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3c696edb-90a0-4719-8de6-93daf724a4d8
📒 Files selected for processing (4)
scripts/checks/test-create-require-budget.tstest/cli/helpers.test.tstest/helpers/source-loader-options.tstest/test-create-require-budget.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/cli/helpers.test.ts
- test/helpers/source-loader-options.ts
- scripts/checks/test-create-require-budget.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
Automated-review follow-up for
Focused verification on this head: 16/16 tests passed, Biome passed, the CLI type-check passed in the normal pre-push hook, and the loader budget remains 35 CLI test files / 8 support files. |
Collaborator
Author
|
Preserving malformed input is the accepted contract here; attempting to surgically remove a token from a string whose boundaries cannot be established would be the unsafe behavior. No further code change is planned for this non-binding finding. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
CI follow-up in The first final-head run and a failed-job-only rerun both reproduced 10-second The fix moves the four rebuild-to-onboard calls behind one lazy typed boundary, converts the snapshot suite to native imports/seam spies, and narrows the shields suite to the pipeline Verification on the new head:
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
On that basis, the requested direct seam tests would add a weaker implementation-coupled loader test and are intentionally not added. The existing behavior, direct-spy compatibility, coverage reproduction, and exact-path ratchet are the accepted validation for these two non-blocking warnings. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
Additional CI root-cause follow-up in After the production rebuild boundary became lazy, two legacy shared harnesses still called Both harnesses now spy Validation on
No timeout was raised and no behavior was bypassed; this removes the cold loader work that the timeout was identifying. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
Final-head cold-cache fix: The remaining shard failures all had the same cause. The main shared rebuild harness mocked This commit:
Verification on a genuinely cold source-transpile cache with V8 coverage:
The commit is DCO-signed and GitHub-verified. |
wscurran
added
area: ci
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 This removes repeated `createRequire` use and CommonJS cache manipulation from the policy-channel, gateway-runtime, and rebuild suites, replaces native-safe seams with typed imports, and keeps heavyweight provider/rebuild dependencies late-bound. It reduces CLI test files using `createRequire` from 44 to 32 and adds exact path guardrails so the remaining seams can only decrease. ## Related Issue Refs NVIDIA#6245 ## Changes - Convert six policy/channel suites plus runner and status tests to native imports and typed dependency seams. - Load policy conflict detection from its leaf module while deferring provider and rebuild graphs until their runtime paths execute. - Replace the gateway-runtime suite's per-test onboard graph load and cache invalidation with a native, late-bound dependency seam. - Move rebuild-to-onboard calls behind one lazy typed boundary and replace two coverage-timeout rebuild suites with native, phase-focused tests. - Centralize source-loader `NODE_OPTIONS` quoting/removal, preserving unrelated options and limiting bypass to the explicit compiled-artifact test. - Enforce exact `createRequire` allowlists for 32 CLI tests and 8 support files across `.ts`, `.mts`, `.cts`, and `.tsx`; production TypeScript remains prohibited and the scanner skips symlinks. - Expand the repository-check hook matcher to cover every TypeScript module extension. - Give the compiled CLI dispatch contract enough polling time under CI contention while retaining cleanup headroom. ## 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 test loading, dependency injection, and repository guardrails only; no user-facing behavior or interface changes. - [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 read-only reviews found no remaining runtime, test-isolation, TypeScript, guardrail, or documentation findings after follow-up. - [ ] 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 — 13 focused source/integration suites passed 147/147; the parser/guard review batch passed 16/16; all 41 rebuild suites passed 311/311, 13 credential integration tests passed, all 10 shared-harness consumer suites passed 105/105, and the three formerly timing-out suites passed 17/17 from a cold source cache with V8 coverage (67–264 ms per file); all 17 package-contract files passed 290/290; the CLI type-check and 32-CLI/8-support budget passed. - [x] Applicable broad gate passed — CI-equivalent five-shard CLI/integration coverage merge passed 11,496 tests with zero failures; coverage passed at 71.48% lines, 72.76% functions, 64.01% branches, and 70.86% statements. - [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> --------- 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 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
3 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
This removes repeated
createRequireuse and CommonJS cache manipulation from the policy-channel, gateway-runtime, and rebuild suites, replaces native-safe seams with typed imports, and keeps heavyweight provider/rebuild dependencies late-bound. It reduces CLI test files usingcreateRequirefrom 44 to 32 and adds exact path guardrails so the remaining seams can only decrease.Related Issue
Refs #6245
Changes
NODE_OPTIONSquoting/removal, preserving unrelated options and limiting bypass to the explicit compiled-artifact test.createRequireallowlists for 32 CLI tests and 8 support files across.ts,.mts,.cts, and.tsx; production TypeScript remains prohibited and the scanner skips symlinks.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com