[app-server] type client response payloads#20050
Conversation
7fafa13 to
9bca771
Compare
| self.outgoing.send_result(request_id, result).await; | ||
| self.outgoing | ||
| .send_result(request_id, result, ClientResponsePayload::LoginAccount) | ||
| .await; |
There was a problem hiding this comment.
Is there a way to avoid updating every callsite that sends an RPC response with the payload type? This seems boilerplatey, especially since the result is typed already.
Codex suggests:
Yes. The cleanest reduction is to make the protocol macro generate From for ClientResponsePayload for response types that are unambiguous, then change send_result/send_response to accept Into.
There was a problem hiding this comment.
There's one annoying gotcha, which is that we have at least one reused response type:
ConfigValueWrite => response: v2::ConfigWriteResponse
ConfigBatchWrite => response: v2::ConfigWriteResponse
but maybe there's a fix/workaround for this one?
0022641 to
a1caa34
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1caa34dbe
ℹ️ 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".
| self.analytics_events_client.track_request( | ||
| connection_id.0, | ||
| connection_request_id.request_id.clone(), | ||
| codex_request.clone(), | ||
| ); |
There was a problem hiding this comment.
Restrict request analytics to turn requests
Avoid tracking every initialized request here. track_request now clones and enqueues all ClientRequests, but the analytics reducer only processes TurnStart/TurnSteer and discards the rest. High-volume or large-payload methods (for example command/exec/write or fs/writeFile with data_base64) can saturate the 256-item analytics queue, causing dropped telemetry and skewed experiment data.
Useful? React with 👍 / 👎.
9bca771 to
87158fb
Compare
a1caa34 to
6a5df25
Compare
b771b23 to
9195578
Compare
7223fa2 to
e593ec4
Compare
e593ec4 to
1c6108c
Compare
1c6108c to
2536ff7
Compare
Why
pr17088adds typed server-originated request/response plumbing, but successful client responses are still erased into bare JSON-RPCresultvalues before app-server can make any typed decision about them.This precursor PR keeps successful client responses typed until the outgoing response seam. It is intentionally limited to protocol/app-server plumbing so the analytics behavior change can review separately on top.
What changed
ClientResponsePayloadas the pre-serialization client response body type.InterruptConversationJSON-RPC-only because it has noClientResponsevariant.Verification
cargo check -p codex-app-servercargo test -p codex-app-server-protocol