test(onboard): add an in-memory machine runtime fixture - #8546
Conversation
Five machine suites each declared the same in-memory OnboardRuntime over a session store, and eight duplicated the deep cloneSession helper byte for byte. Move both into test/helpers/onboard-machine-runtime-fixture.ts, which also fronts the session helpers so the suites drop their direct onboard-session imports; the deterministic clock is overridable where assertions depend on a specific timestamp, and flow-slices keeps its intentionally inert markStepFailed and completeSession behavior local. Every migrated file has a negative line delta. Refs NVIDIA#8289 Signed-off-by: JulienAu <16043912+JulienAu@users.noreply.github.com>
📝 WalkthroughWalkthroughThe pull request adds a shared in-memory onboarding runtime fixture. Eight onboarding machine test files now use its session helpers and runtime factory instead of local implementations. ChangesOnboarding runtime fixture
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/helpers/onboard-machine-runtime-fixture.ts`:
- Around line 29-31: Update cloneSession so the parsed deep copy remains the
fallback when normalizeSession returns null; replace the original session
fallback with the serialized copy, preserving cloneSession’s state isolation.
🪄 Autofix
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: 2439a835-9ac4-4813-9c7e-7f2c0e0b4fea
📒 Files selected for processing (9)
src/lib/onboard/machine/flow-phases/agent-policy-finalization.test.tssrc/lib/onboard/machine/flow-sequence.test.tssrc/lib/onboard/machine/flow-slices.test.tssrc/lib/onboard/machine/runner-sequence.test.tssrc/lib/onboard/machine/runner.test.tssrc/lib/onboard/machine/runtime.test.tssrc/lib/onboard/machine/sequence-runner.test.tssrc/lib/onboard/machine/transition-traces.test.tstest/helpers/onboard-machine-runtime-fixture.ts
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
<!-- markdownlint-disable MD041 --> ## Summary `cloneSession` from #8546 returned the caller-owned session when normalization rejected its serialized copy. This follow-up keeps the serialized copy as the fallback so fixture mutations cannot alter caller-owned top-level or nested state. ## Changes - Use the serialized session copy as both the normalization input and fallback. - Add a regression test with an invalid session version. The test proves that normalization rejection still returns independent top-level and nested state. - Record the detection gap: #8546 exercised valid sessions only, so normalization always succeeded and did not reach the aliasing fallback. - Close the complete #8546 review set. The CodeRabbit isolation finding is fixed. The docstring-coverage warning is advisory because local exports have JSDoc and re-export documentation remains source-owned. The PR Review Advisor reported no findings and failed only because its model request returned HTTP 429. - Inspect the equivalent local clone helpers in `test/helpers/onboard-final-flow-phases.ts`, `src/lib/onboard/resume-machine-repair.test.ts`, `src/lib/onboard/runtime-boundary-step-result.test.ts`, and `src/lib/onboard/runtime-boundary.test.ts`. Their callers construct valid sessions and do not expose #8546's independent-mutation contract, so this PR does not change them. ## 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: this changes an internal test fixture and its regression coverage. No CLI, configuration, output, workflow, default, API, policy schema, or supported user 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: the completed diff changes only `test/helpers/onboard-machine-runtime-fixture.ts` and `test/helpers/onboard-machine-runtime-fixture.test.ts`. The reviewer found no user-visible behavior or documentation impact and approved the test title and existing JSDoc ownership. - Agent: Codex Desktop <!-- docs-review-head-sha: 64c19cf --> <!-- docs-review-agents-blob-sha: c69aad4 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable; `scripts/prepare-dgx-station-host.sh` does not change. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## 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 - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run --project integration test/helpers/onboard-machine-runtime-fixture.test.ts` passed 1/1; the eight #8546 CLI suites passed 63/63; `npm run typecheck:cli` passed; `npm run test:projects:check` reported exact membership for 2,231 candidate files across seven projects. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: not applicable. The change updates one fixture fallback and adds one focused test; targeted validation covers every #8546 consumer. - [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: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved session cloning so copied sessions remain independent even when normalization fails. * Prevented changes to a cloned session from affecting the original session. * **Tests** * Added regression coverage for cloning invalid-version sessions and nested steps. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Julie Yaunches <jyaunches@nvidia.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>
Summary
Five onboarding-machine suites each declared the same in-memory OnboardRuntime over a session store, and eight duplicated the deep
cloneSessionhelper byte for byte. This is sequential PR 2 of issue #8289: it moves both intotest/helpers/onboard-machine-runtime-fixture.tsand migrates the eight suites. The deterministic clock is overridable where assertions depend on a specific timestamp, andflow-sliceskeeps its intentionally inertmarkStepFailedandcompleteSessionbehavior local rather than growing the fixture behavior options.Related Issue
Refs #8289 (sequential PR 2 of 4; PR 1 is #8537)
Changes
test/helpers/onboard-machine-runtime-fixture.ts:cloneSession,createTestRuntime(initialSession?, { now? }), and a re-export of the session helpers the suites consume. Thetest/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 foronboard-session.tsuntouched; the eight suites drop their direct runtime imports ofonboard-sessionin 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.runner,runner-sequence,sequence-runner,flow-sequence, andflow-phases/agent-policy-finalizationto the shared runtime; the two clock families keep their asserted timestamps at their call sites.runtime,flow-slices, andtransition-tracesto the sharedcloneSessiononly.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 — rannpm run validate:pr; every hook passes except hadolint, which reports a pre-existing info-level finding (Dockerfile:1543 SC2015) that is byte-identical on unmodifiedmain; this PR changes no Dockerfilenpx vitest run --project clion the eight migrated suites; 8 files, 63 tests pass in normal and shuffled order.tsc -p tsconfig.cli.jsonandnpm run checks:repositorypass.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
Summary by CodeRabbit