fix(e2e): unset OPENCLAW_GATEWAY_PORT and TOKEN in devices approve guard#4573
Conversation
The #4462 sandbox configure guard only unset OPENCLAW_GATEWAY_URL for `openclaw devices approve`, but the OpenClaw CLI can also derive the gateway connection from OPENCLAW_GATEWAY_PORT (as a fallback) and OPENCLAW_GATEWAY_TOKEN (for authentication). When the CLI falls back to the port-based URL, the approval request fails with "GatewayClientRequestError: scope upgrade pending approval" because the gateway rejects the approve command from a connection that itself requires the upgraded scope. Unset all three gateway env vars (URL, PORT, TOKEN) so the CLI connects directly to the local OpenClaw daemon for the scope approval. Signed-off-by: Hung Le <hple@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR broadens the ChangesExpand gateway credential clearing for devices approve
Sequence Diagram(s)sequenceDiagram
participant UserShell
participant NemoclawWatcher
participant Helper_run
participant SandboxOpenClaw
participant OpenClawBinary
UserShell->>NemoclawWatcher: trigger auto-pair approve
NemoclawWatcher->>Helper_run: run(strip_gateway_env=True)
Helper_run->>SandboxOpenClaw: unset OPENCLAW_GATEWAY_URL/PORT/TOKEN
SandboxOpenClaw->>OpenClawBinary: exec "openclaw devices approve"
OpenClawBinary-->>SandboxOpenClaw: approval result
SandboxOpenClaw-->>UserShell: return result (caller env unchanged)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 |
PR Review AdvisorFindings: 0 needs attention, 0 worth checking, 0 nice ideas This is an automated advisory review. A human maintainer must make the final merge decision. |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Scenario Advisor RecommendationRequired scenario E2E: Dispatch required scenario E2E:
Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Selective E2E Results — ❌ Some jobs failedRun: 26731727341
|
Selective E2E Results — ❌ Some jobs failedRun: 26731778496
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 26732000225
|
There was a problem hiding this comment.
Actionable comments posted: 1
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-issue-4462-scope-upgrade-approval.sh (1)
387-394: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExpand caller environment verification to include PORT and TOKEN.
The
approve_requestfunction verifies thatOPENCLAW_GATEWAY_URLis preserved in the caller's shell before and after runningopenclaw devices approve(lines 387, 394, 418-427). However, since Line 745 now verifies that the guard unsets all three credentials (OPENCLAW_GATEWAY_URL,OPENCLAW_GATEWAY_PORT,OPENCLAW_GATEWAY_TOKEN), this verification should be expanded to confirm that PORT and TOKEN are also preserved in the caller's environment.Currently, if the guard incorrectly leaks PORT or TOKEN mutations back to the caller shell, this test won't catch it.
🔍 Suggested expansion
output=$(sandbox_exec_sh_script 90 ' set -u request_id="$1" if [ ! -r /tmp/nemoclaw-proxy-env.sh ]; then echo "missing /tmp/nemoclaw-proxy-env.sh" >&2 exit 2 fi # shellcheck source=/dev/null . /tmp/nemoclaw-proxy-env.sh printf "__URL_BEFORE__=%s\n" "${OPENCLAW_GATEWAY_URL-unset}" +printf "__PORT_BEFORE__=%s\n" "${OPENCLAW_GATEWAY_PORT-unset}" +printf "__TOKEN_BEFORE__=%s\n" "${OPENCLAW_GATEWAY_TOKEN-unset}" set +e approve_output="$(openclaw devices approve "$request_id" --json 2>&1)" approve_rc=$? set -e printf "__APPROVE_RC__=%s\n" "$approve_rc" printf "__APPROVE_OUTPUT_BEGIN__\n%s\n__APPROVE_OUTPUT_END__\n" "$approve_output" printf "__URL_AFTER__=%s\n" "${OPENCLAW_GATEWAY_URL-unset}" +printf "__PORT_AFTER__=%s\n" "${OPENCLAW_GATEWAY_PORT-unset}" +printf "__TOKEN_AFTER__=%s\n" "${OPENCLAW_GATEWAY_TOKEN-unset}" exit "$approve_rc"Then add corresponding extraction and verification logic similar to lines 418-427 for PORT and TOKEN.
🤖 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-issue-4462-scope-upgrade-approval.sh` around lines 387 - 394, The test's approve_request check only captures OPENCLAW_GATEWAY_URL before/after the approve command; extend this to also capture OPENCLAW_GATEWAY_PORT and OPENCLAW_GATEWAY_TOKEN by printing their values before running approve (similar to the existing printf "__URL_BEFORE__=...") and after (similar to "__URL_AFTER__=..."), and then add extraction and assertions (following the pattern used in the block around lines 418-427) to ensure OPENCLAW_GATEWAY_PORT and OPENCLAW_GATEWAY_TOKEN are unchanged in the caller shell after running openclaw devices approve; update variable names (e.g., PORT_BEFORE/PORT_AFTER, TOKEN_BEFORE/TOKEN_AFTER) and comparison logic in the approve_request function accordingly.
🤖 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 `@scripts/nemoclaw-start.sh`:
- Line 1872: The auto-pair approval path still only unsets OPENCLAW_GATEWAY_URL,
allowing approvals to use the gateway via OPENCLAW_GATEWAY_PORT or
OPENCLAW_GATEWAY_TOKEN; update the auto-pair "devices approve" invocation to
mirror the shell guard by unsetting all three environment variables
(OPENCLAW_GATEWAY_URL, OPENCLAW_GATEWAY_PORT, OPENCLAW_GATEWAY_TOKEN) before
running the approval command so approvals cannot route via the gateway.
---
Outside diff comments:
In `@test/e2e/test-issue-4462-scope-upgrade-approval.sh`:
- Around line 387-394: The test's approve_request check only captures
OPENCLAW_GATEWAY_URL before/after the approve command; extend this to also
capture OPENCLAW_GATEWAY_PORT and OPENCLAW_GATEWAY_TOKEN by printing their
values before running approve (similar to the existing printf
"__URL_BEFORE__=...") and after (similar to "__URL_AFTER__=..."), and then add
extraction and assertions (following the pattern used in the block around lines
418-427) to ensure OPENCLAW_GATEWAY_PORT and OPENCLAW_GATEWAY_TOKEN are
unchanged in the caller shell after running openclaw devices approve; update
variable names (e.g., PORT_BEFORE/PORT_AFTER, TOKEN_BEFORE/TOKEN_AFTER) and
comparison logic in the approve_request function accordingly.
🪄 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: 1a8264ae-fe62-4ecd-87c9-fc8c05a22ce7
📒 Files selected for processing (3)
scripts/nemoclaw-start.shtest/e2e/test-issue-4462-scope-upgrade-approval.shtest/nemoclaw-start.test.ts
Selective E2E Results — ✅ All requested jobs passedRun: 26733382614
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/nemoclaw-start.sh (1)
1539-1543: Run the sandbox E2E matrix for this entrypoint-path change.Please run:
gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=sandbox-survival-e2e,sandbox-operations-e2e,cloud-e2e,openclaw-slack-pairing-e2eAs per coding guidelines:
scripts/nemoclaw-start.shis a sandbox entrypoint script whose changes affect boot behavior and are invisible to unit tests, with the listed E2E recommendations.Also applies to: 1606-1606
🤖 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 `@scripts/nemoclaw-start.sh` around lines 1539 - 1543, The sandbox entrypoint change in scripts/nemoclaw-start.sh (affecting the run function signature run(*args, strip_gateway_env=False)) requires running the nightly E2E matrix to validate boot behavior; trigger the GitHub Actions workflow by running gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=sandbox-survival-e2e,sandbox-operations-e2e,cloud-e2e,openclaw-slack-pairing-e2e (use the branch containing your changes) and verify the listed sandbox E2E jobs pass to ensure the entrypoint change doesn't break runtime boot behavior.
🤖 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.
Nitpick comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 1539-1543: The sandbox entrypoint change in
scripts/nemoclaw-start.sh (affecting the run function signature run(*args,
strip_gateway_env=False)) requires running the nightly E2E matrix to validate
boot behavior; trigger the GitHub Actions workflow by running gh workflow run
nightly-e2e.yaml --ref <branch> -f
jobs=sandbox-survival-e2e,sandbox-operations-e2e,cloud-e2e,openclaw-slack-pairing-e2e
(use the branch containing your changes) and verify the listed sandbox E2E jobs
pass to ensure the entrypoint change doesn't break runtime boot behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1f115b16-6c31-4872-bb2e-f2914f31b611
📒 Files selected for processing (2)
scripts/nemoclaw-start.shtest/nemoclaw-start.test.ts
Selective E2E Results — ✅ All requested jobs passedRun: 26733688513
|
Selective E2E Results — ✅ All requested jobs passedRun: 26733865080
|
Summary
[Agent-generated issue]
The
issue-4462-scope-upgrade-approval-e2e / runjob in nightly run #26698759656 failed because theopenclaw devices approveguard function in the sandbox proxy env only unsetOPENCLAW_GATEWAY_URL, leavingOPENCLAW_GATEWAY_PORTandOPENCLAW_GATEWAY_TOKENin the environment. The OpenClaw CLI falls back to the port-based gateway URL, causing the approval to route through the gateway and fail withGatewayClientRequestError: scope upgrade pending approval.Changes
scripts/nemoclaw-start.sh: Unset all three gateway env vars (OPENCLAW_GATEWAY_URL,OPENCLAW_GATEWAY_PORT,OPENCLAW_GATEWAY_TOKEN) in thedevices approveguard subshell.test/nemoclaw-start.test.ts: Update the fakeopenclawstub to log PORT and TOKEN alongside URL; addOPENCLAW_GATEWAY_PORTandOPENCLAW_GATEWAY_TOKENto the test env; update assertions to verify all three areunsetfordevices approvewhile remaining set for other commands.Root Cause
The #4462 guard (line 1872 of
nemoclaw-start.sh) runsopenclaw devices approvein a subshell that unsets onlyOPENCLAW_GATEWAY_URL. The sandbox proxy env also exportsOPENCLAW_GATEWAY_PORT(line 264) andOPENCLAW_GATEWAY_TOKEN(line 1351). When the OpenClaw CLI resolves the gateway URL, it can fall back toOPENCLAW_GATEWAY_PORT, reconnecting to the gateway and hitting the scope-upgrade Catch-22: the approve command itself requires the upgraded scope.Validation
The
GITHUB_TOKENused by this CI run does not include theworkflowscope, so the-custom-e2evalidation branch could not be pushed. To validate manually:a25b3931Signed-off-by: Hung Le hple@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests