Skip to content

fix: the second review pass — DLP failure tail, caps on every wire, native Responses stickiness, finish and refusal mapping - #126

Merged
CMGS merged 7 commits into
mainfrom
fix/codex-round-followups
Sep 23, 2026
Merged

CMGS merged 7 commits into
mainfrom
fix/codex-round-followups

Conversation

@CMGS

@CMGS CMGS commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Stacked on #125. A second review pass over the final tree (main → #125) raised five actionable items and two ownership/perf notes; a follow-up pass on this branch raised one more. Each was read against the source; the six hold and are fixed here, the two notes are declined with the reason below.

Commit Finding Defect Evidence
a redacted native Responses stream that fails ends with the error event DLP + response.failed under outbound DLP the native frames are replaced by a tail synthesized from the redacted reply, and that tail ended a failed stream with response.completed carrying status: failed and no error (#123 fixed the terminal row, not the client frames) e2e: a native stream that leaks an email and then fails now ends with event: error, no response.completed, no raw email
the output cap reaches the deadline on every Claude wire and on Responses the #124 deadline's scope Bedrock InvokeModel and Converse, and a Responses max_output_tokens, went through the shared send path with no cap, so the 16384 default still met the plain account timeout there unit: the cap sits on the engine base for the direct wire (16384 default), InvokeModel and Converse (client 300), and Responses (64). Live: jp.anthropic.claude-opus-5-5 over Bedrock InvokeModel with timeout_seconds: 20, non-streaming, no max_tokens, a 1200-word essay: before 408 model_timeout_exception after 20 s, after 200 after 39 s with 3730 completion tokens
a native Responses conversation without a user id stays on one variant the #124 key's scope a native Responses request keeps its input in the raw body, so the first turn was split by request id and the reasoning continuation pinned to the requested model handler: two native turns without a user id, the second replaying a reasoning item, both served by the variant. Live: gpt-5.4-mini canaried to gpt-5-mini (reasons by default), native /v1/responses, no user id, include: [reasoning.encrypted_content]: before, turn 1 served by gpt-5-mini with a reasoning item and the replay served by gpt-5.4-mini; after, both turns by gpt-5-mini
a tool-call reply keeps a content_filter or length finish chat_finish every finish but length became tool_calls when tool calls were present, so a filtered reply read as a normal tool call on the chat surface and in batch results unit rows: content_filter and max_tokens survive, only a normal stop becomes tool_calls
the failure message a redacted stream carries back is redacted too the second pass's follow-up the error tail copied the terminal error as the engine saw it while outbound DLP walked only the response, so a vendor error quoting request content carried the quoted text past the redaction e2e: an email only in the vendor's response.failed message comes back as [REDACTED_EMAIL] inside the error event
a Responses refusal part reaches the client as text (#117 reopened, fixed) #117's closing reason the typed response_format does not cross to the Responses body, but a text.format extra rides through like any extra, so structured outputs and their refusal parts are reachable off the native surface; the mapper read only output_text unit: a refusal part and a response.refusal.delta land in the reply text

Declined:

  • record_responses_usage clones the usage object: once per stream at its end, about a hundred bytes, and the native branch must keep the frame it forwards. Moving it means restructuring the completion arm around a shared borrow for a copy that costs less than the frame's parse. Kept.
  • batch_create on the in-memory store scans retained batches on every submit: the path is batch submission only, gated by the key's QPS, over the batches the single-node memory store kept in the last 30 days, and it is the same shape as video_job_put. No mechanism added.

Verification

  • Failing-first: each of the six commits' tests fails with its fix reverted and passes with it (checked one at a time on the final tree).
  • Every commit verified on its own: each of the 6 commits, exported fresh (git archive | tar -m), passes cargo fmt --all -- --check, cargo clippy --workspace --all-targets -- -D warnings and cargo test --workspace; the test count rises 679 → 684.
  • Linux: a rust:1 linux/arm64 container with rustc 1.98.0 at the final commit: fmt ✓, clippy ✓, 684 passed, 0 failed, 4 ignored.
  • Live before/after (in the table): the Bedrock deadline against the real jp endpoint, the native Responses canary against OpenAI. The DLP failure tail and the refusal text are mock-driven (a vendor mid-stream failure and a refusal cannot be triggered on demand); the finish rule is a pure mapping.
  • Hot path: one integer store per Claude and Responses body build (the cap), one JSON lookup on the variant path only when the request has neither a user id nor normalized turns (native Responses), one string compare in chat_finish, and nothing on the DLP-off stream path. No bench: none of these changes a served frame or adds an allocation on the success path.
  • Hygiene ledger: style, judge, comments and loc lenses at 0 due over 102 files and 12 flows.

Size

Production code +47 net (+65 −18), tests +180, docs 0. Comment lines in *.rs: 2 added (one-line docs on the new first_user_turn fallback and the dlp_redact_text helper), 0 removed. Whole-repo non-blank production Rust 26147 → 26193, comment density 8.75% → 8.74%. Per commit (prod net): the DLP failure tail +7, the cap on the engine base +9, the native conversation key +16, the finish rule 0, the refusal text +4, the redacted failure message +12.

@CMGS

CMGS commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator Author

Follow-up pass: one more finding held and is fixed as the sixth commit — under outbound DLP the error tail copied the terminal error's message unredacted, so a vendor error quoting request content could carry it past the redaction. The message now goes through the same outbound redaction; e2e pins an email that appears only in the vendor's response.failed message. Per-commit and Linux gates rerun at the new head (684 passed, 0 failed).

@CMGS
CMGS force-pushed the cut/test-dedup-and-small-cuts branch from bc4230f to 8e123d0 Compare September 23, 2026 12:54
@CMGS
CMGS changed the base branch from cut/test-dedup-and-small-cuts to main September 23, 2026 13:06
…r event

Under outbound DLP the native frames are replaced by a tail synthesized
from the redacted reply, and that tail ended a failed stream with
response.completed carrying status failed and no error. The tail now
closes with the stream's failure, so the client gets the error event as
it does on a live stream.
…Responses

The cap rode the request only from the direct Anthropic wire and OpenAI
chat; Bedrock InvokeModel and Converse, and a Responses
max_output_tokens, went through the shared send path with no cap, so
the 16384 default still met the plain account timeout there. The cap
now sits on the engine base, which every send path reads.
…ariant

The conversation key read only normalized turns; a native Responses
request keeps its input in the raw body, so the first turn was split by
request id and the reasoning continuation pinned to the requested model.
The key now falls back to the input's first user text.
chat_finish turned every finish but length into tool_calls when tool
calls were present, so a filtered reply read as a normal tool call on
the chat surface and in batch results. Only a normal stop becomes
tool_calls.
The reply mapper read only output_text parts and the stream only
output_text deltas, so a refusal under structured outputs came back as
empty content with a normal finish. A chat client can request
structured outputs by sending text.format as an extra, which rides
through to the Responses body, so the refusal path is reachable off the
native surface too; #117 was closed on the wrong reason. Refusal parts
and deltas are now the reply text.

Fixes #117
The error tail a redacted native Responses stream ends with copied the
terminal error as the engine saw it, while outbound DLP walked only the
response. A vendor error that quotes request content carried the
quoted text past the redaction. The terminal error's message now goes
through the same outbound redaction as the reply.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant