fix: accept full LLM endpoint base URLs (fold #1189) - #1238
Draft
cursor[bot] wants to merge 3 commits into
Draft
cursor[bot] wants to merge 3 commits into
cursor[bot] wants to merge 3 commits into
Conversation
`--llm-endpoint` is documented as a URL, but OpenAIClient built its base
URL as f"{endpoint}:{port}/v1", so a URL that already named a port failed
with "Invalid port: '8000:8000'" and a URL with a path (LiteLLM proxy,
reverse-proxied vLLM, a gateway prefix) could not be expressed at all.
Parse endpoint as a URL: append /v1 only when the URL has no path, append
`port` only when the URL names none, reject a conflicting explicit port,
and raise a clear ValueError for malformed endpoints. The CLI reports
those as usage errors before writing to --output-dir, and --llm-port now
defaults to unset so full URLs without a port are left alone. The
("http://localhost", 8000) form is unchanged.
Document `openenv collect` in the CLI reference and show the self-hosted
teacher form in the SFT warmup tutorial and harness README.
Closes #1188
Address review on #1189: - Require an http(s) scheme and a host. `ftp://host` and `http:///v1` used to pass validation and only fail later inside the SDK. - Range-check the port (1-65535) whether it comes from the URL or from `port`/--llm-port, so both paths agree on what a valid port is. - Reject credentials in the URL and never echo them: the endpoint is printed, written to metadata.json and rendered into the dataset card. - Reject query strings and fragments. The OpenAI SDK appends the request path to the raw base URL, so `/v1?api-version=1` was requested as path `/v1` with query `api-version=1/chat/completions`. The tests that claimed this form worked are replaced by one that captures the URL the SDK actually requests through a mock transport. - Print the resolved LLM endpoint when `openenv collect` starts and say in --help that --llm-port has no default, so the move away from the implicit 8000 is visible rather than looking like a dead server.
Typer renders usage errors in colour when GITHUB_ACTIONS or FORCE_COLOR is set, so on CI the escape codes split "--llm-endpoint" and the error text the new invalid-endpoint tests look for, and all six cases failed. Strip ANSI codes before flattening the error panel.
6 tasks
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This branch has not been deployed
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
1 participant
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
On-repo fold of #1189 for the 0.6.0 cut. Accepts full base URLs in
OpenAIClient/collect --llm-endpoint, validates endpoint URLs/ports (rejects credentials and query strings), and strips ANSI from CLI error matching underGITHUB_ACTIONS.Same three commits as #1189, rebased onto current
main(c32c3174). Fork #1189 was refreshed onto main (34220c4d) but fork Actions never start without Approve-and-run (this token gets 403), so this on-repo PR is how we get exact-head CI and merge into the release candidate.Ben: merge into release. Prior APPROVEs: automation @
eccef788/8f516fc6,burtenshaw@e421de47.Type of Change
Breaking (documented): implicit port
8000is removed for bare hosts; callers should usehttp://localhost:8000or--llm-port 8000.Alignment Checklist
.claude/docs/PRINCIPLES.mdand this PR aligns with our principles.claude/docs/INVARIANTS.mdand no invariants are violatedtests/core/test_llm_client.py+tests/test_cli/test_collect.py= 105 passed)RFC Status
Test Plan
PYTHONPATH=src:envs pytest tests/core/test_llm_client.py tests/test_cli/test_collect.pyClaude Code Review
N/A — mechanical rebase of already-reviewed #1189 for release fold.