Skip to content

fix(test): preserve onboarding runtime fixture isolation - #8558

Closed
apurvvkumaria wants to merge 2 commits into
mainfrom
codex/fix-runtime-fixture-isolation-min
Closed

fix(test): preserve onboarding runtime fixture isolation#8558
apurvvkumaria wants to merge 2 commits into
mainfrom
codex/fix-runtime-fixture-isolation-min

Conversation

@apurvvkumaria

@apurvvkumaria apurvvkumaria commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

PR #8546 introduced a shared onboarding runtime fixture whose normalization fallback could return the caller-owned session, allowing later test mutation to leak into earlier state. This follow-up always returns the serialized copy, adds a regression for the isolation contract, and removes imports left unused by the migration.

Related Issue

Refs #8289. Follow-up to #8546 and its unresolved review finding.

Changes

  • Make cloneSession return its serialized copy when normalizeSession rejects test data, preserving the fixture's independent-state contract.
  • Add a regression that uses an unsupported session version, mutates a nested field on the returned clone, and proves the source session remains unchanged.
  • Remove stale runtime and session imports from the five suites migrated to the shared fixture.

Type of Change

  • 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
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: all seven changed files are test suites or a test-only helper; no production behavior, API, CLI output, configuration, default, workflow, or documented surface 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: the nine-category maintainer review found no issues; the test-only change removes a shared-reference hazard and changes no production trust boundary, credential handling, policy, network, or runtime configuration.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: no-docs-needed
  • Evidence: no documentation paths changed. The seven changed files are test suites or a test-only helper, and the isolation regression and unused-import cleanup do not change any user-facing or documented surface.
  • Agent: Codex Desktop

DGX Station Hardware Evidence

  • Tested on DGX Station
  • Tested commit:
  • Station profile/scenario:
  • Result:
  • Supporting evidence:

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • 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 — normal commit and pre-push gates passed; a full diff-scoped hook fallback also passed.
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — all eight fixture-consuming suites pass in normal and shuffled order, 64/64 each; CLI type checking and repository validation also pass.
  • 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. This adjusts one test-helper fallback without changing Vitest configuration, setup, project registration, production code, or runtime behavior; all eight helper consumers were exercised directly.
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Apurv Kumaria akumaria@nvidia.com

Summary by CodeRabbit

  • Bug Fixes

    • Improved onboarding session cloning to preserve an isolated fallback when normalization is rejected.
    • Prevented rejected session normalization from unintentionally returning or modifying the original session.
  • Tests

    • Added coverage confirming that rejected session clones remain isolated and leave the original session unchanged.
    • Cleaned up unused test imports without changing existing test behavior.

Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@apurvvkumaria apurvvkumaria self-assigned this Aug 7, 2026
@apurvvkumaria

Copy link
Copy Markdown
Collaborator Author

Maintainer security review

Verdict: PASS — no findings or warnings.

Category Result Evidence
Secrets and credentials PASS No credentials, tokens, keys, or secret-bearing fixtures are added.
Input validation and sanitization PASS The helper handles internal Session test data; its fallback now returns the serialized copy instead of caller-owned state.
Authentication and authorization PASS No endpoint or access-control changes.
Dependencies PASS No manifest, lockfile, registry, or dependency changes.
Error handling and logging PASS No production error or logging paths change.
Cryptography and data protection PASS No cryptographic or persistence changes.
Configuration and security headers PASS No workflow, container, policy, network, or runtime configuration changes.
Security testing PASS The regression forces normalization rejection, mutates a nested field on the returned clone, and proves the source session is unchanged.
Holistic security posture PASS This test-only correction removes a shared-reference hazard without introducing a production trust boundary, SSRF, credential, sandbox, or concurrency change.

Validation: all eight fixture-consuming suites pass in normal and shuffled order, 64/64 each; diff-scoped repository checks, CLI type checking, normal push gates, and secret scanning pass. Scope is seven test-only files with no production, dependency, workflow, policy, or documentation changes.

@github-code-quality

github-code-quality Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 7c4e077 in the codex/fix-runtime-fi... branch remains at 96%, unchanged from commit 0da798a in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit 7c4e077 in the codex/fix-runtime-fi... branch remains at 81%, unchanged from commit 0da798a in the main branch.

Show a code coverage summary of the most impacted files.
File main 0da798a codex/fix-runtime-fi... 7c4e077 +/-
src/lib/onboard...carryforward.ts 96% 88% -8%
src/lib/messagi...flow-planner.ts 91% 91% 0%
src/lib/core/immutable.ts 97% 97% 0%
src/lib/messagi...etup-applier.ts 68% 68% 0%
src/lib/messaging/utils.ts 76% 76% 0%
src/lib/sandbox...rce-identity.ts 83% 83% 0%
src/lib/onboard...flight-ports.ts 33% 67% +34%

Updated August 07, 2026 12:35 UTC

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a49a2bd0-c510-4c90-9492-532cf74f89ba

📥 Commits

Reviewing files that changed from the base of the PR and between 0da798a and 7c4e077.

📒 Files selected for processing (7)
  • src/lib/onboard/machine/flow-phases/agent-policy-finalization.test.ts
  • src/lib/onboard/machine/flow-sequence.test.ts
  • src/lib/onboard/machine/runner-sequence.test.ts
  • src/lib/onboard/machine/runner.test.ts
  • src/lib/onboard/machine/runtime.test.ts
  • src/lib/onboard/machine/sequence-runner.test.ts
  • test/helpers/onboard-machine-runtime-fixture.ts
💤 Files with no reviewable changes (5)
  • src/lib/onboard/machine/flow-phases/agent-policy-finalization.test.ts
  • src/lib/onboard/machine/sequence-runner.test.ts
  • src/lib/onboard/machine/runner-sequence.test.ts
  • src/lib/onboard/machine/runner.test.ts
  • src/lib/onboard/machine/flow-sequence.test.ts

📝 Walkthrough

Walkthrough

Changes

Onboarding session cloning

Layer / File(s) Summary
Preserve cloned session fallback
test/helpers/onboard-machine-runtime-fixture.ts, src/lib/onboard/machine/runtime.test.ts
cloneSession returns the deep clone when normalization rejects it. A regression test verifies clone isolation and preserves the original session.
Align onboarding test imports
src/lib/onboard/machine/flow-sequence.test.ts, src/lib/onboard/machine/runner-sequence.test.ts, src/lib/onboard/machine/runner.test.ts, src/lib/onboard/machine/sequence-runner.test.ts
Unused imports were removed. MACHINE_SNAPSHOT_VERSION is sourced from ./runner where required.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested labels: chore

Suggested reviewers: cv, senthilr-nv, ericksoa

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: fixing test fixture isolation in the onboarding runtime. It is concise, specific, and directly reflects the core objective of preserving session isolation in the shared fixture.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-runtime-fixture-isolation-min

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Failed

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite against this exact revision.

Recommended E2E: cloud-onboard

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@apurvvkumaria

Copy link
Copy Markdown
Collaborator Author

Closing as superseded by #8557, merged in 61ae0ee568c47a6b2965b6e3643634feca479505. The merged change applies the same safe fallback (normalizeSession(copy) ?? copy) and adds a dedicated invalid-version regression proving that top-level and nested mutations cannot reach the caller-owned session. That fully resolves the isolation blocker for which this PR was opened. The remaining five-file delta here is unused-import cleanup, not part of the isolation fix; it can be considered independently if it remains worthwhile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant