test(e2e): cover openclaw → inference.local and stop matching prompt tokens - #2491
Closed
ericksoa wants to merge 1 commit into
Closed
test(e2e): cover openclaw → inference.local and stop matching prompt tokens#2491ericksoa wants to merge 1 commit into
ericksoa wants to merge 1 commit into
Conversation
…tokens The "live inference" assertions in cloud-e2e (test-full-e2e.sh) and the Hermes e2e were curl-from-sandbox checks. They prove OpenShell's DNS forwarder + proxy can route inference.local; they never invoke openclaw's HTTP client and never reach openclaw's SSRF guard. That is why every openclaw 4.9 nightly-e2e run on PR #2464 reported [LIVE] Sandbox inference: PASS while real users were getting SsrFBlockedError on the same release. Changes: * test-full-e2e.sh: relabel Phase 4b from [LIVE] to [ROUTING] with a comment pointing at #2490; add Phase 4c, an actual openclaw-mediated turn that runs `openclaw agent --json` over SSH, parses result.payloads[].text, and asserts the model produced "42" for "What is 6 multiplied by 7?". The expected token is not a substring of the prompt, --json routes logs to stderr, stderr is dropped — so prompt-echo on an error path cannot satisfy the grep. * test-hermes-e2e.sh: same relabel for the equivalent curl assertion. * test-sandbox-operations.sh TC-SBX-02: replace `Say exactly: HELLO_E2E` prompt + grep on merged stdout/stderr with the same arithmetic-via-JSON pattern. The previous assertion would match the prompt itself in any error path that quoted it back, including the openclaw 4.9 SSRF rejection — false positive that hid the regression for the entire 4.2 → 4.7 → 4.8 → 4.9 bump series. * verify-sandbox-skill-via-agent.sh: stop embedding ${VERIFY_TOKEN} in the prompt (the agent must read it from SKILL.md — that is the test). Add a guard that refuses SKILL_VERIFY_PROMPT overrides which smuggle the token back in, and a negative assertion on SsrFBlockedError, transport errors, and gateway-unavailable markers before the positive grep. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThese changes refactor end-to-end test scripts to clarify the distinction between routing-layer validation (DNS/proxy reachability) and agent-mediated inference validation. Test prompts are updated with additional validation, and assertion logic is revised to use JSON parsing instead of substring matching. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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/e2e-cloud-experimental/features/skill/verify-sandbox-skill-via-agent.sh (1)
99-105: Good early failure detection, but consider aligning error patterns with suite conventions.The fail-fast on provider/transport errors is the right approach to prevent coincidental token matches from masking real failures.
However, the error pattern here is a subset of the standard patterns used elsewhere in the suite. Per
test/e2e/test-inference-routing.sh:378-384, the canonical pattern includes additional codes likeETIMEDOUT,ENETUNREACH,EHOSTUNREACH,ENOTFOUND,"No route to host",dns, etc.Consider aligning for consistency:
♻️ Suggested expanded pattern
-if printf '%s' "$raw_out" | grep -qiE "SsrFBlockedError|Blocked hostname|Blocked: resolves to|transport error|provider error|ECONNREFUSED|EAI_AGAIN|gateway unavailable"; then +if printf '%s' "$raw_out" | grep -qiE "SsrFBlockedError|Blocked hostname|Blocked: resolves to|transport error|provider error|ECONNREFUSED|ETIMEDOUT|ENETUNREACH|EHOSTUNREACH|ENOTFOUND|EAI_AGAIN|No route to host|gateway unavailable|dns"; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e-cloud-experimental/features/skill/verify-sandbox-skill-via-agent.sh` around lines 99 - 105, Update the grep pattern that checks "$raw_out" in the provider/transport error guard (the block using raw_out, agent_rc, SESSION_ID and die) to match the canonical suite error tokens used elsewhere (e.g., include ETIMEDOUT, ENETUNREACH, EHOSTUNREACH, ENOTFOUND, "No route to host", dns, etc.) so the fail-fast behavior is consistent with test/e2e/test-inference-routing.sh; modify the extended regex passed to grep -qiE to add these additional keywords/phrases while preserving the existing terms and the die call that reports the agent_rc and SESSION_ID.
🤖 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-sandbox-operations.sh`:
- Around line 291-294: The captured variable `raw` is polluted because
`sandbox_exec` merges remote stderr into its output; update the test to discard
remote stderr like `test-full-e2e.sh` by running the SSH command directly
(instead of via `sandbox_exec`) so the remote invocation of `openclaw agent
--json --session-id '${session_id}' ... 2>/dev/null` is executed on the remote
side, or alternatively implement a new helper `sandbox_exec_stdout_only` that
runs the remote command with stderr redirected to /dev/null and use that when
assigning `raw` (references: sandbox_exec, raw, session_id, and the `openclaw
agent` invocation).
---
Nitpick comments:
In
`@test/e2e/e2e-cloud-experimental/features/skill/verify-sandbox-skill-via-agent.sh`:
- Around line 99-105: Update the grep pattern that checks "$raw_out" in the
provider/transport error guard (the block using raw_out, agent_rc, SESSION_ID
and die) to match the canonical suite error tokens used elsewhere (e.g., include
ETIMEDOUT, ENETUNREACH, EHOSTUNREACH, ENOTFOUND, "No route to host", dns, etc.)
so the fail-fast behavior is consistent with test/e2e/test-inference-routing.sh;
modify the extended regex passed to grep -qiE to add these additional
keywords/phrases while preserving the existing terms and the die call that
reports the agent_rc and SESSION_ID.
🪄 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: e43b9e1c-eb26-40f8-95bc-50dff449ef0f
📒 Files selected for processing (4)
test/e2e/e2e-cloud-experimental/features/skill/verify-sandbox-skill-via-agent.shtest/e2e/test-full-e2e.shtest/e2e/test-hermes-e2e.shtest/e2e/test-sandbox-operations.sh
Comment on lines
+291
to
+294
| log " Sending one-shot message to agent via SSH (openclaw agent --json)..." | ||
| local session_id raw | ||
| session_id="e2e-sbx-02-$(date +%s)-$$" | ||
| raw=$(sandbox_exec "openclaw agent --agent main --json --session-id '${session_id}' -m 'What is 6 multiplied by 7? Reply with only the integer, no extra words.'" 2>/dev/null) || true |
Contributor
There was a problem hiding this comment.
Stderr pollution may break JSON parsing.
The sandbox_exec helper (lines 75-103) captures the remote command's stderr via 2>&1 into the result. This means raw will contain any stderr output from openclaw agent, which would corrupt the JSON.
In contrast, test-full-e2e.sh handles this correctly by placing 2>/dev/null inside the SSH command itself (line 393), ensuring stderr is discarded before capture.
🐛 Proposed fix
The cleanest fix is to invoke the remote command directly instead of using sandbox_exec, mirroring the pattern in test-full-e2e.sh:
- raw=$(sandbox_exec "openclaw agent --agent main --json --session-id '${session_id}' -m 'What is 6 multiplied by 7? Reply with only the integer, no extra words.'" 2>/dev/null) || true
+ local ssh_cfg
+ ssh_cfg="$(mktemp)"
+ if openshell sandbox ssh-config "$SANDBOX_A" >"$ssh_cfg" 2>/dev/null; then
+ raw=$(run_with_timeout 120 ssh -F "$ssh_cfg" \
+ -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null \
+ -o ConnectTimeout=10 -o LogLevel=ERROR \
+ "openshell-${SANDBOX_A}" \
+ "openclaw agent --agent main --json --session-id '${session_id}' -m 'What is 6 multiplied by 7? Reply with only the integer, no extra words.'" \
+ 2>/dev/null) || true
+ fi
+ rm -f "$ssh_cfg"Alternatively, add a sandbox_exec_stdout_only variant that discards stderr from the remote command.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/test-sandbox-operations.sh` around lines 291 - 294, The captured
variable `raw` is polluted because `sandbox_exec` merges remote stderr into its
output; update the test to discard remote stderr like `test-full-e2e.sh` by
running the SSH command directly (instead of via `sandbox_exec`) so the remote
invocation of `openclaw agent --json --session-id '${session_id}' ...
2>/dev/null` is executed on the remote side, or alternatively implement a new
helper `sandbox_exec_stdout_only` that runs the remote command with stderr
redirected to /dev/null and use that when assigning `raw` (references:
sandbox_exec, raw, session_id, and the `openclaw agent` invocation).
Contributor
Author
|
Folding into #2490. The fix and the tests that prove it are reciprocal — neither validates without the other — so a single PR is the cleaner unit of review and the cleaner unit of nightly-e2e validation. All four files from this PR (test-full-e2e.sh, test-hermes-e2e.sh, test-sandbox-operations.sh, verify-sandbox-skill-via-agent.sh) have been cherry-picked onto fix/openclaw-4-9-inference-ssrf as commit ca03324. |
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
The "live inference" assertions in
cloud-e2e(test/e2e/test-full-e2e.sh) and the Hermes e2e arecurl-from-sandbox checks. They prove that OpenShell's DNS forwarder + proxy can routeinference.local; they never invoke openclaw's HTTP client and never touch openclaw's SSRF guard.That is why every openclaw 4.9
nightly-e2erun gating PR #2464 reported[LIVE] Sandbox inference: model responded with PONG through sandboxPASS while real users were hittingSsrFBlockedError: Blocked hostname or private/internal/special-use IP addresson the same release. The check that should have caught it (TC-SBX-02 intest-sandbox-operations.sh) was also a false-positive: it sent'Say exactly: HELLO_E2E'and grepped the merged stdout/stderr forHELLO_E2E, which any error path that quoted the prompt back could satisfy.This PR closes the coverage gap and stops the prompt-token false-positive class.
Related
Changes
test/e2e/test-full-e2e.sh[LIVE] Sandbox inference→[ROUTING] inference.local DNS + OpenShell proxy reachable from sandbox. Comment points at the openclaw HTTP client gap.openclaw agent --jsonover SSH inside the sandbox, parsesresult.payloads[].text, asserts the model answered42to "What is 6 multiplied by 7?". This is the only assertion in the suite that proves openclaw can complete a turn againstinference.local.test/e2e/test-hermes-e2e.sh: same[LIVE]→[ROUTING]relabel for the equivalent curl-only assertion.test/e2e/test-sandbox-operations.shTC-SBX-02: replaces theSay exactly: HELLO_E2E+ merged-output grep with the arithmetic-via---jsonpattern. Three properties that prevent the prior false positive:--jsonroutes openclaw logs to stderr (openclaw/src/commands/agent-via-gateway.ts:57); stderr is dropped, stdout is a clean JSON envelope.42) is not a substring of the prompt, so an error path that echoes the prompt cannot satisfy the grep.result.payloads[].text, not on merged stdout/stderr.test/e2e/e2e-cloud-experimental/features/skill/verify-sandbox-skill-via-agent.sh:${VERIFY_TOKEN}in the prompt — the agent must read it fromSKILL.md, which is the entire point of the test.SKILL_VERIFY_PROMPToverrides that smuggle the token back in.SsrFBlockedError,Blocked hostname, transport errors, andgateway unavailablemarkers before the positive grep, so an SSRF block can't be hidden by a coincidental token match.Type of Change
Verification
bash -nandshellcheck -S warningclean on all four touched files.npm testnot relevant — this PR only edits e2e shell scripts.test-clipre-commit hook skipped because it shells out to a real OpenShell gateway / Docker on a clean dev machine and times out (same pre-existing flake noted on #2490).Sequencing
This PR is independent of #2490 but they are related: #2490 fixes the runtime regression, this PR fixes the tests that should have caught it. Either order works. Landing this first against an unfixed sandbox image will (correctly) red-light Phase 4c and TC-SBX-02 — that's the proof the new assertions actually see the regression.
Summary by CodeRabbit
Release Notes