ci: add VPN nightly E2E suite - #5746
Closed
ericksoa wants to merge 2 commits into
Closed
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds a reusable VPN E2E composite action and workflow, plus shared helpers, fake APIs, and many new cloud, GPU, channel, credential, gateway, and messaging E2E scripts. ChangesVPN E2E Infrastructure and Test Suite
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested labels
Suggested reviewers
🚥 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
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: 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: None Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
PR Review Advisor — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
![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
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (26)
.github/actions/run-e2e-script-vpn/action.yaml-99-138 (1)
99-138: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDrop the unused
always-artifact-pathgate from the sanitized upload flow.Lines 101 and 133 still require
inputs.always-artifact-path != '', but the sanitized artifact upload only usessteps.sanitize-trace-artifacts.outputs.summary-file. Callers that set the trace source and artifact name, but leave the documented legacy path empty, will silently skip sanitization and upload.🤖 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/actions/run-e2e-script-vpn/action.yaml around lines 99 - 138, The sanitized E2E artifact flow is still gated on inputs.always-artifact-path even though the sanitize-trace-artifacts and Upload E2E artifacts steps only use inputs.always-artifact-name, inputs.always-artifact-trace-source-path, and steps.sanitize-trace-artifacts.outputs.summary-file. Remove the always-artifact-path check from the if conditions in the sanitize-trace-artifacts and upload-artifact steps so callers that provide the trace source and name are not skipped unnecessarily.test/e2e-vpn/lib/openclaw-json.sh-23-27 (1)
23-27: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNarrow this parser to assistant payload text.
This collector walks arbitrary
content/textfields undermessages,choices,response, and similar containers, so a prompt echo or other non-assistant text can satisfy the assertion even when the assistant reply is missing. That also diverges fromtest/e2e-vpn/lib/openclaw-agent-json.py, which only readspayloads[].text.The safest fix is to have this shell helper delegate to the Python helper instead of maintaining a broader second parser.
Suggested fix
parse_openclaw_agent_text() { - python3 -c ' -... -' + python3 "$(dirname "${BASH_SOURCE[0]}")/openclaw-agent-json.py" }Also applies to: 51-88
🤖 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-vpn/lib/openclaw-json.sh` around lines 23 - 27, The openclaw JSON helper is parsing broad text fields from generic containers, which can let non-assistant text satisfy the check; narrow this in openclaw-json.sh by delegating parsing to the same logic used in openclaw-agent-json.py so only assistant payload text is considered. Update the shell helper to call the Python helper (or otherwise reuse its payloads[].text handling) and remove the broader TEXT_KEYS/CONTAINER_KEYS traversal to keep both parsers aligned.test/e2e-vpn/e2e-cloud-experimental/features/skill/verify-sandbox-skill-via-agent.sh-74-79 (1)
74-79: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not splice
SESSION_IDdirectly into the remote shell command.
SKILL_VERIFY_SESSION_IDis a caller-controlled value, but here it is embedded inside single quotes inremote_cmd. A session id containing'breaks the command and can inject extra shell syntax on the sandbox side. Encode or quote it the same way as the prompt/API key before passing it over SSH.Suggested fix
+session_b64=$(printf '%s' "$SESSION_ID" | base64 | tr -d '\n') ... -remote_cmd="pm=\$(printf '%s' '${prompt_b64}' | base64 -d) || exit 1; nv=\$(printf '%s' '${nv_b64}' | base64 -d) || exit 1; export NVIDIA_API_KEY=\"\$nv\"; ${_lock_rm}${AGENT_LAUNCHER}openclaw agent --agent main --local -m \"\$pm\" --session-id '${SESSION_ID}'" +remote_cmd="pm=\$(printf '%s' '${prompt_b64}' | base64 -d) || exit 1; nv=\$(printf '%s' '${nv_b64}' | base64 -d) || exit 1; sid=\$(printf '%s' '${session_b64}' | base64 -d) || exit 1; export NVIDIA_API_KEY=\"\$nv\"; ${_lock_rm}${AGENT_LAUNCHER}openclaw agent --agent main --local -m \"\$pm\" --session-id \"\$sid\""🤖 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-vpn/e2e-cloud-experimental/features/skill/verify-sandbox-skill-via-agent.sh` around lines 74 - 79, The remote shell command in verify-sandbox-skill-via-agent.sh is interpolating SESSION_ID directly into remote_cmd, which can break quoting and allow shell injection. Update the command construction to pass SKILL_VERIFY_SESSION_ID safely using the same encoding/quoting approach already used for prompt_b64 and nv_b64, and keep the fix localized to the remote_cmd assembly and any helper handling around _lock_rm/AGENT_LAUNCHER.test/e2e-vpn/e2e-cloud-experimental/check-docs.sh-354-360 (1)
354-360: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail when the command section cannot be isolated.
Falling back to the full
commands.mdxbody reintroduces the cross-section false positives this phase is trying to avoid, so a broken extractor can silently pass missing per-command flags. Treat an empty section as drift instead of searching the whole document.Suggested fix
- if [[ -z "$_section" ]]; then - # Phase 2 already enforces the heading exists; if the section is - # somehow empty here, fall back to the full doc rather than skipping. - _section="$(cat "$COMMANDS_MD")" - fi + if [[ -z "$_section" ]]; then + echo "check-docs: [cli] could not isolate the '$cmd_line' section in $COMMANDS_MD" >&2 + _flag_drift=1 + continue + 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 `@test/e2e-vpn/e2e-cloud-experimental/check-docs.sh` around lines 354 - 360, The fallback in the check-docs.sh section extraction logic should be removed because it lets a broken extractor hide missing command-specific flags by scanning the full commands.mdx document. In the section handling around extract_md_section and COMMANDS_MD, fail the check when _section is empty instead of assigning cat "$COMMANDS_MD"; keep the phase 2 heading validation separate and treat an empty isolated section as drift.test/e2e-vpn/e2e-cloud-experimental/features/skill/verify-sandbox-skill-via-agent.sh-107-111 (1)
107-111: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail the test when the agent command exits non-zero.
This currently passes as soon as the token appears anywhere in output, even if
openclaw agentfailed afterward. That can turn real turn failures into false-green E2E results.Suggested fix
collapsed_out=$(printf '%s' "$raw_out" | tr -d '\n\r') -if printf '%s' "$collapsed_out" | grep -Fq "$VERIFY_TOKEN"; then +if [[ "$agent_rc" -eq 0 ]] && printf '%s' "$collapsed_out" | grep -Fq "$VERIFY_TOKEN"; then ok "agent output contains ${VERIFY_TOKEN}" exit 0 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 `@test/e2e-vpn/e2e-cloud-experimental/features/skill/verify-sandbox-skill-via-agent.sh` around lines 107 - 111, The verification in verify-sandbox-skill-via-agent.sh currently treats matching VERIFY_TOKEN as success even if openclaw agent exits with a non-zero status. Update the script so the final result depends on the agent command exit code as well as the token check: capture and inspect the exit status from the openclaw agent invocation, and only call ok/exit 0 when the command succeeds and the token is present. Keep the token search logic around collapsed_out and VERIFY_TOKEN, but ensure any non-zero agent failure is reported as test failure instead of a false green.test/e2e-vpn/e2e-cloud-experimental/skip/README.md-1-10 (1)
1-10: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd the required SPDX header to this Markdown file.
New Markdown files in this repo need the SPDX copyright/license block in HTML comments, and this file currently has none.
Suggested fix
+<!-- SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. --> +<!-- SPDX-License-Identifier: Apache-2.0 --> + # Opt-in checks (`skip/`)As per coding guidelines,
**/*.{md,mdx}: Use HTML comments for SPDX headers in Markdown files.🤖 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-vpn/e2e-cloud-experimental/skip/README.md` around lines 1 - 10, This Markdown README is missing the required SPDX copyright/license header. Add the standard SPDX block in HTML comments at the top of the file before the existing “Opt-in checks (`skip/`)” content, following the repo’s Markdown convention for new .md/.mdx files.Source: Coding guidelines
test/e2e-vpn/e2e-cloud-experimental/test-port8080-conflict.sh-54-84 (1)
54-84: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd cleanup traps before tearing down the gateway.
After
openshell gateway destroy, every earlyexit 1before Line 109 leaves the host gateway down, and failures in this section can also leave the dummyhttp.serverholding port 8080. That will poison later E2E phases and local reruns.Suggested fix
+occupier_pid="" +gateway_destroyed=0 +cleanup() { + if [ -n "${occupier_pid:-}" ]; then + kill "$occupier_pid" 2>/dev/null || true + wait "$occupier_pid" 2>/dev/null || true + fi + if [ "$gateway_destroyed" -eq 1 ]; then + openshell gateway start --name nemoclaw >/dev/null 2>&1 || true + openshell forward start --background 18789 "$SANDBOX_NAME" 2>/dev/null || true + fi +} +trap cleanup EXIT + INFO "Stopping nemoclaw gateway so we can bind a non-OpenShell process on 8080..." openshell forward stop 18789 2>/dev/null || true openshell gateway destroy -g nemoclaw 2>/dev/null || true +gateway_destroyed=1 sleep 3🤖 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-vpn/e2e-cloud-experimental/test-port8080-conflict.sh` around lines 54 - 84, Add cleanup traps before destroying the gateway so failures in this section don’t leave the environment broken. In test-port8080-conflict.sh, wrap the teardown/port-occupier setup around a trap that restores the nemoclaw gateway and kills/waits on the dummy http.server using occupier_pid, and make sure all early exits in the preflight block still run that cleanup. Keep the logic anchored around openshell gateway destroy, occupier_pid, and the later exit 1 paths so later E2E phases aren’t poisoned.test/e2e-vpn/e2e-cloud-experimental/checks/03-security-checks.sh-28-50 (1)
28-50: 🔒 Security & Privacy | 🟠 MajorPrefer
ps auxewwbeforeps auxww.ps auxwwcan satisfy the fallback without including environment data, so theNAME=<prefix>scan can miss env-style API key leaks on GNU/Linux.Suggested fix
-ps_lines=$( (ps auxww 2>/dev/null || ps auxeww 2>/dev/null || ps aux 2>/dev/null) || true) +ps_lines=$( (ps auxeww 2>/dev/null || ps auxww 2>/dev/null || ps aux 2>/dev/null) || 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 `@test/e2e-vpn/e2e-cloud-experimental/checks/03-security-checks.sh` around lines 28 - 50, The ps output capture in the security check should prefer the variant that includes environment data before the fallback that may omit it. Update the ps command order in the security-check script so the scan runs against the most informative output first, then keep the existing full-key and argv-style leak checks using _api_key_value, _key_argv_prefix_marker, and _key_argv_needle to detect env-style API key exposure reliably.test/e2e-vpn/e2e-cloud-experimental/features/skill/add-sandbox-skill.sh-67-87 (1)
67-87: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEscape
SKILL_DESCRIPTIONbefore writing YAML front matter.
SKILL_DESCRIPTIONis interpolated verbatim into quoted YAML here and in the template-render branch. A description containing"or a newline produces an invalidSKILL.md, which then breaks downstream skill loading/querying for otherwise valid input. Serialize these fields with proper escaping, or reject unsupported characters before writing the file.🤖 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-vpn/e2e-cloud-experimental/features/skill/add-sandbox-skill.sh` around lines 67 - 87, The YAML front matter written by add-sandbox-skill.sh does not escape SKILL_DESCRIPTION before interpolating it into SKILL.md, so quotes or newlines can make the file invalid. Update both branches that generate payload_source (the inline YAML block and the python3 template rendering) to serialize SKILL_ID and SKILL_DESCRIPTION safely with proper YAML escaping, or validate and reject unsupported characters before writing.test/e2e-vpn/e2e-cloud-experimental/features/skill/lib/README.md-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd the required Markdown SPDX header.
Line 1 starts the README without the mandatory HTML SPDX block, so this file is missing the repository-required license header. As per coding guidelines, "Use HTML comments for SPDX headers in Markdown files".
🤖 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-vpn/e2e-cloud-experimental/features/skill/lib/README.md` at line 1, The README is missing the required Markdown SPDX license header; add the repository-mandated HTML comment SPDX block at the top of this file. Update the opening of the document in the README so it follows the standard Markdown license-header pattern used elsewhere in the repo, keeping the existing content like the “E2E helper libraries” heading intact.Source: Coding guidelines
test/e2e-vpn/e2e-cloud-experimental/features/skill/fixtures/skill-smoke-template.SKILL.md-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd the required Markdown SPDX header.
Line 1 starts the fixture content without the mandatory HTML SPDX block, so this file is missing the repository-required license header. As per coding guidelines, "Use HTML comments for SPDX headers in Markdown files".
🤖 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-vpn/e2e-cloud-experimental/features/skill/fixtures/skill-smoke-template.SKILL.md` at line 1, The Markdown fixture is missing the required SPDX license header, so update the file’s opening content to include the repository-standard HTML comment SPDX block before any fixture text. Use the Markdown fixture’s top-of-file content in skill-smoke-template.SKILL.md and add the SPDX header in the format expected for Markdown files, keeping the rest of the fixture unchanged.Source: Coding guidelines
test/e2e-vpn/test-full-e2e.sh-363-365 (1)
363-365: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the configured hosted-inference model here instead of a second hardcoded one.
These sandbox routing probes currently bypass
HOSTED_INFERENCE_MODELand pinnvidia/nemotron-3-super-120b-a12b. Ifnemoclaw_e2e_hosted_inference_model()changes, Phase 4b can fail even though onboard configured inference correctly and the direct API check in 4a passed against the helper-selected model.Suggested fix
- "curl -s --max-time 60 https://inference.local/v1/chat/completions \ - -H 'Content-Type: application/json' \ - -d '{\"model\":\"nvidia/nemotron-3-super-120b-a12b\",\"messages\":[{\"role\":\"user\",\"content\":\"Reply with exactly one word: PONG\"}],\"max_tokens\":100}'" \ + "curl -s --max-time 60 https://inference.local/v1/chat/completions \ + -H 'Content-Type: application/json' \ + -d '{\"model\":\"${HOSTED_INFERENCE_MODEL}\",\"messages\":[{\"role\":\"user\",\"content\":\"Reply with exactly one word: PONG\"}],\"max_tokens\":100}'" \- "curl -v --max-time 60 https://inference.local/v1/chat/completions \ - -H 'Content-Type: application/json' \ - -d '{\"model\":\"nvidia/nemotron-3-super-120b-a12b\",\"messages\":[{\"role\":\"user\",\"content\":\"Reply with exactly one word: PONG\"}],\"max_tokens\":100}'" \ + "curl -v --max-time 60 https://inference.local/v1/chat/completions \ + -H 'Content-Type: application/json' \ + -d '{\"model\":\"${HOSTED_INFERENCE_MODEL}\",\"messages\":[{\"role\":\"user\",\"content\":\"Reply with exactly one word: PONG\"}],\"max_tokens\":100}'" \Also applies to: 399-401
🤖 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-vpn/test-full-e2e.sh` around lines 363 - 365, The sandbox routing probe in the test script is hardcoding the hosted-inference model instead of using the configured helper-selected value, which can break when the configured model changes. Update the Phase 4b curl payload to reuse the same model source used elsewhere in the script, specifically the value returned by nemoclaw_e2e_hosted_inference_model() / HOSTED_INFERENCE_MODEL, so the direct API check and routing probe stay consistent. Apply the same change to both occurrences referenced by the review comment.test/e2e-vpn/test-device-auth-health.sh-281-285 (1)
281-285: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t treat
401on/healthas a passing result.This file’s contract is that
/healthstays auth-free and returns200. Accepting401in the host probe and restart probe turns the regression test green even if auth has leaked onto the health endpoint again.Also applies to: 328-333
🤖 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-vpn/test-device-auth-health.sh` around lines 281 - 285, Update the host and restart health checks in test-device-auth-health.sh so they only pass when the dashboard /health endpoint returns 200. Remove 401 from the accepted statuses in the HOST_HEALTH_CODE probe and the corresponding restart probe, and keep the pass condition aligned with the health endpoint contract in the existing health-check logic.test/e2e-vpn/test-concurrent-gateway-ports.sh-79-82 (1)
79-82: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCleanup leaks sandbox A and its gateway.
This script only tears down the fake inference server. On a passing path it leaves
e2e-cgp-arunning, which can hold the default gateway/dashboard ports and poison the next nightly E2E on the same runner.Suggested fix
cleanup() { stop_fake_openai_compatible_api + NEMOCLAW_NON_INTERACTIVE=1 timeout 300 "${NEMOCLAW_CMD[@]}" "${SANDBOX_B}" destroy --yes >/dev/null 2>&1 || true + NEMOCLAW_NON_INTERACTIVE=1 timeout 300 "${NEMOCLAW_CMD[@]}" "${SANDBOX_A}" destroy --yes >/dev/null 2>&1 || true rm -f "$FAKE_OPENAI_LOG" }🤖 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-vpn/test-concurrent-gateway-ports.sh` around lines 79 - 82, The cleanup routine only stops the fake inference server, so the sandbox and gateway started by the concurrent gateway ports test can survive and block later runs. Update cleanup() in test-concurrent-gateway-ports.sh to also tear down the e2e-cgp-a sandbox and its gateway using the same setup/teardown helpers used to start it, alongside stop_fake_openai_compatible_api, so all test-created resources are removed on exit.test/e2e-vpn/test-docs-validation.sh-60-63 (1)
60-63: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe repo-root probe still depends on the old
test/e2etree.This VPN copy is supposed to run from
test/e2e-vpn/, but the root detection rejects a workspace unlesstest/e2eexists. In a VPN-only checkout, this script will fail before it even reachescheck-docs.sh.Suggested fix
-if [ -d /workspace ] && [ -f /workspace/package.json ] && [ -d /workspace/test/e2e ]; then +if [ -d /workspace ] && [ -f /workspace/package.json ] && [ -d /workspace/test/e2e-vpn ]; then REPO="/workspace" -elif [ -f "${_candidate}/package.json" ] && [ -d "${_candidate}/test/e2e" ]; then +elif [ -f "${_candidate}/package.json" ] && [ -d "${_candidate}/test/e2e-vpn" ]; then REPO="${_candidate}" # exported for child scripts🤖 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-vpn/test-docs-validation.sh` around lines 60 - 63, The repo-root detection in test-docs-validation.sh still hard-codes the old test/e2e directory, causing VPN-only checkouts to be rejected. Update the REPO probe logic around the workspace and _candidate checks so it validates the VPN tree used by test/e2e-vpn/ instead of requiring test/e2e, and keep the child-script export behavior unchanged.test/e2e-vpn/test-device-auth-health.sh-153-158 (1)
153-158: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThis install step points at
scripts/install.sh, but the suite’s installer contract here is the repo-rootinstall.sh.The other E2E scripts in this PR resolve and invoke
install.shfrom the repository root. If this file is run from the documented entrypoint,scripts/install.shwill fail as soon as that wrapper is absent.Suggested fix
+REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" + NEMOCLAW_SANDBOX_NAME="$SANDBOX_NAME" \ NEMOCLAW_NON_INTERACTIVE=1 \ NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 \ NEMOCLAW_RECREATE_SANDBOX=1 \ GITHUB_TOKEN="${GITHUB_TOKEN:-}" \ - bash scripts/install.sh --non-interactive 2>&1 | tee "$INSTALL_LOG" || INSTALL_EXIT=$? + bash "$REPO_ROOT/install.sh" --non-interactive 2>&1 | tee "$INSTALL_LOG" || INSTALL_EXIT=$?🤖 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-vpn/test-device-auth-health.sh` around lines 153 - 158, The E2E install step is invoking the wrong installer entrypoint; update the test flow to use the repository-root install.sh instead of scripts/install.sh so it matches the suite’s contract and the other E2E scripts. Locate the install command in test-device-auth-health.sh and change the bash invocation to resolve and run the root installer consistently with the surrounding helpers.test/e2e-vpn/test-cron-preflight-inference-local-e2e.sh-66-90 (1)
66-90: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPoint this script at
test/e2e-vpn, not the legacytest/e2etree.The repo-root probe and
E2E_DIRboth hard-codetest/e2e, so this VPN-only script sources the old helper copy instead of the newtest/e2e-vpn/lib/*files. That breaks the isolation goal of this PR and will drift as soon as the two trees diverge. Resolve the repo root without requiringtest/e2e, then deriveE2E_DIRfrom the current script directory (or${REPO}/test/e2e-vpn).Suggested fix
-if [ -d /workspace ] && [ -f /workspace/package.json ] && [ -d /workspace/test/e2e ]; then +if [ -d /workspace ] && [ -f /workspace/package.json ]; then REPO="/workspace" -elif [ -f "${_candidate}/package.json" ] && [ -d "${_candidate}/test/e2e" ]; then +elif [ -f "${_candidate}/package.json" ]; then REPO="${_candidate}" else echo "ERROR: Cannot find repo root." exit 1 fi @@ -E2E_DIR="${REPO}/test/e2e" +E2E_DIR="$(cd "$(dirname "$0")" && pwd)"🤖 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-vpn/test-cron-preflight-inference-local-e2e.sh` around lines 66 - 90, The repo-root detection and E2E_DIR setup in the VPN cron preflight script still depend on the legacy test/e2e tree instead of test/e2e-vpn. Update the probe logic in this script so it resolves the repository root without checking for test/e2e, and set E2E_DIR from the VPN test directory (using the script’s own location or ${REPO}/test/e2e-vpn) so the sourced helpers come from test/e2e-vpn/lib via sandbox-teardown.sh, install-path-refresh.sh, and ci-compatible-inference.sh.test/e2e-vpn/test-cloud-inference-e2e.sh-85-96 (1)
85-96: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the helper-selected hosted model on the compatible-inference path.
nemoclaw_e2e_configure_compatible_inferencecan switch the VPN workflow onto a hosted-compatible provider, butCLOUD_MODELstays pinned to the hardcoded default here. That makes the/v1/chat/completionsprobe send a different model than the one the compatible-inference helper selected, so this test can fail even wheninference.localis working. Mirror thetest-cloud-onboard-e2e.shremap and overwriteCLOUD_MODELfromnemoclaw_e2e_hosted_inference_model()when compatible inference is active.Suggested fix
CLOUD_MODEL="${NEMOCLAW_CLOUD_EXPERIMENTAL_MODEL:-nvidia/nemotron-3-super-120b-a12b}" @@ . "${E2E_DIR}/lib/ci-compatible-inference.sh" nemoclaw_e2e_configure_compatible_inference +if nemoclaw_e2e_using_compatible_inference; then + CLOUD_MODEL="$(nemoclaw_e2e_hosted_inference_model)" +fi register_sandbox_for_teardown "$SANDBOX_NAME"🤖 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-vpn/test-cloud-inference-e2e.sh` around lines 85 - 96, The compatible-inference setup in `test-cloud-inference-e2e.sh` leaves `CLOUD_MODEL` stuck on the default value, so the chat-completions probe can use a different model than the one selected by `nemoclaw_e2e_configure_compatible_inference`. Update the script to mirror the remap used in `test-cloud-onboard-e2e.sh`: after calling `nemoclaw_e2e_configure_compatible_inference`, overwrite `CLOUD_MODEL` with the result of `nemoclaw_e2e_hosted_inference_model()` when the hosted-compatible path is active, so the probe and helper-selected provider/model stay aligned.test/e2e-vpn/lib/slack-api-proof.sh-520-528 (1)
520-528: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not send placeholder Slack tokens in Authorization headers for the body-rewrite proof.
These calls put
slackAccount.botTokeninto both the form body andAuthorization. Withrequest-body-credential-rewrite, only the body token is rewritten, so the fake server can observe a placeholder bearer token. Either omit the header for this hermetic proof and validate the body rewrite, or add a matching header-rewrite policy.Also applies to: 675-684
🤖 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-vpn/lib/slack-api-proof.sh` around lines 520 - 528, The body-rewrite proof in the chat.postMessage flow is sending the Slack bot token in both the form body and the Authorization header, which leaves a placeholder bearer token observable when only request-body-credential-rewrite is applied. Update the proof around the postForm calls in slack-api-proof.sh so it validates the body token rewrite without relying on Authorization, either by omitting the header entirely for this hermetic test or by adding a matching header-rewrite policy for the same call sites, including the later similar block.test/e2e-vpn/lib/telegram-api-proof.sh-81-82 (1)
81-82: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftUse a rewrite policy that covers Telegram path tokens.
requestFakeTelegramsends the bot token in/bot${token}/..., butapply_fake_telegram_api_policyonly enablesrequest-body-credential-rewrite. Unless OpenShell rewrites URL path credentials under that mode, this proof will either leak the placeholder token in the path or fail token validation. Switch to the Telegram/path rewrite mode or route the hermetic proof through a token-free fake endpoint.Also applies to: 176-182
🤖 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-vpn/lib/telegram-api-proof.sh` around lines 81 - 82, The Telegram proof policy only enables request-body-credential-rewrite, but requestFakeTelegram sends the token in the URL path via /bot${token}/..., so the current policy may not rewrite or validate the path token correctly. Update apply_fake_telegram_api_policy and the related openshell policy update usage in telegram-api-proof.sh to use the Telegram/path credential rewrite mode (or otherwise route the proof through a token-free fake endpoint), and make sure the same adjustment is applied anywhere the fake Telegram endpoint policy is defined or reused.test/e2e-vpn/lib/fake-slack-api.cjs-193-195 (1)
193-195: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAlign Slack auth validation with the body-rewrite policy.
authAcceptedcurrently requires both the bearer header and the formtokento match. The proof policy usesrequest-body-credential-rewrite, so body-rewritten Slack calls can still fail if the Authorization header remains a placeholder or is omitted. Accept the rewritten body token independently, or change the policy/proof to rewrite headers too.🤖 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-vpn/lib/fake-slack-api.cjs` around lines 193 - 195, The Slack auth check in fake-slack-api.cjs is too strict because authAccepted currently depends on both the Authorization header and the form token matching, which breaks body-rewritten requests. Update the validation around authAccepted so the rewritten body token is accepted on its own in the Slack request handling path, or adjust the corresponding proof/policy to also rewrite the header; use the existing tokenMatchesExpected, bodyMatchesExpected, and authAccepted logic to locate the change.test/e2e-vpn/test-brave-search-e2e.sh-258-276 (1)
258-276: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winB3b misses revision-scoped Brave env leaks.
B3a already accepts placeholders like
openshell:resolve:env:v11_BRAVE_API_KEY, but B3b only probesprintenv BRAVE_API_KEY. If the real secret is exposed under a scoped name such asv11_BRAVE_API_KEY, this check still passes. Scan the sandbox env for any(^|_)BRAVE_API_KEY=entry instead of only the unscoped variable.🤖 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-vpn/test-brave-search-e2e.sh` around lines 258 - 276, B3b only checks printenv BRAVE_API_KEY, so it can miss revision-scoped leaks such as v11_BRAVE_API_KEY. Update the sandbox env probe in test-brave-search-e2e.sh to inspect the full output of printenv and detect any BRAVE_API_KEY assignment matching the scoped pattern, not just the unscoped name, then keep the existing fail/pass handling in the B3b block consistent with that broader match.test/e2e-vpn/test-channels-add-remove.sh-99-105 (1)
99-105: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThis regression gate still self-skips instead of using the new hermetic Telegram fake.
With a fake token, Line 100-Line 104 exports
NEMOCLAW_SKIP_TELEGRAM_REACHABILITY=1, and Line 545-Line 546 later turns an inconclusive probe intoSKIP. That means the core post-add egress assertion can pass without ever exercising Telegram connectivity. Since this PR already introducestest/e2e-vpn/lib/fake-telegram-api.cjs, this test should be rewired to the fake API/policy path so nightly runs stay deterministic.Also applies to: 541-549
🤖 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-vpn/test-channels-add-remove.sh` around lines 99 - 105, The E2E still bypasses Telegram verification by setting NEMOCLAW_SKIP_TELEGRAM_REACHABILITY and later converting an inconclusive probe into SKIP, so it never exercises the real add/remove egress path. Update maybe_skip_telegram_reachability_for_fake_token and the probe handling near the post-add check to use the new hermetic fake Telegram API instead of self-skipping, so the test runs deterministically without relying on live Telegram connectivity.test/e2e-vpn/lib/fake-openai-compatible-api.mts-95-103 (1)
95-103: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject malformed JSON instead of treating it as
{}.Right now a truncated or invalid POST body falls through as an empty object and can still get a
200from/v1/chat/completionsor/v1/responses. That makes this fake server hide client serialization bugs and can turn compatibility regressions into false-green E2Es.Suggested fix
-function parseJsonBody(raw: Buffer): JsonObject { - if (raw.length === 0) return {}; +function parseJsonBody(raw: Buffer): JsonObject | null { + if (raw.length === 0) return {}; try { const parsed = JSON.parse(raw.toString("utf8")); return parsed && typeof parsed === "object" && !Array.isArray(parsed) ? parsed : {}; } catch { - return {}; + return null; } } @@ const raw = await readBody(req); const payload = parseJsonBody(raw); + if (payload === null) { + sendJson(res, 400, { error: { message: "invalid JSON body" } }); + return; + }Also applies to: 115-177
🤖 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-vpn/lib/fake-openai-compatible-api.mts` around lines 95 - 103, Reject malformed JSON in parseJsonBody instead of returning {} for parse failures. Update fake-openai-compatible-api.mts so parseJsonBody either throws or returns an error signal for invalid/truncated bodies, and have the /v1/chat/completions and /v1/responses request handlers surface that as a non-200 response rather than continuing with an empty object. Keep the valid empty-body behavior only if explicitly intended, and make the change in the shared parser used by the chat/completions and responses endpoints.test/e2e-vpn/test-credential-sanitization.sh-228-294 (1)
228-294: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThese checks reimplement the code under test instead of invoking it.
The sanitization, digest-verification, and credential-field rules are copied into inline
node -esnippets here, so this script can stay green even if the real production functions diverge. For a security regression suite, that gives false confidence. Please import/call the actual implementation path, or drive the CLI flow that exercises it, instead of duplicating the logic in the test.Also applies to: 513-617, 633-763
🤖 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-vpn/test-credential-sanitization.sh` around lines 228 - 294, The e2e credential-sanitization script is duplicating production logic in an inline node snippet instead of exercising the real implementation. Replace the ad hoc CREDENTIAL_FIELDS, isCredentialField, stripCredentials, and file-walking behavior in test-credential-sanitization.sh with a call into the actual sanitization path used by sanitizeCredentialsInBundle or the CLI flow that invokes it, so the test verifies the real code rather than a copy.test/e2e-vpn/lib/openai-compatible-api-proof.sh-25-35 (1)
25-35: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAuthenticated mode never becomes ready.
The readiness loop always probes
/v1/modelswithout anAuthorizationheader. WhenFAKE_OPENAI_REQUIRE_AUTH=1, a healthy server will return 401 here, so this helper kills it and reports startup failure.Suggested fix
- if curl -sf "http://${ready_host}:${FAKE_OPENAI_PORT}/v1/models" >/dev/null 2>&1; then + curl_args=(-sf) + if [ "${FAKE_OPENAI_REQUIRE_AUTH:-0}" = "1" ] && [ -n "${FAKE_OPENAI_API_KEY:-}" ]; then + curl_args+=(-H "Authorization: Bearer ${FAKE_OPENAI_API_KEY}") + fi + if curl "${curl_args[@]}" "http://${ready_host}:${FAKE_OPENAI_PORT}/v1/models" >/dev/null 2>&1; then🤖 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-vpn/lib/openai-compatible-api-proof.sh` around lines 25 - 35, The readiness check in openai-compatible-api-proof.sh always calls /v1/models without auth, so authenticated startup never passes. Update the readiness probe in the loop that uses FAKE_OPENAI_REQUIRE_AUTH and FAKE_OPENAI_PORT to send the same Authorization header expected by the fake OpenAI server when FAKE_OPENAI_REQUIRE_AUTH=1, while keeping the unauthenticated probe for the open mode.
🟡 Minor comments (6)
test/e2e-vpn/lib/anthropic-switch-provider.sh-170-172 (1)
170-172: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the env-var name in the failure message.
This branch validates
SWITCH_ENDPOINT_URL, but the error tells callers to provideNEMOCLAW_SWITCH_ENDPOINT_URL. That will misdirect anyone debugging a failed switch setup.Suggested fix
- fail "NEMOCLAW_SWITCH_ENDPOINT_URL is required for compatible Anthropic inference switches" + fail "SWITCH_ENDPOINT_URL is required for compatible Anthropic inference switches"🤖 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-vpn/lib/anthropic-switch-provider.sh` around lines 170 - 172, The failure message in the `SWITCH_ENDPOINT_URL` validation branch points to the wrong environment variable name, which will mislead callers during setup. Update the `fail` message in `anthropic-switch-provider.sh` so it refers to `SWITCH_ENDPOINT_URL` instead of `NEMOCLAW_SWITCH_ENDPOINT_URL`, keeping the validation logic unchanged and matching the variable checked in this branch.test/e2e-vpn/e2e-cloud-experimental/test-inference-local-chat.sh-20-22 (1)
20-22: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the usage example path.
The snippet tells users to run
test/e2e-vpn/demo-inference-local-chat.sh, but this file lives undere2e-cloud-experimental/test-inference-local-chat.sh. The current command is a dead path.Suggested fix
-# bash test/e2e-vpn/demo-inference-local-chat.sh +# bash test/e2e-vpn/e2e-cloud-experimental/test-inference-local-chat.sh🤖 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-vpn/e2e-cloud-experimental/test-inference-local-chat.sh` around lines 20 - 22, The usage example in the script header points to the wrong command, so update the commented “Usage” line in test-inference-local-chat.sh to reference the actual script path instead of the dead demo-inference-local-chat.sh path. Keep the rest of the usage text unchanged and ensure the example matches the script’s real location under e2e-cloud-experimental so users can copy-paste a working command.test/e2e-vpn/test-gateway-drift-preflight.sh-315-317 (1)
315-317: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTighten the protobuf assertion so the wrong fallback cannot satisfy it.
Line 315 currently treats
Skipping 'alpha' (not running)as acceptable evidence, but Line 317 forbids that exact output. That weakens this regression guard: a misclassification-to-stopped regression can still satisfy the first check. Require protobuf/schema-mismatch evidence in the positive assertion and leave theSkipping ...text exclusively to the negative assertion.Suggested fix
-assert_contains "$CASE_DIR/command.out" 'protobuf|schema mismatch|invalid wire type|Skipping '\''?alpha'\''? \(not running\)' "protobuf failure is not silently swallowed" +assert_contains "$CASE_DIR/command.out" 'protobuf|schema mismatch|invalid wire type' "protobuf failure is not silently swallowed"🤖 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-vpn/test-gateway-drift-preflight.sh` around lines 315 - 317, Tighten the protobuf regression check in the gateway drift preflight test so it only passes on real protobuf/schema-mismatch failures and cannot be satisfied by the stopped-sandbox fallback. Update the positive assertion in the test script to match only protobuf-related error text, and keep the “Skipping 'alpha' (not running)” pattern only in the negative assertion. Use the existing assertion calls in the test script to make the change around the command.out checks.test/e2e-vpn/test-common-egress-agent-e2e.sh-92-105 (1)
92-105: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle
message.reasoningin the Hermes parser too.Other new E2E scripts in this PR fall back to
reasoningas well. This parser drops that field, so reasoning-only responses turn into empty strings and false failures in the Hermes assertion path.Suggested fix
- content = message.get("content") or message.get("reasoning_content") or "" + content = message.get("content") or message.get("reasoning_content") or message.get("reasoning") or ""🤖 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-vpn/test-common-egress-agent-e2e.sh` around lines 92 - 105, The Hermes parser in parse_chat_content only falls back to content and reasoning_content, so reasoning-only responses can be lost. Update the Python JSON handling in parse_chat_content to also check message.reasoning before defaulting to an empty string, keeping the existing fallback order consistent with the other E2E scripts.test/e2e-vpn/lib/openai-compatible-api-proof.sh-54-60 (1)
54-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear stale endpoint state on stop.
stop_fake_openai_compatible_apiresets only the PID. If a laterstart_fake_openai_compatible_apiattempt fails, callers can still see the previousFAKE_OPENAI_BASE_URL/FAKE_OPENAI_PORTand talk to a dead server.Suggested fix
stop_fake_openai_compatible_api() { if [ -n "${FAKE_OPENAI_PID:-}" ] && kill -0 "$FAKE_OPENAI_PID" 2>/dev/null; then kill "$FAKE_OPENAI_PID" 2>/dev/null || true wait "$FAKE_OPENAI_PID" 2>/dev/null || true fi FAKE_OPENAI_PID="" + unset FAKE_OPENAI_BASE_URL FAKE_OPENAI_PORT }🤖 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-vpn/lib/openai-compatible-api-proof.sh` around lines 54 - 60, The stop_fake_openai_compatible_api helper only clears FAKE_OPENAI_PID, leaving stale FAKE_OPENAI_BASE_URL and FAKE_OPENAI_PORT behind, which can cause callers to reuse a dead endpoint after a failed restart. Update stop_fake_openai_compatible_api in openai-compatible-api-proof.sh to clear all fake API state variables when shutting down, and make sure start_fake_openai_compatible_api reinitializes them consistently so the endpoint state always matches the running process.test/e2e-vpn/test-hermes-discord-e2e.sh-594-605 (1)
594-605: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate fake-discord scratch cleanup before rebuilding.
This cleanup is a prerequisite for the credential-regression check, but both
sudo rmandrmare best-effort. If those files survive, the nextnemoclaw rebuildcan fail with the EACCES path described in the comment and get misreported as a credential regression.Suggested fix
if [ -d "$REPO/.tmp" ]; then sudo rm -rf "$REPO/.tmp"/fake-discord.* 2>/dev/null || rm -rf "$REPO/.tmp"/fake-discord.* 2>/dev/null || true + if find "$REPO/.tmp" -maxdepth 1 -name 'fake-discord.*' -print -quit | grep -q .; then + fail "Fake Discord scratch files are still present under $REPO/.tmp; aborting rebuild regression check" + exit 1 + fi 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 `@test/e2e-vpn/test-hermes-discord-e2e.sh` around lines 594 - 605, The fake-discord scratch cleanup in the test setup is only best-effort, so a leftover root-owned directory can still break the next rebuild and be misreported as a credential failure. In the cleanup block around FAKE_DISCORD_GATEWAY_CONTAINER and the $REPO/.tmp/fake-discord.* removal, add an explicit verification step that the scratch path is gone before continuing. If it still exists after the docker rm and rm attempts, fail early with a clear message so the later nemoclaw rebuild only runs after cleanup succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 944acb2c-7ca6-46c3-8547-3989d5a01c2c
📒 Files selected for processing (115)
.github/actions/run-e2e-script-vpn/action.yaml.github/actions/run-e2e-script-vpn/sanitize-trace-artifacts.py.github/workflows/e2e-script-vpn.yaml.github/workflows/nightly-e2e-vpn.yamltest/e2e-vpn/Dockerfile.full-e2etest/e2e-vpn/README.mdtest/e2e-vpn/e2e-cloud-experimental/check-docs.shtest/e2e-vpn/e2e-cloud-experimental/checks/02-inference-local-http.shtest/e2e-vpn/e2e-cloud-experimental/checks/03-security-checks.shtest/e2e-vpn/e2e-cloud-experimental/checks/04-landlock-readonly.shtest/e2e-vpn/e2e-cloud-experimental/checks/05-deepagents-code-landlock-readonly.shtest/e2e-vpn/e2e-cloud-experimental/checks/06-deepagents-code-python-egress.shtest/e2e-vpn/e2e-cloud-experimental/cleanup.shtest/e2e-vpn/e2e-cloud-experimental/expect-interactive-install.shtest/e2e-vpn/e2e-cloud-experimental/features/skill/add-sandbox-skill.shtest/e2e-vpn/e2e-cloud-experimental/features/skill/fixtures/skill-smoke-template.SKILL.mdtest/e2e-vpn/e2e-cloud-experimental/features/skill/lib/README.mdtest/e2e-vpn/e2e-cloud-experimental/features/skill/lib/validate_repo_skills.shtest/e2e-vpn/e2e-cloud-experimental/features/skill/lib/validate_sandbox_openclaw_skills.shtest/e2e-vpn/e2e-cloud-experimental/features/skill/verify-sandbox-skill-via-agent.shtest/e2e-vpn/e2e-cloud-experimental/openclaw-tui-in-sandbox.shtest/e2e-vpn/e2e-cloud-experimental/skip/README.mdtest/e2e-vpn/e2e-cloud-experimental/test-inference-local-chat.shtest/e2e-vpn/e2e-cloud-experimental/test-port8080-conflict.shtest/e2e-vpn/e2e-timeout.shtest/e2e-vpn/lib/anthropic-switch-provider.shtest/e2e-vpn/lib/ci-compatible-inference.shtest/e2e-vpn/lib/cloudflared-version-resolver.shtest/e2e-vpn/lib/discord-gateway-proof.shtest/e2e-vpn/lib/discord-rest-policy-proof.shtest/e2e-vpn/lib/fake-discord-gateway.cjstest/e2e-vpn/lib/fake-discord-message-api.cjstest/e2e-vpn/lib/fake-discord-rest-api.cjstest/e2e-vpn/lib/fake-openai-compatible-api.mtstest/e2e-vpn/lib/fake-slack-api.cjstest/e2e-vpn/lib/fake-telegram-api.cjstest/e2e-vpn/lib/inference-switch-retry.shtest/e2e-vpn/lib/install-path-refresh.shtest/e2e-vpn/lib/openai-compatible-api-proof.shtest/e2e-vpn/lib/openclaw-agent-json.pytest/e2e-vpn/lib/openclaw-json.shtest/e2e-vpn/lib/sandbox-teardown.shtest/e2e-vpn/lib/security-posture-assertions.shtest/e2e-vpn/lib/slack-api-proof.shtest/e2e-vpn/lib/telegram-api-proof.shtest/e2e-vpn/test-agent-turn-latency-e2e.shtest/e2e-vpn/test-bedrock-runtime-compatible-anthropic.shtest/e2e-vpn/test-brave-search-e2e.shtest/e2e-vpn/test-channels-add-remove.shtest/e2e-vpn/test-channels-stop-start.shtest/e2e-vpn/test-cloud-inference-e2e.shtest/e2e-vpn/test-cloud-onboard-e2e.shtest/e2e-vpn/test-common-egress-agent-e2e.shtest/e2e-vpn/test-concurrent-gateway-ports.shtest/e2e-vpn/test-credential-migration.shtest/e2e-vpn/test-credential-sanitization.shtest/e2e-vpn/test-cron-preflight-inference-local-e2e.shtest/e2e-vpn/test-dashboard-remote-bind.shtest/e2e-vpn/test-device-auth-health.shtest/e2e-vpn/test-diagnostics.shtest/e2e-vpn/test-docs-validation.shtest/e2e-vpn/test-double-onboard.shtest/e2e-vpn/test-full-e2e.shtest/e2e-vpn/test-gateway-drift-preflight.shtest/e2e-vpn/test-gateway-health-honest.shtest/e2e-vpn/test-gpu-double-onboard.shtest/e2e-vpn/test-gpu-e2e.shtest/e2e-vpn/test-hermes-discord-e2e.shtest/e2e-vpn/test-hermes-e2e.shtest/e2e-vpn/test-hermes-inference-switch.shtest/e2e-vpn/test-hermes-root-entrypoint-smoke.shtest/e2e-vpn/test-hermes-sandbox-secret-boundary.shtest/e2e-vpn/test-hermes-slack-e2e.shtest/e2e-vpn/test-inference-routing.shtest/e2e-vpn/test-issue-2478-crash-loop-recovery.shtest/e2e-vpn/test-issue-4434-tui-unreachable-inference.shtest/e2e-vpn/test-issue-4462-scope-upgrade-approval.shtest/e2e-vpn/test-jetson-nvmap-gpu.shtest/e2e-vpn/test-kimi-inference-compat.shtest/e2e-vpn/test-launchable-smoke.shtest/e2e-vpn/test-messaging-compatible-endpoint.shtest/e2e-vpn/test-messaging-providers.shtest/e2e-vpn/test-model-router-provider-routed-inference.shtest/e2e-vpn/test-network-policy.shtest/e2e-vpn/test-ollama-auth-proxy-e2e.shtest/e2e-vpn/test-onboard-negative-paths.shtest/e2e-vpn/test-onboard-repair.shtest/e2e-vpn/test-onboard-resume.shtest/e2e-vpn/test-openclaw-discord-pairing.shtest/e2e-vpn/test-openclaw-inference-switch.shtest/e2e-vpn/test-openclaw-plugin-runtime-exdev.shtest/e2e-vpn/test-openclaw-skill-cli-e2e.shtest/e2e-vpn/test-openclaw-slack-pairing.shtest/e2e-vpn/test-openclaw-tui-chat-correlation.shtest/e2e-vpn/test-openshell-gateway-upgrade.shtest/e2e-vpn/test-openshell-version-pin.shtest/e2e-vpn/test-overlayfs-autofix.shtest/e2e-vpn/test-rebuild-hermes.shtest/e2e-vpn/test-rebuild-openclaw.shtest/e2e-vpn/test-runtime-overrides.shtest/e2e-vpn/test-sandbox-operations.shtest/e2e-vpn/test-sandbox-rebuild.shtest/e2e-vpn/test-sandbox-survival.shtest/e2e-vpn/test-sessions-agents-cli.shtest/e2e-vpn/test-shields-config.shtest/e2e-vpn/test-skill-agent-e2e.shtest/e2e-vpn/test-snapshot-commands.shtest/e2e-vpn/test-spark-install.shtest/e2e-vpn/test-state-backup-restore.shtest/e2e-vpn/test-telegram-injection.shtest/e2e-vpn/test-token-rotation.shtest/e2e-vpn/test-tunnel-lifecycle.shtest/e2e-vpn/test-upgrade-stale-sandbox.shtest/e2e-vpn/test-vm-driver-privileged-exec-routing.shtest/nightly-e2e-vpn-workflow.test.ts
Comment on lines
+6
to
+15
| RUN apt-get update && apt-get install -y --no-install-recommends \ | ||
| curl git ca-certificates bash python3 sudo jq \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| # Install Docker CLI only (NOT Docker daemon — will use host socket at runtime) | ||
| RUN curl -fsSL https://download.docker.com/linux/ubuntu/gpg | gpg --dearmor -o /etc/apt/keyrings/docker.gpg && \ | ||
| echo "deb [arch=$(dpkg --print-architecture) signed-by=/etc/apt/keyrings/docker.gpg] \ | ||
| https://download.docker.com/linux/ubuntu $(. /etc/os-release && echo $VERSION_CODENAME) stable" \ | ||
| > /etc/apt/sources.list.d/docker.list && \ | ||
| apt-get update && apt-get install -y --no-install-recommends docker-ce-cli && \ |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🌐 Web query:
Does the official ubuntu:24.04container image include thegpgbinary by default, or mustgnupg/gpgbe installed before runninggpg --dearmor in a Dockerfile?
💡 Result:
No—do not assume ubuntu:24.04 includes the gpg (GnuPG) binary by default; you should install gnupg (which provides gpg) in your Dockerfile before running gpg --dearmor. Evidence: gpg --dearmor is part of the GnuPG tooling, and common, current Dockerfile/apt-key migration snippets install gnupg before using gpg --dearmor (e.g., DigitalOcean’s Ubuntu 24.04 Docker-related instructions install ca-certificates curl gnupg as prerequisites before dearmoring). [1] Additionally, a Docker repository issue/request explicitly documents using gpg --dearmor and notes the need for gpg tooling as part of the process; it doesn’t treat gpg as already present. [2]
Citations:
- 1: https://www.digitalocean.com/community/tutorials/how-to-install-and-use-docker-on-ubuntu-20-04
- 2: Use trusted.gpg.d instead of apt-key docker/docs#11625
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Dockerfile context and the base image declaration.
file="test/e2e-vpn/Dockerfile.full-e2e"
wc -l "$file"
cat -n "$file" | sed -n '1,40p'Repository: NVIDIA/NemoClaw
Length of output: 1915
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether gpg is installed anywhere in this Dockerfile or related build setup.
rg -n --hidden --glob 'test/e2e-vpn/Dockerfile.full-e2e' --glob 'test/e2e-vpn/**' '\bgpg\b|\bgnupg\b'Repository: NVIDIA/NemoClaw
Length of output: 1915
Install gnupg before gpg --dearmor. gpg is not present in the base Ubuntu image here, so this step can fail when adding the Docker repo key.
🤖 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-vpn/Dockerfile.full-e2e` around lines 6 - 15, The Docker repository
setup in the e2e VPN Dockerfile can fail because gpg is used before it is
installed; update the install step that prepares the Docker CLI so gnupg is
included in the initial apt-get package list before the gpg --dearmor command,
keeping the rest of the Docker apt source setup in place.
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 @.github/workflows/nightly-e2e-vpn-smoke.yaml:
- Around line 6-10: The workflow trigger in the nightly VPN smoke job only
listens to push events on add-vpn-nightly-e2e, so it will not run on its own
schedule. Update the top-level on block in the nightly-e2e-vpn-smoke workflow to
include a schedule trigger for the nightly cadence, and keep the existing push
trigger only if you still want rollout testing. Use the workflow’s existing
trigger section to add the schedule without changing the rest of the job
definition.
🪄 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: fbf4e8ec-3330-4130-936b-717a8bd8eafa
📒 Files selected for processing (1)
.github/workflows/nightly-e2e-vpn-smoke.yaml
Comment on lines
+6
to
+10
| on: | ||
| push: | ||
| branches: | ||
| - add-vpn-nightly-e2e | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Nightly workflow currently won’t run on a nightly cadence.
Lines 6–10 only trigger on pushes to add-vpn-nightly-e2e, so this workflow won’t execute nightly unless a push happens. Add a schedule trigger (optionally keep push for rollout testing).
Suggested change
on:
+ schedule:
+ - cron: "0 6 * * *"
+ workflow_dispatch:
push:
branches:
- add-vpn-nightly-e2e📝 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
| on: | |
| push: | |
| branches: | |
| - add-vpn-nightly-e2e | |
| on: | |
| schedule: | |
| - cron: "0 6 * * *" | |
| workflow_dispatch: | |
| push: | |
| branches: | |
| - add-vpn-nightly-e2e |
🤖 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/nightly-e2e-vpn-smoke.yaml around lines 6 - 10, The
workflow trigger in the nightly VPN smoke job only listens to push events on
add-vpn-nightly-e2e, so it will not run on its own schedule. Update the
top-level on block in the nightly-e2e-vpn-smoke workflow to include a schedule
trigger for the nightly cadence, and keep the existing push trigger only if you
still want rollout testing. Use the workflow’s existing trigger section to add
the schedule without changing the rest of the job definition.
Contributor
Author
|
Closing in favor of a replacement PR that migrates the existing hosted NVIDIA path to inference.nvidia.com directly and removes Build as a target. |
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
test/e2e-vpn/and rewires the new workflow to that copied tree.NVIDIA_API_KEYfor VPN inference and routes compatible inference throughhttps://inference.nvidia.com/v1.test/nightly-e2e-vpn-workflow.test.tsto lock the new workflow/job graph, runner labels, VPN runner usage, and legacy-hosted-inference absence contract.Validation
npx vitest run test/nightly-e2e-vpn-workflow.test.ts test/validate-e2e-coverage.test.tsnpm run typecheckNotes
test/e2e-script-workflow.test.tsparse failure and missing localnemoclaw/node_modules/json5during broad CLI tests.Summary by CodeRabbit