test(e2e): migrate test-hermes-discord-e2e.sh to vitest - #5610
Merged
Conversation
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 (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds a new live Vitest e2e scenario ( ChangesHermes Discord Live E2E Vitest Scenario
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
![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)
test/e2e-scenario/live/hermes-discord.test.ts (1)
633-635: 🧹 Nitpick | 🔵 TrivialConsider using a clearer assertion pattern for better readability.
The current code uses
expect([200, 401], message).toContain(statusCode), which is valid Vitest syntax (the second argument is a custom error message). However, this pattern reads counterintuitively since it checks if the expected-values array contains the actual status code, rather than checking if the status code is in the allowed values.For improved clarity, consider using
.includes()with a boolean assertion:💡 Suggested refactor
- expect([200, 401], `Unexpected Discord users/@me response: ${discordApi.stdout}`).toContain( - discordApiResult.statusCode, - ); + expect( + [200, 401].includes(discordApiResult.statusCode!), + `Unexpected Discord users/@me response (got ${discordApiResult.statusCode}): ${discordApi.stdout}`, + ).toBe(true);This makes the intent more explicit: validating that the status code is one of the allowed values.
🤖 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/hermes-discord.test.ts` around lines 633 - 635, The assertion for validating the Discord API response status code in the discordApiResult check uses toContain() in a counterintuitive way. Replace the current pattern where the array of expected status codes is passed to expect() with toContain checking against the actual status code. Instead, call includes() directly on the array of allowed status codes to check if discordApiResult.statusCode is one of the permitted values, then use toBe(true) to assert the result is truthy. This makes the validation logic more explicit and easier to understand - it clearly shows you are checking if the status code is in the allowed values list.
🤖 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 @.github/workflows/e2e-vitest-scenarios.yaml:
- Around line 1663-1668: The "Clean up Docker auth" step uses
`"${DOCKER_CONFIG}"` in the rm -rf command which will fail under `set -u` if
DOCKER_CONFIG is unset. Replace the variable expansion with a defaulted
expansion syntax `"${DOCKER_CONFIG:-}"` to ensure the variable safely expands to
an empty string if unset, preventing the script from failing during cleanup.
In `@test/e2e-scenario/live/hermes-discord.test.ts`:
- Line 441: The codebase growth guardrail flagged 4 conditional statements in
the test file that exceeded acceptable limits. To resolve this, either extract
the conditional logic from the test body into helper functions outside the test
(such as a helper function for the health polling break condition at line 441, a
handleDiscordApiResult or similar function for the timeout/error differentiation
at lines 626-636, and a conditionalCleanup function for the
NEMOCLAW_E2E_KEEP_SANDBOX check at line 706), or alternatively add documentation
comments explaining why each conditional is necessary for the live E2E test
orchestration and request a guardrail exemption from the team for live E2E
scenario test files.
---
Nitpick comments:
In `@test/e2e-scenario/live/hermes-discord.test.ts`:
- Around line 633-635: The assertion for validating the Discord API response
status code in the discordApiResult check uses toContain() in a counterintuitive
way. Replace the current pattern where the array of expected status codes is
passed to expect() with toContain checking against the actual status code.
Instead, call includes() directly on the array of allowed status codes to check
if discordApiResult.statusCode is one of the permitted values, then use
toBe(true) to assert the result is truthy. This makes the validation logic more
explicit and easier to understand - it clearly shows you are checking if the
status code is in the allowed values list.
🪄 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: f340c8eb-eb33-427c-8a16-9cac2d8e2de1
📒 Files selected for processing (3)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/live/hermes-discord.test.tstools/e2e-scenarios/workflow-boundary.mts
Comment on lines
+1663
to
+1668
| - name: Clean up Docker auth | ||
| if: always() | ||
| run: | | ||
| set -euo pipefail | ||
| docker logout docker.io || true | ||
| rm -rf "${DOCKER_CONFIG}" |
Contributor
There was a problem hiding this comment.
Guard DOCKER_CONFIG expansion in cleanup path.
Line 1668 can fail under set -u when DOCKER_CONFIG is unset (for example, if earlier steps didn’t export $GITHUB_ENV). Use a defaulted expansion before rm -rf to keep always() cleanup robust.
Suggested fix
- name: Clean up Docker auth
if: always()
run: |
set -euo pipefail
docker logout docker.io || true
- rm -rf "${DOCKER_CONFIG}"
+ if [[ -n "${DOCKER_CONFIG:-}" ]]; then
+ rm -rf "${DOCKER_CONFIG}"
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Clean up Docker auth | |
| if: always() | |
| run: | | |
| set -euo pipefail | |
| docker logout docker.io || true | |
| rm -rf "${DOCKER_CONFIG}" | |
| - name: Clean up Docker auth | |
| if: always() | |
| run: | | |
| set -euo pipefail | |
| docker logout docker.io || true | |
| if [[ -n "${DOCKER_CONFIG:-}" ]]; then | |
| rm -rf "${DOCKER_CONFIG}" | |
| fi |
🤖 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 1663 - 1668, The
"Clean up Docker auth" step uses `"${DOCKER_CONFIG}"` in the rm -rf command
which will fail under `set -u` if DOCKER_CONFIG is unset. Replace the variable
expansion with a defaulted expansion syntax `"${DOCKER_CONFIG:-}"` to ensure the
variable safely expands to an empty string if unset, preventing the script from
failing during cleanup.
| redactionValues, | ||
| timeoutMs: 20_000, | ||
| }); | ||
| if (health.exitCode === 0 && /"ok"/i.test(resultText(health))) break; |
Contributor
There was a problem hiding this comment.
Address pipeline failure: codebase growth guardrails flagged conditional logic.
The CI check test-conditionals:scan failed due to 4 if statements in this test file. The conditionals are:
- Line 441: Health polling break condition
- Lines 626-636: Discord API timeout/error differentiation
- Line 706:
NEMOCLAW_E2E_KEEP_SANDBOXconditional cleanup
For a live E2E test with phased orchestration, some conditional logic is necessary. Consider either:
- Option A: Extract conditionals into helper functions outside the test body (e.g.,
handleDiscordApiResult(...)andconditionalCleanup(...)) - Option B: Document why these conditionals are necessary and request a guardrail exemption for live E2E scenario tests
The timeout-to-skip pattern at lines 626-630 explicitly matches legacy shell script behavior per the PR description, which is a valid justification.
Also applies to: 626-636, 706-737
🤖 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/hermes-discord.test.ts` at line 441, The codebase
growth guardrail flagged 4 conditional statements in the test file that exceeded
acceptable limits. To resolve this, either extract the conditional logic from
the test body into helper functions outside the test (such as a helper function
for the health polling break condition at line 441, a handleDiscordApiResult or
similar function for the timeout/error differentiation at lines 626-636, and a
conditionalCleanup function for the NEMOCLAW_E2E_KEEP_SANDBOX check at line
706), or alternatively add documentation comments explaining why each
conditional is necessary for the live E2E test orchestration and request a
guardrail exemption from the team for live E2E scenario test files.
Source: Pipeline failures
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 1 item to resolve/justify, 0 in-scope improvements
|
Contributor
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 27983197863
|
Contributor
Vitest E2E Scenario Results —
|
| Job | Result |
|---|---|
| agent-turn-latency-vitest | |
| bedrock-runtime-compatible-anthropic-vitest | |
| brave-search-vitest | |
| channels-add-remove-vitest | |
| channels-stop-start-vitest | |
| cloud-inference-vitest | |
| cloud-onboard-vitest | |
| common-egress-agent-vitest | |
| concurrent-gateway-ports-vitest | |
| credential-migration-vitest | |
| credential-sanitization-vitest | |
| cron-preflight-inference-local-vitest | |
| device-auth-health-vitest | |
| diagnostics-vitest | |
| double-onboard-vitest | |
| full-e2e-vitest | |
| gateway-drift-preflight-vitest | |
| gateway-guard-recovery | |
| gateway-health-honest-vitest | |
| gpu-double-onboard-vitest | |
| gpu-e2e-vitest | |
| hermes-discord-vitest | |
| hermes-e2e-vitest | |
| hermes-inference-switch-vitest | |
| hermes-root-entrypoint-smoke-vitest | |
| inference-routing-vitest | |
| issue-2478-crash-loop-recovery-vitest | |
| issue-4434-tui-unreachable-inference-vitest | |
| issue-4462-scope-upgrade-approval-vitest | |
| kimi-inference-compat-vitest | |
| launchable-smoke-vitest | |
| live-scenarios | |
| messaging-compatible-endpoint-vitest | |
| messaging-providers-vitest | |
| model-router-provider-routed-inference-vitest | |
| network-policy-vitest | |
| ollama-auth-proxy-vitest | |
| onboard-negative-paths-vitest | |
| onboard-repair-vitest | |
| onboard-resume-vitest | |
| openclaw-discord-pairing-vitest | |
| openclaw-inference-switch-vitest | |
| openclaw-skill-cli-vitest | |
| openclaw-slack-pairing-vitest | |
| openclaw-tui-chat-correlation-vitest | |
| openshell-version-pin-vitest | |
| rebuild-hermes-stale-base-vitest | |
| rebuild-hermes-vitest | |
| rebuild-openclaw-vitest | |
| runtime-overrides-vitest | |
| sandbox-rebuild-vitest | |
| sandbox-survival-vitest | |
| sessions-agents-cli-vitest | |
| shields-config-vitest | |
| skill-agent-vitest | |
| snapshot-commands-vitest | |
| state-backup-restore-vitest | |
| telegram-injection-vitest | |
| token-rotation-vitest | |
| tunnel-lifecycle-vitest | |
| upgrade-stale-sandbox-vitest |
Contributor
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 27983713604
|
Contributor
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 27995992856
|
This was referenced Jun 23, 2026
79 tasks
This was referenced Jun 25, 2026
wscurran
added
area: e2e
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Migrate
test-hermes-discord-e2e.shwith equivalent live Vitest coverage intest/e2e-scenario/live/hermes-discord.test.ts.Legacy shell deletion and nightly shell lane retirement are deferred to #5098 Phase 11.
Related Issues
Refs #5098
Refs #3032
Assertion parity
NEMOCLAW_NON_INTERACTIVE=1; third-party software acceptance is set; hosted inference key is present.dockerInfo(...)exit zero plus env expectations andsecrets.required("NVIDIA_INFERENCE_API_KEY").coveredcleanupHermesDiscord(..., "preclean-hermes-discord")calls realnemoclaw destroy,openshell sandbox delete, andopenshell gateway destroy.coveredbash install.sh --non-interactivesucceeds withNEMOCLAW_AGENT=hermes, Discord env enabled, policy tier open, fresh/recreated sandbox, hosted CI inference.host.command("bash", ["install.sh", "--non-interactive"], ...)exit zero with Hermes Discord env.coverednemoclawandopenshellare on PATH after install.bash -lc "command -v nemoclaw && openshell --version"exit zero and stdout containsnemoclaw.coverednemoclaw listcontains the Hermes Discord sandbox.host.command("nemoclaw", ["list"])exit zero and output containsSANDBOX_NAME.covered${SANDBOX_NAME}-discord-bridgeexists.openshell provider get ${SANDBOX_NAME}-discord-bridgeexit zero.covered"ok"within 15 attempts.curl -sf http://localhost:8642/health, then assert exit zero and"ok".covered/sandbox/.hermes/config.yamlhas top-leveldiscord, expected booleans/defaults,platforms.discord.enabled=true,platforms.api_server, and noDISCORD_BOT_TOKENliteral.DISCORD_BOT_TOKENappears.covered/sandbox/.hermes/.envcontains Discord placeholder, guild IDs, allowed users, andAPI_SERVER_PORT=18642..envlines with normalized IDs.coveredstartHermesFakeDiscordGateway(...)startsfake-discord-gateway.cjsin Docker and returns a published port/capture file.coveredapplyHermesFakeDiscordPolicy(...)runsopenshell policy updatewith websocket rewrite, allowed methods/IPs, and node/python/Hermes binary allowlist; exit zero.coveredrunHermesPythonDiscordGatewayProof(...)runs/opt/hermes/.venv/bin/pythonin sandbox and asserts all protocol markers plus absence ofIMPORT_DISCORD_FAILED.coveredassertDiscordGatewayCapture(...)assertstokenMatchesExpected=true,tokenLooksPlaceholder=false, and capture does not contain raw token.covered/sandbox/.hermes/config.yamland.env.assertRawTokenAbsentFromFiles(...)decodes token in sandbox and greps both files, expectingOK.coveredDISCORD_PROXYenv remains.rawTokenSurfaceProbe(..., "env")expectsABSENT; emits distinct failures for token orDISCORD_PROXY.coveredrawTokenSurfaceProbe(..., "process")scans/proc/*/cmdline, expectingABSENT./procprocess boundary.covered/sandbox,/home,/etc,/tmp,/var.rawTokenSurfaceProbe(..., "filesystem")recursive grep expectsABSENT.covered/api/v10/users/@mereaches Discord with configured placeholder; 200/401 pass, timeout is explicitly skipped, other errors fail..hermes/.env, calls Discord, accepts 200/401, writes timeout skip artifact, fails other errors.coveredDISCORD_PROXYresidue in env, files,/tmp, binaries, or process list.phase-7-no-local-discord-bridgeshell probe reproduces legacy env/file/tmp/binary/process checks and expects empty stdout.covereddocker rm -f,fs.rmSync(fakeGateway.dir), and repo.tmp/fake-discord.*cleanup.coverednemoclaw <sandbox> rebuild --yessucceeds afterNVIDIA_INFERENCE_API_KEY,NVIDIA_API_KEY, andCOMPATIBLE_API_KEYare unset; output does not containprovider credential not found.phase-8-rebuild-without-inference-envdeletes credential env keys, asserts exit zero and no provider-credential error.coveredNEMOCLAW_E2E_KEEP_SANDBOX=1.nemoclaw destroy --yes, gateway destroy, and registry grep expectingABSENT.coveredAll legacy assertions are covered or intentionally stronger in Vitest. No row is
missing,partial, orcandidate only.Contract mapping
test/e2e-scenario/live/hermes-discord.test.tsassertions A1-A22.bash install.sh, Docker, OpenShell provider/policy, sandbox exec, Hermes Python runtime, fake Discord Gateway,/proc, filesystem scans, Discord REST, andnemoclaw rebuild.Simplicity check
.github/workflows/nightly-e2e.yamljobhermes-discord-e2e, reusablee2e-script.yaml, defaultubuntu-latest, 60 minutes, Docker/OpenShell/Hermes sandbox,NVIDIA_INFERENCE_API_KEY, fake Discord token/env,github_token: true..github/workflows/e2e-vitest-scenarios.yamljobhermes-discord-vitest,ubuntu-latest, 75 minutes, same Docker/OpenShell/Hermes/fake Discord boundaries andNVIDIA_INFERENCE_API_KEY.phase6-messaging-helpers/ fake Discord Docker helper.hermes-discord-vitestfree-standing Vitest job and workflow-boundary selector validation.gh workflow run e2e-vitest-scenarios.yaml --repo NVIDIA/NemoClaw --ref e2e-migrate-test-hermes-discord-vitest -f jobs=hermes-discord-vitest -f pr_number=<PR>.Pre-push parity gate
test/e2e/test-hermes-discord-e2e.sh222200yes — nightly-e2e hermes-discord-e2e ubuntu-latest -> e2e-vitest-scenarios hermes-discord-vitest ubuntu-latestVerification
NEMOCLAW_RUN_E2E_SCENARIOS=1 npx vitest run --project e2e-scenarios-live test/e2e-scenario/live/hermes-discord.test.ts --silent=false --reporter=default(local no-secret gate skips cleanly)npx tsx -e "import {validateE2eVitestScenariosWorkflowBoundary,evaluateE2eVitestWorkflowDispatchSelectors} from './tools/e2e-scenarios/workflow-boundary.mts'; const errors=validateE2eVitestScenariosWorkflowBoundary(); if (errors.length) { console.error(JSON.stringify(errors,null,2)); process.exit(1); } const evalResult=evaluateE2eVitestWorkflowDispatchSelectors({scenarios:'hermes-discord'}); if (!evalResult.valid || evalResult.liveScenariosRuns || evalResult.selectedFreeStandingJobs[0] !== 'hermes-discord-vitest') { console.error(JSON.stringify(evalResult,null,2)); process.exit(1); } console.log('workflow boundary ok');"npm run build:cligit diff --check && git diff --cached --checkin_progress)Summary by CodeRabbit