ci: add nightly E2E with inference test - #386
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a nightly E2E GitHub Actions workflow and refactors the E2E script to run a non-interactive installer, adjust sandbox/policy/CLI checks, make openshell teardown conditional, and upload install logs on failure. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions Runner
participant Script as test/e2e/test-full-e2e.sh
participant Installer as install.sh
participant CLI as Nemoclaw / OpenShell CLIs
participant NVIDIA as NVIDIA Cloud API
participant Store as Artifact Store
rect rgba(100,150,240,0.5)
GH->>Script: triggered (cron or workflow_dispatch)
end
Script->>Installer: bash install.sh --non-interactive
Installer-->>Script: exit code + logs
alt install failed
Script->>Store: upload /tmp/nemoclaw-e2e-install.log (ignore-missing)
else install succeeded
Script->>CLI: nemoclaw list / nemoclaw status
CLI->>NVIDIA: provider/status queries (e.g., nvidia-nim)
Script->>CLI: openshell inference get (verify nvidia-nim)
Script->>CLI: openshell policy get --full (verify network_policies)
Script->>CLI: nemoclaw <sandbox> logs
end
Script->>GH: job completes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![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
🧹 Nitpick comments (1)
test/e2e/test-full-e2e.sh (1)
185-188:status_outputis captured but unused.The variable captures command output but only the exit code is checked. Consider either verifying the output content (e.g., checking for expected sandbox state) or simplifying to avoid the unused variable warning.
Option 1: Verify output content
status_output=$(nemoclaw "$SANDBOX_NAME" status 2>&1) -[ $? -eq 0 ] \ - && pass "nemoclaw ${SANDBOX_NAME} status exits 0" \ - || fail "nemoclaw ${SANDBOX_NAME} status failed" +if [ $? -eq 0 ]; then + pass "nemoclaw ${SANDBOX_NAME} status exits 0" + echo "$status_output" | grep -qi "running\|ready" \ + && pass "Sandbox status indicates running" \ + || info "Sandbox status: ${status_output:0:100}" +else + fail "nemoclaw ${SANDBOX_NAME} status failed" +fiOption 2: Simplify to avoid unused variable
-status_output=$(nemoclaw "$SANDBOX_NAME" status 2>&1) -[ $? -eq 0 ] \ +nemoclaw "$SANDBOX_NAME" status > /dev/null 2>&1 \ && pass "nemoclaw ${SANDBOX_NAME} status exits 0" \ || fail "nemoclaw ${SANDBOX_NAME} status failed"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-full-e2e.sh` around lines 185 - 188, The script captures the output into status_output but never uses it; update the nemoclaw status check so either (a) assert expected content by testing status_output for a known string (e.g., grep/[[ "$status_output" =~ "expected state" ]]) before calling pass/fail, or (b) drop the capture and run nemoclaw "$SANDBOX_NAME" status directly (so only the exit code is checked). Modify the block around the status command and the pass/fail invocation—references: variable status_output, command nemoclaw, and the pass/fail helper—to implement one of these two fixes and remove the unused-variable warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/test-full-e2e.sh`:
- Line 115: The cd invocation at test/e2e/test-full-e2e.sh (the line containing
cd "$REPO") must be made fail-fast so the script stops if the directory change
fails; update that cd "$REPO" command to exit the script on failure (use a
short-circuit exit on nonzero status) so subsequent commands do not run in the
wrong directory.
---
Nitpick comments:
In `@test/e2e/test-full-e2e.sh`:
- Around line 185-188: The script captures the output into status_output but
never uses it; update the nemoclaw status check so either (a) assert expected
content by testing status_output for a known string (e.g., grep/[[
"$status_output" =~ "expected state" ]]) before calling pass/fail, or (b) drop
the capture and run nemoclaw "$SANDBOX_NAME" status directly (so only the exit
code is checked). Modify the block around the status command and the pass/fail
invocation—references: variable status_output, command nemoclaw, and the
pass/fail helper—to implement one of these two fixes and remove the
unused-variable warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f235b653-d043-4792-a1f1-7f2bdb836342
📒 Files selected for processing (2)
.github/workflows/nightly-e2e.yamltest/e2e/test-full-e2e.sh
Signed-off-by: Jaya Venkatesh <jjayabaskar@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/test-full-e2e.sh (1)
185-188: Unused variablestatus_output.Static analysis flagged
status_outputas unused. Either incorporate it into the pass/fail message or remove the capture.-status_output=$(nemoclaw "$SANDBOX_NAME" status 2>&1) -[ $? -eq 0 ] \ - && pass "nemoclaw ${SANDBOX_NAME} status exits 0" \ - || fail "nemoclaw ${SANDBOX_NAME} status failed" +if nemoclaw "$SANDBOX_NAME" status > /dev/null 2>&1; then + pass "nemoclaw ${SANDBOX_NAME} status exits 0" +else + fail "nemoclaw ${SANDBOX_NAME} status failed" +fiAlternatively, include the output in the failure message for debugging:
status_output=$(nemoclaw "$SANDBOX_NAME" status 2>&1) -[ $? -eq 0 ] \ - && pass "nemoclaw ${SANDBOX_NAME} status exits 0" \ - || fail "nemoclaw ${SANDBOX_NAME} status failed" +if [ $? -eq 0 ]; then + pass "nemoclaw ${SANDBOX_NAME} status exits 0" +else + fail "nemoclaw ${SANDBOX_NAME} status failed: ${status_output:0:200}" +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-full-e2e.sh` around lines 185 - 188, The captured variable status_output is unused; either remove the capture or include it in the messages. Fix by using the command substitution result when checking exit status: reference the status_output variable so failure logs include it (e.g., include status_output in the fail message) or simply run nemoclaw without assigning to status_output and keep the existing status check; update the pass/fail invocations that currently reference ${SANDBOX_NAME} status to also reference status_output when logging failures for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e/test-full-e2e.sh`:
- Around line 185-188: The captured variable status_output is unused; either
remove the capture or include it in the messages. Fix by using the command
substitution result when checking exit status: reference the status_output
variable so failure logs include it (e.g., include status_output in the fail
message) or simply run nemoclaw without assigning to status_output and keep the
existing status check; update the pass/fail invocations that currently reference
${SANDBOX_NAME} status to also reference status_output when logging failures for
debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ac12d0b-b0ef-49b3-abfd-a3dce817e361
📒 Files selected for processing (2)
.github/workflows/nightly-e2e.yamltest/e2e/test-full-e2e.sh
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/nightly-e2e.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/test-full-e2e.sh (1)
217-225:⚠️ Potential issue | 🟠 MajorAvoid putting
NVIDIA_API_KEYin process arguments.Line 220 interpolates the secret into curl's
-Hargument, exposing it to process inspection (visible viapson CI runners). Use curl's--config -to pass configuration via stdin instead:Proposed fix
-api_response=$(curl -s --max-time 30 \ - -X POST https://integrate.api.nvidia.com/v1/chat/completions \ - -H "Content-Type: application/json" \ - -H "Authorization: Bearer $NVIDIA_API_KEY" \ - -d '{ +api_response=$(curl --config - <<EOF +silent +max-time = 30 +request = "POST" +url = "https://integrate.api.nvidia.com/v1/chat/completions" +header = "Content-Type: application/json" +header = "Authorization: Bearer $NVIDIA_API_KEY" +data = '{ "model": "nvidia/nemotron-3-super-120b-a12b", "messages": [{"role": "user", "content": "Reply with exactly one word: PONG"}], "max_tokens": 100 - }' 2>/dev/null) || true + }' +EOF +2>/dev/null) || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-full-e2e.sh` around lines 217 - 225, The curl invocation that builds api_response embeds the secret via the -H "Authorization: Bearer $NVIDIA_API_KEY" argument which can leak the key in process listings; change the call that sets api_response to use curl's --config - option and pass the headers and request body via stdin (so the NVIDIA_API_KEY is not present in the process arguments), ensuring the same URL, method, JSON payload (model/messages/max_tokens) and timeout are preserved; update the code that currently references the inline -H Authorization header to instead read the Authorization header from the --config - input and keep the surrounding error handling (2>/dev/null) and the || true behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/test-full-e2e.sh`:
- Around line 179-182: The test currently greps command output regardless of the
command's exit status (e.g., the nemoclaw list invocation that sets
list_output), which can mask failures; update each place where output is
captured and asserted (commands like nemoclaw list and any similar captures
around the SANDBOX_NAME checks) to first check the command exit status and fail
with the command's output if it failed, only proceeding to grep/assert the
content when the command succeeded; use the captured variable (e.g.,
list_output) and the command's exit code to determine pass/fail and include the
raw output in failure messages for debugging.
- Around line 180-182: Replace the regex grep usage with fixed-string matching
to avoid treating SANDBOX_NAME as a regex: change occurrences that use grep -q
"$SANDBOX_NAME" (e.g., the snippet using echo "$list_output" | grep -q
"$SANDBOX_NAME") to use grep -Fq -- "$SANDBOX_NAME" instead, and apply the same
replacement for the other occurrences noted (around the 302-304 check); keep the
surrounding logic (&& pass ... || fail ...) unchanged.
---
Outside diff comments:
In `@test/e2e/test-full-e2e.sh`:
- Around line 217-225: The curl invocation that builds api_response embeds the
secret via the -H "Authorization: Bearer $NVIDIA_API_KEY" argument which can
leak the key in process listings; change the call that sets api_response to use
curl's --config - option and pass the headers and request body via stdin (so the
NVIDIA_API_KEY is not present in the process arguments), ensuring the same URL,
method, JSON payload (model/messages/max_tokens) and timeout are preserved;
update the code that currently references the inline -H Authorization header to
instead read the Authorization header from the --config - input and keep the
surrounding error handling (2>/dev/null) and the || true behavior intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05a71b34-34b9-494b-99ae-e62512f2553a
📒 Files selected for processing (1)
test/e2e/test-full-e2e.sh
jacobtomlinson
approved these changes
Mar 19, 2026
jacobtomlinson
left a comment
jacobtomlinson
left a comment
Member
There was a problem hiding this comment.
Amazing work Jaya thank you! I've added the secret. Let's merge and then we can verify it runs happily overnight and fix any issues tomorrow.
Ryuketsukami
pushed a commit
to Ryuketsukami/NemoClaw
that referenced
this pull request
Mar 24, 2026
* ci: add nightly full E2E with install.sh, policy enforcement, and CLI tests * fix: pass GITHUB_TOKEN for openshell install via gh CLI * fix: avoid tee pipe hang from openshell background port-forward * fix: use SSH for sandbox command execution test * refactor: remove OpenShell-tested checks from NemoClaw E2E * fixed section header Signed-off-by: Jaya Venkatesh <jjayabaskar@nvidia.com> * ci: skip nightly E2E on forks without secrets * fix: fail-fast on cd and include status output in failure message * fix: validate command exit status before assertions and use fixed-string grep --------- Signed-off-by: Jaya Venkatesh <jjayabaskar@nvidia.com>
jessesanford
pushed a commit
to jessesanford/NemoClaw
that referenced
this pull request
Mar 24, 2026
* ci: add nightly full E2E with install.sh, policy enforcement, and CLI tests * fix: pass GITHUB_TOKEN for openshell install via gh CLI * fix: avoid tee pipe hang from openshell background port-forward * fix: use SSH for sandbox command execution test * refactor: remove OpenShell-tested checks from NemoClaw E2E * fixed section header Signed-off-by: Jaya Venkatesh <jjayabaskar@nvidia.com> * ci: skip nightly E2E on forks without secrets * fix: fail-fast on cd and include status output in failure message * fix: validate command exit status before assertions and use fixed-string grep --------- Signed-off-by: Jaya Venkatesh <jjayabaskar@nvidia.com>
mafueee
pushed a commit
to mafueee/NemoClaw
that referenced
this pull request
Mar 28, 2026
Signed-off-by: Will Burford <will@lmstudio.ai>
wscurran
added
area: ci
wscurran
added
feature
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
3 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.
This adds a nightly workflow that replicates a typical user flow: run
install.sh --non-interactiveon a clean Ubunturunner, set up everything (Node, openshell, NemoClaw, gateway, sandbox, inference), then verify the sandbox
is working with live inference from build.nvidia.com
What's in the workflow:
NVIDIA_API_KEYrepo secret,GITHUB_TOKENfor openshell install via gh CLIWhat changed in
test-full-e2e.sh:install.sh --non-interactive(thanks to feat: add non-interactive mode for CI/CD onboarding #318) instead of manualnpm install && npm link+ piped stdin onboardtest should break
tail -finstead ofteebecause openshell's port-forward holds the pipe openforever
Tested on my fork, with an API key created from my account. @jacobtomlinson
@jacobtomlinson or @ericksoa, can we configure an API key to add
NVIDIA_API_KEYto the repo secrets for the nightly CI pipeline to work?This workflow also faces the issue of the API key being briefly visible in the process run, which is tracked by #325, and any fixes made to close that issue can be made here to fix the vulnerability as well.
Summary by CodeRabbit