Conversation
acoliver
changed the title
acoliver
changed the title
janfeddersen-wq
left a comment
janfeddersen-wq
left a comment
There was a problem hiding this comment.
Thanks for this — the transport work here is careful (the header fix in serdes-ai-streaming, the WS/SSE parsing, the terminal-event discipline and the fake-server tests are all good). I ran it locally and everything you claim checks out: fmt clean, clippy clean under -D warnings, 48/48 tests in serdes-ai-responses, 63/63 in serdes-ai-streaming.
The reason I'm requesting changes is the overall shape rather than the code quality: conversation state stored inside the Model instance breaks the framework's stateless Model contract, and the crate duplicates a Responses API implementation we already have. Details below, with what I'd suggest instead.
Blocking
1. Duplicates OpenAIResponsesModel. serdes-ai-models/src/openai/responses.rs already ships OpenAIResponsesModel with ResponsesApiRequest, ResponseOutputItem and previous_response_id support. This PR adds a parallel crate with ~1,800 lines of Responses API types and conversion in types.rs / convert.rs. Issue #65 explicitly asked for "reuse, not duplication". The shape I'd like to see is a WebSocket transport plus an opt-in session-chaining mode added to the existing OpenAIResponsesModel, not a second model in a second crate.
2. A second conversation on the same model is silently chained onto the first, with its input dropped. Session (serdes-ai-responses/src/client/mod.rs:78-84) stores only previous_response_id and sent_requests: usize, and continuation_skip (mod.rs:263-274) skips by count without checking that the prefix matches what was actually sent. Agent::run starts every run with fresh history, so agent.run("A") followed by agent.run("B") sends B as {previous_response_id: resp_A, input: []}. I reproduced this against a scripted fake server: the second frame had previous_response_id = Some("resp_A") and zero input items. The unit test an_empty_turn_serializes_input_as_a_list (mod.rs:885-905) codifies this failure mode. Fix: fingerprint the sent prefix (e.g. hash each ModelRequest) and reset the chain on mismatch, or make chaining opt-in per conversation rather than per model instance.
3. Whole-turn session lock serialises all concurrent runs. run_ws_turn holds inner.session.lock() for the entire turn (mod.rs:283), and the task spawned by request_stream holds it until the consumer drains the channel (mod.rs:590-620). One shared model therefore means one conversation at a time, which conflicts with the orchestrator / a2a fan-out that just landed on main. Same root cause as (2).
4. Release breaks. serdes-ai/Cargo.toml adds open-responses to the full feature, but .github/workflows/publish.yml has no tier for serdes-ai-responses, so cargo publish -p serdes-ai would fail on an unpublished dependency.
5. Needs a rebase onto main. #64 just merged and this branch conflicts in Cargo.toml (workspace members / workspace deps) and Cargo.lock. Both trivial. After resolving locally, clippy is clean and the tests pass under edition 2024, but cargo fmt --check fails with ~25 hunks from the 2024 import ordering, so run cargo fmt after rebasing.
Non-blocking
- Docs contradict the code:
lib.rs:8, the crate README "Transports" section andCHANGELOG.mdall describe{"type":"response.create","response":{…}}, but the code sends flat frames (mod.rs:60-67), which your PR body says was the live fix. - Root
README.md: a✅in the provider table was replaced with[OK](around line 315). error.rs:1still says "Error types for the Open Responses server", and server-flavouredstatus()codes ship in the default build.Cargo.toml:tokio-tungstenite = "0.21"andtokio-stream = "0.1"bypass the workspace deps;reqwestis listed in both deps and dev-deps.- Dependency weight: the default build pulls
rustls 0.22viatokio-tungstenite 0.21while workspacereqwestusesrustls 0.23, soserdes-ai --features fullnow links rustls 0.21, 0.22 and 0.23.axum/towerare correctly confined totest-server(checked withcargo tree). The rig (server.rs,store.rs,engine.rs,websocket.rs, ~1,700 lines) is excluded from the default build, but a full server inside a client crate is a lot; atests/rig/module would be cleaner. - HTTP stream: a
[DONE]without a priorresponse.completedreturnsOk(())with noStreamComplete(mod.rsaround line 810), violating the "exactly one terminal event" contract. HTTP errors map toModelError::httpwhile WS errors map toModelError::provider; pick one. - Builder methods panic via
Arc::get_mut().expect(...)(mod.rs:191-230), which is unusual for this codebase. examples/codex_haiku.rshardcodeswss://chatgpt.com/backend-api/codex/responses(:35), theOpenAI-Beta/originatorheaders (:230-232), a macOS-onlyopen(:80) and a plaintext token cache (:46-51). I'd trim the published example toOPENAI_API_KEYagainstwss://api.openai.com/v1/responsesand leave the codex path behind an env var.
Suggested path forward
Keep the serdes-ai-streaming header/TLS fix as its own small PR (that one is ready to merge on its own). Then bring the WebSocket transport and stateful-session mode into OpenAIResponsesModel in serdes-ai-models, with chaining keyed to the actual conversation rather than the model instance. Happy to review that quickly.
|
Thanks for running it locally, and for the detailed review. You're right on all five blocking points. I confirmed each against the trees, including your chaining repro: Plan, following your suggested path:
Non-blocking items: the flat-frame doc contradictions, the README ✅, the |
…feddersen-wq#66) rustfmt's edition 2024 style rules reordered imports and calls across the crate after rebasing onto the janfeddersen-wq#64 dependency-upgrade main.
b53a672 to
1386f3d
Compare
The client kept one session per model instance: a single socket, one previous_response_id, and a count of already-sent requests. A second agent.run through the same model chained onto the first conversation's last response, and the shared sent-count then skipped the new conversation's input down to nothing: the turn went out carrying the first conversation's continuation id and an empty input, so the reply had nothing to do with the new prompt. The whole-model turn lock also serialized every conversation, though only turns of the same conversation need to be sequential. Conversation state now lives in per-conversation records, keyed by the fingerprint of the history's first request: a DefaultHasher over the serialized request parts, in-process only (never persisted, never sent on the wire; DefaultHasher is not stable across processes). Each record carries its own socket, continuation id, and the fingerprints of the requests the server already holds. A turn's history must extend that recorded prefix exactly; a mutated or truncated history voids the chain and forces a full replay, so a continuation id is only offered when the server actually holds the prefix. Turns of one conversation still serialize on the record's lock while different conversations run concurrently, each on its own socket. The HTTP paths key and chain per conversation the same way. (janfeddersen-wq#66)
…esponses from full (janfeddersen-wq#66) tokio-tungstenite was pinned at 0.21 in serdes-ai-responses while the workspace itself moved to 0.30 (PR janfeddersen-wq#70), keeping a second tungstenite generation in the lockfile. Both tokio-tungstenite and tokio-stream now come from [workspace.dependencies], and the reqwest dev-dependency that duplicated the main one is gone. tungstenite 0.30 wraps text and ping payloads in Utf8Bytes/Bytes, so the fake-server send sites in tests/websocket.rs convert with .into(). Removing open-responses from the serdes-ai `full` feature list keeps `cargo publish -p serdes-ai` working while serdes-ai-responses has no publish tier in publish.yml yet. The standalone `open-responses` feature stays, so the facade re-export is still reachable.
…feddersen-wq#66) The client sends {"type":"response.create","model":...,"input":[...]} with the turn parameters flat on the frame root; the crate docs, the crate README and the changelog still described a nested {"type":"response.create","response":{...}} wrapper. The error module doc now says "client" instead of "server", matching what the crate is, and the macros row in the root README feature table recovers its check mark that an earlier edit had replaced with [OK].
|
Update on this branch: rebased onto Also landed here per your non-blocking list: flat-frame doc corrections (lib.rs, crate README, CHANGELOG), the README ✅ restore, the The streaming header/TLS fixes are up separately as #73. Next: folding the WebSocket transport and session mode into |
WebSocketConfig::with_auth stored headers that never reached the wire: connect_async received only the URL, so authenticated endpoints rejected every handshake. Build the handshake request explicitly, apply the configured headers to it, and bound the connect with the configured timeout so a hanging endpoint fails fast instead of stalling the caller indefinitely. The websocket feature also refused every wss:// URL with 'TLS support not compiled in' because no TLS backend was enabled behind it. TLS is part of the wss contract rather than an optional extra, so the feature now enables rustls with native root certificates. Verified against tokio-tungstenite 0.30 as upgraded on main. (#66)
|
Re-reviewed at Status of the earlier items
New in the fix commits
What would make the consolidated version mergeable
Verified locally at |
Adds serdes-ai-responses, an Open Responses profile server that exposes
any serdesAI Model through the OpenAI Responses API, so standard clients
(including the codex CLI with wire_api="responses") can drive serdesAI
agents without new client code.
- POST /v1/responses returns JSON, or SSE when stream:true (data frames
ending with [DONE]); POST /responses aliases it for codex base_url use.
- GET /v1/responses/{id} retrieves stored responses; GET /v1/responses
upgrades to the WebSocket transport: response.create frames in, the
same events out, one turn at a time, sequence_number restarting per
turn, 60-minute connection lifetime enforced between turns.
- Stateful mode: store:true (default) persists responses in a pluggable
ResponseStore and turns chain via previous_response_id, with chained
instructions replacing stored ones. store:false on a WebSocket keeps
state connection-local (the codex default): nothing persists globally,
chaining still works on the socket, and a failed continuation evicts
the id so the client replays full input.
- codex wire compatibility: unprefixed mid-stream event names, usage on
response.completed, and retryable error codes
(previous_response_not_found, websocket_connection_limit_reached).
- Hosted tools, background:true and item_reference inputs are rejected
by design; only client-side function tools are brokered.
Exposed via the serdes-ai facade feature open-responses (part of full).
…sen-wq#65) The crate shipped as a server; the product need is the client side. serdesAI could already call Responses over HTTP+SSE, but had no WebSocket transport, no session-stateful mode, and no codex-endpoint story. Agents talking to codex/Open Responses backends resent full history every turn. OpenResponsesModel implements Model over both transports: - WebSocket: response.create frames, session-held previous_response_id, store:false + delta-only input. The assistant echo the caller appends to history is not re-sent on chained turns (the server already has it); previous_response_not_found clears the chain and replays; connection lifetime limits or socket death trigger reconnect + replay. Recovery stops once any event reached the caller, so output is never duplicated. - HTTP: store:true chaining, SSE parsing for request_stream, codex endpoint shape via explicit URL + bearer + extra headers. - serdes-ai-streaming: connect() now applies configured headers to the upgrade request (auth was silently dropped) and bounds the handshake. The server stays as a wire-accurate test rig behind non-default 'test-server'; its between-turn TTL now always allows a connection's first turn so reconnect recovery is testable. ResponseStore trait, GET /v1/responses/{id}, and the POST /responses alias are gone.
…anfeddersen-wq#65) Review follow-up on the OpenResponsesModel client and its test rig; each fix closes a gap the shipped tests could not see. - ResponsesTool::Function serialized without the required "type":"function" tag, so emitted tool definitions were invalid Responses wire form and did not deserialize back. A marker field now emits and requires the tag. - run_http_stream read session.sent_requests directly instead of continuation_skip, so a chained streaming turn re-sent the assistant echo the server already holds. It also returned non-2xx bodies unparsed, which left a stale previous_response_id behind: one 404 stranded the session permanently. The status-line loop now mirrors the non-streaming path (parse the error code, clear the chain, replay once); replay stays impossible once SSE events have escaped, so output is never duplicated. - Test rig: InMemoryResponseStore evicted on a check-then-act race (concurrent puts could oversubscribe capacity) and both stores evicted by wall-clock stored_at, where same-instant ties made eviction order nondeterministic. Eviction is now check+evict+insert under one write lock ordered by a monotonic insertion counter. - Test rig: a chained turn's function_call_output could not resolve a tool name for a call made in an earlier turn (call_ids were learned only from the current input); stored history now seeds the map. New tests cover the mid-stream no-replay invariant, chained streaming delta-only input, and the function-tool wire round-trip.
The client was rig-verified only; the first live run against wss://chatgpt.com/backend-api/codex/responses (OAuth via the new codex_haiku example) found five wire divergences. All are corrected, the rig now mirrors the real backend, and the example streamed a full response from gpt-5.6-luna with usage before this commit. - serdes-ai-streaming: the websocket feature now compiles TLS in (rustls, native roots). Without it every wss:// URL failed at connect time with "TLS support not compiled in". - response.create frames are flat: model and the turn parameters sit on the frame root, not under a "response" wrapper. The live backend reads model from the root and rejected nested frames with "The 'None' model is not supported when using Codex with a ChatGPT account". - mid-stream event names are prefixed on the wire (response.output_text.delta, not output_text.delta); terminal names already matched, which masked the mismatch until a live turn completed with usage but empty text. - error envelopes accept the live spelling (status, error.type, optional code) so real backend errors surface as Provider errors instead of being skipped as unparseable frames and blind-retried. - websocket retry causes are logged per attempt and the last one is carried into the exhausted-retries error, so live failures are diagnosable. The rig strips HTTP-only keys (stream, stream_options, background) that codex sends instead of rejecting the frame. The rig's forbidden_keys test became stream_keys_are_ignored and the frame fixtures moved to the flat shape. Examples gain dev-deps on serdes-ai-providers (PKCE login) and tracing-subscriber.
…sen-wq#65) Text streaming was already live-proven; tool calls were not, and the wire path has three legs no rig can vouch for: the backend accepting our tools array, function_call events parsing with arguments intact, and a chained turn resolving a function_call_output against the session state. codex_haiku gains a `tools` mode that runs the whole loop: offer get_weather, model calls it (call_id + streamed arguments), the example answers locally, and a chained second turn returns the final answer. Verified against the production backend with gpt-5.6-luna: arguments arrived non-empty, the chained turn resolved the call, and the answer used the returned values. Empty arguments or a textless second turn fail loudly so the mode keeps working as a regression tripwire.
A chained turn skips the assistant reply the server already holds. When that leaves nothing new, the input was built as a Text variant holding an empty string, which an untagged enum serializes to "" rather than []. The codex backend rejects it with "Input must be a list", so the turn fails with an invalid_request_error. The agent loop reaches this whenever it calls again with the reply as the last message, which is why it showed up on a second turn in a tool-carrying conversation and not on a plain one. The empty case is now an empty list. There is nothing to add to a chained turn that adds nothing, and the server already has the rest.
…feddersen-wq#66) rustfmt's edition 2024 style rules reordered imports and calls across the crate after rebasing onto the janfeddersen-wq#64 dependency-upgrade main.
Commit e675674 accidentally tracked three scratch files: a code-review dump, its stderr companion, and a commit message draft. They are workflow artifacts, not source. This restores the state merged to the remote in PR #4 so the rebase force-push does not undo it, and adds a tmp/ ignore rule to prevent a recurrence. The files remain on disk, now untracked.
The client kept one session per model instance: a single socket, one previous_response_id, and a count of already-sent requests. A second agent.run through the same model chained onto the first conversation's last response, and the shared sent-count then skipped the new conversation's input down to nothing: the turn went out carrying the first conversation's continuation id and an empty input, so the reply had nothing to do with the new prompt. The whole-model turn lock also serialized every conversation, though only turns of the same conversation need to be sequential. Conversation state now lives in per-conversation records, keyed by the fingerprint of the history's first request: a DefaultHasher over the serialized request parts, in-process only (never persisted, never sent on the wire; DefaultHasher is not stable across processes). Each record carries its own socket, continuation id, and the fingerprints of the requests the server already holds. A turn's history must extend that recorded prefix exactly; a mutated or truncated history voids the chain and forces a full replay, so a continuation id is only offered when the server actually holds the prefix. Turns of one conversation still serialize on the record's lock while different conversations run concurrently, each on its own socket. The HTTP paths key and chain per conversation the same way. (janfeddersen-wq#66)
…esponses from full (janfeddersen-wq#66) tokio-tungstenite was pinned at 0.21 in serdes-ai-responses while the workspace itself moved to 0.30 (PR janfeddersen-wq#70), keeping a second tungstenite generation in the lockfile. Both tokio-tungstenite and tokio-stream now come from [workspace.dependencies], and the reqwest dev-dependency that duplicated the main one is gone. tungstenite 0.30 wraps text and ping payloads in Utf8Bytes/Bytes, so the fake-server send sites in tests/websocket.rs convert with .into(). Removing open-responses from the serdes-ai `full` feature list keeps `cargo publish -p serdes-ai` working while serdes-ai-responses has no publish tier in publish.yml yet. The standalone `open-responses` feature stays, so the facade re-export is still reachable.
…feddersen-wq#66) The client sends {"type":"response.create","model":...,"input":[...]} with the turn parameters flat on the frame root; the crate docs, the crate README and the changelog still described a nested {"type":"response.create","response":{...}} wrapper. The error module doc now says "client" instead of "server", matching what the crate is, and the macros row in the root README feature table recovers its check mark that an earlier edit had replaced with [OK].
) responses.rs has grown to 1595 lines and is about to receive the shared Open Responses wire and event model. Converting it to the responses/mod.rs module layout first keeps that landing a pure addition of sibling files, with no diff churn inside the existing model code and no changes to the public paths serdes_ai_models::openai::OpenAIResponsesModel or serdes_ai_models::openai::responses::.
…anfeddersen-wq#66) The rig-side serdes-ai-responses crate currently owns the only copy of the Open Responses wire types, stream events, and their translation onto serdesAI model stream events, which forces the model layer to depend on rig-side plumbing to speak the protocol. Landing these types in serdes_ai_models::openai::responses (wire.rs for requests, response objects, tools, and error envelopes; events.rs for StreamEvent and the translator) gives the model crate its own protocol vocabulary so later stages can move the OpenAI Responses model onto it without a reverse dependency. Types are copied verbatim from serdes-ai-responses, with the error envelopes decoupled from ResponsesError; the rig-side copies stay in place until stages S6/S7 remove them.
…ddersen-wq#66) The unified OpenAI Responses model could not carry the websocket transport without the pieces that make session turns work: client-side history conversion, per-conversation continuation state, and the frame/retry machinery. Move all three from serdes-ai-responses so the websocket path can function behind a feature flag. - convert.rs gains the client half of the protocol crate's conversions (history_to_wire and friends), with a single-variant ResponsesError so the moved code keeps its error surface and stable wire code. - session.rs holds the Transport enum, the fingerprint-keyed conversation map (std::sync::Mutex, held only for lookup/insert, never across an await), Conv::plan chain alignment, and the event sinks. - ws.rs (responses-ws feature) sends the flat response.create frame and recovers stale continuations, dead sockets, and connection-limit errors before any caller-visible event escapes. request()/request_stream() branch on the transport: websocket turns run through the moved paths, non-chaining HTTP stays byte-identical, and chaining over HTTP fails fast until that stage lands. Selecting the websocket transport without the feature fails at request time with a configuration error. Errors surfaced by the unified model now carry the "openai" provider tag instead of the crate's "open-responses".
…anfeddersen-wq#66) The websocket transport's recovery paths decide whether a conversation survives provider failures: a stale continuation must replay the full input, the connection lifetime limit must reconnect without losing the turn, ordinary provider errors must surface with their wire code, and once a stream event has escaped to the caller no retry may replay a frame. A live rig cannot be coaxed into these states on demand, so the tests script fake servers that observe the exact frames the client sends and assert each behavior against the unified model behind the responses-ws feature. - stale_continuation_clears_chain_and_replays_full_input: the 404 previous_response_not_found path clears the chain and the retry carries every item with no continuation id. - connection_limit_error_reconnects_on_a_fresh_socket: the 429 websocket_connection_limit_reached path starts a fresh session on a new socket. - hard_error_surfaces_as_model_error: a non-recoverable envelope becomes ModelError::Provider carrying the wire code. - mid_stream_error_is_surfaced_without_replay: after a delta reaches the caller, a would-be-recoverable error surfaces as an error item instead of triggering a replay (300ms frame-silence assertion).
…feddersen-wq#66) The responses client is validated against a wire-accurate fake server, not hand-built stubs, and that fake lived as feature-gated product source in serdes-ai-responses. The janfeddersen-wq#66 review asked for it to move to test support so the client crate sheds its only non-product surface; stage S4a lands the rig inside serdes-ai-models ahead of the client test migration, where the wire and event model it serves already lives. - tests/rig/{engine,server,store,websocket}.rs move over byte-for-byte; only import paths change: wire types now come from serdes_ai_models::openai::responses::{wire,events} and sibling modules reference each other through super::. convert.rs carries the server half (request-to-history, response-to-output items); the client half stays in serdes-ai-responses until S7 deletes it. - rig/error.rs keeps ResponsesError and its status/code/body mapping; the envelope types are the wire model's own, so the inherent from_error constructors become a local FromResponsesError extension trait and call sites keep their shape. - mod.rs hosts the spawn helpers and recording_model from the old tests/common; any test binary can mount the rig with `mod rig;`. - responses_rig.rs proves the rig end-to-end with the basic JSON turn; the remaining rig tests port in S4b. serdes-ai-responses is untouched; its copy of the rig keeps compiling and its tests keep passing until the crate is retired. The moved unit tests run twice during the transition (12 new: 5 engine, 3 store, 3 error, 1 smoke), so the workspace count grows from 1761 to 1773.
…n-wq#66) The wire-accurate Open Responses rig now carries its full raw-protocol suite next to the client tests that consume it: sequential websocket turns, session-local store:false state, continuation errors, forbidden key stripping, ping/pong, and the connection lifetime limit over the socket; SSE framing, stateful HTTP chaining, and the error envelopes over HTTP. The basic JSON turn was already the adopted smoke test, so only the remaining thirteen tests move; serdes-ai-responses stays untouched until its remaining suites land here.
…ddersen-wq#66) The old client suite's rig end-to-end tests (delta-only chained sends, terminal-last streaming, TTL reconnect, function-tool type tags, conversation isolation and concurrency, chain reset on mutated history) now run against the adopted rig here, and the scripted fake servers share one helper module with the recovery-path tests. The four fake recovery tests already ported in the previous stage are not duplicated. HTTP-transport tests stay ungated and run with chaining off: chaining over HTTP is not implemented yet, so each turn replays full input and the tests pin the same history lens a chained turn would produce, and HTTP streaming's buffered fallback is drained through its PartStart content until real SSE deltas land.
…ersen-wq#66) The responses model kept a private request mapping alongside the wire model it already ships, so the two could drift: role:"tool" messages where the API expects function_call_output items, last-wins system prompts, and reasoning that lost its encrypted payload between turns. One shared mapping now serves every transport. - ResponsesApiRequest carries the wire forms directly: input is a list of wire::InputItem, tool_choice is the wire type, and stream becomes Option<bool> that HTTP always sends and websocket frames omit. parallel_tool_calls joins as an optional key. - history_to_wire, tool_to_wire, and tool_choice_to_wire from convert.rs are the only path from ModelRequest history to request items, for HTTP and the flattened response.create frame alike. compose_request applies a per-transport overlay (skip, stream, store, continuation id, service tier, truncation) over the shared mapping. Unsupported media parts now fail the request with InvalidRequest instead of being silently dropped. - System prompts join into one instructions string instead of the last prompt winning. Legacy map_messages, convert_user_*, convert_tool_return, convert_response_to_input, and the ResponseInput/ResponseInputContent/ResponseInputPart/ResponseTool types are deleted; a workspace-wide grep finds no remaining users. - Completed-output mapping moves into parts_from_output: reasoning items keep their encrypted_content, stashed in thinking-part provider details and replayed on the next request as a wire reasoning item. Tool returns map to function_call_output items. The two input-serialization tests move to the new wire forms, and seven pins cover the deltas: joined system prompts, typed-item assistant echo with encrypted reasoning replay, dropped retry prompts, function_call_output mapping, unsupported-media rejection, stream key presence by transport, and the wire forms over real HTTP. Workspace suite: 1826 passed, 0 failed (baseline 1819).
…es model (janfeddersen-wq#66) The unified responses model rejected chaining over HTTP, so opting into session state still meant unchained turns on the default transport, and HTTP streaming stayed a buffered fallback. The responses crate's HTTP client now ports into the unified model, and one session-state implementation serves both transports. - Chained HTTP turns share the websocket conversation state: every turn persists with store:true (the crate's client never sent store:false over HTTP, so a fresh conversation's first full-replay turn persists too), a completed turn records its response id, and the next turn sends only the new input items under previous_response_id. A previous_response_not_found envelope before any output escapes clears the chain and replays the full input, bounded by MAX_ATTEMPTS. - Chained streaming runs real SSE through the shared event translator and enforces the terminal-event contract: a [DONE] sentinel arriving without response.completed/incomplete/failed, or a body ending without one, surfaces as an error instead of ending silently. A completed/incomplete event advances the chain; a failed one does not. - Error envelopes map onto ModelError::Provider with the wire code on every HTTP path, matching the websocket mapping: the non-chaining fallback previously degraded code-only Open Responses envelopes to bare status errors because the legacy parser demands a `type` field. The bare status error remains for bodies without any envelope. - Non-chaining HTTP is byte-identical, including the buffered streaming fallback, whose comment now marks it deliberate instead of a TODO. The four HTTP client tests flip back to chaining-on with the original chained assertions, and new pins cover what the rig cannot observe: the store:true/delta-only/stale-replay wire shapes, envelope and non-envelope error mapping on both chaining paths, and the [DONE] contract against a test-only malformed-SSE rig route. Workspace suite: 1831 passed, 0 failed (baseline 1826).
…n-wq#66) The responses client already lives as openai::responses in serdes-ai-models (wire, events, http, ws, session), so the standalone crate duplicated every type and its ~54 tests re-tested ported equivalents. Keeping one implementation removes the drift risk between the two copies. - Delete serdes-ai-responses; its tests were ported to serdes-ai-models in earlier commits. - Move the codex_haiku smoke example to serdes-ai-providers/examples, remapped onto OpenAIResponsesModel with Transport::WebSocket and session chaining. It defaults to wss://api.openai.com/v1/responses with OPENAI_API_KEY; the codex backend (PKCE OAuth, codex headers) activates behind --codex or CODEX=1. The example lives in providers because a serdes-ai-providers dev-dep on serdes-ai-models would break models' publish in the dependency-ordered release (models publishes first); providers' dev-deps resolve at its own, later turn. - Facade: replace the open-responses feature with openai-responses-ws = ["serdes-ai-models/responses-ws"] (part of full); drop the serdes_ai::responses re-export. - Fix test doc comments that still pointed at the removed crate. Publish dry-runs for models/providers/facade fail only on the pre-existing condition that the 0.3.0 suite is not yet on crates.io (latest published is 0.2.6); no new resolution hazard introduced. BREAKING CHANGE: crate serdes-ai-responses removed; facade feature open-responses replaced by openai-responses-ws.
The janfeddersen-wq#65/janfeddersen-wq#66 changelog bullets still presented the responses client as a standalone crate that S7 removed, so readers would hunt for a package that no longer exists. The entries now describe the same work as additions to serdes-ai-models (websocket transport behind responses-ws, session chaining on both transports, wire-accurate building, rig as test support, trimmed codex_haiku example) plus explicit Breaking entries for the crate removal, the facade feature rename, and the request-mapping behavior changes. The models README gains a Responses API section covering the transport feature, session chaining, and the new example home; the root README drops the removed crate row and renames the facade feature. Also repairs pre-existing scrambled prose in the models README where the "Streaming fallback boundary" heading had been inserted mid-sentence, orphaning the paragraph tail below the migration bullets.
…anly (janfeddersen-wq#66) Four maintainer follow-ups on the consolidated responses session model. A continuation must add material to the history the server holds. Re-sending a history identical to the recorded chain used to chain onto the old response with empty input; Conv::plan now treats full prefix-equality as a replay of the same prompt and resets the chain, so the full input goes out with no continuation id and the conversation re-chains from the replay's response. The ported empty-turn wire test kept its `input: []` pin under the corrected rationale (only a truly empty history serializes an empty input now). Request fingerprints covered only the serialized parts, so requests differing only in ModelRequest.kind collided in the conversation map; the kind now joins the hash, keeping the in-process-only DefaultHasher guarantee documented. Conversation state was immortal and each conversation's websocket socket lived forever. Conversations now record their last use and are evicted at the next lookup once idle past a configurable TTL (default five minutes), their socket closed with a proper Close handshake first. Eviction is lazy on lookup: a whole-map scan is cheap at the expected scale of a few conversations per model and avoids a background task. Sockets discarded on reconnect, eviction, and model drop now perform the websocket close handshake instead of leaving the server with a ResetWithoutClosingHandshake: reconnect and eviction await the close in async context, and the model's Drop hands still-open sockets to a spawned close task when a tokio runtime is available (plain drop otherwise, documented). Drop only runs on the last model handle, so the clone each streaming task holds cannot tear down live sockets.
703c954 to
12dc469
Compare
acoliver
changed the title
|
The consolidation you asked for is on the branch now (rebased onto
PR description rewritten to match the consolidated shape. Still draft for now; will mark ready after our internal review. |
…n-wq#65) Keep HTTP and WebSocket authentication, header overrides, and event handling consistent so transport choice does not change request behavior or lose terminal errors. Separate session identity by the initial conversation prefix and tie socket cleanup and idle activity to the conversation lifecycle. Make the test rig cancel disconnected or saturated turns without persistence while preserving wire-correct instructions, tool calls, and reasoning. Protect token-cache writes before exposing credentials, respect token expiry, and invalidate rejected tokens without replaying requests or discarding a replacement token. Fixes janfeddersen-wq#75
|
Pushed Session lookup now includes the prefix through the first non-system request, so conversations sharing system boilerplate keep distinct sessions. Full-prefix validation and identical-history resets remain in place. Socket ownership, completion-based idle timestamps, and eviction of unused sessions are covered by regressions. The description now states the remaining identity limit: independent conversations with identical initial prefixes need separately constructed models. The rig now cancels disconnected or input-overflowed turns without persisting them, preserves instruction order and encrypted reasoning, and rejects unknown call IDs. The example protects cached credentials and invalidates rejected tokens without replaying output or tool effects. The findings-only OCR follow-up covered all 16 changed files. Its two retained findings were fixed with regressions. Final gates: formatting and workspace Clippy with warnings denied passed; workspace tests 1,804 passed / 0 failed, plus 9 example tests run separately. No live API credentials were used for this verification. The PR description reflects the current architecture and verification. It remains draft pending Andrew's sign-off. |
janfeddersen-wq
left a comment
janfeddersen-wq
left a comment
There was a problem hiding this comment.
Reviewed the consolidated version at 0452ce7. This is the shape we asked for and it fits the framework: the parallel crate is gone, the transport and session mode sit on the existing OpenAIResponsesModel behind the opt-in responses-ws feature, default behaviour is unchanged (HTTP, chaining off, full history, no conversation state), the default build of serdes-ai-models pulls no tungstenite, and the rig's axum/tower are dev-deps only. Approving. Three small doc fixes below and then we merge.
Earlier findings, all confirmed fixed
I re-ran the probes against ws_fakes_common (temporary tests, not committed):
- Identical history sent twice →
prev=None, full input, no reconnect (session.rs:125-144,ws_identical_history_restarts_the_chain). - Shared system prompt, different first user message → second socket, distinct sessions, first chain continues with its own
previous_response_id. - After the idle TTL, the next lookup closes the idle socket with a proper Close frame (
session.rs:317-349,ws_idle_conversations_are_evicted_with_a_clean_close). - Chaining off with
Transport::WebSocket→ full history every turn. - Drop closes via
Handle::try_current()and does not panic outside a runtime (session.rs:169-179). - Per-conversation
tokio::Mutex; the map'sstd::sync::Mutexis never held across an await (session.rs:292-305, 318-337). [DONE]withoutresponse.completederrors (http.rs:250-254); body end without a terminal errors too (:297).- Shared header/auth builder for both transports (
mod.rs:759, used athttp.rs:74andws.rs:81), which is the #75 fix. - No
Arc::get_mut().expectleft; the remainingexpects are invariant-guarded or on the never-awaited map lock. - Example defaults to
wss://api.openai.com/v1/responseswithOPENAI_API_KEY, codex behind--codex/CODEX=1.
Verification: cargo fmt --check clean, cargo clippy --workspace --all-features -D warnings clean, cargo test --workspace --all-features 1804 passed / 0 failed, git merge-tree against main clean.
Please fix before merge (docs only)
- README contradicts the #75 fix.
serdes-ai-models/README.md:63still does.with_header("Authorization", format!("Bearer {api_key}"))in the WebSocket example. Drop it; the constructor key now reaches the handshake. - CHANGELOG under-discloses the breaking change. The "Breaking" section lists the removal of
ResponseInput/ResponseInputContent/ResponseInputPart, butResponseTool,UserLocation,ContainerConfig,RankingOptionsandImageMaskare also removed from the publicopenai::responsesmodule, andResponsesApiRequest.toolschanged type toVec<wire::ResponsesTool>. Please list those. Conversely, the entries about removing theserdes-ai-responsescrate and theopen-responsesfacade feature can go: neither ever shipped in a release, so nobody can break on them. - Public
wiresurface.mod.rs:14exposesCreateResponseRequest/ResponseObject, which onlytests/riguses and which duplicateResponsesApiRequest/ResponsesApiResponse. Make thempub(crate)(and give the rig what it needs via a test-only path), or add a doc note saying they are the raw wire shapes and not the model API.
Noted, not blocking
- Non-chained HTTP keeps the legacy status-code mapping (
mod.rs:954-970, 401→auth, 429→rate limited) before trying the envelope, while chained HTTP goes straight toprovider_with_status(http.rs:124-136). Preserves old behaviour, but the two HTTP modes now differ. Fine for now; worth unifying later. - Two independent conversations with a byte-identical initial prefix on one instance share a session. The PR documents it, and fresh
UserPromptParttimestamps make it rare in practice. - With
Transport::WebSocketand chaining off, a socket is still kept per conversation prefix (ws.rs:71) until eviction. Acceptable, eviction handles it.
Because public types in serdes-ai-models are removed, this lands in the next minor bump rather than a patch release. Thanks for turning the whole restructure around this fast.
2 participants
Summary
Implements the client side of #65 on the existing
OpenAIResponsesModelinserdes-ai-models. It adds a WebSocket transport, wire-accurate request building, and opt-in session chaining on both transports. There is no new published crate; the protocol test rig lives underserdes-ai-models/tests/rig/.Closes #65. Fixes #75.
Architecture and behavior
HTTP and WebSocket requests share authentication and header construction. Constructor API keys and organization/project settings reach both transports. Explicit headers override defaults case-insensitively, including Codex bearer credentials. HTTP modes share their POST construction rather than maintaining separate header paths.
Both transports use shared event decoding. Unknown event types are ignored; malformed known events error. Streaming preserves terminal events and errors under backpressure. A
[DONE]sentinel withoutresponse.completedis an error. Provider envelopes retain their wire error codes, and committed streams are not replayed on failure.Session lookup fingerprints the initial history prefix through its first non-system request, including request kind. Distinct user conversations sharing leading system prompts no longer share one session. Full sent-prefix validation remains mandatory: mutated or identical histories reset the chain, while continuations send only new input with
previous_response_id. WebSocket turns sendstore: false; chained HTTP sendsstore: true.Content-based lookup cannot distinguish independent conversations with identical initial prefixes. Those require separately constructed model instances, not clones. Idle eviction is lazy at lookup, uses completion-based timestamps, and preserves reserved conversation handles. Conversation state owns socket cleanup; best-effort clean Close on drop requires an active Tokio runtime.
Request conversion joins system instructions, emits tagged function tools and
function_call_outputitems, and preserves encrypted reasoning. Unsupported media input errors instead of silently disappearing. The rig preserves instruction ordering and reasoning metadata, rejects unknown tool call IDs, and uses bounded, cancellable event delivery. Input queue overflow cancels stalled work rather than blocking disconnect handling.The
codex_haikuexample defaults toOPENAI_API_KEYandapi.openai.com; Codex remains opt-in through--codexorCODEX=1. Its cache uses private replacement files and expiry-aware reuse. Identified authentication rejection invalidates the rejected cached token without replaying output or tool effects. Argument parsing and browser launching now handle platform differences explicitly.API and feature changes
responses-wsinserdes-ai-models, exposed asopenai-responses-wsthrough the facade; select withwith_transport(Transport::WebSocket).with_session_chaining(true); idle TTL is configurable throughwith_conversation_idle_ttl.ResponsesApiRequest.inputuses wireInputItems; the formerResponseInput/ResponseInputContent/ResponseInputParttypes are removed.serdes-ai-responsescrate and facadeopen-responsesfeature are removed. No additional publish tier is needed.Verification at
0452ce7Feature checks also cover HTTP-only, WebSocket, and no-provider builds. The existing no-provider unused-import warning is tracked in #74. An intermittent unchanged CLI PTY test failure is tracked in #76; the final full workspace run passed.
The findings-only OCR follow-up covered all 16 changed files without skipped or failed files. Its two retained findings were subsequently fixed and regression-tested; no further review cycle was run. Current verification uses local protocol servers, not live API credentials.