test(e2e): prove OpenClaw Discord pairing approval - #4115
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 (1)
📝 WalkthroughWalkthroughAdds a nightly OpenClaw Discord pairing E2E job and a comprehensive Bash test that provisions a sandbox, runs a hermetic fake Discord gateway, generates and approves pairing requests, validates state transitions, uploads failure artifacts, and reports results. ChangesDiscord Pairing End-to-End Test
Sequence DiagramsequenceDiagram
participant Workflow as workflow_dispatch
participant Runner as Actions runner
participant Installer as install.sh
participant Sandbox as Sandbox
participant Gateway as FakeDiscordGateway
participant Node as NodeCapture
participant OpenClaw as OpenClaw CLI
participant Aggregator as notify/scorecard/report
Workflow->>Runner: trigger openclaw-discord-pairing-e2e
Runner->>Installer: run install.sh --non-interactive (with logs)
Runner->>Sandbox: provision and wait ready
Runner->>Gateway: start FakeDiscordGateway (hermetic)
Gateway->>Node: capture traffic (token rewrite)
Node->>Runner: validate token rewrite capture
Runner->>Sandbox: run runtime to emit pairing request (prints code)
Runner->>OpenClaw: run pairing list / approve commands in sandbox
OpenClaw->>Sandbox: consume pairing code, update discord-default-allowFrom.json
Runner->>Aggregator: upload artifacts and report job result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
ericksoa
added
bug
integration: discord
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
cv
approved these changes
May 23, 2026
Contributor
PR Review AdvisorFindings: 0 needs attention, 2 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
![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-openclaw-discord-pairing.sh (1)
226-233: ⚡ Quick winScope host gateway teardown in pre-cleanup
test/e2e/test-openclaw-discord-pairing.shunconditionally runsopenshell gateway destroy -g nemoclawin Phase 1 (lines 226-233). This exact teardown is used in multiple othertest/e2e/*.shscripts as well, so it may be repo-standard for isolating the shared defaultnemoclawgateway; if you want to avoid disrupting a developer’s existing host gateway, gate this destroy (e.g., behind an env var or by detecting a test-owned gateway) instead of always deleting it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/test-openclaw-discord-pairing.sh` around lines 226 - 233, The script unconditionally runs openshell gateway destroy -g nemoclaw during pre-cleanup which may tear down a developer’s host gateway; update the block that calls openshell gateway destroy -g nemoclaw to only run when a guarding condition is true (e.g., an environment variable like CI, TEST_OWN_GATEWAY, or SANDBOX_OWN_GATEWAY is set) or when you detect the gateway is test-owned (check openshell gateway list/output for a marker tied to SANDBOX_NAME); wrap the openshell gateway destroy -g nemoclaw and related openshell sandbox delete "$SANDBOX_NAME" calls behind that guard and leave the existing comandos (nemoclaw destroy and other quiet fallbacks) unchanged so local gateways aren’t removed unintentionally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/test-openclaw-discord-pairing.sh`:
- Around line 156-166: The sandbox_exec_sh_script function currently calls
openshell sandbox exec without a timeout; wrap that final openshell invocation
in run_with_timeout 60 so the remote command is killed after 60s if it hangs.
Locate sandbox_exec_sh_script (and the openshell sandbox exec --name
"$SANDBOX_NAME" -- sh -lc "$remote_cmd" invocation) and replace that direct call
with a run_with_timeout 60 wrapper that passes the same arguments and preserves
quoting/variable expansion behavior.
---
Nitpick comments:
In `@test/e2e/test-openclaw-discord-pairing.sh`:
- Around line 226-233: The script unconditionally runs openshell gateway destroy
-g nemoclaw during pre-cleanup which may tear down a developer’s host gateway;
update the block that calls openshell gateway destroy -g nemoclaw to only run
when a guarding condition is true (e.g., an environment variable like CI,
TEST_OWN_GATEWAY, or SANDBOX_OWN_GATEWAY is set) or when you detect the gateway
is test-owned (check openshell gateway list/output for a marker tied to
SANDBOX_NAME); wrap the openshell gateway destroy -g nemoclaw and related
openshell sandbox delete "$SANDBOX_NAME" calls behind that guard and leave the
existing comandos (nemoclaw destroy and other quiet fallbacks) unchanged so
local gateways aren’t removed unintentionally.
🪄 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: 06072a73-d6ec-4832-903d-da603c5a0bfe
📒 Files selected for processing (2)
.github/workflows/nightly-e2e.yamltest/e2e/test-openclaw-discord-pairing.sh
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 26319880876
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/e2e/test-openclaw-discord-pairing.sh (2)
584-589:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert the second approval fails with a non-zero exit code.
This phase only matches the error text. If
openclaw pairing approveregresses to exit0while printing the same message, the E2E still passes and no longer proves the "fails closed" contract from the PR objective.Suggested change
repeat_approve=$(sandbox_exec "openclaw pairing approve discord '$pairing_code' 2>&1") -if echo "$repeat_approve" | grep -q "No pending pairing request found"; then +repeat_approve_status=$? +if [ $repeat_approve_status -ne 0 ] \ + && echo "$repeat_approve" | grep -q "No pending pairing request found"; then pass "Second approval fails closed after request consumption" else fail "Second approval did not report missing pending request: ${repeat_approve:0:300}" 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/test-openclaw-discord-pairing.sh` around lines 584 - 589, The test currently only inspects stdout in variable repeat_approve from sandbox_exec "openclaw pairing approve ..."; update the test to also assert that the command exited with a non-zero status: capture the exit code returned by sandbox_exec (or run sandbox_exec in a way that exposes its exit status), and fail if that exit code is 0 while the output contains "No pending pairing request found"; update the logic around repeat_approve and the sandbox_exec invocation (references: repeat_approve, sandbox_exec, the "openclaw pairing approve" invocation and the subsequent pass/fail branches) so the test requires both the expected error text and a non‑zero exit code.
236-245:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound
install.shwith a timeout.The install step is the longest external call in the script, but it currently runs unbounded. If onboarding wedges, this job sits until the runner-level timeout instead of failing fast.
Suggested change
INSTALL_LOG="/tmp/nemoclaw-e2e-openclaw-discord-pairing-install.log" info "Running install.sh --non-interactive..." -bash install.sh --non-interactive >"$INSTALL_LOG" 2>&1 & +run_with_timeout 1800 bash install.sh --non-interactive >"$INSTALL_LOG" 2>&1 & install_pid=$! tail -f "$INSTALL_LOG" --pid=$install_pid 2>/dev/null & tail_pid=$! wait $install_pid 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/test-openclaw-discord-pairing.sh` around lines 236 - 245, Wrap the long-running install step with a bounded timeout so the job fails fast if onboarding hangs: introduce an INSTALL_TIMEOUT variable (or use a default) and run bash install.sh --non-interactive under the timeout utility (or implement a watchdog that kills install_pid after INSTALL_TIMEOUT). Update the existing flow that sets INSTALL_LOG, captures install_pid and tail_pid, and uses wait/install_exit so that if timeout kills install_pid you detect that condition (timeout exit code, e.g. 124) and exit non‑zero after cleaning up tail_pid; ensure any kill/wait cleanup still runs to avoid orphaned background tails.
♻️ Duplicate comments (1)
test/e2e/test-openclaw-discord-pairing.sh (1)
156-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap
sandbox_exec_sh_scriptwith the existing timeout guard.This helper is still the only sandbox exec path that can hang indefinitely, so a stuck remote shell can burn the whole workflow.
Suggested change
def sandbox_exec_sh_script() { local script="$1" shift local encoded remote_cmd arg encoded="$(printf '%s' "$script" | base64 | tr -d '\n')" remote_cmd="tmp=\$(mktemp); trap 'rm -f \"\$tmp\"' EXIT; printf %s $(quote_for_remote_sh "$encoded") | base64 -d > \"\$tmp\"; sh \"\$tmp\"" for arg in "$@"; do remote_cmd+=" $(quote_for_remote_sh "$arg")" done - openshell sandbox exec --name "$SANDBOX_NAME" -- sh -lc "$remote_cmd" + run_with_timeout 60 \ + openshell sandbox exec --name "$SANDBOX_NAME" -- sh -lc "$remote_cmd" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/test-openclaw-discord-pairing.sh` around lines 156 - 166, The sandbox_exec_sh_script helper can hang indefinitely; wrap the openshell invocation inside the same timeout guard used by the other sandbox helpers so a stuck remote shell can't block the workflow. Modify sandbox_exec_sh_script to call the existing timeout wrapper (the same helper used elsewhere in tests) around the openshell sandbox exec --name "$SANDBOX_NAME" -- sh -lc "$remote_cmd" call, preserving argument quoting/propagation and exit code semantics and keeping the tmp/trap behavior in remote_cmd unchanged.
🤖 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.
Outside diff comments:
In `@test/e2e/test-openclaw-discord-pairing.sh`:
- Around line 584-589: The test currently only inspects stdout in variable
repeat_approve from sandbox_exec "openclaw pairing approve ..."; update the test
to also assert that the command exited with a non-zero status: capture the exit
code returned by sandbox_exec (or run sandbox_exec in a way that exposes its
exit status), and fail if that exit code is 0 while the output contains "No
pending pairing request found"; update the logic around repeat_approve and the
sandbox_exec invocation (references: repeat_approve, sandbox_exec, the "openclaw
pairing approve" invocation and the subsequent pass/fail branches) so the test
requires both the expected error text and a non‑zero exit code.
- Around line 236-245: Wrap the long-running install step with a bounded timeout
so the job fails fast if onboarding hangs: introduce an INSTALL_TIMEOUT variable
(or use a default) and run bash install.sh --non-interactive under the timeout
utility (or implement a watchdog that kills install_pid after INSTALL_TIMEOUT).
Update the existing flow that sets INSTALL_LOG, captures install_pid and
tail_pid, and uses wait/install_exit so that if timeout kills install_pid you
detect that condition (timeout exit code, e.g. 124) and exit non‑zero after
cleaning up tail_pid; ensure any kill/wait cleanup still runs to avoid orphaned
background tails.
---
Duplicate comments:
In `@test/e2e/test-openclaw-discord-pairing.sh`:
- Around line 156-166: The sandbox_exec_sh_script helper can hang indefinitely;
wrap the openshell invocation inside the same timeout guard used by the other
sandbox helpers so a stuck remote shell can't block the workflow. Modify
sandbox_exec_sh_script to call the existing timeout wrapper (the same helper
used elsewhere in tests) around the openshell sandbox exec --name
"$SANDBOX_NAME" -- sh -lc "$remote_cmd" call, preserving argument
quoting/propagation and exit code semantics and keeping the tmp/trap behavior in
remote_cmd unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dadf4a30-5d6f-4983-ae91-1f349bf295e4
📒 Files selected for processing (1)
test/e2e/test-openclaw-discord-pairing.sh
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26320047082
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26320202290
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/test-openclaw-discord-pairing.sh`:
- Around line 239-241: The script currently calls run_with_timeout to enforce
INSTALL_TIMEOUT_SECONDS but run_with_timeout silently falls back to running the
command un-timed if neither timeout nor gtimeout is available; add an explicit
pre-check before invoking run_with_timeout that verifies at least one of
"timeout" or "gtimeout" is present (e.g., using command -v) and if not, print a
clear error and exit non-zero so NEMOCLAW_E2E_INSTALL_TIMEOUT_SECONDS /
INSTALL_TIMEOUT_SECONDS cannot be bypassed; reference the run_with_timeout
invocation and the INSTALL_TIMEOUT_SECONDS /
NEMOCLAW_E2E_INSTALL_TIMEOUT_SECONDS variables when adding this check.
🪄 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: 99caef43-5687-47b8-936a-e2887d463ab7
📒 Files selected for processing (1)
test/e2e/test-openclaw-discord-pairing.sh
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26320376483
|
12 tasks
wscurran
added
area: e2e
wscurran
added
chore
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.
Summary
Validation
Local E2E note
Fixes/proves #4061
Summary by CodeRabbit
Tests
Chores