Skip to content

fix(e2e): treat ERR_PROXY_TUNNEL as proxy wiring success in M12 test#3880

Closed
kagura-agent wants to merge 1 commit into
NVIDIA:mainfrom
kagura-agent:fix/3836-proxy-test-conflation
Closed

fix(e2e): treat ERR_PROXY_TUNNEL as proxy wiring success in M12 test#3880
kagura-agent wants to merge 1 commit into
NVIDIA:mainfrom
kagura-agent:fix/3836-proxy-test-conflation

Conversation

@kagura-agent

@kagura-agent kagura-agent commented May 20, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

The M12 reachability test in test-messaging-providers.sh conflates proxy wiring validation with upstream proxy policy. When the configured proxy correctly forwards the CONNECT request but returns HTTP 403 (policy deny), Node.js emits ERR_PROXY_TUNNEL. This proves proxy wiring is correct — traffic was routed through the proxy — but the test fell through to the fail branch because it only recognized:

  • HTTP_* → pass
  • TIMEOUT → skip
  • ECONNRESET|ENETUNREACH|... → skip
  • everything else → fail

ERR_PROXY_TUNNEL didn't match any of the skip patterns, so it was treated as a failure even though NemoClaw's proxy integration is functioning correctly.

Changes

  • test/e2e/test-messaging-providers.sh: Add a new elif branch before the network-error skip that recognizes ERR_PROXY_TUNNEL and PROXY_CONNECTION_LOST as pass outcomes, since they prove Node.js honored the HTTPS_PROXY env var and routed through the configured proxy endpoint.
  • test/e2e/docs/parity-map.yaml: Add the new pass verdict to the M12 parity entries.
  • test/e2e/docs/parity-inventory.generated.json: Add the new pass entry with updated line references.

How it works

Before:  ERR_PROXY_TUNNEL → (no match) → fail ❌
After:   ERR_PROXY_TUNNEL → pass ✅ (proxy wiring confirmed, policy blocked destination)

The fix distinguishes:

  1. Proxy wiring broken → traffic bypasses proxy, direct connection fails (ECONNREFUSED to destination) → still fails as expected
  2. Proxy wiring works, proxy allows destination → HTTP_200/HTTP_301 → pass (unchanged)
  3. Proxy wiring works, proxy blocks destination → ERR_PROXY_TUNNEL → now passes (was: fail)

Closes #3836

Signed-off-by: kagura-agent kagura-agent@users.noreply.github.com

Summary by CodeRabbit

  • Tests
    • Enhanced network reachability tests for messaging providers with improved proxy scenario handling and updated test assertions to reflect expected outcomes.

Review Change Stack

@copy-pr-bot

copy-pr-bot Bot commented May 20, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented May 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 01da6d9b-7241-493d-b59d-f3a2dd30b611

📥 Commits

Reviewing files that changed from the base of the PR and between 942d9c0 and a62fab6.

📒 Files selected for processing (2)
  • test/e2e/docs/parity-inventory.generated.json
  • test/e2e/test-messaging-providers.sh
📝 Walkthrough

Walkthrough

The M12 Telegram reachability test now distinguishes proxy wiring success from upstream proxy policy blocks by treating Node.js ERR_PROXY_TUNNEL errors as PASS outcomes. The test script adds a conditional branch for these proxy-specific errors, and the parity inventory is updated to reflect the new expected passing assertion.

Changes

M12 Telegram Reachability Test Refinement

Layer / File(s) Summary
M12 Telegram reachability proxy error classification
test/e2e/test-messaging-providers.sh, test/e2e/docs/parity-inventory.generated.json		 The M12 test adds a conditional branch that catches proxy-specific error patterns (ERR_PROXY_TUNNEL, PROXY_CONNECTION_LOST) and logs them as PASS with deferred status, indicating successful proxy routing while upstream policy blocked the destination. The parity inventory is updated from a fail assertion to a pass assertion with updated text and normalized_id.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Suggested labels

Networking, status: rfr

Suggested reviewers

  • jyaunches

Poem

