test(e2e): bridge onboard negative paths to Vitest#5152
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR introduces an E2E test for the ChangesOnboard negative-path E2E test and workflow validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
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
PR Review AdvisorFindings: 0 needs attention, 4 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. |
Collaborator
|
Thanks for pushing the onboard-negative-paths slice forward. This needs to rebase/reshape before review because #5126 is now green and establishes the migration policy for this stack. Concrete blockers: targets |
Collaborator
|
Update after #5126 moved the foundation to the fixtures namespace: this onboard-negative-paths slice should be reshaped onto #5126 before review, with no retired ledger update and no |
This was referenced Jun 10, 2026
Contributor
Author
|
Migration-principle review for #5098: this PR is currently not aligned with the “simple Vitest tests, not new unnecessary framework” direction. Reference from Concerns in this PR:
Recommended path:
Other acceptable options:
Suggested immediate direction:
|
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-5152.docs.buildwithfern.com/nemoclaw |
prekshivyas
approved these 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). Bridges the legacy test-onboard-negative-paths.sh (#2573) into the registry-driven Vitest framework — flips 3 existing canonical scenario ids to supported, adds 3 onboarding-fixture dispatch paths + a local-registry-fields-match state probe. Test-only, no src//workflow changes.
✅ Approve — coverage parity verified: the baseline scenario file is byte-identical main↔head (the ids already existed), the negative-path expected-states assert absence (gateway/sandbox absent) rather than probing a non-existent gateway, and the deferred legacy phases (policy-mode fallback, non-NVIDIA bypass) are genuinely covered in policy-tiers-onboard.test.ts / validation.test.ts — so keeping the legacy script covered (not deletionReady) is correct.
Security: all 9 pass — sandbox names still go through validateSandboxName, the port-conflict stub is loopback-only and closed in finally, and the real key is never injected into the invalid-key path. Nits: redactionValues:["not-a-nvidia-key"] redacts a non-secret (harmless); the invalid-key scenario declares requiredSecrets:[NVIDIA_API_KEY] but hardcodes the bad key.
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agents/skills/nemoclaw-user-configure-security/references/best-practices.md (1)
1-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSPDX header compliance is missing across multiple changed files.
Please add SPDX copyright and Apache-2.0 license headers at the top of each file using the file-appropriate comment syntax (HTML comments for Markdown,
#comments for YAML).As per coding guidelines, all listed source file types in this repository must include SPDX headers.
🤖 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 @.agents/skills/nemoclaw-user-configure-security/references/best-practices.md around lines 1 - 3, Add SPDX copyright and Apache-2.0 license headers to the top of this Markdown file using HTML comments (e.g. <!-- SPDX-FileCopyrightText: YEAR YourOrg --> and <!-- SPDX-License-Identifier: Apache-2.0 -->) so the file containing the "# NemoClaw Security Best Practices" title and the import of AgentOnly includes the required headers; apply the same pattern to all changed files of their respective types (use HTML comments for .md and # comments for YAML) and place them as the first lines before any content or imports.Source: Coding guidelines
🧹 Nitpick comments (7)
Dockerfile (1)
424-431: ⚡ Quick winFail closed if the reviewed preflight object already has a
mode.This shape gate only distinguishes “already patched exact literal” vs. “not patched yet.” If a future OpenClaw bundle keeps the same
auditContextbut adds a differentmode:at that callsite, this branch will still run and inject a secondmodeproperty instead of stopping on an unreviewed layout. Add an explicit pre-patch check for any non-trusted_env_proxymodenearcron-model-provider-preflightandpatch_failon it.Suggested hardening
- patched_count="$(grep -Fc 'mode: "trusted_env_proxy", auditContext: "cron-model-provider-preflight"' "$f" || true)"; \ + if grep -Eq 'mode: "[^"]+", auditContext: "cron-model-provider-preflight"' "$f" \ + && ! grep -Fq 'mode: "trusted_env_proxy", auditContext: "cron-model-provider-preflight"' "$f"; then \ + patch_fail "Patch 6 shape gate: $f already specifies a different mode for cron-model-provider-preflight"; \ + fi; \ + patched_count="$(grep -Fc 'mode: "trusted_env_proxy", auditContext: "cron-model-provider-preflight"' "$f" || true)"; \🤖 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 `@Dockerfile` around lines 424 - 431, The current patch logic only checks for the exact literal 'mode: "trusted_env_proxy", auditContext: "cron-model-provider-preflight"' and will blindly insert a second mode if a different mode already exists; update the pre-patch checks in the block around patched_count/new_patched_count to first grep for any occurrence of 'auditContext: "cron-model-provider-preflight"' combined with a 'mode:' nearby (e.g., grep for 'mode:' within the same line or context) and if such a mode exists and is not 'mode: "trusted_env_proxy"' call patch_fail with a clear message, otherwise proceed with the existing exact-literal insertion and verification logic; reference the functions/variables patched_count, sed invocation that performs the replacement, new_patched_count, and patch_fail when adding this protective branch.docs/manage-sandboxes/messaging-channels.mdx (1)
87-87: ⚡ Quick winSplit semicolon-joined clauses to separate lines.
Line 87 contains two independent clauses joined by a semicolon. Per docs coding guidelines, use one sentence per line in source to make diffs readable.
📝 Suggested split
-To exercise Slack channel setup and the `slack` policy preset with placeholder tokens in a restricted network or hermetic test environment, set `NEMOCLAW_SKIP_SLACK_AUTH_VALIDATION=1` to skip the live credential probes; Slack token format checks still apply. +To exercise Slack channel setup and the `slack` policy preset with placeholder tokens in a restricted network or hermetic test environment, set `NEMOCLAW_SKIP_SLACK_AUTH_VALIDATION=1` to skip the live credential probes. +Slack token format checks still apply.🤖 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/manage-sandboxes/messaging-channels.mdx` at line 87, The sentence containing `SLACK_ALLOWED_USERS` currently joins two independent clauses with a semicolon; split it into two separate sentences on separate lines in the docs source so each clause stands alone: one line stating "Set `SLACK_ALLOWED_USERS` to comma-separated Slack member IDs to authorize those users for DMs." and a second line stating "Set `SLACK_ALLOWED_USERS` to authorize users for channel `@mention` events in channels where the Slack app is present." Update the line that mentions `SLACK_ALLOWED_USERS` accordingly so each sentence is on its own line for clearer diffs.Source: Coding guidelines
test/hermes-gateway-wrapper.test.ts (1)
78-78: 💤 Low valueConsider using literal
:for PATH separator consistency.The test is correctly gated to Linux-only (line 35), and
path.delimiterwill be:on Linux. However, based on learnings, the established pattern in this repo's test suite is to use the literal POSIX separator:when constructing PATH in tests that run on Linux runners.♻️ Optional consistency improvement
- pathPrefix = `${evilBin}${path.delimiter}`; + pathPrefix = `${evilBin}:`;🤖 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 but the repo tests use the POSIX literal ':' for Linux-only tests; update the assignment that builds pathPrefix (where evilBin is concatenated) to use the literal ':' instead of path.delimiter so the test uses a consistent POSIX PATH separator (reference the pathPrefix variable and evilBin concatenation).Source: Learnings
test/e2e-scenario/docs/MIGRATION.md (1)
6-10: ⚡ Quick winFormat documentation with one sentence per line.
Multiple paragraphs in this file place several sentences on the same line, which makes diffs harder to review. As per coding guidelines for docs/**, "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
For example, lines 6-10 contain:
This file describes how to move coverage into the single Vitest E2E system without confusing that work with the retired typed-shell scenario runner or a second bash-driven harness. Vitest is the harness, GitHub Actions is the matrix, and NemoClaw fixtures may invoke real subprocess and system boundaries when those boundaries are the contract.This should be formatted as:
This file describes how to move coverage into the single Vitest E2E system without confusing that work with the retired typed-shell scenario runner or a second bash-driven harness. Vitest is the harness, GitHub Actions is the matrix, and NemoClaw fixtures may invoke real subprocess and system boundaries when those boundaries are the contract.The same issue appears throughout the file in lines 19-29, 31-36, 38-55, 58-70, 74-87, and others.
Also applies to: 19-29, 31-36, 38-55, 58-70, 74-87
🤖 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/docs/MIGRATION.md` around lines 6 - 10, The paragraphs in MIGRATION.md currently have multiple sentences on the same line (e.g., the paragraph beginning "This file describes how to move coverage into the single Vitest E2E system..." and the following sentence "Vitest is the harness, GitHub Actions is the matrix..."); fix by reflowing those paragraphs so each sentence is its own line throughout the document (apply to all similar paragraphs flagged in the review), preserving existing sentence order and punctuation and avoiding merging or splitting sentences—ensure every sentence ends with its punctuation and starts on a new line.Source: Coding guidelines
test/generate-openclaw-config-slack-allowlist.test.ts (1)
1-1: ⚡ Quick winConsider removing
@ts-nocheckand fixing underlying type issues.The
@ts-nocheckdirective disables all TypeScript checking for this file. This test appears straightforward and similar test files in the repo don't use this directive. Removing it would surface any type errors that should be addressed.🤖 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-line "// `@ts-nocheck`" and re-enable TypeScript for this test, then fix any surfaced type errors by adding explicit types or casts where needed (e.g., for mocked inputs, expected config objects, and helper functions used in the test). Locate and update the test declarations and any helper variables in this file (the test case that builds the OpenClaw Slack allowlist config) to use correct types or use "as" casts for third-party/mocked values, and add any missing imports or type annotations so the file passes TypeScript checks without disabling them.tools/pr-review-advisor/analyze.mts (1)
1004-1021: 💤 Low valueMinor: inconsistent verb tense in finding description.
Line 1013 uses past tense "adds" for the added case but present tense "modifies" for the modified case. For consistency, consider using present tense for both:
"adds"→"is adding"or"modifies"→"modified".🤖 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, In addDeterministicFindings, make the description tense consistent by updating the template that uses ledger.change (currently `This PR ${ledger.change === "added" ? "adds" : "modifies"} ${ledger.file}`) so both branches use the same tense (e.g., change to `This PR ${ledger.change === "added" ? "is adding" : "is modifying"} ${ledger.file}` or `This PR ${ledger.change === "added" ? "adds" : "modifies"} ${ledger.file}` depending on preferred style) and update any related strings (description, title, evidence if needed) to match.tools/e2e-scenarios/workflow-boundary.mts (1)
213-301: ⚡ Quick winConsider extracting shared workflow job validation logic.
validateOnboardNegativePathsVitestJobandvalidateOpenShellVersionPinVitestJobshare ~85 lines of nearly identical validation logic (job existence, runs-on, env checks, secret exposure guards, pinned actions, artifact config). Extracting a parameterized helper (e.g.,validateIndependentVitestJob(jobName, stepChecks)) would reduce duplication and make common contract changes easier to maintain.🤖 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 213 - 301, The two functions validateOnboardNegativePathsVitestJob and validateOpenShellVersionPinVitestJob duplicate large swaths of logic (job existence, runs-on, env vars, requireEnvDoesNotExposeSecret checks, action SHA enforcement via requireFullShaAction, and artifact upload validation); refactor by extracting a parameterized helper (e.g., validateIndependentVitestJob or validateVitestJobContract) that accepts the jobName and callbacks/flags for job-specific assertions (like required steps names and specific run/run-contains checks) and move common checks (runs-on, absence of needs/if, NEMOCLAW_RUN_E2E_SCENARIOS, E2E_ARTIFACT_DIR, secret guards, checkout/setup-node/upload validations) into that helper, then update both validateOnboardNegativePathsVitestJob and validateOpenShellVersionPinVitestJob to call the new helper with their specific step checks.
🤖 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 `@agents/hermes/Dockerfile`:
- Around line 105-109: Move the real hermes binary out of PATH and make the
wrapper exec it by absolute path: in the Dockerfile stop installing the real
binary as /usr/local/bin/hermes.real and instead place it at
/usr/local/lib/nemoclaw/hermes.real (ensure permissions), and update
agents/hermes/hermes-wrapper.sh to set
REAL_HERMES=/usr/local/lib/nemoclaw/hermes.real and exec "$REAL_HERMES" "$@"
(instead of relying on a PATH-resolvable hermes.real); this prevents bypassing
the wrapper (see cmdline_is_hermes_gateway) and ensures the wrapper always
invokes the non-PATH absolute binary.
In `@src/lib/actions/sandbox/policy-channel.ts`:
- Around line 511-514: The catch block that logs the Slack Socket Mode gateway
conflict failure should not "fail open" by returning true; instead preserve the
guard so adds don't proceed on transient verification failures unless the caller
explicitly passed the force override. Update the catch for the gateway
verification in policy-channel.ts (the try/catch that computes message from err
and logs with console.log(` ${YW}⚠${R} Could not verify Slack Socket Mode
gateway conflicts: ${message}`)) to either rethrow the error or return false by
default, and only swallow and allow progress when the caller's force flag is
true (log that the check was bypassed when force is used). Ensure the function
that performs the add respects this boolean/exception behavior so --force still
controls bypassing the verification.
In `@src/lib/onboard.ts`:
- Around line 2924-2935: The disabledChannels state is read before stale sandbox
rows are pruned, so a stale registry entry can cause old disabled-channel lists
to be reused; move the call to resolveDisabledChannels (and the creation of
disabledChannelNames) to after pruneStaleSandboxEntry(sandboxName) (or re-run
resolveDisabledChannels immediately after prune) so the disabled list is loaded
from the up-to-date registry before it is used by the token-filtering logic and
when calling enforceMessagingChannelConflicts; update references to
disabledChannelNames accordingly (functions: resolveDisabledChannels,
pruneStaleSandboxEntry, enforceMessagingChannelConflicts).
In `@src/lib/onboard/bridge-dns-preflight.ts`:
- Around line 437-440: Replace the copy-paste verification that calls
require('node:dns').resolve(...) with a command that uses dns.lookup so the
check follows the OS resolver (e.g., swap the resolve call in the console.error
block that references hostname to use require('node:dns').lookup or node's
dns.lookup in the inline one-liner). Update the second console.error string that
currently contains `require('node:dns').resolve('${hostname}', ...)` to call
`dns.lookup('${hostname}', ...)` (keeping the same throw/log behavior) so the
remediation check uses the system resolution path.
In `@src/lib/onboard/messaging-conflict-guard.ts`:
- Around line 108-116: The Slack-axis false negatives happen because
backfillMessagingChannels() is only called inside the hasPlanCredentials branch,
so legacy rows without entry.messaging.plan never get messagingChannels
populated and Slack gateway conflicts can be missed; fix this by invoking
backfillMessagingChannels(registry, probe) earlier (or unconditionally) before
any Slack gateway checks (e.g., before calling
findSlackSocketModeGatewayConflicts() / resolveActiveChannelsFromEntry() or
before findChannelConflictsFromPlan()), or alternatively make the Slack helper
(resolveActiveChannelsFromEntry()/findSlackSocketModeGatewayConflicts()) fall
back to using the probe when messagingChannels is absent; reference
createMessagingConflictProbe, backfillMessagingChannels,
findChannelConflictsFromPlan, findSlackSocketModeGatewayConflicts, and
resolveActiveChannelsFromEntry to locate where to move/duplicate the call.
In `@test/onboard-selection.test.ts`:
- Around line 135-140: The synthetic 401 branch only changes the HTTP status but
leaves the successful fixture in the response body, which can mask regressions;
when the branch that sets status="401" (the code that checks has_config and url,
used by startOllamaAuthProxy readiness probing) is taken, also overwrite the
probe output file (the same -o target written in other branches) with an
authentication-error body (e.g., a JSON error/unauthorized payload) so the
response body matches the 401 status; update the branch that sets status to
"401" to write this auth error body to the output file instead of the
success/tool-call fixture.
---
Outside diff comments:
In
@.agents/skills/nemoclaw-user-configure-security/references/best-practices.md:
- Around line 1-3: Add SPDX copyright and Apache-2.0 license headers to the top
of this Markdown file using HTML comments (e.g. <!-- SPDX-FileCopyrightText:
YEAR YourOrg --> and <!-- SPDX-License-Identifier: Apache-2.0 -->) so the file
containing the "# NemoClaw Security Best Practices" title and the import of
AgentOnly includes the required headers; apply the same pattern to all changed
files of their respective types (use HTML comments for .md and # comments for
YAML) and place them as the first lines before any content or imports.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 424-431: The current patch logic only checks for the exact literal
'mode: "trusted_env_proxy", auditContext: "cron-model-provider-preflight"' and
will blindly insert a second mode if a different mode already exists; update the
pre-patch checks in the block around patched_count/new_patched_count to first
grep for any occurrence of 'auditContext: "cron-model-provider-preflight"'
combined with a 'mode:' nearby (e.g., grep for 'mode:' within the same line or
context) and if such a mode exists and is not 'mode: "trusted_env_proxy"' call
patch_fail with a clear message, otherwise proceed with the existing
exact-literal insertion and verification logic; reference the
functions/variables patched_count, sed invocation that performs the replacement,
new_patched_count, and patch_fail when adding this protective branch.
In `@docs/manage-sandboxes/messaging-channels.mdx`:
- Line 87: The sentence containing `SLACK_ALLOWED_USERS` currently joins two
independent clauses with a semicolon; split it into two separate sentences on
separate lines in the docs source so each clause stands alone: one line stating
"Set `SLACK_ALLOWED_USERS` to comma-separated Slack member IDs to authorize
those users for DMs." and a second line stating "Set `SLACK_ALLOWED_USERS` to
authorize users for channel `@mention` events in channels where the Slack app is
present." Update the line that mentions `SLACK_ALLOWED_USERS` accordingly so
each sentence is on its own line for clearer diffs.
In `@test/e2e-scenario/docs/MIGRATION.md`:
- Around line 6-10: The paragraphs in MIGRATION.md currently have multiple
sentences on the same line (e.g., the paragraph beginning "This file describes
how to move coverage into the single Vitest E2E system..." and the following
sentence "Vitest is the harness, GitHub Actions is the matrix..."); fix by
reflowing those paragraphs so each sentence is its own line throughout the
document (apply to all similar paragraphs flagged in the review), preserving
existing sentence order and punctuation and avoiding merging or splitting
sentences—ensure every sentence ends with its punctuation and starts on a new
line.
In `@test/generate-openclaw-config-slack-allowlist.test.ts`:
- Line 1: Remove the top-line "// `@ts-nocheck`" and re-enable TypeScript for this
test, then fix any surfaced type errors by adding explicit types or casts where
needed (e.g., for mocked inputs, expected config objects, and helper functions
used in the test). Locate and update the test declarations and any helper
variables in this file (the test case that builds the OpenClaw Slack allowlist
config) to use correct types or use "as" casts for third-party/mocked values,
and add any missing imports or type annotations so the file passes TypeScript
checks without disabling them.
In `@test/hermes-gateway-wrapper.test.ts`:
- Line 78: The test sets pathPrefix using path.delimiter but the repo tests use
the POSIX literal ':' for Linux-only tests; update the assignment that builds
pathPrefix (where evilBin is concatenated) to use the literal ':' instead of
path.delimiter so the test uses a consistent POSIX PATH separator (reference the
pathPrefix variable and evilBin concatenation).
In `@tools/e2e-scenarios/workflow-boundary.mts`:
- Around line 213-301: The two functions validateOnboardNegativePathsVitestJob
and validateOpenShellVersionPinVitestJob duplicate large swaths of logic (job
existence, runs-on, env vars, requireEnvDoesNotExposeSecret checks, action SHA
enforcement via requireFullShaAction, and artifact upload validation); refactor
by extracting a parameterized helper (e.g., validateIndependentVitestJob or
validateVitestJobContract) that accepts the jobName and callbacks/flags for
job-specific assertions (like required steps names and specific run/run-contains
checks) and move common checks (runs-on, absence of needs/if,
NEMOCLAW_RUN_E2E_SCENARIOS, E2E_ARTIFACT_DIR, secret guards,
checkout/setup-node/upload validations) into that helper, then update both
validateOnboardNegativePathsVitestJob and validateOpenShellVersionPinVitestJob
to call the new helper with their specific step checks.
In `@tools/pr-review-advisor/analyze.mts`:
- Around line 1004-1021: In addDeterministicFindings, make the description tense
consistent by updating the template that uses ledger.change (currently `This PR
${ledger.change === "added" ? "adds" : "modifies"} ${ledger.file}`) so both
branches use the same tense (e.g., change to `This PR ${ledger.change ===
"added" ? "is adding" : "is modifying"} ${ledger.file}` or `This PR
${ledger.change === "added" ? "adds" : "modifies"} ${ledger.file}` depending on
preferred style) and update any related strings (description, title, evidence if
needed) to match.
🪄 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: fd8f1dba-0c07-4e23-9a64-46ee23184119
📒 Files selected for processing (143)
.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/e2e-vitest-scenarios.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/openclaw-config-merge.test.tssrc/lib/state/openclaw-config-merge.tssrc/lib/state/registry.tssrc/lib/state/sandbox.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-negative-paths.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/e2e-scenarios/workflow-boundary.mtstools/pr-review-advisor/analyze.mtsvitest.config.ts
💤 Files with no reviewable changes (2)
- test/e2e-scenario/framework-tests/e2e-migration-inventory.test.ts
- test/e2e-scenario/migration/legacy-inventory.json
✅ Files skipped from review due to trivial changes (25)
- test/e2e-scenario/live/registry-scenarios.test.ts
- test/e2e-scenario/fixtures/shell/trusted-command.ts
- src/lib/actions/gateway-drift-preflight.test.ts
- test/e2e-scenario/fixtures/secrets.ts
- test/e2e-scenario/fixtures/shell-probe.ts
- test/e2e-scenario/fixtures/phases/lifecycle.ts
- .agents/skills/nemoclaw-user-manage-sandboxes/references/backup-restore.md
- .agents/skills/nemoclaw-user-reference/references/network-policies.md
- test/e2e-scenario/live/ubuntu-repo-cli-smoke.test.ts
- test/e2e-scenario/fixtures/clients/provider.ts
- test/e2e-scenario/support-tests/e2e-fixture-context.test.ts
- test/e2e-scenario/fixtures/shell/supervisor.ts
- .agents/skills/nemoclaw-user-configure-inference/SKILL.md
- test/e2e-scenario/support-tests/e2e-phase-lifecycle.test.ts
- docs/inference/use-local-inference.mdx
- test/e2e-scenario/fixtures/availability-env.ts
- test/e2e-scenario/live/openshell-version-pin.test.ts
- .agents/skills/nemoclaw-user-overview/references/release-notes.md
- tools/e2e-advisor/scenarios-schema.json
- docs/reference/commands-nemohermes.mdx
- test/e2e-scenario/fixtures/phases/state-validation.ts
- test/e2e-scenario/docs/RETIREMENT.md
- test/e2e-scenario/docs/README.md
- test/e2e-scenario/scenarios/types.ts
- docs/about/release-notes.mdx
This was referenced Jun 11, 2026
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, free-standing Vitest live test for the first contract from
test/e2e/test-onboard-negative-paths.sh: invalid NVIDIA API key handling during non-interactive onboard.This is intentionally not a registry-driven bridge and does not retire the legacy bash script. Per #5126 and #5098 migration guidance, this PR keeps the scope narrow and preserves the real CLI/onboard boundary from Vitest instead of expanding typed registry/state-validation machinery for one script migration.
Scope
Covered in this PR:
invalid NVIDIA key exits non-zeroinvalid NVIDIA key message is explicit(Invalid NVIDIA API key+Must start with nvapi-)invalid NVIDIA key no stack traceNot covered / left for follow-up slices:
ubuntu-invalid-nvidia-key-negative,ubuntu-gateway-port-conflict-negative, orubuntu-repo-cloud-openclaw-custom-policiesShape
test/e2e-scenario/live/onboard-negative-paths.test.tsas a direct live Vitest test.onboard-negative-paths-vitestto.github/workflows/e2e-vitest-scenarios.yamlas a free-standing job.Simplicity check
Verification
npm run build:clinpm run typecheck:clinpx vitest run --project cli test/e2e-scenario/live/onboard-negative-paths.test.ts --silent=false --reporter=defaultNEMOCLAW_RUN_E2E_SCENARIOS=1 npx vitest run --project e2e-scenarios-live test/e2e-scenario/live/onboard-negative-paths.test.ts --silent=false --reporter=defaultnpx vitest run --project e2e-vitest-support test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts --silent=false --reporter=defaultRefs #5098, #2573
Summary by CodeRabbit
Release Notes