test(e2e): make snapshot commands inference hermetic - #6621
Merged
Conversation
Signed-off-by: harjoth <harjoth.khara@gmail.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:
📝 WalkthroughWalkthroughThe snapshot E2E test now uses an authenticated fake OpenAI-compatible server, passes its configuration through installation and sandbox commands, and verifies recorded requests. Workflow configuration and boundary validation reject hosted-inference flags and NVIDIA inference credentials. ChangesSnapshot inference workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant SnapshotTest
participant FakeOpenAI
participant Sandbox
participant SnapshotCLI
SnapshotTest->>FakeOpenAI: Start authenticated compatible endpoint
SnapshotTest->>Sandbox: Run install.sh with endpoint and credentials
Sandbox->>FakeOpenAI: Send authenticated chat completion
FakeOpenAI-->>SnapshotTest: Record request path, model, and authentication
SnapshotTest->>SnapshotCLI: Run snapshot command lifecycle checks
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: harjoth <harjoth.khara@gmail.com>
Signed-off-by: harjoth <harjoth.khara@gmail.com>
harjothkhara
marked this pull request as ready for review
July 10, 2026 01:35
The snapshot-commands live target strips NEMOCLAW_E2E_USE_HOSTED_INFERENCE from its child env. That strip is load-bearing: the flag is in FIXTURE_ENV_ALLOWLIST, so an ambient value is forwarded into the child, and stageHostedInferenceSourceSecretEnv treats the flag alone as sufficient to force hosted-custom staging even when COMPATIBLE_API_KEY names an explicit endpoint. Removing the strip would silently route the target back at hosted inference. Until now the strip was only exercised by the live target, which does not run in ordinary CI. Extract the env builder into snapshot-commands-helpers.ts and cover it from the e2e-support lane, including a negative control asserting the flag really is forwarded by the shared probe env so the strip assertion cannot pass vacuously. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: harjoth <harjoth.khara@gmail.com>
![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
🤖 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/e2e/support/snapshot-commands-helpers.test.ts`:
- Around line 69-76: Make the credential-leak test meaningful by setting both
NVIDIA_INFERENCE_API_KEY and NVIDIA_API_KEY in process.env before calling
buildSnapshotCommandEnv, then assert they are filtered from the returned
environment. Extend the test cleanup in afterEach to delete or restore both
variables so state cannot leak between tests; update the test identified by
“never exposes a hosted NVIDIA inference credential to the child env” and its
shared cleanup setup.
🪄 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: 118b2d78-c1ce-4a55-9f95-4c403ae63dff
📒 Files selected for processing (3)
test/e2e/live/snapshot-commands-helpers.tstest/e2e/live/snapshot-commands.test.tstest/e2e/support/snapshot-commands-helpers.test.ts
The credential-leak assertion never set NVIDIA_INFERENCE_API_KEY or NVIDIA_API_KEY in the ambient env, so it passed whether or not the fixture env allowlist actually filtered them. Stage both credentials, assert they are present in the ambient env, and only then assert they are absent from the child env, so the test fails if either key ever becomes forwardable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: harjoth <harjoth.khara@gmail.com>
Collaborator
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Collaborator
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
|
cv
approved these changes
Jul 11, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Approved on exact head 79fe326. All required checks pass; DCO/signatures are verified; CodeRabbit has no current threads. Exact E2E run 29149963135 passed both required cloud-onboard and snapshot-commands targets. Trusted GPT advisor is high-confidence merge_as_is with zero findings/all security categories PASS. Nemotron succeeded on the isolated infrastructure retry; its sole warning requests cloud-onboard platform-install evidence, which the exact run supplies. Attempt skew reflects advisor tool-protocol infrastructure, not missing review.
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary Makes the snapshot-commands live E2E hermetic by replacing its hosted NVIDIA inference dependency with the repository's authenticated fake OpenAI-compatible endpoint. This removes the external credential, rate-limit retries, and upstream-provider skip path while preserving the real install, sandbox, snapshot, and restore lifecycle. ## Related Issue Related to NVIDIA#5747. The original issue covered the retired Bash snapshot lane; this applies the same hermeticity requirement to its current Vitest successor. ## Changes - Start an authenticated fake OpenAI-compatible endpoint for the snapshot-commands live test and expose it to the sandbox through `host.openshell.internal`. - Exercise `inference.local` from inside the sandbox and assert that OpenShell injects the stored credential into the expected-model `/v1/chat/completions` request. - Remove the hosted-inference flag, NVIDIA API key secret, retry loop, and transient upstream skip path. - Ratchet the workflow boundary against future hosted-inference flags or NVIDIA API keys at job and step scope. - Add negative workflow-contract coverage for each forbidden environment entry. - Use an `nvapi-`-shaped fake key so the snapshot credential scanner exercises its concrete token-pattern boundary. - Extract the live target's child-env builder into `test/e2e/live/snapshot-commands-helpers.ts` and cover the `NEMOCLAW_E2E_USE_HOSTED_INFERENCE` strip from the non-live `e2e-support` lane, including a negative control asserting the flag really is forwarded by the shared probe env so the strip assertion cannot pass vacuously. - Accept Biome's formatting of one adjacent, behaviorally unchanged Jetson workflow-boundary error string. ## 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 — workflow-only E2E reliability change with no user-facing behavior or interface change. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — the focused review verifies that no real inference credential reaches the job and the workflow contract rejects future secret reintroduction. - [ ] 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 — workflow-contract suite (17 passed) and exact live Docker/OpenShell snapshot target (1 passed in 93.6s with the final secret-shaped fixture key). - [ ] Applicable broad gate passed — not runnable in full here; scoped evidence below. The diff touches only `.github/workflows/`, `test/e2e/`, and `tools/e2e/`, and no `src/` file, so the `cli`, `integration`, `plugin`, and `package-contract` lanes are structurally unreachable from this change. `e2e-support` is the only affected lane: run on a clean worktree of this branch it fails 7 tests, every one of which also fails on a clean worktree of `origin/main` (this branch's failing set is a strict subset of the base's, and the residual failures are restricted-macOS `/proc`, ownership, and shell fixture failures unrelated to this PR). `npm test` in full is not runnable in this environment for the same reason. - [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) ### Additional validation performed - `tsc -p tsconfig.cli.json` - `tsx scripts/find-source-shape-tests.ts --check` - `biome check` on all changed files - `git diff --check` - GitHub reports all commits (`71b0013b6`, `2419a7c87`, `be308d358`) as `Verified`. - Exact live target: `NEMOCLAW_RUN_LIVE_E2E=1 npx vitest run --project e2e-live test/e2e/live/snapshot-commands.test.ts --silent=false --reporter=default --reporter=test/e2e/risk-signal-reporter.ts` (1 passed in 93.6s with an isolated temporary `HOME`). - Red/green on the new workflow guard: reverting `tools/e2e/workflow-boundary.mts` to `origin/main` makes the contract test fail on exactly the three added assertions; reintroducing `NEMOCLAW_E2E_USE_HOSTED_INFERENCE` and the `NVIDIA_INFERENCE_API_KEY` step env into `e2e.yaml` makes the boundary checker reject the workflow. Unmutated, the checker reports zero errors. - Red/green on the new env guard: deleting the `NEMOCLAW_E2E_USE_HOSTED_INFERENCE` strip fails exactly the two strip tests in `test/e2e/support/snapshot-commands-helpers.test.ts` while the negative control stays green (6 passed). - Live artifacts prove an authenticated `POST /v1/chat/completions` for `snapshot-commands-model`, snapshot v1/v2 creation, latest and timestamp-targeted restores, zero leaked credential files (including the `nvapi-`-shaped fixture key), and successful sandbox/fixture cleanup. > AI-assisted: implemented and reviewed with Codex; the normal private cross-model pre-push review was invoked. --- Signed-off-by: harjoth <harjoth.khara@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Testing** * Updated live snapshot-commands E2E to use a self-contained authenticated OpenAI-compatible inference backend, verifying in-sandbox authentication and that expected chat requests are captured. * Added dedicated E2E coverage for the snapshot-command environment builder to ensure hosted-inference flags/hosted inference credentials never leak into sandbox runs, while allowing explicit custom inference settings. * **CI Reliability** * Strengthened workflow boundary validation to forbid hosted-inference enablement at job scope and block sensitive inference credential environment variables at both job and step levels. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: harjoth <harjoth.khara@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
wscurran
added
area: ci
23 tasks
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
4 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
Makes the snapshot-commands live E2E hermetic by replacing its hosted NVIDIA inference dependency with the repository's authenticated fake OpenAI-compatible endpoint. This removes the external credential, rate-limit retries, and upstream-provider skip path while preserving the real install, sandbox, snapshot, and restore lifecycle.
Related Issue
Related to #5747. The original issue covered the retired Bash snapshot lane; this applies the same hermeticity requirement to its current Vitest successor.
Changes
host.openshell.internal.inference.localfrom inside the sandbox and assert that OpenShell injects the stored credential into the expected-model/v1/chat/completionsrequest.nvapi--shaped fake key so the snapshot credential scanner exercises its concrete token-pattern boundary.test/e2e/live/snapshot-commands-helpers.tsand cover theNEMOCLAW_E2E_USE_HOSTED_INFERENCEstrip from the non-livee2e-supportlane, including a negative controlasserting the flag really is forwarded by the shared probe env so the strip assertion cannot pass vacuously.
Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailable.github/workflows/,test/e2e/, andtools/e2e/, and nosrc/file, so thecli,integration,plugin, andpackage-contractlanes are structurally unreachable from thischange.
e2e-supportis the only affected lane: run on a clean worktree of this branch it fails 7 tests, every one ofwhich also fails on a clean worktree of
origin/main(this branch's failing set is a strict subset of the base's, andthe residual failures are restricted-macOS
/proc, ownership, and shell fixture failures unrelated to this PR).npm testin full is not runnable in this environment for the same reason.npm run docsbuilds without warnings (doc changes only)Additional validation performed
tsc -p tsconfig.cli.jsontsx scripts/find-source-shape-tests.ts --checkbiome checkon all changed filesgit diff --check71b0013b6,2419a7c87,be308d358) asVerified.NEMOCLAW_RUN_LIVE_E2E=1 npx vitest run --project e2e-live test/e2e/live/snapshot-commands.test.ts --silent=false --reporter=default --reporter=test/e2e/risk-signal-reporter.ts(1 passed in 93.6s with an isolated temporaryHOME).tools/e2e/workflow-boundary.mtstoorigin/mainmakes the contracttest fail on exactly the three added assertions; reintroducing
NEMOCLAW_E2E_USE_HOSTED_INFERENCEand theNVIDIA_INFERENCE_API_KEYstep env intoe2e.yamlmakes the boundary checker reject the workflow. Unmutated, thechecker reports zero errors.
NEMOCLAW_E2E_USE_HOSTED_INFERENCEstrip fails exactly the two striptests in
test/e2e/support/snapshot-commands-helpers.test.tswhile the negative control stays green (6 passed).POST /v1/chat/completionsforsnapshot-commands-model, snapshot v1/v2 creation, latest and timestamp-targeted restores, zero leaked credential files (including thenvapi--shaped fixture key), and successful sandbox/fixture cleanup.Signed-off-by: harjoth harjoth.khara@gmail.com
Summary by CodeRabbit