fix(cli): harness-arg forwarding, provider/env-key resolution, serve errors, proxy bypass#107
fix(cli): harness-arg forwarding, provider/env-key resolution, serve errors, proxy bypass#107elyasmnvidian wants to merge 4 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 |
Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
1 participant
CLI fixes. Rebased onto
mainafter #98 ("serve libsy algorithms without profiles") landed — #98 added its own clean duplicate-model-id message, so that fix is dropped here.launch <agent> -- <args>forwards documented harness args after a single--. Before,main()stripped the first--wherever it was, solaunch claude -- --versionmade argparse reject--versionand exit 2. Now only a--that comes before the subcommand token is stripped, so the harness--survives intoargparse.REMAINDER. Same fix coverscodex/openclaw.configureandconfigure --list-modelshonor the saveddefault_provideragain (--providerno longer defaults toopenrouterat the argparse level), and non-interactiveconfigureresolves an API key fromOPENROUTER_API_KEY/NVIDIA_API_KEY/OPENAI_API_KEYor secrets.json instead of demanding--api-key.servewith neither--confignor a saved bundle points at--config PATHfirst, and names--routing-profiles PATHas the legacy route bundle.127.0.0.1readiness and health probes (wait_for_proxy_ready,ProxyHealthMonitor, andverify's round-trip) bypass env proxies, so a configuredHTTP_PROXYno longer breaks the loopback checks. The upstream backend probe still honors env proxies.Regression tests cover each behavior.