test(e2e): add Vitest live coverage for onboard resume#5147
Merged
Conversation
jyaunches
added
the
area: e2e
|
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
|
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:
📝 WalkthroughWalkthroughThis PR adds a comprehensive live Vitest e2e test ( ChangesLive onboard-resume disruption-recovery test
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 |
Contributor
E2E Advisor RecommendationRequired E2E: None 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
|
Mirrors `openshell sandbox get <name>` exit-code probe from test/e2e/test-onboard-resume.sh. Lives on SandboxClient (not host) per fix-location convention: openshell-area helpers belong with the existing sandbox.list / sandbox.status wrappers. `exists()` returns boolean from exit code so callers can branch on sandbox presence without bridging through assertExitZero. Refs: #4348, #5098
Contributor
PR Review AdvisorFindings: 1 needs attention, 1 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. |
Discrete free-standing job for the typed migration of test/e2e/test-onboard-resume.sh, mirroring the openshell-version-pin-vitest precedent from #5107. Uses bash install.sh to bootstrap openshell CLI and node, then runs the live Vitest scenario with NVIDIA_API_KEY exposed. Bash script remains scheduled under nightly-e2e `onboard-resume-e2e`; this job runs only on workflow_dispatch until typed coverage soaks per epic #5098 policy. Refs: #4348, #5098
Migrates the assertion contracts from test/e2e/test-onboard-resume.sh into a free-standing Vitest live test under test/e2e-scenario/live/. Drives the real `nemoclaw onboard` CLI via NEMOCLAW_E2E_FAILURE_INJECTION to deterministically fail at the policies step, then runs `nemoclaw onboard --resume --non-interactive` with NVIDIA_API_KEY stripped from the environment to prove credential hydration from the session file. 22 assertions, organized into Phase 1 (prereqs), Phase 2 (interrupted onboard), and Phase 3 (resume completion). Uses SandboxClient.exists (added in prior commit) for the `openshell sandbox get` probe; reads the onboard session JSON inline for state assertions; registers sandbox/session cleanup with the cleanup fixture. Bash script at test/e2e/test-onboard-resume.sh kept untouched per epic #5098 suite-separation rule until typed coverage soaks; it remains scheduled in nightly-e2e.yaml under onboard-resume-e2e. Refs: #446, #4348, #5098
Collaborator
|
Thanks for pushing the onboard-resume coverage forward. I think this needs a direction/stacking rework before it should join the #4941 / #5098 migration stack. Post-#5106, the target state is one Vitest-driven E2E system: TypeScript tests, small shared Vitest fixtures/helpers, and workflow matrix/direct-job fanout where useful. We should avoid reviving a second runner, a separate "scenario framework" roadmap, or stale tracking files. Concrete asks:
The |
79 tasks
Pass 1 of phase-5 convergence on dispatch 27283289318. First blocking assertion: openshell-installed (#3 in chain), failing with `spawn openshell ENOENT`. Root cause: shell-probe.ts:127-129 \u2014 when inheritEnv is unset, the child gets only options.env. My prereq probes passed no env, so spawn had no PATH to resolve `openshell` against. The onboard runs worked only because they used inheritEnv:true. Fix: use buildAvailabilityProbeEnv() everywhere (framework allowlist incl. PATH/HOME/CI; explicitly excludes NVIDIA_API_KEY). Layer the secret explicitly only on the first onboard; the resume run's env deliberately omits it to test credential hydration from the session file \u2014 this is the typed expression of the bash test's `env -u NVIDIA_API_KEY` invariant. Also drops inheritEnv:true on the onboard runs in favor of the same allowlist composition pattern, matching OnboardingPhaseFixture.commandEnv. Refs: #4348, #5098
Pass 2 of phase-5 convergence on dispatch 27284469157. Line moved forward (75ms → 127s) clearing all Phase 1 prereqs and the Phase 2 first onboard. New blocking assertion: sandbox-exists-after-interrupt (#9 in chain), failing again with `spawn openshell ENOENT` \u2014 same root cause class as pass 1, missed during the previous fix because the sandbox.exists() call passed no options so options.env was empty. Fix: thread `env: buildAvailabilityProbeEnv()` into the exists() call. The SandboxClient correctly does not auto-supply env (mirrors HostCliClient.expectNemoclawAvailable convention \u2014 callers stay explicit about the env boundary, helpers-not-bridges per #5049). Refs: #4348, #5098
Convergence achieved on PR #5147 (epic #5098, owner #4348). Two consecutive PASS dispatches confirmed the typed live test (test/e2e-scenario/live/onboard-resume.test.ts) reproduces the bash script's 22 assertions end-to-end: RED (pass 1, openshell-installed): runs/27283289318 RED (pass 2, sandbox-exists-after-i.): runs/27284469157 GREEN (pass 3, all 22 asserts, 144.3s): runs/27285136703 GREEN (verification, 140.5s): runs/27285846914 Inventory transitions to covered. Bash guard at test/e2e/test-onboard-resume.sh remains scheduled in nightly-e2e.yaml under onboard-resume-e2e until typed coverage soaks per epic policy; deletion is a follow-up PR with #4357 approval. Refs: #446, #4348, #5098
Two PR-caused CI failures resolved in one batch (Batch-Safe Fix Policy:
same-class mechanical fixes, low risk, testable together):
1. cli-test-shards (1) / cli-tests / checks: the cli vitest project's
glob 'test/**/*.test.{js,ts}' picks up test/e2e-scenario/live/**
even though the e2e-scenarios-live project gates that directory on
NEMOCLAW_RUN_E2E_SCENARIOS=1. Without the env var, this test ran in
cli-test-shards (1) and failed at the openshell-installed prereq
probe with spawn ENOENT — runner has no real openshell, no Docker,
no NVIDIA_API_KEY. Add test.skipIf(!shouldRunLiveE2EScenarios()) so
the test self-skips when the live gate isn't set, mirroring the
project-level include glob.
2. static-checks (Files were modified by following hooks: biome format):
biome reformatted the test body. Reformat is mechanical only, no
semantic change.
Verified locally:
- cli project (gate unset): 1 skipped, 0 failed
- e2e-scenarios-live project: test still discovered and runs
- framework-tests e2e-clients: 23/23 pass
- prek pre-push hooks: all pass
ShellCheck SARIF check is also red but is ambient (recent PRs alternate
success/failure with the same 'Requires authentication' GH API error on
the SARIF upload step; permissions in code-scanning.yaml are correct).
Not addressed in this commit.
Refs: #4348, #5098
Collaborator
|
#5126 is now green, so this PR should pause before ready review until it is rebased/reshaped onto the #5126 foundation. It still targets |
Collaborator
|
Update after #5126 moved the foundation to the fixtures namespace: the salvage path here should now be retarget/rebase onto #5126, remove the retired ledger update, move any support coverage under |
12 tasks
Contributor
Author
|
Migration-principle review for #5098: this should stay a simple Vitest migration, not grow the old “scenario framework” path back in. Reference from Concerns in this PR:
Recommended path:
Other acceptable options:
Suggested immediate changes:
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
jyaunches
changed the title
![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-scenario/live/onboard-resume.test.ts`:
- Around line 317-318: The registry assertion currently uses a substring check
which can yield false positives; instead, read and parse the registry JSON from
REGISTRY_FILE (already read via fs.readFileSync), parse it with JSON.parse,
locate the sandbox name token within the parsed structure (e.g., the array or
object key that holds sandbox names), and assert equality against SANDBOX_NAME
(use strict equality or expect(...).toBe(SANDBOX_NAME)) so the test requires an
exact match; update the test around the existing fs.existsSync(REGISTRY_FILE)
check and replace the toContain assertion with the JSON parse + exact match
assertion referencing REGISTRY_FILE and SANDBOX_NAME.
🪄 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: f2d1e696-0c52-43f5-83cf-1a263902ebc7
📒 Files selected for processing (1)
test/e2e-scenario/live/onboard-resume.test.ts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-5147.docs.buildwithfern.com/nemoclaw |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tools/pr-review-advisor/analyze.mts (1)
1004-1021: 💤 Low valueOptional: Unnecessary nullable coalescing.
Line 1006 uses
?? []for defensive null-handling, butretiredE2eMigrationLedgerChangesis typed as a required array field (line 166), not optional. The coalescing is harmless but unnecessary.♻️ Optional simplification
- for (const ledger of metadata.deterministic.retiredE2eMigrationLedgerChanges ?? []) { + for (const ledger of metadata.deterministic.retiredE2eMigrationLedgerChanges) {🤖 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/pr-review-advisor/analyze.mts` around lines 1004 - 1021, The code uses unnecessary null-coalescing on a required array: remove the defensive "?? []" in addDeterministicFindings and iterate directly over metadata.deterministic.retiredE2eMigrationLedgerChanges (which is typed as a non-null array), so replace the for-loop source from metadata.deterministic.retiredE2eMigrationLedgerChanges ?? [] to metadata.deterministic.retiredE2eMigrationLedgerChanges to simplify the function while preserving behavior.docs/about/release-notes.mdx (1)
22-23: ⚡ Quick winSplit multi-sentence bullet lines into one sentence per source line.
Several changed bullets place multiple sentences on one source line, which breaks the docs diff/readability rule.
As per coding guidelines, docs require “One sentence per line in source (makes diffs readable).”
Also applies to: 27-28, 33-33
🤖 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 `@docs/about/release-notes.mdx` around lines 22 - 23, The two multi-sentence bullet points in the release notes (the one starting "GPU sandbox creation and local inference checks now match the runtime paths agents use..." and the one starting "Onboarding and recovery fail earlier and stay quieter on common host drift...") must be split so each sentence is on its own source line; update those bullets (and the other noted bullets at lines 27-28 and 33) so every sentence ends with a newline in the MDX source, preserving existing punctuation and links (e.g., the reference links like [Use a Local Inference Server] and [NemoClaw CLI Commands Reference]) and keeping exact wording but breaking them into one-sentence-per-line for readable diffs.Source: Coding guidelines
docs/reference/troubleshooting.mdx (1)
1135-1135: ⚡ Quick winKeep exactly one sentence per source line in the new section.
These lines currently contain two sentences on the same source line; split them so each line contains a single sentence.
As per coding guidelines, “One sentence per line in source (makes diffs readable).”
Also applies to: 1142-1142, 1161-1161, 1168-1168
🤖 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 `@docs/reference/troubleshooting.mdx` at line 1135, The marked source lines contain multiple sentences on the same line (e.g., the line mentioning `controlui.bootstrap.config.json` and the other occurrences referenced); edit the section so each sentence is on its own source line by splitting any line that holds two sentences into two lines, ensuring every sentence in the new section occupies exactly one line.Source: Coding guidelines
test/hermes-gateway-wrapper.test.ts (1)
78-78: ⚡ Quick winUse POSIX PATH separator
:directly instead ofpath.delimiter.The tests are already gated on
process.platform === "linux"(line 35). Per repo convention, prefer the established POSIX PATH separator:when constructingprocess.env.PATHin tests, rather thanpath.delimiter. This aligns with the repo's Linux-only test execution on CI runners.🔧 Proposed change
- pathPrefix = `${evilBin}${path.delimiter}`; + pathPrefix = `${evilBin}:`;Based on learnings: "In this repo's test suite, prefer the established POSIX PATH separator
:when constructingprocess.env.PATHin tests (e.g.,PATH:${fakeBin}:${process.env.PATH || ""}``). Do not replace it withpath.delimiterin these unit/integration tests, because they only run on Linux runners in CI here."🤖 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/hermes-gateway-wrapper.test.ts` at line 78, The test sets pathPrefix using path.delimiter which can vary by platform; since the test is gated to Linux (process.platform === "linux"), replace path.delimiter with the POSIX separator ':' when constructing pathPrefix (the variable assigned on the line with pathPrefix = `${evilBin}${path.delimiter}`) so the test uses a stable ':' separator consistent with other tests that build process.env.PATH.Source: Learnings
test/e2e/test-cron-preflight-inference-local-e2e.sh (1)
35-35: 💤 Low valueConsider adding
-eto the set flags for stricter error handling.The script uses
set -uo pipefailwithout-e. While the manual pass/fail tracking works, adding-ewould make the script exit immediately on unexpected errors during setup (e.g., directory creation, file writes), which is safer for E2E tests. The pass/fail tracking logic in test assertions can still catch and handle expected failures explicitly.🔧 Proposed change
-set -uo pipefail +set -euo pipefail🤖 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/test-cron-preflight-inference-local-e2e.sh` at line 35, The script's shell flags are too permissive—update the top-level set invocation in test-cron-preflight-inference-local-e2e.sh to include -e so the script exits on unexpected errors; change the existing set -uo pipefail to set -euo pipefail (or set -o errexit -uo pipefail) and verify any manual pass/fail tracking or traps in the script still behave as intended after adding -e.test/generate-openclaw-config-slack-allowlist.test.ts (1)
1-1: ⚡ Quick winVerify whether
@ts-nocheckis necessary.The
@ts-nocheckdirective disables TypeScript checking for the entire file. This is unusual for test files in this repository. If there's a specific type issue with thebuildConfigimport or theanyreturn types in the helpers, prefer fixing the types explicitly (e.g., proper type imports or targeted@ts-expect-errorcomments) rather than disabling all type checking. This improves type safety and catches potential bugs during development.🤖 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/generate-openclaw-config-slack-allowlist.test.ts` at line 1, Remove the top-level "// `@ts-nocheck`" and restore TypeScript checking; then fix the specific type issues instead of disabling checks by importing or declaring the correct type for the buildConfig import (use the repository's BuildConfig or equivalent type) and replace any broad "any" return types in helper functions with precise types or use targeted `@ts-expect-error` comments for individual problematic lines; ensure symbols like buildConfig and any helper function names in this test are properly typed so the file passes type-checking without disabling it globally.
🤖 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 `@src/lib/onboard.ts`:
- Around line 2924-2949: The call to resolveDisabledChannels() currently runs
before pruneStaleSandboxEntry(sandboxName), allowing disabled channel state from
a stale registry row to leak into disabledChannels used by
enforceMessagingChannelConflicts; either move the
resolveDisabledChannels(sandboxName) invocation to after
pruneStaleSandboxEntry(sandboxName) completes, or change
resolveDisabledChannels/resolveDisabledChannels fallback logic so it does not
call registry.getDisabledChannels() for sandboxes that are not live (i.e.,
consult pruneStaleSandboxEntry or a gateway-liveness check first); update
references to disabledChannels/disabledChannelNames and the
enforceMessagingChannelConflicts call so they use the post-prune resolved value.
---
Nitpick comments:
In `@docs/about/release-notes.mdx`:
- Around line 22-23: The two multi-sentence bullet points in the release notes
(the one starting "GPU sandbox creation and local inference checks now match the
runtime paths agents use..." and the one starting "Onboarding and recovery fail
earlier and stay quieter on common host drift...") must be split so each
sentence is on its own source line; update those bullets (and the other noted
bullets at lines 27-28 and 33) so every sentence ends with a newline in the MDX
source, preserving existing punctuation and links (e.g., the reference links
like [Use a Local Inference Server] and [NemoClaw CLI Commands Reference]) and
keeping exact wording but breaking them into one-sentence-per-line for readable
diffs.
In `@docs/reference/troubleshooting.mdx`:
- Line 1135: The marked source lines contain multiple sentences on the same line
(e.g., the line mentioning `controlui.bootstrap.config.json` and the other
occurrences referenced); edit the section so each sentence is on its own source
line by splitting any line that holds two sentences into two lines, ensuring
every sentence in the new section occupies exactly one line.
In `@test/e2e/test-cron-preflight-inference-local-e2e.sh`:
- Line 35: The script's shell flags are too permissive—update the top-level set
invocation in test-cron-preflight-inference-local-e2e.sh to include -e so the
script exits on unexpected errors; change the existing set -uo pipefail to set
-euo pipefail (or set -o errexit -uo pipefail) and verify any manual pass/fail
tracking or traps in the script still behave as intended after adding -e.
In `@test/generate-openclaw-config-slack-allowlist.test.ts`:
- Line 1: Remove the top-level "// `@ts-nocheck`" and restore TypeScript checking;
then fix the specific type issues instead of disabling checks by importing or
declaring the correct type for the buildConfig import (use the repository's
BuildConfig or equivalent type) and replace any broad "any" return types in
helper functions with precise types or use targeted `@ts-expect-error` comments
for individual problematic lines; ensure symbols like buildConfig and any helper
function names in this test are properly typed so the file passes type-checking
without disabling it globally.
In `@test/hermes-gateway-wrapper.test.ts`:
- Line 78: The test sets pathPrefix using path.delimiter which can vary by
platform; since the test is gated to Linux (process.platform === "linux"),
replace path.delimiter with the POSIX separator ':' when constructing pathPrefix
(the variable assigned on the line with pathPrefix =
`${evilBin}${path.delimiter}`) so the test uses a stable ':' separator
consistent with other tests that build process.env.PATH.
In `@tools/pr-review-advisor/analyze.mts`:
- Around line 1004-1021: The code uses unnecessary null-coalescing on a required
array: remove the defensive "?? []" in addDeterministicFindings and iterate
directly over metadata.deterministic.retiredE2eMigrationLedgerChanges (which is
typed as a non-null array), so replace the for-loop source from
metadata.deterministic.retiredE2eMigrationLedgerChanges ?? [] to
metadata.deterministic.retiredE2eMigrationLedgerChanges to simplify the function
while preserving behavior.
🪄 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: cbe77e94-ddf9-479a-bef3-3a2de6dd1447
📒 Files selected for processing (138)
.agents/skills/nemoclaw-user-configure-inference/SKILL.md.agents/skills/nemoclaw-user-configure-security/references/best-practices.md.agents/skills/nemoclaw-user-manage-sandboxes/references/backup-restore.md.agents/skills/nemoclaw-user-overview/references/release-notes.md.agents/skills/nemoclaw-user-reference/references/commands.md.agents/skills/nemoclaw-user-reference/references/network-policies.md.coderabbit.yaml.github/workflows/nightly-e2e.yamlDockerfileagents/hermes/Dockerfileagents/hermes/hermes-wrapper.shagents/hermes/start.shagents/openclaw/manifest.yamlci/test-file-size-budget.jsondocs/about/release-notes.mdxdocs/inference/use-local-inference.mdxdocs/manage-sandboxes/messaging-channels.mdxdocs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxdocs/reference/troubleshooting.mdxnemoclaw/src/commands/shields-status.test.tsnemoclaw/src/commands/shields-status.tsnemoclaw/src/commands/slash.test.tsnemoclaw/src/commands/slash.tsscripts/install.shscripts/ollama-auth-proxy.jssrc/lib/actions/gateway-drift-preflight.test.tssrc/lib/actions/sandbox/policy-channel-conflict.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/snapshot.tssrc/lib/actions/upgrade-sandboxes.tssrc/lib/agent/defs.test.tssrc/lib/domain/maintenance/upgrade.test.tssrc/lib/domain/maintenance/upgrade.tssrc/lib/inference/model-prompts.test.tssrc/lib/inference/model-prompts.tssrc/lib/inference/ollama/proxy.tssrc/lib/inference/vllm-models.test.tssrc/lib/inference/vllm-models.tssrc/lib/inference/vllm-prompt.tssrc/lib/inference/vllm.test.tssrc/lib/inference/vllm.tssrc/lib/inventory/index.test.tssrc/lib/inventory/index.tssrc/lib/messaging/applier/conflict-detection-slack-gateway.test.tssrc/lib/messaging/applier/conflict-detection/index.tssrc/lib/messaging/applier/conflict-detection/slack-socket-mode.tssrc/lib/messaging/applier/conflict-detection/types.tssrc/lib/onboard.tssrc/lib/onboard/bridge-dns-preflight.tssrc/lib/onboard/host-dns-preflight.test.tssrc/lib/onboard/machine/handlers/sandbox.tssrc/lib/onboard/messaging-conflict-guard.test.tssrc/lib/onboard/messaging-conflict-guard.tssrc/lib/onboard/preflight.tssrc/lib/onboard/sandbox-agent.tssrc/lib/security/credential-filter.test.tssrc/lib/security/credential-filter.tssrc/lib/state/registry.tssrc/lib/status-command-deps.tstest/cli/list-share-live-inference.test.tstest/control-ui-config-endpoint-docs.test.tstest/e2e-scenario-advisor.test.tstest/e2e-scenario/docs/MIGRATION.mdtest/e2e-scenario/docs/README.mdtest/e2e-scenario/docs/RETIREMENT.mdtest/e2e-scenario/fixtures/artifacts.tstest/e2e-scenario/fixtures/availability-env.tstest/e2e-scenario/fixtures/cleanup.tstest/e2e-scenario/fixtures/clients/command.tstest/e2e-scenario/fixtures/clients/gateway.tstest/e2e-scenario/fixtures/clients/host.tstest/e2e-scenario/fixtures/clients/index.tstest/e2e-scenario/fixtures/clients/provider.tstest/e2e-scenario/fixtures/clients/sandbox.tstest/e2e-scenario/fixtures/clients/state.tstest/e2e-scenario/fixtures/e2e-test.tstest/e2e-scenario/fixtures/live-project-gate.tstest/e2e-scenario/fixtures/phases/environment.tstest/e2e-scenario/fixtures/phases/index.tstest/e2e-scenario/fixtures/phases/lifecycle.tstest/e2e-scenario/fixtures/phases/onboarding.tstest/e2e-scenario/fixtures/phases/runtime.tstest/e2e-scenario/fixtures/phases/state-validation.tstest/e2e-scenario/fixtures/redaction.tstest/e2e-scenario/fixtures/secrets.tstest/e2e-scenario/fixtures/shell-probe.tstest/e2e-scenario/fixtures/shell/supervisor.tstest/e2e-scenario/fixtures/shell/trusted-command.tstest/e2e-scenario/framework-tests/e2e-migration-inventory.test.tstest/e2e-scenario/live/onboard-resume.test.tstest/e2e-scenario/live/openshell-version-pin.test.tstest/e2e-scenario/live/registry-scenarios.test.tstest/e2e-scenario/live/ubuntu-repo-cli-smoke.test.tstest/e2e-scenario/migration/legacy-inventory.jsontest/e2e-scenario/scenarios/types.tstest/e2e-scenario/support-tests/e2e-clients.test.tstest/e2e-scenario/support-tests/e2e-expected-state.test.tstest/e2e-scenario/support-tests/e2e-fixture-context.test.tstest/e2e-scenario/support-tests/e2e-live-project-config.test.tstest/e2e-scenario/support-tests/e2e-live-registry-discovery.test.tstest/e2e-scenario/support-tests/e2e-live-skip-name-contract.test.tstest/e2e-scenario/support-tests/e2e-manifests.test.tstest/e2e-scenario/support-tests/e2e-migration-policy.test.tstest/e2e-scenario/support-tests/e2e-migration-source-of-truth.test.tstest/e2e-scenario/support-tests/e2e-phase-environment.test.tstest/e2e-scenario/support-tests/e2e-phase-lifecycle.test.tstest/e2e-scenario/support-tests/e2e-phase-onboarding.test.tstest/e2e-scenario/support-tests/e2e-phase-runtime.test.tstest/e2e-scenario/support-tests/e2e-phase-state-validation.test.tstest/e2e-scenario/support-tests/e2e-redaction-entry.test.tstest/e2e-scenario/support-tests/e2e-redaction-parity.test.tstest/e2e-scenario/support-tests/e2e-scenario-matrix.test.tstest/e2e-scenario/support-tests/e2e-scenario-registry.test.tstest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstest/e2e-scenario/support-tests/e2e-shell-supervisor.test.tstest/e2e-script-workflow.test.tstest/e2e/docs/parity-inventory.generated.jsontest/e2e/test-cron-preflight-inference-local-e2e.shtest/e2e/test-hermes-root-entrypoint-smoke.shtest/fetch-guard-patch-regression.test.tstest/generate-openclaw-config-slack-allowlist.test.tstest/helpers/installer-sourced-env.tstest/hermes-gateway-wrapper.test.tstest/hermes-start.test.tstest/install-preflight-docker-bootstrap.test.tstest/install-preflight.test.tstest/ollama-proxy-startup.test.tstest/onboard-selection.test.tstest/pr-review-advisor.test.tstest/pr-workflow-contract.test.tstest/release-latest-tag.test.tstest/snapshot.test.tstools/e2e-advisor/scenario-comment.mtstools/e2e-advisor/scenarios-schema.jsontools/e2e-advisor/scenarios.mtstools/pr-review-advisor/analyze.mtsvitest.config.ts
💤 Files with no reviewable changes (2)
- test/e2e-scenario/migration/legacy-inventory.json
- test/e2e-scenario/framework-tests/e2e-migration-inventory.test.ts
✅ Files skipped from review due to trivial changes (27)
- .agents/skills/nemoclaw-user-reference/references/network-policies.md
- test/e2e-scenario/fixtures/phases/lifecycle.ts
- test/e2e-scenario/fixtures/clients/provider.ts
- test/e2e-scenario/fixtures/secrets.ts
- src/lib/inference/model-prompts.ts
- test/e2e-scenario/fixtures/shell/trusted-command.ts
- test/e2e-scenario/live/ubuntu-repo-cli-smoke.test.ts
- docs/inference/use-local-inference.mdx
- test/e2e-scenario/support-tests/e2e-fixture-context.test.ts
- agents/openclaw/manifest.yaml
- test/e2e-scenario/scenarios/types.ts
- test/e2e-scenario/support-tests/e2e-phase-lifecycle.test.ts
- tools/e2e-advisor/scenarios-schema.json
- agents/hermes/start.sh
- test/release-latest-tag.test.ts
- test/e2e-scenario/fixtures/shell/supervisor.ts
- test/e2e-scenario/fixtures/shell-probe.ts
- tools/e2e-advisor/scenario-comment.mts
- .agents/skills/nemoclaw-user-overview/references/release-notes.md
- .agents/skills/nemoclaw-user-manage-sandboxes/references/backup-restore.md
- .agents/skills/nemoclaw-user-reference/references/commands.md
- .agents/skills/nemoclaw-user-configure-inference/SKILL.md
- docs/reference/commands.mdx
- test/e2e-scenario/docs/MIGRATION.md
- test/e2e-scenario/support-tests/e2e-phase-onboarding.test.ts
- docs/reference/commands-nemohermes.mdx
- test/e2e-scenario/fixtures/phases/state-validation.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e-scenario/live/onboard-resume.test.ts
prekshivyas
requested changes
Jun 10, 2026
prekshivyas
left a comment
prekshivyas
left a comment
Contributor
There was a problem hiding this comment.
Reviewed (code + 9-cat security). New gated live Vitest test for onboard --resume: forces a policies-step failure via the E2E injection hook, then resumes with NVIDIA_API_KEY stripped to prove credential hydration from the session file. The resume/skip strings, the session-failure assertions (status:"failed", failure.step:"policies"), and the credential-layering design are all verified correct against source, and credential handling is exemplary.
🔴 Request changes — the no-rerun guard is vacuous. The negative assertions use stale step labels: not.toContain("[1/7] Preflight checks"), "[2/7] …gateway", "[5/7] Creating sandbox"). But the flow is 8 steps, so the CLI emits [1/8], [2/8], and sandbox is [6/8] (verified against machine/definition.ts + prompt-helpers.ts). The /7 strings never appear in output, so all three not.toContain checks pass even if those steps were re-run — defeating the test's headline purpose. Same at line 281: ranInference checks [4/7] (actual [4/8]), so that positive branch is dead.
Fix: correct the labels to [1/8] / [2/8] / [6/8] / [4/8] — or assert on the positive [resume] Skipping … lines (those are verified correct). Minor: SessionStateComplete omits the 8th step (agent_setup).
Security: all 9 pass; the source-shape budget gate is NOT tripped (the file reads target $HOME state files, not production paths).
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Contributor
Author
|
@prekshivyas addressed the requested-change item in |
12 tasks
12 tasks
13 tasks
cv
added a commit
that referenced
this pull request
Jun 16, 2026
## Summary Migrate `test-onboard-resume.sh` into the live Vitest E2E system. Wires the existing `test/e2e-scenario/live/onboard-resume.test.ts` replacement into `e2e-vitest-scenarios.yaml` as `onboard-resume-vitest`, closing out the partial migration from PR #5147 with a dispatchable same-runner Vitest path. ## Related Issue Refs #5098 ## Changes - Adds or wires the free-standing live Vitest scenario `onboard-resume`. - Adds selective workflow dispatch via `onboard-resume-vitest` in `.github/workflows/e2e-vitest-scenarios.yaml`. - Preserves the legacy system boundaries from `test-onboard-resume.sh` while leaving legacy shell retirement to #5098 Phase 11. ## 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) ## Verification - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `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) Targeted local checks run while preparing these branches: - `npx vitest run --project e2e-vitest-support test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts --silent=false --reporter=default` - `npm run typecheck:cli` for branches adding new TypeScript tests - `git diff --check` Selective same-runner dispatch: https://github.com/NVIDIA/NemoClaw/actions/runs/27636907033 — passed after hosted-compatible env fix --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved resume behavior to avoid repeating sandbox creation and gateway startup. * Updated resume verification to correctly respect the hosted provider and to allow steps to be either completed or skipped without failing. * **Tests / CI** * Added automated live e2e coverage for the onboarding resume flow and ensured its results are included in PR feedback. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
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
Adds a focused Vitest live test for the onboard resume disruption-recovery contract from
test/e2e/test-onboard-resume.sh.This is intentionally a minimal coverage slice, not a migration closeout:
onboard-resume-e2ebash lane scheduled for now.Related Issue
Refs #4348
Refs #5098
Refs #446
Test shape
test/e2e-scenario/live/onboard-resume.test.tsdrives the realnemoclaw onboardCLI through the deterministic E2E failure-injection hook, then resumes withNVIDIA_API_KEYomitted from the resume environment to verify credential hydration from session state.Assertion chain:
NVIDIA_API_KEYis available.policiesviaNEMOCLAW_E2E_FAILURE_INJECTION/NEMOCLAW_E2E_FORCE_FAIL_AT_STEP.nemoclaw onboard --resume --non-interactivecompletes without rerunning cached preflight/gateway/sandbox steps.Simplicity check
Validation
npm test -- --run test/e2e-scenario/framework-tests/e2e-clients.test.ts test/e2e-scenario/live/onboard-resume.test.tsnpx biome check test/e2e-scenario/live/onboard-resume.test.tsgit diff --checkSigned-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit