Strip params - #4
Merged
Merged
Conversation
The check job ran pull request code with a writable token, no time limit, and actions on mutable tags. Its lint step ran no linter at all. .github/workflows/ci.yml: - Read-only token. - persist-credentials: false, so the token is not left in .git/config. - timeout-minutes on both jobs, instead of the 6 hour default. - Actions pinned to commit SHAs. - Force-pushing a PR cancels the superseded run. - Installs golangci-lint from a pinned, checksummed tarball. The runner image does not ship one. Makefile: make lint now fails when golangci-lint is missing and CI is set, instead of falling back to go vet. .golangci.yml: new. Removes the default output limits, which cap findings at 3 of a kind and 50 per linter and keep only one per line. .github/dependabot.yml: new. Keeps the pins current. 23 lint fixes, so the job passes. Mostly unchecked error returns from Close and Fprintf. Three others: tint.NewHandler is deprecated, replaced with tint.NewTextHandler; a deliberately discarded config.Load return now uses _; one switch is now tagged. Fork PRs are gated by a repository setting rather than this file. It is set to require approval for all outside collaborators. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FnZG8whBis7HMNVNAWFxDr
TestNonStreamingTruncatedBody failed about one full-suite run in four, blaming
the client for a body the vendor cut mid-JSON:
Side = client (kind=client_disconnect_ctx), want upstream
verdict: your LLM tool hung up first. The vendor did not interrupt anything.
postmortem demoted an upstream fault to a client one by reading
rec.ClientGone() — live, mutable state — after the copy loop had already
reached its verdict. The client watcher stays armed until ServeHTTP returns,
well past that point, so any client close landing in the window retroactively
rewrote the answer.
The client was doing nothing wrong. The upstream sends a well-formed HTTP
response whose body ends mid-JSON: 64 bytes, Content-Length satisfied. The
proxy forwards all 64. The client sees a complete response and closes, which is
what any client without keep-alives does on every successful request. Whether
the vendor got blamed for its own truncated body came down to whether that FIN
was processed before one line of Go ran. Real clients hit this, not just the
test harness.
AsInduced is a fallback, not the main path. A client that hangs up mid-stream is
already handled structurally: the proxy cancels the upstream read itself with a
stamped ErrClientGone, and fromCancellation reads that cause — which is why
TestClientDisconnectsWhileUpstreamIsSilent can assert the verdict is not
induced. So the demotion only needs the narrower causal question, and now asks
it as a single atomic read: was the client gone, *and* were bytes still owed to
it? responseWasComplete already compares BytesToClient against
BytesFromUpstream, so the comparison is not a new idea here. A client that
received every byte read from the upstream interrupted nothing, whenever its FIN
arrives. The ordering stops mattering rather than being won.
Verified by forcing the race with a temporary sleep before the check: the old
predicate fails 10/10, the new one passes 10/10, and the forced failure is
character-identical to the flake. 0 failures in 12 consecutive full-suite runs
under -race, against 1 in 4 before.
The second ClientGone() use in postmortem is deliberately left alone. That one
wants the late observation, with completeness deciding.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016X5Bmd7YUcjf9ydMGMtWQg
nebul answers every request with:
{"message":"Validation: Unsupported parameter(s): `prompt_cache_key`",
"type":"Bad Request","code":400}
prompt_cache_key is sent unconditionally by the OpenAI SDK and by the tools
built on it. There is no client-side switch, so the proxy is the only place the
fix can go:
routes:
- name: nebul
upstream: https://api.nebul.example
strip_params:
- prompt_cache_key
This is the first code that edits what a client sent, which is a real departure
from the thing the README promises on the first screen. It is worth being
precise about what is being traded away and what is not.
What it does not touch: attribution. The copy loop still reads upstream and
writes downstream in that order, and a rewrite that happens before the first
attempt cannot blur which side an error came from. What it does touch is the
standing of a verdict. "The vendor rejected your request" means something
different when we changed the request, so every strip that removed something is
disclosed three times over — a warning at config load, a `rewriting requests:`
line in the banner and in -check, and `stripped=…` on the request line of the
report itself. A rewrite the report did not mention would quietly turn every
verdict on that route into a guess.
Kept narrow on purpose:
- Top-level keys only. Nested paths would need a selector language, and vendor
"unsupported parameter" errors name top-level parameters.
- Values are carried as json.RawMessage, so everything kept survives
byte-for-byte: number precision, string escaping, nested shapes. Only the top
level is rebuilt, which sorts keys and drops the client's whitespace. That is
the smallest edit that can remove a key.
- SetEscapeHTML(false), because encoding/json would otherwise rewrite every
<tag> in a prompt as \u003ctag\u003e. Semantically identical, gratuitously
different on the wire, and not small for the tag-heavy prompts agents send.
- Nothing happens unless a key is actually present. A configured route that
sees an unrelated body still gets byte-for-byte passthrough.
- Past max_request_body the body is streamed and was never buffered, so there
is no document to rewrite. Half a rewrite would corrupt it; the request goes
through intact and a strip_params_skipped warning says the shim did not run.
- model, messages and stream are refused at startup. Removing the first two
makes the request invalid on arrival and removing the third answers a client
that asked for SSE with a single JSON body — three ways to manufacture a
fault the proxy caused and then reports as the vendor's.
Applied to any JSON object body, not only /chat/completions: a vendor that
rejects a parameter rejects it on /v1/embeddings too, and anything that is not
a JSON object comes back untouched anyway.
Tests are end-to-end through the harness, per the rule that a hand-built
classifier input cannot fail when the wiring is missing: the nebul case, the
default still being byte-for-byte, no re-encode when the key is absent, value
preservation, a retry replaying the rewritten body identically, a non-chat
path, a non-JSON body, and the oversize-body warning. Verified by eye against a
fake vendor as well — the vendor received the body without the key and with its
<b> tags intact, under the report line `body=70 B stripped=prompt_cache_key`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ET59HnMDoFJcF89rhNYit
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.
Add a feature to strip parameters from requests (e.g. if an LLM vendor enforces some hard parameter validation, that breaks a client's requests)