test(e2e): migrate sessions agents cli scenario#5363
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
📝 WalkthroughWalkthroughAdds a live Vitest scenario testing NemoClaw sessions/agents CLI flows with robust output parsing and scope-recovery, a GitHub Actions job to run it (with Node/Docker/OpenShell setup and artifact upload), PR reporting integration, and selector/boundary test updates. ChangesSessions and Agents CLI E2E Vitest Scenario
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorFailed: Could not parse JSON from advisor output; see /home/runner/work/NemoClaw/NemoClaw/artifacts/e2e-advisor/e2e-advisor-raw-output.txt |
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: None Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorFailed: Could not parse JSON from advisor output; see /home/runner/work/NemoClaw/NemoClaw/artifacts/e2e-advisor/e2e-scenario-advisor-raw-output.txt |
Contributor
PR Review AdvisorFindings: 0 needs attention, 1 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
This is an automated advisory review. A human maintainer must make the final merge decision. |
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27438223490
|
![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: 2
🧹 Nitpick comments (1)
tools/e2e-scenarios/workflow-boundary.mts (1)
2112-2112: 📐 Maintainability & Code Quality | ⚡ Quick winAdd a dedicated contract validator for
sessions-agents-cli-vitest.This new line only enforces selector shape; it doesn’t lock down job-level invariants (secret exposure boundaries, expected run command, artifact path/name). Adding a
validateSessionsAgentsCliVitestJob(...)(as done for other free-standing jobs) will prevent silent workflow drift.🤖 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` at line 2112, The PR adds a selector-only check via validateFreeStandingJobSelector for "sessions-agents-cli-vitest" but lacks a full job-level contract validator; add a new validateSessionsAgentsCliVitestJob(errors, jobs, "sessions-agents-cli-vitest") function (modeled after other free-standing job validators) and call it alongside or immediately after validateFreeStandingJobSelector to enforce secrets boundaries, expected run command, artifact path/name and other invariants; implement the validator to locate the "sessions-agents-cli-vitest" job in jobs and assert its secrets/exposed variables, the exact run/steps structure, and artifact naming/paths consistent with project conventions.
🤖 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/sessions-agents-cli.test.ts`:
- Around line 140-158: The current cleanupSandbox function swallows all errors
via bestEffort and is reused for setup preconditions, which masks failures;
split it into two helpers: rename the existing function to
bestEffortCleanupSandbox (keep its calls to runNemoclaw([... "destroy" ...]) and
host.command("openshell", ["sandbox", "delete", ...]) wrapped with bestEffort)
and create a new strict cleanupSandbox that runs the same two operations without
bestEffort so failures propagate (i.e., do not catch/suppress errors) — update
callers used during setup to call the strict cleanupSandbox and only use
bestEffortCleanupSandbox for final teardown; align naming with the fixture
client’s bestEffortCleanupSandbox in host.ts to avoid confusion.
- Around line 520-562: The current verification uses "sessions list --agent" and
parseJsonEnvelope to infer agent deletion, which is unreliable; replace that
block to call runNemoclaw with [SANDBOX_NAME, "agents", "list", "--json"] (use
the same apiKey/options pattern as deleteAgent), then parse the JSON envelope
(use parseJsonEnvelope) and assert that no agent entry matches TEST_AGENT_ID
(similar to how sessionEntries(...) was used earlier); remove the
deletedAgentStillVisible boolean/exitCode inversion logic and instead fail if
the agents-list command returns exitCode !== 0 or if the parsed agents list
contains TEST_AGENT_ID. Ensure you reference the existing variables deleteAgent,
TEST_AGENT_ID, runNemoclaw, and parseJsonEnvelope when implementing the
replacement.
---
Nitpick comments:
In `@tools/e2e-scenarios/workflow-boundary.mts`:
- Line 2112: The PR adds a selector-only check via
validateFreeStandingJobSelector for "sessions-agents-cli-vitest" but lacks a
full job-level contract validator; add a new
validateSessionsAgentsCliVitestJob(errors, jobs, "sessions-agents-cli-vitest")
function (modeled after other free-standing job validators) and call it
alongside or immediately after validateFreeStandingJobSelector to enforce
secrets boundaries, expected run command, artifact path/name and other
invariants; implement the validator to locate the "sessions-agents-cli-vitest"
job in jobs and assert its secrets/exposed variables, the exact run/steps
structure, and artifact naming/paths consistent with project conventions.
🪄 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: 96fa9be4-316a-45d4-951e-c269bd5a3f0c
📒 Files selected for processing (5)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/live/sessions-agents-cli.test.tstest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/free-standing-jobs.envtools/e2e-scenarios/workflow-boundary.mts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27444257786
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/e2e-scenarios/workflow-boundary.mts (1)
2156-2156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the shared secret denylist to include
NVIDIA_INFERENCE_API_KEY.This new selector mapping means
sessions-agents-cli-vitestis enforced via shared boundary checks, but the shared denylist still keys onNVIDIA_API_KEY. That missesNVIDIA_INFERENCE_API_KEYexposure for free-standing jobs without dedicated per-job validators.Suggested fix
const COMMON_SECRET_ENV_NAMES = [ - "NVIDIA_API_KEY", + "NVIDIA_INFERENCE_API_KEY", + "NVIDIA_API_KEY", "DOCKERHUB_USERNAME", "DOCKERHUB_TOKEN", "GITHUB_TOKEN", ];🤖 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` at line 2156, The shared-boundary denylist for free-standing job validation is missing the new secret key; update the shared denylist used by the free-standing validator so that NVIDIA_INFERENCE_API_KEY is included alongside NVIDIA_API_KEY. Modify the code path referenced by validateFreeStandingJobSelector (the shared denylist/validator used for "sessions-agents-cli-vitest" / "sessions-agents-cli") to add "NVIDIA_INFERENCE_API_KEY" to the denylist of secret keys checked during shared boundary validation so free-standing jobs are correctly blocked from exposing that secret.
🤖 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.
Outside diff comments:
In `@tools/e2e-scenarios/workflow-boundary.mts`:
- Line 2156: The shared-boundary denylist for free-standing job validation is
missing the new secret key; update the shared denylist used by the free-standing
validator so that NVIDIA_INFERENCE_API_KEY is included alongside NVIDIA_API_KEY.
Modify the code path referenced by validateFreeStandingJobSelector (the shared
denylist/validator used for "sessions-agents-cli-vitest" /
"sessions-agents-cli") to add "NVIDIA_INFERENCE_API_KEY" to the denylist of
secret keys checked during shared boundary validation so free-standing jobs are
correctly blocked from exposing that secret.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 467d7b17-5d74-4a5c-853f-b9de470208d4
📒 Files selected for processing (3)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts
- .github/workflows/e2e-vitest-scenarios.yaml
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
2 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
Migrates
test/e2e/test-sessions-agents-cli.shinto a focused live Vitest scenario. The new coverage preserves the host-sidenemoclaw <name> sessionsandagentsCLI contracts, including live credential gating, OpenClaw pairing/scope approval handling, JSON-envelope assertions, cleanup, and rate-limit-aware pre-contract skips.Related Issue
Refs #5098
Changes
test/e2e-scenario/live/sessions-agents-cli.test.tsfor the live sessions/agents CLI migration.sessions-agents-cli-vitestinto.github/workflows/e2e-vitest-scenarios.yamland the free-standing scenario inventory.Type of Change
Verification
Targeted checks passed:
npm run build:cli,cd nemoclaw && npm ci --ignore-scripts && npm run build, workflow support Vitest undere2e-vitest-supportandcli, live-test gated smoke withoutNVIDIA_API_KEY, Biome lint, CLI typecheck, test-size/source-shape checks, andgit diff --check. File-scopedprek/commit/push hooks were attempted; the broadtest-clihook hit unrelated local flake/timeouts in existing CLI tests, so only that hook was skipped for commit/push after targeted verification.npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Tests
Chores