fix(mcp): preserve SSE frame terminator when filtering tools/list - #484
Open
mprokopov wants to merge 3 commits into
Open
fix(mcp): preserve SSE frame terminator when filtering tools/list#484mprokopov wants to merge 3 commits into
mprokopov wants to merge 3 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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fede2a01ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…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.
Author
|
Both Codex findings addressed in P1 (origin too permissive): the gateway-key fallback is now scoped to LAP's actual MCP routes rather than its origin — P2 (non-auth header suppressed the fallback): the guard now looks for an existing One thing worth surfacing, because it argues for the tests: scoping to only Verification
Note on unrelated CI
|
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.
Fixes #483.
Two related fixes; together they make an MCP server that LAP itself proxies usable from a runtime. They are independent commits and can be split if you'd prefer separate PRs.
1.
fix(mcp): preserve SSE frame terminator when filtering tools/listfilter_event_stream_toolsrebuilt the event-stream body withtext.lines()+join("\n"), which is not round-trip safe for trailing newlines:"a\nb\n\n".lines()yields["a","b",""], so joining gives"a\nb\n"and drops one newline — the blank line that terminates an SSE event.Any MCP server with a non-empty
allowed_toolstherefore returned atools/listframe that never closed. Real clients hang and report a generic failure (opencode 1.18.9:{"status":"failed","error":"Failed to get tools"}). Servers with an emptyallowed_toolswere fine, becausehandle_responsestreams upstream bytes verbatim on that path — which made the bug look server-specific.Switched 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 the proxy with 41
allowed_tools:}}\nfailed}}\n\nconnectedAn agent then successfully called
github_get_file_contentsend to end.Adds two regression tests: one asserting the terminator survives filtering, one asserting the newline count is unchanged when nothing is filtered out.
2.
fix(templates/opencode): let agents reach LAP-hosted MCP serversTwo things blocked an opencode agent from using an MCP server proxied at
/{alias}/mcp:writeMcpConfigemitted only{type,url,enabled}for remote servers, dropping anyheaders. opencode's remote transport supports aheadersobject, so there was no way to authenticate.normalize_mcp_serversretains only{type,name,url}per server entry, so headers set on the agent never reach the runtime — meaning (1) alone can't fix it.Since the proxy requires a gateway key, the net effect was a guaranteed 401.
This forwards
headerswhen present, and otherwise attaches the gateway key the runtime already holds for model calls — but only when the MCP URL's origin matchesLITELLM_BASE_URL, so the key is never sent to a third-party MCP host.Happy to drop this commit if you'd rather solve it in
normalize_mcp_serversby allowingheadersthrough to runtimes; that would be the more general fix, but it changes the SDK contract.Testing
cargo test --lib→ 139 passed, 0 failedconnected, agent invoked a real MCP tool and returned the correct result.