test(e2e): migrate test-credential-sanitization.sh to vitest#5336
Merged
Conversation
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:
📝 WalkthroughWalkthroughAdds a live credential-sanitization Vitest scenario, a free-standing GitHub Actions job to run it, and workflow-boundary validator and test updates to register and depend on that job. ChangesCredential sanitization E2E workflow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tools/e2e-scenarios/workflow-boundary.mts (1)
1691-1693: ⚡ Quick winGive
credential-sanitization-vitesta dedicated boundary validator.Line 1692 only checks selector wiring. The new job also carries secret scope, Docker auth, artifact-path, and cleanup contracts, but none of those are locked down here the way the other secret-bearing live jobs are.
🤖 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 `@tools/e2e-scenarios/workflow-boundary.mts` around lines 1691 - 1693, Add a dedicated boundary validator for the "credential-sanitization-vitest" job instead of only calling validateFreeStandingJobSelector for selector wiring; implement and invoke a new validator (e.g., validateCredentialSanitizationVitestBoundary) that enforces the same constraints applied to other secret-bearing live jobs: validate secret scope, Docker auth, artifact-path, and cleanup contract restrictions. Locate the existing validateFreeStandingJobSelector calls and replace or augment the line for "credential-sanitization-vitest" to call your new validator so those additional contracts are locked down for that job.
🤖 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-scenario/live/credential-sanitization.test.ts`:
- Around line 321-336: The call to secrets.required("NVIDIA_API_KEY") runs
unconditionally and should be moved so it only executes when Docker is
available; change the logic in the test so you first run host.command("docker",
["info"], ...) and check docker.exitCode, then inside the success path (before
running the live sandbox checks) call secrets.required("NVIDIA_API_KEY") and
assert its prefix, and only throw the Docker-required Error/skip when docker
fails (using process.env.GITHUB_ACTIONS and skip as currently done); update any
references to resultText(docker) accordingly so the secret is not required for
the local skip path.
---
Nitpick comments:
In `@tools/e2e-scenarios/workflow-boundary.mts`:
- Around line 1691-1693: Add a dedicated boundary validator for the
"credential-sanitization-vitest" job instead of only calling
validateFreeStandingJobSelector for selector wiring; implement and invoke a new
validator (e.g., validateCredentialSanitizationVitestBoundary) that enforces the
same constraints applied to other secret-bearing live jobs: validate secret
scope, Docker auth, artifact-path, and cleanup contract restrictions. Locate the
existing validateFreeStandingJobSelector calls and replace or augment the line
for "credential-sanitization-vitest" to call your new validator so those
additional contracts are locked down for that job.
🪄 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: 49733c8c-e012-4313-838d-984eb23ecbaf
📒 Files selected for processing (4)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/live/credential-sanitization.test.tstest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/workflow-boundary.mts
Comment on lines
+321
to
+336
| const apiKey = secrets.required("NVIDIA_API_KEY"); | ||
| expect(apiKey.startsWith("nvapi-"), "NVIDIA_API_KEY must start with nvapi-").toBe(true); | ||
|
|
||
| const docker = await host.command("docker", ["info"], { | ||
| artifactName: "prereq-docker-info-credential-sanitization", | ||
| env: buildAvailabilityProbeEnv(), | ||
| timeoutMs: 30_000, | ||
| }); | ||
| if (docker.exitCode !== 0) { | ||
| if (process.env.GITHUB_ACTIONS === "true") { | ||
| throw new Error( | ||
| `Docker is required for credential sanitization live E2E: ${resultText(docker)}`, | ||
| ); | ||
| } | ||
| skip("Docker is required for credential sanitization live E2E"); | ||
| } |
Contributor
There was a problem hiding this comment.
Move the NVIDIA_API_KEY requirement behind the local skip path.
secrets.required("NVIDIA_API_KEY") runs before the Docker/local skip branch, so a local run without that secret throws immediately instead of executing the contract checks and skipping only the live sandbox portion.
Proposed fix
- const apiKey = secrets.required("NVIDIA_API_KEY");
- expect(apiKey.startsWith("nvapi-"), "NVIDIA_API_KEY must start with nvapi-").toBe(true);
+ const apiKey = process.env.NVIDIA_API_KEY ?? "";
+ if (!apiKey) {
+ if (process.env.GITHUB_ACTIONS === "true") {
+ throw new Error("NVIDIA_API_KEY is required for credential sanitization live E2E");
+ }
+ skip("NVIDIA_API_KEY is required for credential sanitization live E2E");
+ }
+ expect(apiKey.startsWith("nvapi-"), "NVIDIA_API_KEY must start with nvapi-").toBe(true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const apiKey = secrets.required("NVIDIA_API_KEY"); | |
| expect(apiKey.startsWith("nvapi-"), "NVIDIA_API_KEY must start with nvapi-").toBe(true); | |
| const docker = await host.command("docker", ["info"], { | |
| artifactName: "prereq-docker-info-credential-sanitization", | |
| env: buildAvailabilityProbeEnv(), | |
| timeoutMs: 30_000, | |
| }); | |
| if (docker.exitCode !== 0) { | |
| if (process.env.GITHUB_ACTIONS === "true") { | |
| throw new Error( | |
| `Docker is required for credential sanitization live E2E: ${resultText(docker)}`, | |
| ); | |
| } | |
| skip("Docker is required for credential sanitization live E2E"); | |
| } | |
| const apiKey = process.env.NVIDIA_API_KEY ?? ""; | |
| if (!apiKey) { | |
| if (process.env.GITHUB_ACTIONS === "true") { | |
| throw new Error("NVIDIA_API_KEY is required for credential sanitization live E2E"); | |
| } | |
| skip("NVIDIA_API_KEY is required for credential sanitization live E2E"); | |
| } | |
| expect(apiKey.startsWith("nvapi-"), "NVIDIA_API_KEY must start with nvapi-").toBe(true); | |
| const docker = await host.command("docker", ["info"], { | |
| artifactName: "prereq-docker-info-credential-sanitization", | |
| env: buildAvailabilityProbeEnv(), | |
| timeoutMs: 30_000, | |
| }); | |
| if (docker.exitCode !== 0) { | |
| if (process.env.GITHUB_ACTIONS === "true") { | |
| throw new Error( | |
| `Docker is required for credential sanitization live E2E: ${resultText(docker)}`, | |
| ); | |
| } | |
| skip("Docker is required for credential sanitization live E2E"); | |
| } |
🤖 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/e2e-scenario/live/credential-sanitization.test.ts` around lines 321 -
336, The call to secrets.required("NVIDIA_API_KEY") runs unconditionally and
should be moved so it only executes when Docker is available; change the logic
in the test so you first run host.command("docker", ["info"], ...) and check
docker.exitCode, then inside the success path (before running the live sandbox
checks) call secrets.required("NVIDIA_API_KEY") and assert its prefix, and only
throw the Docker-required Error/skip when docker fails (using
process.env.GITHUB_ACTIONS and skip as currently done); update any references to
resultText(docker) accordingly so the secret is not required for the local skip
path.
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27421946896
|
Contributor
PR Review AdvisorFindings: 1 needs attention, 2 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27422540435
|
Contributor
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27423613232
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27425214870
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27426332138
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27427632562
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27428479727
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27431351010
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27432354309
|
This was referenced Jun 12, 2026
79 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
3 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
Migrate
test/e2e/test-credential-sanitization.shwith the simplest equivalent Vitest coverage.Related Issues
Refs #5098
Refs #156
Refs #1438
Contract mapping
auth-profiles.json, preserves non-secret workspace/config data, and keeps symlink traversal out of sensitive-file removal.test/e2e-scenario/live/credential-sanitization.test.tslocal product-code assertions usingsanitizeConfigFile,stripCredentials,isSensitiveFile, andisCredentialField.credential-sanitization.test.tsparsesnemoclaw-blueprint/blueprint.yamland assertssha256:<64hex>plus image-digest equality.auth-profiles.jsonor secret-shapednvapi-/ghp_/npm_values.credential-sanitization.test.tsrunsbash install.sh --non-interactive --yes-i-accept-third-party-software, then probes the real OpenShell sandbox filesystem from Vitest.install.sh, Docker/OpenShell, real sandbox exec,NVIDIA_API_KEYsecret redaction.Simplicity check
nightly-e2e.yamljobcredential-sanitization-e2e,runs-on: ubuntu-latest, Docker/OpenShell,NVIDIA_API_KEY, timeout 60 minutes.ubuntu-latestclass in.github/workflows/e2e-vitest-scenarios.yamljobcredential-sanitization-vitest, with Docker/OpenShell andNVIDIA_API_KEY.credential-sanitization-vitest; legacy shell deletion and shell workflow retirement deferred to Epic: Migrate legacy bash E2E into the Vitest E2E system #5098 Phase 11.gh workflow run e2e-vitest-scenarios.yaml --repo NVIDIA/NemoClaw --ref e2e-migrate/test-credential-sanitization -f jobs=credential-sanitization-vitest -f pr_number=<PR>.Verification
npm ci --ignore-scriptsnpm run build:clinpx vitest run --project e2e-vitest-support test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts --silent=false --reporter=defaultNEMOCLAW_RUN_E2E_SCENARIOS=1 npx vitest run --project e2e-scenarios-live test/e2e-scenario/live/credential-sanitization.test.ts --silent=false --reporter=default(local no-secret run: product-code assertions pass, live sandbox section skips on missingNVIDIA_API_KEY)npx vitest run test/validate-blueprint.test.ts src/lib/security/credential-filter.test.ts --silent=false --reporter=defaultnpm run typecheck:cligit diff --checkcredential-sanitization-vitestpassed onubuntu-latestwith Docker/OpenShell andNVIDIA_API_KEYafter latest main merge/dependency fix (f2cd7cc46).Summary by CodeRabbit
Tests
Chores