feat(templates/opencode): per-model token limits via LITELLM_MODEL_LIMITS - #488
Draft
mprokopov wants to merge 4 commits into
Draft
feat(templates/opencode): per-model token limits via LITELLM_MODEL_LIMITS#488mprokopov wants to merge 4 commits into
mprokopov wants to merge 4 commits into
Conversation
`filter_event_stream_tools` rebuilt the event-stream body with
`text.lines()` + `join("\n")`. That is not round-trip safe for trailing
newlines: `"a\nb\n\n".lines()` yields `["a","b",""]`, so joining produces
`"a\nb\n"` and silently drops one newline — exactly the blank line that
terminates an SSE event.
Effect: any MCP server registered WITH a non-empty `allowed_tools` returned a
`tools/list` frame that never closed. Real MCP clients (opencode 1.18.9, and any
client using a spec-compliant SSE parser) wait forever for the frame and report a
generic failure — opencode surfaces `{"status":"failed","error":"Failed to get
tools"}`. Servers with an empty `allowed_tools` were unaffected, because that
path streams the upstream bytes verbatim.
This was easy to misdiagnose: `curl` appears to work fine, since it just prints
bytes until the connection closes and never needs the terminator. The bug is only
visible byte-wise (`}}\n` instead of `}}\n\n`) or via a real client.
Switch to `split_inclusive('\n')`, which keeps each line's terminator, so blank
lines and the trailing newline survive. CRLF is preserved too.
Verified against GitHub's remote MCP server through LAP's proxy with 41
`allowed_tools` configured:
- before: 109722 bytes, tail `0a`, opencode -> "failed"
- after: 109723 bytes, tail `0a0a`, opencode -> "connected"
and an agent then successfully called `github_get_file_contents` end to end.
Adds two regression tests: one asserting the terminator survives filtering, one
asserting newline count is unchanged when nothing is filtered out.
Two problems stopped an opencode agent from using an MCP server that LAP itself
proxies at `/{alias}/mcp`:
1. `writeMcpConfig` wrote only `{type,url,enabled}` for remote servers, dropping
any `headers`. opencode's remote transport supports a `headers` object, so
there was no way to authenticate at all.
2. LAP's `normalize_mcp_servers` retains ONLY `{type,name,url}` on each server
entry, so headers configured on the agent never survive the trip to the
runtime — meaning (1) alone cannot fix it.
LAP's MCP proxy requires a gateway key, so the net effect was a guaranteed 401.
Forward `headers` when present, and otherwise attach the gateway key we already
hold for model calls — but ONLY when the MCP URL's origin matches
`LITELLM_BASE_URL`, so the key is never sent to a third-party MCP host.
Verified: agent on gemini-3.6-flash called `github_get_file_contents` through
LAP's proxy against GitHub's remote MCP server.
…routes Addresses both Codex review findings on LiteLLM-Labs#484. **P1 — origin-only check was too permissive.** Comparing `URL.origin` ignores the path, so a gateway mounted under a prefix on a shared host (`LITELLM_BASE_URL=https://co.example/litellm/v1`) made every other path on that host look LAP-owned — including an unrelated `https://co.example/customer-mcp`, which would then receive the gateway key (possibly the master key). Now the URL must sit under LAP's own root *and* match one of LAP's actual MCP routes: {root}/{alias}/mcp — dynamic MCP proxy {root}/mcp/platform/{agent_id} — platform MCPs Both enforce `require_any_gateway_key`, so both legitimately need the credential; anything else on the origin no longer gets it. The `/v1` model-API suffix is stripped from the base to derive the root, so path-mounted gateways still work. Note: scoping to only `/{alias}/mcp` was initially too narrow — it silently dropped auth for platform MCPs (`request_human_approval`, `agent_memory`, …), which live at `/mcp/platform/{agent_id}`. Caught by live verification, hence both shapes are allowlisted and covered by tests. **P2 — a non-auth header suppressed the fallback.** The guard tested whether `headers` was non-empty, so an entry carrying only e.g. `x-tenant` skipped the credential and LAP answered 401. It now checks specifically for an existing `Authorization` or `x-api-key` header (case-insensitive), and an explicit caller auth header is still never overwritten. Adds 9 tests to templates/opencode/test/opencode.test.mjs covering: LAP proxy and platform routes get the key; same-origin non-LAP path does not; other origins never do; path-mounted gateway still matches; query strings are ignored; non-auth headers coexist with the injected credential; explicit `Authorization` and lowercase `x-api-key` are preserved. Verified: `node --test test/opencode.test.mjs` → 11 passed, 0 failed; and live against a rebuilt runtime both `github` and `platform` MCP entries receive the credential while non-LAP URLs receive none.
…MITS
OpenCode looks models up in models.dev to learn their context and output
limits. A gateway-specific model name is not there, so opencode falls back to
a conservative output budget. A reasoning model then spends that budget
thinking and returns a response with no content block at all, which the
gateway surfaces as a 500 ("list index out of range") and which ends an agent
turn silently — the agent does all of its work and reports nothing.
Adds LITELLM_MODEL_LIMITS="<model>=<context>:<output>,..." so such models can
be given explicit limits. Only listed models get one; models opencode already
knows are left untouched so their real ceilings are not capped.
Both numbers are required, and the code refuses to emit a partial limit,
because opencode's schema rejects a `limit` missing `context` with
ConfigInvalidError — and it does so lazily. The server still starts and
reports healthy; every session create then fails with 400. A partial limit is
therefore worse than none, which is why this is enforced rather than defaulted.
Verified against gemini-3.6-flash behind a LiteLLM gateway: session create
returns 200 with both keys and 400 with only `output`.
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.
OpenCode reads a model's context/output limits from models.dev. A gateway-specific model name is not there, so opencode falls back to a conservative output budget. A reasoning model then spends that budget thinking and returns a response with no content block, which the gateway surfaces as a 500 (
list index out of range) and which ends an agent turn silently — the agent does all of its work and reports nothing.This adds
LITELLM_MODEL_LIMITS="<model>=<context>:<output>,..."so those models can be given explicit limits.That last point is the subtle one. OpenCode's schema rejects a
limitmissingcontextwithConfigInvalidError, but lazily: the server starts and reports healthy, and then every session create fails with a 400. A partial limit is worse than no limit, so this is enforced rather than defaulted.Verification
Against
gemini-3.6-flashbehind a LiteLLM gateway (provider idanthropic):POST /sessionlimit: { context, output }200limit: { output }only400 ConfigInvalidError … limit.context "Missing key"200node --test templates/opencode— 14 pass, including three new cases covering both-keys, partial/invalid rejection, and preservation ofensureProviderModelentries across a restart.Note: two tests in
templates/opencode/test/store.test.mjsfail on the base commit as well; they are unrelated to this change.Branched from
fix/mcp-sse-frame-terminator(#484) rather thanmain, since that is what the deployed runtime image is built from — happy to rebase ontomainif preferred.