Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements gateway-side emulation of Anthropic stop_sequences for the OpenAI Responses API (addressing issue #605). Since the Responses API lacks a native stop parameter, the gateway now intercepts and emulates stop sequences during the Responses-to-Anthropic translation. This is achieved by introducing a StopScanner that buffers and scans assistant text, truncating the output and immediately aborting the upstream connection (for both HTTP and WebSocket transports, streaming and non-streaming) when a match is found. The PR also updates documentation across multiple languages and adds comprehensive unit and integration tests to verify the truncation and connection abort behaviors. No review comments were provided, and the implementation is clean, well-tested, and fully aligned with the style guide; therefore, I have no additional feedback to provide.
![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: 9da6278020
ℹ️ 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".
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The OpenAI Responses API has no `stop` parameter (Chat Completions does), so the Anthropic `stop_sequences` field was silently dropped for every Responses-protocol upstream, over HTTP and WebSocket alike. Claude Code's auto-mode permission classifier depends on it (`</block>`, `</severity>`) and retried its first stage on every check routed to a Responses model. Emulate it gateway-side in the Responses -> Anthropic SSE translation: - `StopScanner` holds back at most `max_len - 1` bytes of assistant text (char-boundary safe) so a stop split across deltas is still caught; reasoning summaries and tool-call arguments are never scanned. - On a match the machine emits the text before it, closes the block, and ends the message with `stop_reason: "stop_sequence"` and the matched string; later upstream events are ignored and non-streaming output is truncated at the same point. No configured stop sequences leaves the output byte-identical. - Transports abort the upstream at the match: HTTP drops the byte stream, the WebSocket paths drop the event receiver (evicting the half-consumed pooled socket), and the non-streaming collectors return immediately. - A held-back prefix that never completes is flushed before the block closes, including when the first delta is entirely a prefix. Usage on such a turn carries the local input estimate and `output_tokens: 0`, because the upstream's completion event never arrives. Docs: provider page (en/ko/ja/zh-cn), config reference `kind` row, and docs/m1-responses-translation.md §8b. Closes #605
Four fixes from an ensemble review of the emulation added in #605. Abort the upstream only on an emulated stop. Both HTTP transports keyed their byte-stream drop on `is_stopped()`, which `stop_events` sets on every terminal — so an ordinary `response.completed` dropped the reqwest body before EOF, costing the connection its place in the idle pool on the overwhelming majority of turns, since `stop_sequences` is usually unset. Now keyed on `hit_stop_sequence()`, the distinction `ws_stream.rs` already made deliberately and that the translation doc already explained. Release the stop-scanner holdback on a backend `error`/`response.failed`. `close_any` was the only flush site, so up to `max_len - 1` bytes of text the client was owed were dropped silently. Extracted `flush_stop_holdback` and call it from both paths; the open block is still deliberately not closed on that arm, as before. Parse a final unterminated SSE frame in `json_response`. Switching from `upstream.text()` + `parse_sse_events` to the incremental `SseParser` lost the trailing frame a cut upstream leaves behind — possibly the last text delta or the `response.completed` carrying usage, under a 200 OK. Reattach `without_content_accumulation`'s doc comment, which the original commit left documenting `with_stop_sequences`.
…stop_sequences `classifier_model` (#608) documents itself as an in-provider remap because "the classifier carries `stop_sequences`, which the Responses translation drops". Emulating them on the Responses path makes that sentence false, and it appears in nine places — the Rust doc on the config field, the `classifier_model` row of the configuration reference, and the auto-mode classifier section of the Anthropic provider page, each across en/ko/ja/zh-cn. Keep the measurement that motivated the key — a classifier pointed at a Codex upstream retried its first stage and cost 5-7 s per permission check — as the historical observation it was, note that the emulation removes that specific failure, and state the in-provider constraint as the validation rule it actually is rather than deriving it from upstream behaviour that has changed. The configuration-reference cells drop the rationale entirely: the table states what the key accepts, and the provider page carries the reasoning. Locale pages link their own translated `stop sequences` anchors (`#정지-시퀀스`, `#ストップシーケンス`, `#停止序列`), since Astro derives heading ids from rendered text and an English fragment would be a dead link.
3b8cb61 to
8e70e66
Compare
…nses kind `stop_sequences` support is a user-visible capability of `kind = "responses"`, and AGENTS.md puts a capability change in the README in the same PR. The translation section already said what that kind does; this adds why `stop_sequences` survives it despite the Responses API having no `stop` parameter. English source plus the ko/ja/zh-CN translations, per the README translation rule.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebedaa3592
ℹ️ 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".
| pub(super) fn machine(self) -> AnthropicSseMachine { | ||
| AnthropicSseMachine::new(self.model, self.thinking_enabled, self.tool_search_native) | ||
| .with_stop_sequences(self.stop_sequences) | ||
| } |
There was a problem hiding this comment.
When an HTTP Responses request uses stream: false and the upstream continues generating after a stop marker, this routes the stop-aware relay into json_response, which awaits upstream.text() before it ever applies the machine. It therefore cannot detect the match, return to the client, or drop the connection until the upstream eventually closes; a long-lived upstream makes the request hang and continues billing post-stop output. This also makes the new documentation's claim that non-streaming HTTP aborts at the match false.
AGENTS.md reference: AGENTS.md:L51-L53
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Docs corrected in 93bc562 — the claim was false and Greptile flagged the same sentence independently, so thank you both.
On the code, though: this is not a regression from the stop emulation. json_response reads the whole body with upstream.text() on origin/main today (src/adapters/responses/http.rs:191), for every non-streaming turn, stop sequence or not. The client asked for a single JSON body, which is exactly the case AGENTS.md carves out — "do not buffer upstream SSE responses unless the client requested non-streaming output". A stop sequence does not make that buffering worse; it just cannot make it better, because the machine sees no event until the body is complete.
Making this arm abort means turning the non-streaming relay into an incremental parse, on a path this PR does not otherwise touch, with its own failover and backend-error surface to re-review. That is a separate change. What this PR owes is an honest description, so the docs now say plainly that non-streaming HTTP truncates at the match like every other path but cannot cut the upstream short.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Merging this PR will regress 2 benchmarks
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | resolve_model[gpt-5-codex] |
3.9 µs | 4.9 µs | -20.52% |
| ❌ | parse_body_to_value[50] |
756.7 µs | 850.1 µs | -10.98% |
| ⚡ | resolve_model[claude-sonnet-4-5-via-codex] |
5.5 µs | 4 µs | +38.05% |
| ⚡ | parse_body_to_value[10] |
186 µs | 166.2 µs | +11.92% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing amondnet/fix-responses-emulate-stop_sequences-on-the-resp (93e1d9d) with main (bd2cf38)
An emulated stop sequence makes every later event a no-op, including the `response.completed` that carries the upstream's usage. On a streaming turn that costs nothing — `forward` already seeds a local tiktoken estimate for `message_start`, and `final_json`/`usage_value` fall back to it whenever no usage was observed. A non-streaming turn emits no `message_start`, so the gate skipped the estimate entirely and `json_response` never received one: a stopped turn serialized `input_tokens: 0` for a non-empty prompt, where the same request without `stop_sequences` would have reported real usage. Widen `forward`'s gate to cover a non-streaming request that carries `stop_sequences`, and thread the resolved estimate into `json_response` the way `stream_response` already takes it. The pool path already had the value resolved in `relay_success` and simply was not passing it on. No new machinery: the fallback in `final_json` was already there. The estimate stays behind the provider's `count_tokens = "tiktoken"` opt-in, and is resolved with the same 1s `bounded_input_estimate` budget as the streaming path — on this arm there is no commit to protect, so it is resolved after the body read rather than overlapped with the round-trip.
… abort The rebase onto main took `http.rs` wholesale, so `json_response` reads the whole body with `upstream.text()` again instead of parsing incrementally. Both doc surfaces still claimed the abort covers every transport: - `docs/m1-responses-translation.md` said `http.rs` "breaks out of the body read (non-streaming)"; - the OpenAI provider page said the abort applies on "every transport (HTTP and WebSocket, streaming and non-streaming)". Neither is true. Streaming HTTP aborts (`sse_parse.rs` drops the byte stream rather than handing it to `spawn_terminal_drain`), and both WebSocket paths abort (`ws_stream.rs`), but non-streaming HTTP reads the body to EOF before the machine sees an event — it truncates at the match like every other path, it just cannot cut the upstream short. That is the pre-existing shape of the non-streaming relay, not something the emulation introduced. Greptile and Codex each flagged the stale claim independently. Also correct the usage caveat on both surfaces. It described only the streaming shape (`message_delta.usage`) while the section claimed to cover every transport; it now names the non-streaming shape too and states the `count_tokens = "tiktoken"` opt-in the estimate depends on. Updates the ko/ja/zh-cn copies of the provider page in the same change.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93bc56233b
ℹ️ 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".
… collector Two review findings on 476a200, both about the input estimate that a stopped turn depends on. Greptile: the non-streaming HTTP arm spawned the tiktoken encode only after the upstream responded, then awaited it before reading the body, putting tokenization on the critical path of every eligible request even when no stop sequence matched — and letting a saturated blocking pool add up to the full one-second bound. Now spawned before the send, like the pool loop's own handle, so the encode overlaps connect/RTT; the bounded await after the send normally resolves instantly. That also corrects 476a200's commit message, which claimed the estimate was "resolved after the body read". It was resolved before it. The claim was wrong when written. Codex: `forward`'s widened gate produces an estimate for a non-streaming websocket turn too, but `forward_websocket` awaited and passed it only on the streaming branch — `json_events_response` built an unseeded machine, so a stopped websocket turn still reported `input_tokens: 0`. The estimate is now resolved once before the branch and threaded into both arms. An audit of every `.machine()` construction site found these were the only two gaps: of the eight non-test sites, `json_events_response` was the sole one missing `with_input_estimate`. Covered by `json_events_response_reports_the_input_estimate_for_a_stopped_turn`, verified non-vacuous — removing the seed fails it on the assertion, not on a compile error.
Greptile, on f57d60d: hoisting the estimate above the streaming branch gave non-streaming websocket turns a wait they never had, and it was the bare `handle.await` the streaming arm has always used rather than the one-second `bounded_input_estimate` the HTTP and pooled paths use. The websocket transport is where that matters most. Unlike the HTTP arm, whose body stream has not been touched yet, the turn is already open by this point: blocking here stops the collector consuming events and backpressures the bounded `CodexWsEvents` channel until tokenization finishes. A large request or a saturated blocking pool turns a best-effort progress figure into a stall on the response itself. Now bounded, which also tightens the pre-existing streaming branch — noting that deliberately rather than burying it, since it is behavior this PR did not otherwise own. It is strictly the safer direction, it makes all four paths (HTTP streaming, HTTP non-streaming, pooled, websocket) share one discipline, and the estimate was always best-effort: `bounded_input_estimate` falls back to 0, the same value an encode failure already produced.
|
1 participant
Summary
The OpenAI Responses API has no
stopparameter (Chat Completions does), so the Anthropicstop_sequencesfield was silently dropped for every Responses-protocol upstream, over HTTP and WebSocket alike. Claude Code's auto-mode permission classifier depends on it (</block>,</severity>) and retried its first stage on every check routed to a Responses model.This change emulates
stop_sequencesgateway-side in the Responses → Anthropic SSE translation:StopScannerholds back at mostmax_len - 1bytes of assistant text (char-boundary safe) so a stop split across deltas is still caught; reasoning summaries and tool-call arguments are never scanned.stop_reason: "stop_sequence"and the matched string; later upstream events are ignored and non-streaming output is truncated at the same point. With no configured stop sequences the output stays byte-identical.Known limitation: usage on a stop-sequence turn carries the local input estimate and
output_tokens: 0, because the upstream's completion event never arrives.Closes #605
Milestone / spec
M1 —
docs/m1-responses-translation.md§8b (stop-sequence emulation).Checklist
cargo buildpassescargo testpasses (new behavior is covered; tests run without network/loopback where possible) —cargo test --all-features --workspace: 2726 passed, 0 failed, 2 ignoredcargo clippy --all-targets -- -D warningsclean (run as--all-targets --all-features)cargo fmt --all --checkcleansrc/model/stop_sequences.rsis 211 lines, but several touched files (src/model/responses.rs,src/adapters/responses/pool.rs,src/adapters/responses/http.rs) were already over the limit before this change and are not split heredocs/updated if this change deviates from it —docs/m1-responses-translation.md§8bsite/src/content/docs/providers/openai.mdxplus the ko/ja/zh-cn copies, andsite/src/content/docs/reference/configuration.mdplus its locale copies.README.mdhas no matching section, andCHANGELOG.mdis release-please generated, so both are intentionally untouched.Notes for reviewers
Summary by cubic
Emulates Anthropic
stop_sequenceson the Responses path, which the Responses API cannot accept. Previously the field was silently dropped for every Responses upstream, breaking Claude Code's permission classifier; now the gateway-side translation truncates assistant text at the first stop and ends the turn withstop_reason: "stop_sequence"on all transports (HTTP/WebSocket, streaming/non-streaming). Closes #605.Behavior
response.failedevent.output_tokens: 0, because the upstream's completion event never arrives; the estimate wait is bounded to one second so a saturated tokenizer can't stall the response.classifier_modelthat Responses dropsstop_sequences.Written for commit 93e1d9d. Summary will update on new commits.
Site build
site/is not built on pull requests by CI, so the site build was run locally:npm run buildcompleted cleanly, and the new "Stop sequences" section renders onproviders/openaiin all four locales (en, ko, ja, zh-cn).