Accept full base URLs in OpenAIClient and collect --llm-endpoint

[surajsharan]

Summary

--llm-endpoint is documented as an OpenAI-compatible endpoint URL, but OpenAIClient built its base URL as f"{endpoint}:{port}/v1", so --llm-endpoint http://localhost:8000 failed with Invalid port: '8000:8000' and a URL with a path (LiteLLM proxy, vLLM behind a reverse proxy, a gateway prefix) could not be expressed at all.

endpoint is now parsed as a URL:

• /v1 is appended when the URL has no path; a URL with a path is used as-is (the OpenAI SDK convention). http://localhost:8000 and http://localhost:8000/v1 are equivalent.
• port is appended only when the URL names none. An explicit port that differs from the URL's port raises instead of being silently overridden.
• Only http(s) URLs with a host are accepted. A double or empty port, a port outside 1-65535 (in the URL or via port), credentials, query strings and fragments raise a ValueError naming the URL with any credentials redacted; openenv collect reports them as a usage error before anything is written to --output-dir. Query strings are rejected because the OpenAI SDK appends the request path to the raw base URL, which turns /v1?api-version=1 into path /v1 with query api-version=1/chat/completions.
• The ("http://localhost", 8000) form used by existing callers, the rubrics tutorial and the factory is unchanged.

Behaviour change to note: --llm-port now defaults to unset instead of 8000, so a bare --llm-endpoint http://localhost no longer gets :8000 injected. That default was never documented and it broke every full URL without an explicit port (https://api.groq.com/openai became https://api.groq.com:8000/openai). The example's --llm-port follows the same rule. To keep this visible, openenv collect prints the resolved LLM endpoint: when it starts, and --help and the CLI reference state that there is no default and that earlier releases assumed 8000.

Also documents the collect command in the CLI reference (it was missing) and shows the self-hosted teacher form in the SFT warmup tutorial and the harness README.

Closes #1188

Type of Change

• Bug fix
• Documentation

Alignment Checklist

• I have read .claude/docs/PRINCIPLES.md and this PR aligns with our principles
• I have checked .claude/docs/INVARIANTS.md and no invariants are violated
• I have run bash .claude/hooks/lint.sh and tests and addressed all issues

RFC Status

• Not required (bug fix, docs, minor refactoring)

No new API: LLMClient.port becomes optional and endpoint accepts what its docstring already called "the base URL".

Test Plan

Unit (PYTHONPATH=src:envs pytest tests/core/test_llm_client.py tests/test_cli/test_collect.py):

• LLMClient.base_url for endpoints with a port, a path, a trailing slash, IPv6; invalid forms (double port, empty port, missing or non-http scheme, missing host, unclosed IPv6, query string, fragment, credentials, out-of-range port in the URL or via port) raise Invalid endpoint URL; the error never echoes a credential; a conflicting explicit port raises.
• OpenAIClient resolves http://localhost:8000, .../v1, .../v1/ and ("http://localhost", 8001) to a /v1 base URL, and leaves http://proxy:4000/litellm, https://api.groq.com/openai/v1 and Gemini's /v1beta/openai untouched.
• End to end through the real AsyncOpenAI with an httpx.MockTransport: complete() requests <base>/chat/completions for each accepted endpoint form.
• openenv collect --llm-endpoint with each URL form reaches AsyncOpenAI with the expected base URL and prints the resolved endpoint; an invalid or conflicting endpoint or port exits 2 with a --llm-endpoint usage error, without echoing credentials, and neither metadata.json nor a rollout is written.

Full suite: 2551 passed, 157 skipped, 2 failed (PYTHONPATH=src:envs pytest tests/ -q). The two TestProtocolWebSocketClient failures are pre-existing on main (integration tests, excluded in CI) and unrelated.

Live: vLLM serving Qwen/Qwen3.5-4B on http://localhost:8000, reasoning_gym_env server on :8001.

• Before: openenv collect reasoning_gym:chain_sum --llm-endpoint http://localhost:8000 ... → Error: Invalid port: '8000:8000'.
• After, each of --llm-endpoint http://localhost:8000, --llm-endpoint http://localhost:8000/v1 and the old --llm-endpoint http://localhost --llm-port 8000 form: Collected=2 failed=1 avg_reward=1.000 success_rate=100% over 3 episodes, identical results.jsonl shape (prompt, parsed answer tool call, reward 1.0 from the env). vLLM ran with --enable-auto-tool-choice --tool-call-parser qwen3_xml --reasoning-parser qwen3.
• The one failed episode per run is RuntimeError: Event loop is closed on the second episode; it reproduces on unpatched main with the split form, so it is a separate collector issue and not touched here.

Claude Code Review

N/A

[cursor]

Stale comment

Release-manager review at head ea38caec. The underlying defect is real and the fix is the right shape: base_url no longer produces http://host:8000:8000 or drops a /v1 prefix, the port conflict is rejected instead of silently overridden, and _openai_base_url correctly leaves a gateway path alone. Test coverage across test_llm_client.py and test_collect.py is good. Three things to resolve before this can go in.

