test(onboard): add a child-process harness for four pilot suites - #8537
Conversation
Four onboarding suites each hand-rolled the same child-process mechanics: mkdtemp workspace with a fake bin, 0o755 stub writer, HOME/PATH environment composition, a synchronous node spawn from the repository root, and trailing JSON payload extraction. Move those mechanics into test/helpers/onboard-child-process-harness.ts and migrate the pilots. Stub script contents, scenario environment values, and assertions stay in each test, and every migrated file has a negative line delta. Refs NVIDIA#8289 Signed-off-by: JulienAu <16043912+JulienAu@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe pull request adds a reusable onboarding child-process harness. Four onboarding tests now use shared workspace creation, fake executable setup, environment construction, process execution, output capture, JSON extraction, and cleanup. ChangesOnboard child-process harness
Onboarding test migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: None 3 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Maintainer validation and security disposition
|
<!-- markdownlint-disable MD041 --> ## Summary Five onboarding-machine suites each declared the same in-memory OnboardRuntime over a session store, and eight duplicated the deep `cloneSession` helper byte for byte. This is sequential PR 2 of issue #8289: it moves both into `test/helpers/onboard-machine-runtime-fixture.ts` and migrates the eight suites. The deterministic clock is overridable where assertions depend on a specific timestamp, and `flow-slices` keeps its intentionally inert `markStepFailed` and `completeSession` behavior local rather than growing the fixture behavior options. ## Related Issue Refs #8289 (sequential PR 2 of 4; PR 1 is #8537) ## Changes - Add `test/helpers/onboard-machine-runtime-fixture.ts`: `cloneSession`, `createTestRuntime(initialSession?, { now? })`, and a re-export of the session helpers the suites consume. The `test/helpers/` placement follows the existing shared-fixture convention (`base-image-test-harness`, `messaging-conflict-fixtures`, `env-test-helpers`) and keeps the source-architecture fan-in budget for `onboard-session.ts` untouched; the eight suites drop their direct runtime imports of `onboard-session` in the process. Requirement and consumers: the eight machine suites below. A direct change cannot remove the duplication because each suite owns an inline copy; the protecting tests are the migrated suites themselves. The fixture holds no filesystem or global state, so the PR 1 review convention about registering workspace cleanup does not apply here. - Migrate `runner`, `runner-sequence`, `sequence-runner`, `flow-sequence`, and `flow-phases/agent-policy-finalization` to the shared runtime; the two clock families keep their asserted timestamps at their call sites. - Migrate `runtime`, `flow-slices`, and `transition-traces` to the shared `cloneSession` only. - Every migrated file has a negative line delta (net -139 across the PR). ## 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 - [ ] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: test-only consolidation; the eight migrated suites are the protected behavior and pass unchanged (63 tests). - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: test fixture only; no user-facing surface or documented behavior changes. - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: test-only fixture and suite migrations; no user-facing surface changes. Docstrings on the new fixture reviewed against the writing rules. - Agent: Claude Code <!-- docs-review-head-sha: 18924fa --> <!-- docs-review-agents-blob-sha: c69aad4 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable — ran `npm run validate:pr`; every hook passes except hadolint, which reports a pre-existing info-level finding (`Dockerfile:1543 SC2015`) that is byte-identical on unmodified `main`; this PR changes no Dockerfile - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result: `npx vitest run --project cli` on the eight migrated suites; 8 files, 63 tests pass in normal and shuffled order. `tsc -p tsconfig.cli.json` and `npm run checks:repository` pass. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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: JulienAu <16043912+JulienAu@users.noreply.github.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Standardized onboarding flow and runtime tests on a shared test fixture. * Improved test isolation through consistent session cloning, persistence, lifecycle handling, and event simulation. * Added configurable fixed timestamps for more deterministic test execution. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: JulienAu <16043912+JulienAu@users.noreply.github.com> Co-authored-by: JulienAu <16043912+JulienAu@users.noreply.github.com>
<!-- markdownlint-disable MD041 --> ## Summary Four installer suites each declared the same sourced-installer spawn with small env, PATH, and timeout variations, and two duplicated the npm stub with divergent ci handling. This is sequential PR 3 of issue #8289: it moves the sourced-body runner, the npm stub builder, and a disposable checkout scaffold into `test/helpers/installer-run-fixture.ts` and migrates six pilot suites, which keep their scenario prefixes, stub snippets, and environment values at the call sites through one-line wrappers. ## Related Issue Refs #8289 (sequential PR 3 of 4; PR 1 is #8537, PR 2 is #8546) ## Changes - Add `test/helpers/installer-run-fixture.ts`: `runInstallerSourcedBody(body, { home?, homePrefix?, extraEnv?, includeNodeOnPath?, timeoutMs? })` consolidating the four spawn copies behavior for behavior (SIGKILL applies exactly when a timeout does, and the environment stays minimal with no process.env spread, as in every copy), `writeNpmStub(fakeBin, { installSnippet?, handleCi? })` preserving each family's unexpected-invocation guard (`handleCi` reproduces the preflight family's `npm ci` handling without weakening the other family's stricter stub), and `createInstallerCheckout` for the bin-plus-prefix scaffold. It builds on the existing `installer-sourced-env.ts` constants rather than duplicating them. - Migrate `install-preflight`, `install-build-dependency-preflight`, `install-express-wsl-ollama`, `install-station-controller-binding`, `install-station-pair-preparation`, and `install-station-vllm-continuation`. Every migrated file has a negative line delta. - Ratchet the `ci/test-file-size-budget.json` entry for `install-preflight.test.ts` from 3,921 to its new 3,908-line size, as the size gate requires after shrinking a legacy file. - Register test-finished cleanup at every migrated wrapper that creates a temporary HOME, while preserving caller ownership when an existing HOME is supplied. ## 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: test fixtures only; no user-facing surface or documented behavior changes. - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: Test-only installer fixture refactor, caller-owned HOME preservation, and helper-owned HOME cleanup coverage; no user-visible surface changed. - Agent: Codex Desktop <!-- docs-review-head-sha: ac123e9 --> <!-- docs-review-agents-blob-sha: 12ad395 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable — `npm run check:diff` passes on the current-main refresh, including repository checks, secret scan, commit lint, and pre-push type checking - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result: the completed helper and six-suite aggregate passes 203/203 tests; current-main revalidation passes the four affected suites 105/105 plus helper ownership coverage 2/2. Isolated cleanup verification leaves no helper-owned HOME directories. CLI typecheck, repository checks, formatting, structural gates, commit hooks, and pre-push hooks pass. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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: JulienAu <16043912+JulienAu@users.noreply.github.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved installer test reliability with shared utilities for isolated environments, temporary checkouts, configurable package-manager behavior, output handling, and timeouts. * Standardized installer test setup while preserving existing scenarios, coverage, and simulated failures. * Added coverage for temporary environment cleanup and preservation of caller-provided settings. * Updated the legacy test file size budget to reflect current coverage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: JulienAu <16043912+JulienAu@users.noreply.github.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: JulienAu <16043912+JulienAu@users.noreply.github.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Nine config-generation suites carried a byte-identical BASE_ENV literal, six duplicated the buildTestEnv composition, and four the fake-openclaw stub writer. This is sequential PR 4 of issue #8289: it moves the ordinary valid generation environment, the stub writer, and the environment composition into `test/helpers/openclaw-env-fixture.ts` and migrates the nine suites. Following the isolation follow-up in #8557, the fixture ships with its own regression test proving per-call object independence. ## Related Issue Refs #8289 (sequential PR 4 of 4; PRs 1-3 are #8537, #8546, #8556) ## Changes - Add `test/helpers/openclaw-env-fixture.ts`: `baseOpenClawGenerationEnv()` returning a fresh copy of the ordinary valid environment per call, `ensureFakeOpenClaw(dir)`, and `buildOpenClawTestEnv(dir, baseEnv, overrides)` composing PATH, base entries, overrides, and HOME without mutating its inputs. - Add `test/helpers/openclaw-env-fixture.test.ts`: regression coverage for per-call independence and for composition that leaves base entries and overrides untouched, in the spirit of the #8557 isolation fix. - Migrate the nine suites with the byte-identical environment (`generate-openclaw-config` family, `generate-openclaw-tool-disclosure-config`, `agents-manifest-policy-conformance`, `generate-openclaw-config-plugin-entries`). The four suites whose BASE_ENV carries scenario-specific values (`reasoning-effort`, `security-audit`, `audit-suppressions-real`, `generate-hermes-config`) keep their local variants per the acceptance criteria, and the messaging-plan wrap stays at its call site. - Ratchet the `ci/test-file-size-budget.json` entry for `generate-openclaw-config.test.ts` from 1,941 to its new 1,915-line size, as the size gate requires after shrinking a legacy file. - Scope note: the issue's canonical corporate-CA test asset is deliberately not part of this PR. The inline PEM blocks are per-file scenario material (no two files share one), so canonicalizing them would change tested certificate content; that decision is left to a maintainer and can follow as its own change if wanted. ## 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: test fixtures only; no user-facing surface or documented behavior changes. - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: Test-only consolidation of repeated OpenClaw config-generation environments and an exact internal test-size-budget ratchet; no user-facing behavior, CLI, configuration, architecture, or documentation route changes. - Agent: Codex Desktop <!-- docs-review-head-sha: 251d5a1 --> <!-- docs-review-agents-blob-sha: 12ad395 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, and `npm run check:diff` passed after the current-main refresh. - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result: after the current `main` refresh, `npx vitest run --project integration` on the nine migrated suites plus the new fixture test passed (10 files, 178 tests). `npm run typecheck:cli`, `npm run test-size:check`, the normal commit hooks, and pre-push checks passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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: JulienAu <16043912+JulienAu@users.noreply.github.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Standardized test environment setup across configuration and policy tests. * Added coverage for isolated fixtures, environment overrides, temporary-directory handling, and executable setup. * Updated the recorded test file size budget to reflect the reduced test footprint. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: JulienAu <16043912+JulienAu@users.noreply.github.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: JulienAu <16043912+JulienAu@users.noreply.github.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com>
Summary
Four onboarding suites each hand-rolled the same child-process mechanics: a mkdtemp workspace with a fake bin directory, a 0o755 stub writer, HOME and PATH environment composition, a synchronous node spawn from the repository root, and trailing JSON payload extraction. This is sequential PR 1 of issue #8289: it moves those mechanics into
test/helpers/onboard-child-process-harness.tsand migrates the four pilot suites, leaving stub script contents, scenario environment values, and assertions in each test.Related Issue
Refs #8289 (sequential PR 1 of 4; the issue stays open for the remaining slices)
Changes
test/helpers/onboard-child-process-harness.ts:createOnboardProcessWorkspace(disposable workspace with created bin and optional separate home),workspaceEnvandminimalSpawnEnv(fresh environment objects; the minimal variant carries the Windows spawn keys),runOnboardProcess(synchronous node spawn from the repository root with decoded output), andtrailingJsonPayload. Requirement and consumers: the four pilot suites below, with the remaining onboard suites planned as follow-ups in Establish root test fixtures for repeated process and environment setup #8289. A direct change cannot remove the duplication because each suite owns an inline copy; the protecting tests are the migrated suites themselves.test/onboard-remote-recreate-credential-reuse.test.ts(four near-identical env blocks become one scenario env factory),test/onboard-prepared-gateway-handoff.test.ts,test/onboard-gateway-port-conflict-fast-fail.test.ts, andtest/onboard-reservation-recreate.test.ts. Every migrated file has a negative line delta; the security-relevant scenario values (blanked credential env vars, probe logs) stay explicit in the tests.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable — contributor validation passed except for the documented pre-existing hadolint information message on an unchanged Dockerfile; the maintainer cleanup commit passed normal pre-commit, commit-msg, and pre-push hooks.onboard-gateway-port-conflict-fast-failtimes out identically on unmodified main in the contributor's WSL2 environment, so hosted CI is the arbiter for that suite. Maintainer result: all three reservation cleanup scenarios pass after the lifecycle fix.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: JulienAu 16043912+JulienAu@users.noreply.github.com