You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
There's a TODO comment in protocol.rs indicating that model_context_window should not be optional, but it's currently marked as Option<i64>. I'd like to understand the design intent and whether this should be addressed.
#[derive(Debug,Clone,Deserialize,Serialize,JsonSchema,TS)]pubstructTurnStartedEvent{// TODO(aibrahim): make this not optionalpubmodel_context_window:Option<i64>,#[serde(default)]pubcollaboration_mode_kind:ModeKind,}
Questions for the Team
What is the current behavior when model_context_window is None?
Does the code have a fallback/default value?
Does it cause issues for clients that expect this value?
What prevents making this field required?
Are there legacy clients that don't provide this value?
Are there certain models where this value is unknown/unavailable?
What is the timeline for addressing this TODO?
Is this planned for a specific version?
Would a migration path be needed (e.g., default value + deprecation period)?
Potential Approaches
Option 1: Make it required (Breaking Change)
pub model_context_window:i64,// Always present
Pros: Clear API contract, no handling of None case needed
Cons: Breaking change, requires all clients to provide this value
Cons: May hide cases where the value should be explicitly set
Option 3: Keep optional with clear documentation
/// The model's context window size in tokens./// Will be None for older sessions or models where this information is not available.pub model_context_window:Option<i64>,
Pros: Backward compatible, explicit about uncertainty
Cons: Requires clients to handle None case
Impact
Priority: Medium (affects API clarity and client implementation)
Type: API Design / Technical Debt
Related Code
This TODO appears in multiple locations in the codebase, suggesting this may be a known architectural decision point:
codex-rs/protocol/src/protocol.rs
codex-rs/app-server-protocol/src/protocol/v2.rs
Note: I'm aware that external PRs are by invitation only. I'm submitting this issue to seek clarification and contribute analysis as outlined in the contributing guidelines.
documentationImprovements or additions to documentation
1 participant
Converted from issue
This discussion was converted from issue #11253 on February 10, 2026 02:10.
Heading
Bold
Italic
Quote
Code
Link
Numbered list
Unordered list
Task list
Attach files
Mention
Reference
Menu
reacted with thumbs up emoji reacted with thumbs down emoji reacted with laugh emoji reacted with hooray emoji reacted with confused emoji reacted with heart emoji reacted with rocket emoji reacted with eyes emoji
Uh oh!
There was an error while loading. Please reload this page.
-
Summary
There's a
TODOcomment inprotocol.rsindicating thatmodel_context_windowshould not be optional, but it's currently marked asOption<i64>. I'd like to understand the design intent and whether this should be addressed.Location
File:
codex-rs/protocol/src/protocol.rsLine: 1157-1158
Questions for the Team
What is the current behavior when
model_context_windowisNone?What prevents making this field required?
What is the timeline for addressing this TODO?
Potential Approaches
Option 1: Make it required (Breaking Change)
Option 2: Provide sensible default
Option 3: Keep optional with clear documentation
Impact
Related Code
This TODO appears in multiple locations in the codebase, suggesting this may be a known architectural decision point:
codex-rs/protocol/src/protocol.rscodex-rs/app-server-protocol/src/protocol/v2.rsNote: I'm aware that external PRs are by invitation only. I'm submitting this issue to seek clarification and contribute analysis as outlined in the contributing guidelines.
Beta Was this translation helpful? Give feedback.
All reactions