🐰 A proxy test now sees the truth—
The tunnel's fine, but policy says "nope,"
ERR_PROXY_TUNNEL means it tried, forsooth!
Success at routing, blocked upstream in scope.
One error caught, one assertion changed,
The test now knows what wiring rearranged. ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: treating ERR_PROXY_TUNNEL as a successful proxy wiring outcome in the M12 test.
Linked Issues check ✅ Passed The PR fully implements the objectives from issue #3836: recognizing ERR_PROXY_TUNNEL and PROXY_CONNECTION_LOST as pass outcomes in the M12 test, distinguishing proxy wiring success from upstream proxy policy blocks, and updating test expectations and documentation accordingly.
Out of Scope Changes check ✅ Passed All changes directly address the linked issue #3836: modifying test logic to handle ERR_PROXY_TUNNEL correctly and updating corresponding documentation files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@wscurran

wscurran commented May 20, 2026

Copy link
Copy Markdown
Contributor

✨ Thanks for submitting this detailed PR about treating ERR_PROXY_TUNNEL as proxy wiring success in the M12 test. This PR proposes a way to modify the test-messaging-providers.sh script to recognize ERR_PROXY_TUNNEL as a pass outcome, proving that NemoClaw's proxy integration is functioning correctly.

Related open issues:

This was referenced May 20, 2026
Merged
Merged
Merged
Merged
Merged
Merged
@kagura-agent
fix(e2e): treat ERR_PROXY_TUNNEL as proxy wiring success in M12 test
a62fab6 
The M12 reachability test in test-messaging-providers.sh conflates
proxy wiring validation with upstream proxy policy. When a proxy
correctly forwards the CONNECT request but the proxy's ACL returns
403 Forbidden, Node.js emits ERR_PROXY_TUNNEL. This proves proxy
wiring is correct (traffic routed through the proxy), but the test
treated it as a failure because it only recognized HTTP status codes
as success and specific network errors as skip-worthy.

Add a new branch that recognizes ERR_PROXY_TUNNEL and
PROXY_CONNECTION_LOST as proxy-wiring-success outcomes, since they
prove Node.js honored the HTTPS_PROXY env var and sent the CONNECT
through the configured proxy endpoint.

Fixes NVIDIA#3836

Signed-off-by: kagura-agent <kagura.agent.ai@gmail.com>
@kagura-agent kagura-agent force-pushed the fix/3836-proxy-test-conflation branch from e824f49 to a62fab6 Compare May 22, 2026 00:14
@kagura-agent kagura-agent mentioned this pull request May 23, 2026
Open
@jyaunches jyaunches closed this May 27, 2026
@jyaunches jyaunches reopened this May 27, 2026
@kagura-agent

kagura-agent commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @wscurran for reviewing and linking the related issue! Happy to adjust the approach if needed.

@kagura-agent

kagura-agent commented May 31, 2026

Copy link
Copy Markdown
Contributor Author

Friendly ping — this has been open for 11 days. Happy to adjust the approach if needed, or I can close it if the timing isn't right. 🙏

@kagura-agent

kagura-agent commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Friendly ping — this PR has been open for 13 days. Let me know if there's anything I should adjust. 🙏

@wscurran wscurran added the v0.0.58 Release target label Jun 2, 2026
@cv cv self-assigned this Jun 3, 2026
@wscurran wscurran added area: e2e End-to-end tests, nightly failures, or validation infrastructure bug-fix PR fixes a bug or regression feature PR adds or expands user-visible functionality labels Jun 3, 2026
@kagura-agent

kagura-agent commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Closing this — it's been open for 14 days with no review activity. Happy to reopen or submit a fresh PR if there's interest in this approach. Thanks! 🙏

@wscurran wscurran removed the feature PR adds or expands user-visible functionality label Jun 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@cv cv

Labels

area: e2e End-to-end tests, nightly failures, or validation infrastructure bug-fix PR fixes a bug or regression v0.0.58 Release target

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Nemoclaw][All Platforms] Proxy test conflates wiring with proxy policy: Node https.get reports ERR_PROXY_TUNNEL when upstream proxy returns 403

4 participants

@kagura-agent @wscurran @cv @jyaunches