Repository navigation
perf(test): reduce subprocess isolation in rebuild and setup tests - #6279
Merged
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
📝 WalkthroughWalkthroughExtracts Hermes config generation into a reusable ChangesHermes Config Generation Extraction
Estimated code review effort: 4 (Complex) | ~60 minutes Sandbox Rebuild Flow: Confirmation Injection and Credential Preflight Testing
Estimated code review effort: 4 (Complex) | ~60 minutes Messaging Plan Helper and Policy Tier Onboarding Test Refactors
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as generate-config.ts
participant Gen as generateHermesConfig
participant Config as hermes-config.ts / hermes-env.ts
participant Matrix as managed-tool-gateway.ts
participant FS as Config Files
CLI->>Gen: generateHermesConfig({env, scriptDir})
Gen->>Gen: read build settings from env
Gen->>Config: buildHermesConfig(settings, env)
Config->>Matrix: loadManagedToolGatewayMatrix(env)
Matrix-->>Config: matrix data
Gen->>Config: buildHermesEnvLines(settings, env)
Config->>Matrix: loadManagedToolGatewayMatrix(env)
Gen->>Gen: finalize platform toolsets
Gen->>FS: write config files
Gen-->>CLI: {settings, config, envLines, written}
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 — 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/helpers/rebuild-flow-test-harness.ts (1)
141-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract a shared
hydrateCredentialEnvspy factory The twohydrateCredentialEnvspies share the same wrapper logic; factor it once so the implementations 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 `@test/helpers/rebuild-flow-test-harness.ts` around lines 141 - 158, The two hydrateCredentialEnv spies in rebuild-flow-test-harness.ts duplicate the same wrapper logic, so extract that shared implementation into a single helper or spy factory and use it for both the onboardMod and onboardCredentialEnv spies. Keep the existing override behavior and defaultHydrateCredentialEnv fallback intact, but centralize the mockImplementation body so hydrateCredentialEnv stays aligned in one place.agents/hermes/generate-config.ts (1)
20-28: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winVerify CLI error UX after removing explicit error handling.
main()invokesgenerateHermesConfigwith notry/catch. Any thrown validation error (e.g., the various "fails fast"/"fails closed" cases now tested in-process) will surface as Node's default uncaught-exception output (full stack trace, exit code 1) instead of a curated one-line message, if the prior implementation had such handling. The PR states CLI output is unchanged — worth confirming that stderr formatting for failure cases still matches expectations for real subprocess invocations.Regardless, wrapping the entrypoint in a try/catch is generally good practice for a CLI script to keep stderr clean.
♻️ Optional: cleaner CLI error output
export function main(): void { - generateHermesConfig({ env: process.env, scriptDir: import.meta.dirname }); + try { + generateHermesConfig({ env: process.env, scriptDir: import.meta.dirname }); + } catch (error) { + console.error(`[config] ${error instanceof Error ? error.message : String(error)}`); + process.exitCode = 1; + } }🤖 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 `@agents/hermes/generate-config.ts` around lines 20 - 28, The CLI entrypoint in generate-config.ts now calls generateHermesConfig from main() without any error handling, so thrown validation failures will fall back to Node’s default uncaught-exception output. Update the main/isMainModule entrypath to catch errors around generateHermesConfig, print the intended concise stderr message, and exit with code 1 so subprocess behavior stays consistent with the expected CLI UX.
🤖 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 `@agents/hermes/generate-config.ts`:
- Around line 20-28: The CLI entrypoint in generate-config.ts now calls
generateHermesConfig from main() without any error handling, so thrown
validation failures will fall back to Node’s default uncaught-exception output.
Update the main/isMainModule entrypath to catch errors around
generateHermesConfig, print the intended concise stderr message, and exit with
code 1 so subprocess behavior stays consistent with the expected CLI UX.
In `@test/helpers/rebuild-flow-test-harness.ts`:
- Around line 141-158: The two hydrateCredentialEnv spies in
rebuild-flow-test-harness.ts duplicate the same wrapper logic, so extract that
shared implementation into a single helper or spy factory and use it for both
the onboardMod and onboardCredentialEnv spies. Keep the existing override
behavior and defaultHydrateCredentialEnv fallback intact, but centralize the
mockImplementation body so hydrateCredentialEnv stays aligned in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9d0dc180-7155-4be6-a99d-7541c59956ec
📒 Files selected for processing (14)
agents/hermes/config/generate.tsagents/hermes/generate-config.tssrc/lib/actions/sandbox/rebuild-flow-helpers.test.tssrc/lib/actions/sandbox/rebuild-flow.test.tssrc/lib/actions/sandbox/rebuild-preflight-confirmation.test.tssrc/lib/actions/sandbox/rebuild-preflight-confirmation.tstest/generate-hermes-config.test.tstest/helpers/rebuild-flow-credential-preflight-cases.tstest/helpers/rebuild-flow-lifecycle-cases.tstest/helpers/rebuild-flow-test-harness.tstest/helpers/rebuild-flow-test-support.tstest/messaging-plan-test-helper.tstest/policy-tiers-onboard.test.tstest/rebuild-credential-preflight.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28721215887
|
This was referenced Jul 6, 2026
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
…VIDIA#6279) <!-- markdownlint-disable MD041 --> ## Summary Reduce unnecessary process isolation across the rebuild credential/preflight, Hermes config generation, and policy-tier onboarding suites while retaining representative executable and adapter contracts. This removes 128 of 140 test-launched processes across the three areas (91%) and keeps product defaults unchanged. ## Related Issue Part of NVIDIA#6245 ## Changes - Move rebuild credential/preflight decision coverage into the direct rebuild-flow harness, retaining six CLI/process contracts plus the DCode liveness child. - Add an explicit, import-safe Hermes config generation seam and build legacy messaging plans in process, retaining executable and copied-script contracts. - Exercise policy-tier selection, prompt, env, and resolution seams directly while retaining adapter contracts for CLI rejection and registry persistence. - Thread the explicit Hermes generation environment through managed-tool matrix loading and verify direct/executable parity against an ambient decoy. - Reduce test-launched processes from 27 to 7 for rebuild preflight, 75 to 2 for Hermes config, and 38 to 3 for policy tiers. ## 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-harness and dependency-seam refactoring only; CLI output, config shape, and user-facing 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: independent local reviews covered rebuild mutation ordering and interactive wiring, Hermes generation and messaging semantics, policy-tier persistence, and process-wide state restoration; all required findings were addressed. - [ ] 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 — 228/228 passed in 19.97s across the eight changed and adjacent test surfaces; `npm run typecheck:cli` passed - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — local `npm test` reached 13,096 passing tests; seven mode assertions inherited the host's `077` umask and passed 68/68 under `022`, while one E2E source-shape failure is byte-identical on `origin/main` - [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) ## Benchmark - Final six-surface run under `strace`: 43.58s for 182 tests. - Previous rebuild credential/preflight file alone under `strace`: 115.16s. - In the ordinary final run, rebuild credential/preflight completed in 9.44s, Hermes config in 0.39s, and policy tiers in 1.15s. --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Hermes configuration generation now uses the provided environment consistently, making generated outputs more predictable. * Sandbox rebuild confirmations can now be reused in more places, with support for custom prompt handling. * **Bug Fixes** * Improved rebuild preflight checks and warnings for active sessions, missing credentials, and unsupported multi-agent rebuilds. * Hardened rebuild flows so failures surface clearer messages and avoid unintended changes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
10 of 21 tasks
wscurran
added
area: ci
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
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 unnecessary process isolation across the rebuild credential/preflight, Hermes config generation, and policy-tier onboarding suites while retaining representative executable and adapter contracts. This removes 128 of 140 test-launched processes across the three areas (91%) and keeps product defaults unchanged.
Related Issue
Part of #6245
Changes
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 typecheck:clipassednpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — localnpm testreached 13,096 passing tests; seven mode assertions inherited the host's077umask and passed 68/68 under022, while one E2E source-shape failure is byte-identical onorigin/mainnpm run docsbuilds without warnings (doc changes only)Benchmark
strace: 43.58s for 182 tests.strace: 115.16s.Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes