code-mode: define process host wire protocol - #29804
Conversation
| use tokio::io::AsyncWriteExt; | ||
|
|
||
| /// Maximum JSON payload size accepted for one IPC frame. | ||
| pub const MAX_FRAME_BYTES: usize = 8 * 1024 * 1024; |
There was a problem hiding this comment.
this is low no? Looks lower than current code mode
There was a problem hiding this comment.
Bumped to 64MiB to match exec-server protocol. I chose something small because the usual token limit for output is around 10_000 tokens ~40kb so was just padding for large tool calls. We can go higher even if needed.
| #[serde(rename = "execute/initialResponse")] | ||
| InitialResponse { | ||
| id: RequestId, | ||
| result: WireResult<RuntimeResponse>, |
There was a problem hiding this comment.
Can we avoid putting the session/domain types directly on the V1 wire? These nested types use their default permissive Serde forms, so the outer deny_unknown_fields does not apply and internal changes can silently alter V1
There was a problem hiding this comment.
Yeah, sorry. dumb miss.
| @@ -94,16 +100,36 @@ fn handshake_wire_contract_is_explicit_and_round_trips() { | |||
| fn session_lifecycle_wire_contract_is_explicit_and_round_trips() { | |||
There was a problem hiding this comment.
Can we pin every V1 variant here?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc071daeec
ℹ️ 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".
| /// The complete wait request shape supported by protocol V1. | ||
| #[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct WireWaitRequest { |
There was a problem hiding this comment.
How do we replay a canceled/lost wait response? Once the host receives the frontier it has consumed that output, and retrying advances to the next frontier
I think this is out of scope but we will need to answer this one day
There was a problem hiding this comment.
out of scope for now, we crash on this today.
I think we can either do 3 way handshake here to confirm transfer and release resources on the sender. Alternative we keep a ringbuffer and read offsets. But yeah lets think about this some more.
| pub enabled_tools: Vec<WireToolDefinition>, | ||
| pub source: String, | ||
| pub yield_time_ms: Option<u64>, | ||
| pub max_output_tokens: Option<usize>, |
There was a problem hiding this comment.
nit can you use i32 here please? For a lot of reason we should avoid unsigned int (should be very small to fix)
| use serde::Serializer; | ||
| use serde::de::Error as _; | ||
|
|
||
| pub type RequestId = u64; |
There was a problem hiding this comment.
type aliases are dangerous. Either keep just i64 (please no unsigned int when possible) or use something like
#[serde(transparent)]
pub struct RequestId(u64);
e0fac5a to
125a59b
Compare
125a59b to
c52c04b
Compare
Why
The process-owned code mode implementation needs an explicit, bounded wire contract before either side depends on it. Keeping framing and message semantics in
codex-code-mode-protocolgives the client and sidecar one shared source of truth and makes compatibility failures detectable during connection setup.What changed
Testing
just test -p codex-code-mode-protocolStack
Part 1 of 6. Followed by #29805.