1. The promised malformed-URL validation does not actually validate the URL. _join_endpoint_port only checks "://" not in endpoint and then whether an existing port parses. That means ftp://host and http:///v1 (no authority at all) both pass and are returned as an "endpoint", so the ValueError/typer.BadParameter the docstring and the --llm-endpoint error path promise never fires — the failure just resurfaces later inside the OpenAI SDK with a worse message. Please require an http/https scheme and a non-empty hostname. While you are there, reject userinfo in the authority (http://user:token@host): --llm-endpoint values get echoed into logs and persisted into rollout metadata, so a credential-bearing URL should not be accepted silently.

2. The appended port is never range-checked. When the URL names no port, port is interpolated straight into the netloc with no 1 <= port <= 65535 check, so --llm-port 0 and --llm-port 99999 build an endpoint that only fails later. urlsplit().port raises for an out-of-range port in a URL, so the two paths disagree about what a valid port is. A shared check would fix both.

3. --llm-port changing from 8000 to unset is a public default change. In both openenv collect and examples/ttt_collect_with_llm.py the default moves from 8000 to None. Anyone running --llm-endpoint http://localhost today reaches port 8000; after this they reach port 80, with no error and no warning — it will look like the server is down. That is defensible as part of "the endpoint is now a full base URL", but it needs to be deliberate: either keep the old default for the pathless case, or keep the change and call it out in the PR description so it lands in the release notes.

One thing to double-check rather than a blocker: the docstring says query and fragment are preserved, and there is a test asserting a query-bearing base URL is supported. The OpenAI SDK joins relative paths (chat/completions) against base_url, and that join discards the query, so a ?x=1 base URL probably does not behave as the test implies once a real request goes out. Worth either an end-to-end assertion against the SDK's constructed request URL, or rejecting query/fragment and passing those parameters through a supported SDK seam.

For context on timing: this is not in the 0.5.0 candidate (#1190) and is not holding the release, so there is room to get it right. Repository CI has not run on this head yet either — it needs a maintainer to approve the workflow run.

View PR

  

_{Sent by Cursor Automation: Release}

[cursor]

Re-reviewed at exact head eccef788. All three items from my 2026-09-17 review are genuinely fixed, and I verified each one by running the code rather than reading it.

1. URL validation is real now. _join_endpoint_port requires an http/https scheme and a non-empty host, rejects userinfo, query and fragment, and catches the empty-port case that urlsplit silently tolerates. Confirmed rejections: ftp://host, http:///v1, localhost:8000, http://user:token@host, http://host?x=1, http://host:. The credential case is reported as Invalid endpoint URL 'http://***@host', so the token never reaches the log line, and there is a test pinning that.

2. Ports are range-checked on both sides. --llm-port 0 and --llm-port 99999 are rejected before any request, and an in-URL out-of-range port is rejected too. One cosmetic wrinkle: the in-URL case surfaces urlsplit's own wording (Port out of range 0-65535) while your own check says 1-65535. Worth normalizing eventually; not worth another cycle.

3. The default change is now deliberate. --llm-port help says "No default: earlier releases assumed 8000", and collect prints the resolved LLM endpoint: before running, so a user who relied on the implicit 8000 sees port 80 immediately instead of debugging a silent connection failure. Moving the model-step construction above RolloutSerializer is a good side effect: an invalid endpoint now fails before any output directory is written.

The earlier query-string question is resolved by rejecting query and fragment outright, which is the right call given the SDK appends relative paths to base_url.

What I ran locally at this head: tests/core/test_llm_client.py and tests/test_cli/test_collect.py — 105 passed. Round-trips: http://localhost + 8000, http://localhost:8000/v1 + None, http://localhost:8000 + 8000, and https://gw.example.com/openai/v1 all produce the expected base URL, and the OpenAI SDK ends up with http://localhost:8000/v1/ in each self-hosted case. Hosted providers are unaffected: create_llm_client("openai", …) still yields https://api.openai.com/v1/.

Still not mergeable, for two reasons outside the code:

• Repository CI has never run on this branch. As a fork PR it sits at action_required, so only Bugbot reported; a maintainer has to click "Approve and run" before required checks exist.
• LLMClient.__init__ now takes port: int | None and the --llm-port default moves from 8000 to unset. That is a public API and CLI behavior change, so it needs a maintainer's explicit sign-off rather than mine, and it needs a release-note line when it ships — I will carry it into the next release notes once it lands.

My earlier change request is withdrawn; the code is ready from my side.

  

_{Sent by Cursor Automation: Release}

[bot-ci-comment]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[surajsharan]

Thanks for running CI. The failures were the six new invalid-endpoint cases in tests/test_cli/test_collect.py: Typer colours its error panel when GITHUB_ACTIONS is set, and the escape codes split the text those tests match on. The code under test was fine. Reproduced locally with GITHUB_ACTIONS=true, fixed by stripping ANSI codes before matching, and the full suite passes with the CI dependency set in both plain and GitHub Actions mode.