test(e2e): migrate credential migration to Vitest [IND-5]#5228
Conversation
Selective E2E Results — ❌ Some jobs failedRun: 27349913431
|
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
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
|
|
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:
📝 WalkthroughWalkthroughReplaces the legacy shell-based credential-migration E2E with a Vitest live scenario and adds/updates workflows, contract tests, validators, and a TypeScript workflow-job type extension. ChangesCredential Migration E2E Vitest Migration
Sequence DiagramsequenceDiagram
participant GitHubActions
participant Runner
participant DockerHub
participant NodeSetup
participant Vitest
participant NemoclawCLI
participant OpenShell
participant ArtifactUploader
GitHubActions->>Runner: start credential-migration-vitest job / nightly job
Runner->>DockerHub: attempt login (retry/fallback)
Runner->>NodeSetup: setup Node 22, npm ci, build CLI
Runner->>Vitest: run credential-migration.test.ts
Vitest->>NemoclawCLI: write legacy credentials, run onboard
NemoclawCLI->>OpenShell: register/read providers
NemoclawCLI->>Vitest: remove legacy credentials file / test symlink unlink
Vitest->>ArtifactUploader: upload e2e-artifacts/vitest/credential-migration/
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 unit tests (beta)
Comment |
Selective E2E Results — ❌ Some jobs failedRun: 27350040869
|
Selective E2E Results — ❌ Some jobs failedRun: 27350181215
|
Selective E2E Results — ❌ Some jobs failedRun: 27350290064
|
PR Review AdvisorFindings: 0 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. |
Selective E2E Results — ❌ Some jobs failedRun: 27350987810
|
Selective E2E Results — ✅ All requested jobs passedRun: 27350836737
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e-scenario/live/credential-migration.test.ts (1)
46-46: ⚡ Quick winPrefer POSIX
:for test PATH composition in this repo.Use
":"instead ofpath.delimiterin this test helper to stay consistent with established test conventions across this suite.♻️ Proposed change
- PATH: [path.join(home, ".local", "bin"), base.PATH].filter(Boolean).join(path.delimiter), + PATH: [path.join(home, ".local", "bin"), base.PATH].filter(Boolean).join(":"),Based on learnings: in this repo’s tests, PATH construction should use
:(Linux CI convention), notpath.delimiter.🤖 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-migration.test.ts` at line 46, The test's PATH construction uses path.delimiter when joining entries—replace the join delimiter with the POSIX ":" literal so the PATH property is built as [...].filter(Boolean).join(":"); update the expression that composes PATH (the array containing path.join(home, ".local", "bin") and base.PATH) to use ":" instead of path.delimiter to match the repo's POSIX test convention.Source: Learnings
🤖 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.
Nitpick comments:
In `@test/e2e-scenario/live/credential-migration.test.ts`:
- Line 46: The test's PATH construction uses path.delimiter when joining
entries—replace the join delimiter with the POSIX ":" literal so the PATH
property is built as [...].filter(Boolean).join(":"); update the expression that
composes PATH (the array containing path.join(home, ".local", "bin") and
base.PATH) to use ":" instead of path.delimiter to match the repo's POSIX test
convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f9ad2087-7f09-4e12-ba4b-ef02888ee261
📒 Files selected for processing (4)
.github/workflows/nightly-e2e.yamltest/e2e-scenario/live/credential-migration.test.tstest/e2e-script-workflow.test.tstest/helpers/e2e-workflow-contract.ts
Selective E2E Results — ✅ All requested jobs passedRun: 27351979452
|
|
Temporarily closing to make room for the maintainer PR-limit exemption fix; will reopen after that lands. |
|
Update: addressed the still-valid advisor/CodeRabbit items.
Left unchanged intentionally: the compatible-endpoint |
Selective E2E Results — ✅ All requested jobs passedRun: 27352148185
|
Selective E2E Results — ❌ Some jobs failedRun: 27352857067
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/e2e-vitest-scenarios.yaml (1)
527-528: 💤 Low valueConsider accessing dispatch inputs via context object instead of template expansion.
The
toJSON()wrapper provides JSON escaping that should prevent JavaScript injection, andworkflow_dispatchis limited to users with write access, so actual risk is low. However, for defense-in-depth, you could access inputs from the context object instead:-const prNumberInput = ${{ toJSON(inputs.pr_number) }} || ''; -const requestedScenarios = ${{ toJSON(inputs.scenarios) }} || ''; +const prNumberInput = context.payload.inputs?.pr_number || ''; +const requestedScenarios = context.payload.inputs?.scenarios || '';This eliminates the template expansion entirely and avoids static analysis warnings.
🤖 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 @.github/workflows/e2e-vitest-scenarios.yaml around lines 527 - 528, The current workflow uses template expansion with toJSON(inputs.pr_number) and toJSON(inputs.scenarios) to populate prNumberInput and requestedScenarios; change these to read inputs from the context object at runtime instead (e.g., use the context.inputs/pr_number and context.inputs/scenarios equivalents) to avoid template expansion and static-analysis warnings—update the assignments for prNumberInput and requestedScenarios to pull from the GitHub Actions context rather than using toJSON template expansion.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In @.github/workflows/e2e-vitest-scenarios.yaml:
- Around line 527-528: The current workflow uses template expansion with
toJSON(inputs.pr_number) and toJSON(inputs.scenarios) to populate prNumberInput
and requestedScenarios; change these to read inputs from the context object at
runtime instead (e.g., use the context.inputs/pr_number and
context.inputs/scenarios equivalents) to avoid template expansion and
static-analysis warnings—update the assignments for prNumberInput and
requestedScenarios to pull from the GitHub Actions context rather than using
toJSON template expansion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f8ca3460-0888-455c-a3ce-977fe18be1e2
📒 Files selected for processing (5)
.github/workflows/e2e-vitest-scenarios.yaml.github/workflows/nightly-e2e.yamltest/e2e-scenario/live/credential-migration.test.tstest/e2e-script-workflow.test.tstest/helpers/e2e-workflow-contract.ts
✅ Files skipped from review due to trivial changes (1)
- test/helpers/e2e-workflow-contract.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/nightly-e2e.yaml
- test/e2e-script-workflow.test.ts
- test/e2e-scenario/live/credential-migration.test.ts
Selective E2E Results — ❌ Some jobs failedRun: 27354068402
|
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27353934442
|
Selective E2E Results — ✅ All requested jobs passedRun: 27354389656
|
Selective E2E Results — ❌ Some jobs failedRun: 27370155805
|
Selective E2E Results — ✅ All requested jobs passedRun: 27370465224
|
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27370480288
|
Selective E2E Results — ✅ All requested jobs passedRun: 27374864318
|
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27375068521
|
Selective E2E Results — ❌ Some jobs failedRun: 27376415002
|
Selective E2E Results — ✅ All requested jobs passedRun: 27376759693
|
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27376761867
|
Selective E2E Results — ✅ All requested jobs passedRun: 27378164078
|
Selective E2E Results — ✅ All requested jobs passedRun: 27378533297
|
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27378535332
|
Summary
Migrate
test/e2e/test-credential-migration.shwith equivalent live Vitest coverage and run the existingcredential-migration-e2enightly lane on the sameubuntu-latestrunner through Vitest.Related Issues
Refs #5098
Contract mapping
~/.nemoclaw/credentials.jsonfeedsnemoclaw onboardwhenNVIDIA_API_KEYis absent from the child env.test/e2e-scenario/live/credential-migration.test.tsruns realnode bin/nemoclaw.js onboard --non-interactivewith a temp HOME containing the legacy file.OPENSHELL_GATEWAY,NODE_OPTIONS) are ignored.openshell -g nemoclaw provider list --names.credentials.jsonandnemoclaw credentials listreads providers from the gateway.node bin/nemoclaw.js credentials listassertions in the live Vitest test.removeLegacyCredentialsFile().Simplicity check
credential-migration-e2enow runs the Vitest test directly onubuntu-latest, preserving the same nightly lane/runner while removing that lane's shell-script execution. Legacy shell deletion is deferred to Epic: Migrate legacy bash E2E into the Vitest E2E system #5098 Phase 11.Verification
npm run build:clinpx vitest run --project cli test/e2e-script-workflow.test.ts test/e2e-scenario/live/credential-migration.test.ts --reporter=defaultNEMOCLAW_RUN_E2E_SCENARIOS=1 npx vitest run --project e2e-scenarios-live test/e2e-scenario/live/credential-migration.test.ts --reporter=default(local import/skip path; live body requiresNVIDIA_API_KEY)npm run typecheck:cligit diff --checkCI / live runner
After opening this PR, dispatch
E2E / Nightlywithjobs=credential-migration-e2eon this branch so the migrated Vitest test runs on the sameubuntu-latestrunner as the legacy lane.Summary by CodeRabbit