test(e2e): prove Discord proxy binary whitelist#3663
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 (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds a hermetic fake Discord REST HTTPS server, bash test helpers to manage it and apply Node-only policies, refactors E2E messaging tests to perform policy/proxy inspection and Node-vs-curl hermetic checks, and updates generated parity docs and assertion totals (now 2040). ChangesDiscord REST Hermetic Testing
E2E Advisor Auto-dispatch
Sequence DiagramsequenceDiagram
participant TestScript as test-messaging-providers.sh
participant StartHelper as start_fake_discord_rest_api()
participant PolicyHelper as apply_fake_discord_rest_policy()
participant NodeRunner as run_fake_discord_rest_node_request()
participant CurlRunner as run_fake_discord_rest_curl_request()
participant Capture as JSONL_Capture
participant PolicyEngine as nemoclaw
TestScript->>StartHelper: start fake REST server (get port)
TestScript->>PolicyHelper: write & apply Node-only policy via nemoclaw
TestScript->>NodeRunner: Node GET /api/v10/gateway (expect 200)
TestScript->>CurlRunner: curl GET /api/v10/gateway (expect denial patterns)
TestScript->>Capture: query node vs curl counts (expect node=1, curl=0)
PolicyHelper->>PolicyEngine: nemoclaw policy-add with CONNECT rules
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/lib/discord-rest-policy-proof.sh`:
- Around line 29-67: Register the cleanup trap immediately after creating the
temporary directory so failures in cert generation or docker startup still
cleanup; move the call to append_exit_trap_for_fake_discord_rest_api
cleanup_fake_discord_rest_api to right after FAKE_DISCORD_REST_DIR is created
(immediately after the mkdir/mktemp block where FAKE_DISCORD_REST_DIR,
FAKE_DISCORD_REST_* variables are set and the capture file is initialized) so
any subsequent early returns (from openssl or docker run) will trigger cleanup;
also ensure the script has a proper shebang and is executable to satisfy
shellfmt/ShellCheck guidelines.
In `@test/e2e/test-messaging-providers.sh`:
- Around line 981-998: The test currently treats a live Discord "CONNECT ...
403" response as definitive proof that curl was blocked by the sandbox (the
branch that calls pass), but that same response can come from an upstream proxy;
change the branch that checks for CONNECT 403/HTTP 403 (the condition using
live_dc_curl and grep) to stop calling pass and instead emit a
diagnostic/ambiguous signal (use info() or a diagnostic helper) that includes
the live_dc_curl snippet and states the result is ambiguous and that
M13-rest-d/M13-rest-e provide hermetic whitelist proof; update the message text
formerly passed to pass("M13-curl: ...") to something like info("M13-curl:
ambiguous – live CONNECT 403 may be upstream; see hermetic whitelist tests;
output: ...") so the test no longer claims a whitelist pass when the live probe
is inconclusive.
🪄 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: 07ec8054-7517-4e45-88d5-7cdbd0a53c27
📒 Files selected for processing (5)
test/e2e/docs/parity-inventory.generated.jsontest/e2e/docs/parity-map.yamltest/e2e/lib/discord-rest-policy-proof.shtest/e2e/lib/fake-discord-rest-api.cjstest/e2e/test-messaging-providers.sh
Collaborator
Author
|
Addressed the two CodeRabbit comments in 2fb97f4: moved fake Discord REST cleanup trap before cert/docker startup failure points, and made the live Discord curl CONNECT-403 probe diagnostic-only instead of a passing assertion. Refreshed parity metadata and re-ran |
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-messaging-providers.sh`:
- Around line 970-975: The proxy check currently only verifies
HTTPS_PROXY/https_proxy point to 10.200.0.1:3128 but ignores NO_PROXY/no_proxy
exemptions; update the validation around live_proxy_env so it also scans
NO_PROXY and no_proxy and fails if either contains discord.com or
cdn.discordapp.com (or equivalent host patterns), i.e. keep the existing grep
that asserts proxy endpoints but add an additional negative check that rejects
when live_proxy_env matches
NO_PROXY=.*(discord\.com|cdn\.discordapp\.com)|no_proxy=.*(discord\.com|cdn\.discordapp\.com);
ensure the failure message still references M13-proxy (and M13-curl if
duplicated) and include the truncated live_proxy_env for debugging.
🪄 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: 7db736c3-6b86-44a5-9960-82fe014b7a80
📒 Files selected for processing (4)
test/e2e/docs/parity-inventory.generated.jsontest/e2e/docs/parity-map.yamltest/e2e/lib/discord-rest-policy-proof.shtest/e2e/test-messaging-providers.sh
✅ Files skipped from review due to trivial changes (1)
- test/e2e/docs/parity-inventory.generated.json
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/docs/parity-map.yaml
- test/e2e/lib/discord-rest-policy-proof.sh
Comment on lines
+970
to
+975
| live_proxy_env=$(sandbox_exec 'printf "HTTPS_PROXY=%s\nhttps_proxy=%s\nNO_PROXY=%s\nno_proxy=%s\n" "$HTTPS_PROXY" "$https_proxy" "$NO_PROXY" "$no_proxy"' 2>/dev/null || true) | ||
| info "Sandbox proxy env: ${live_proxy_env//$'\n'/ }" | ||
| if echo "$live_proxy_env" | grep -qE "https?_proxy=.*10\.200\.0\.1:3128|HTTPS_PROXY=.*10\.200\.0\.1:3128"; then | ||
| pass "M13-proxy: Sandbox uses the OpenShell gateway proxy" | ||
| else | ||
| fail "M13-proxy: Sandbox proxy env does not point at OpenShell gateway: ${live_proxy_env:0:200}" |
Contributor
There was a problem hiding this comment.
Fail when NO_PROXY exempts Discord hosts.
This check can pass even when Discord traffic bypasses the OpenShell gateway, because it ignores NO_PROXY/no_proxy. If discord.com or cdn.discordapp.com is exempted there, M13 and M13-curl can report the right outcome for the wrong path.
Suggested check
live_proxy_env=$(sandbox_exec 'printf "HTTPS_PROXY=%s\nhttps_proxy=%s\nNO_PROXY=%s\nno_proxy=%s\n" "$HTTPS_PROXY" "$https_proxy" "$NO_PROXY" "$no_proxy"' 2>/dev/null || true)
info "Sandbox proxy env: ${live_proxy_env//$'\n'/ }"
-if echo "$live_proxy_env" | grep -qE "https?_proxy=.*10\.200\.0\.1:3128|HTTPS_PROXY=.*10\.200\.0\.1:3128"; then
+if echo "$live_proxy_env" | grep -qiE '^(NO_PROXY|no_proxy)=.*(discord\.com|cdn\.discordapp\.com)'; then
+ fail "M13-proxy: Discord hosts are exempted via NO_PROXY/no_proxy, so the live proxy checks can bypass OpenShell"
+elif echo "$live_proxy_env" | grep -qE "https?_proxy=.*10\.200\.0\.1:3128|HTTPS_PROXY=.*10\.200\.0\.1:3128"; then
pass "M13-proxy: Sandbox uses the OpenShell gateway proxy"
else
fail "M13-proxy: Sandbox proxy env does not point at OpenShell gateway: ${live_proxy_env:0:200}"
fi📝 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
| live_proxy_env=$(sandbox_exec 'printf "HTTPS_PROXY=%s\nhttps_proxy=%s\nNO_PROXY=%s\nno_proxy=%s\n" "$HTTPS_PROXY" "$https_proxy" "$NO_PROXY" "$no_proxy"' 2>/dev/null || true) | |
| info "Sandbox proxy env: ${live_proxy_env//$'\n'/ }" | |
| if echo "$live_proxy_env" | grep -qE "https?_proxy=.*10\.200\.0\.1:3128|HTTPS_PROXY=.*10\.200\.0\.1:3128"; then | |
| pass "M13-proxy: Sandbox uses the OpenShell gateway proxy" | |
| else | |
| fail "M13-proxy: Sandbox proxy env does not point at OpenShell gateway: ${live_proxy_env:0:200}" | |
| live_proxy_env=$(sandbox_exec 'printf "HTTPS_PROXY=%s\nhttps_proxy=%s\nNO_PROXY=%s\nno_proxy=%s\n" "$HTTPS_PROXY" "$https_proxy" "$NO_PROXY" "$no_proxy"' 2>/dev/null || true) | |
| info "Sandbox proxy env: ${live_proxy_env//$'\n'/ }" | |
| if echo "$live_proxy_env" | grep -qiE '^(NO_PROXY|no_proxy)=.*(discord\.com|cdn\.discordapp\.com)'; then | |
| fail "M13-proxy: Discord hosts are exempted via NO_PROXY/no_proxy, so the live proxy checks can bypass OpenShell" | |
| elif echo "$live_proxy_env" | grep -qE "https?_proxy=.*10\.200\.0\.1:3128|HTTPS_PROXY=.*10\.200\.0\.1:3128"; then | |
| pass "M13-proxy: Sandbox uses the OpenShell gateway proxy" | |
| else | |
| fail "M13-proxy: Sandbox proxy env does not point at OpenShell gateway: ${live_proxy_env:0:200}" | |
| 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-messaging-providers.sh` around lines 970 - 975, The proxy check
currently only verifies HTTPS_PROXY/https_proxy point to 10.200.0.1:3128 but
ignores NO_PROXY/no_proxy exemptions; update the validation around
live_proxy_env so it also scans NO_PROXY and no_proxy and fails if either
contains discord.com or cdn.discordapp.com (or equivalent host patterns), i.e.
keep the existing grep that asserts proxy endpoints but add an additional
negative check that rejects when live_proxy_env matches
NO_PROXY=.*(discord\.com|cdn\.discordapp\.com)|no_proxy=.*(discord\.com|cdn\.discordapp\.com);
ensure the failure message still references M13-proxy (and M13-curl if
duplicated) and include the truncated live_proxy_env for debugging.
Collaborator
Author
|
Removed the E2E advisor auto-dispatch job cap in 25e99d2. The dispatcher now sends all dispatchable required advisor jobs in one nightly-e2e dispatch; |
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25983649324
|
4 tasks
ericksoa
added a commit
that referenced
this pull request
May 21, 2026
## Summary - add Discord-specific policy-add guidance that curl is not a valid success signal because it is outside the preset binary allowlist - document Node HTTPS probes for Discord REST/CDN validation behind proxy and call out DNS-only checks as inconclusive - cover the direct warning helper plus interactive and named policy-add discord flows ## Reopen Context Issue #3477 was closed by #3663, then reopened from a v0.0.44 QA repro that still used curl and dns.resolve as validation signals. That same repro showed Node HTTPS to https://discord.com/api/v10/gateway returning 200, which is the signal this PR now documents and surfaces in policy-add. ## Related Issue Fixes #3477 ## Verification - [x] npm run build:cli - [x] npm test -- test/policies.test.ts - [x] npm test -- test/validate-blueprint.test.ts - [x] make docs Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Documentation - Added troubleshooting guidance for Discord preset validation behind proxies: explains why curl probes can mislead, clarifies the built-in preset’s behavior, warns about DNS-only checks, and provides Node HTTPS one-liners to verify Discord API/CDN egress and next investigative steps. ## Tests - Extended coverage to assert Discord preset validation warnings and to verify interactive and non-interactive policy-add flows print the new guidance and proceed with apply as expected. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3934?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
This was referenced May 21, 2026
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
Adds Discord messaging E2E coverage for the #3477 proxy/binary-whitelist distinction. The test now proves that curl is denied by the Discord preset while Node can use the same OpenShell proxy path, and adds a hermetic fake HTTPS REST proof so the behavior is reproducible without depending only on live Discord.
Related Issue
Fixes #3477
Changes
test/e2e/test-messaging-providers.shto verify live Discord policy entries, OpenShell proxy env, curl denial, and Node API/CDN reachability.test/e2e/lib/discord-rest-policy-proof.shplus a fake HTTPS Discord-shaped REST server fixture to prove Node-only CONNECT authorization hermetically.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit