Update default yield time #6610
Conversation
![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.
ℹ️ 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".
| #[serde(default = "default_login")] | ||
| login: bool, | ||
| #[serde(default)] | ||
| yield_time_ms: Option<u64>, | ||
| yield_time_ms: u64, | ||
| #[serde(default)] |
There was a problem hiding this comment.
The change on this struct makes yield_time_ms a bare u64, but ExecCommandRequest::yield_time_ms (core/src/unified_exec/mod.rs:71) still expects an Option<u64>. The struct literal at lines 162‑168 now tries to assign a u64 to that Option, which does not compile (expected enum Option<u64>, found integer). Please keep this field optional or wrap it in Some(...) and update the downstream request type so that cargo check -p codex-core succeeds again.
Useful? React with 👍 / 👎.
- Replace Option<u64> yield_time_ms with u64 in ExecCommand/WriteStdin requests and structs - Assign explicit default yield_time_ms per command/write case - Remove DEFAULT_YIELD_TIME_MS const; in favor of field defaults - Simplify clamp_yield_time to accept a u64 (no Option) - Adjust tests and internal logic accordingly
10s for exec and 250ms for write_stdin