fix(cli): forward harness args after --, honor saved provider, bypass proxies on loopback probes#107
fix(cli): forward harness args after --, honor saved provider, bypass proxies on loopback probes#107elyasmnvidian wants to merge 5 commits into
Conversation
WalkthroughChangesConfigure defaults
Loopback health checks
CLI robustness
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@switchyard/cli/configure_command.py`:
- Around line 425-437: Restrict the environment key and base-URL candidates
passed by the configure flow around resolve_provider_connectivity() to the
selected provider, so provider nvidia cannot consume an OPENROUTER_API_KEY (and
vice versa). Preserve provider/key consistency before assigning
prompt_default_api_key, and add a regression test covering mixed environment
variables with --provider nvidia.
In `@switchyard/cli/switchyard_cli.py`:
- Around line 1463-1470: Update the argument scanning around _SUBCOMMANDS,
sep_idx, and cmd_idx to skip values consumed by global options such as
--routing-profiles before identifying the real subcommand. Ensure a value equal
to a subcommand name is not treated as the command, then remove only a "--"
separator occurring before that resolved subcommand while preserving separators
after it.
In `@tests/test_launcher_proxy_bypass.py`:
- Around line 30-33: Update the proxy setup in the affected test to use a
test-owned proxy stub or dynamically allocated local port instead of hard-coded
127.0.0.1:9. Ensure the endpoint reliably returns a connection error for the
negative proxy case, and apply the dynamically selected endpoint to both
HTTP_PROXY and http_proxy.
In `@tests/test_user_config.py`:
- Around line 346-358: Update the test setup before saving the user
configuration by calling _clear_provider_env_vars(monkeypatch). Place it after
setting SWITCHYARD_CONFIG_DIR and before save_user_config so
resolve_provider_connectivity() cannot use ambient provider credentials instead
of the saved Nvidia credentials.
🪄 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: 8cc12026-f435-4d2f-8989-8c77ebe7ef06
📒 Files selected for processing (9)
switchyard/cli/configure_command.pyswitchyard/cli/launchers/launcher_runtime.pyswitchyard/cli/launchers/proxy_health_monitor.pyswitchyard/cli/switchyard_cli.pyswitchyard/server/verify.pytests/test_launch_claude.pytests/test_launcher_proxy_bypass.pytests/test_serve_profile_config.pytests/test_user_config.py
| monkeypatch.setenv("HTTP_PROXY", "http://127.0.0.1:9") | ||
| monkeypatch.setenv("http_proxy", "http://127.0.0.1:9") | ||
| monkeypatch.delenv("NO_PROXY", raising=False) | ||
| monkeypatch.delenv("no_proxy", raising=False) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use a test-owned or dynamically allocated proxy endpoint.
The test assumes 127.0.0.1:9 is unused. If a service is listening there, the pre-fix path may not fail deterministically. Start a local proxy stub that returns an error, or allocate an unused port for the negative proxy case.
🤖 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 `@tests/test_launcher_proxy_bypass.py` around lines 30 - 33, Update the proxy
setup in the affected test to use a test-owned proxy stub or dynamically
allocated local port instead of hard-coded 127.0.0.1:9. Ensure the endpoint
reliably returns a connection error for the negative proxy case, and apply the
dynamically selected endpoint to both HTTP_PROXY and http_proxy.
There was a problem hiding this comment.
The test asserts wait_for_proxy_ready returns True, which only happens if the loopback probe ignores the proxy and reaches the health server. Port 9 (discard) won't answer as a working HTTP forward proxy, so if the bypass ever regressed the probe would fail through :9 and the test would go red. What the test checks doesn't depend on :9 being unbound, so I'll keep the fixed port rather than spin up a stub proxy thread.
There was a problem hiding this comment.
@elyasmnvidian, understood. The test will catch the expected regression as long as port 9 is the normal discard service—or otherwise not an HTTP forward proxy. My concern is limited to the atypical, host-dependent case where another process provides a functioning proxy on that fixed port; using an owned endpoint would remove that dependency. I’ll leave the decision with you.
2e74ffb to
21551d1
Compare
Live before/after — CLI behaviorThese are CLI-behavior fixes, so the reproduction is command output rather than token usage. Ran the same commands on 1) 2) 3) Harness args after a single The doubled- The saved-provider, non-interactive env-key, and Audit follow-up in this push: |
21551d1 to
783977a
Compare
Reworked onto the new main (post-#98) — live before/after#98 landed and rewrote the 1) 2) Harness args after a single The saved-provider, non-interactive env-key, and |
3dce996 to
e57e5e0
Compare
e57e5e0 to
2323193
Compare
elyasmnvidian
changed the title
…, clarify serve errors Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com> (cherry picked from commit 967f72b)
Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com> (cherry picked from commit aad16ca)
Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
2323193 to
dca3cb1
Compare
2 participants
CLI bug fixes for
launch,configure, and the loopback health probes. No serve or routing behavior changes.Rebased onto current
main. The earlier serve-error wording change is dropped:mainremoved the--configserve flag when it deleted components-v2 (#119), soservekeeps its existing--routing-profilesmessage.launch <agent> -- <args>forwards harness args after a single--.Before,
main()stripped the first--anywhere in the command, solaunch claude -- --versionmade argparse reject--versionand exit 2. Now only a--that comes before the subcommand token is stripped; a--after it survives intoargparse.REMAINDERand reaches the harness. Same fix coverscodexandopenclaw.configurehonors the saveddefault_providerand can read the key from the environment.--providerno longer defaults toopenrouterat the argparse level (the default is nowNone), soconfigureandconfigure --list-modelsstop overwriting a saveddefault_provider. When you pass no--api-keyand none is saved, non-interactiveconfigurereads the key for the selected provider from<PROVIDER>_API_KEY/<PROVIDER>_BASE_URLorsecrets.json. The lookup is scoped to the chosen provider, soconfigure --provider nvidiacan't save anOPENROUTER_API_KEYundernvidia; if that provider's own key isn't set,configurestill requires--api-key.Loopback health probes bypass env proxies.
wait_for_proxy_ready,ProxyHealthMonitor, andverify's round-trip send their127.0.0.1requests through an opener/client that ignoresHTTP_PROXY/HTTPS_PROXY, so a configured proxy no longer breaks the local readiness and health checks. The upstream backend probe still honors env proxies.Unchanged:
servewith--routing-profilesroute bundles, and every non-loopback request.Regression tests cover each behavior:
tests/test_launch_claude.py,tests/test_user_config.py,tests/test_launcher_proxy_bypass.py.