Skip to content

fix codex first turn model - #30

Merged
Stephen Belanger (Qard) merged 1 commit into
mainfrom
max/fix-codex-first-turn-model
Aug 25, 2026
Merged

fix codex first turn model#30
Stephen Belanger (Qard) merged 1 commit into
mainfrom
max/fix-codex-first-turn-model

Conversation

@max-braintrust

Copy link
Copy Markdown
Contributor

Codex writes task_started before turn_context, so the first turn span is created before its model is known. The daemon already uses the later turn_context event to backfill the session root; this change also backfills the matching turn span using its turn_id.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fe7c705b5a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".

Comment on lines +496 to +498
if let Some(turn) = model_turn_id.as_deref().and_then(|turn_id| {
scope.open_turns.iter().find(|turn| turn.turn_id == turn_id)
}) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Backfill the active turn when turn_id is absent

For rollout variants whose turn_context payload contains model but no turn_id—the format still used by the other Codex translator and replay fixtures—this expression always returns None. If such a context follows task_started, the already-inserted turn therefore retains a null model, so the change does not fix the first-turn trace for those sessions. Fall back to the sole/latest open turn when the context lacks an explicit ID.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm I think I'd rather not fill this in the ambiguous case - seems like that could get you into a weird state if there are somehow overlapping turns? From what I can see turn_id is never missing from these spans, and it looks like it should always get populated on the existing code path.

@Qard
Stephen Belanger (Qard) merged commit 0eae1d5 into main Aug 25, 2026
20 checks passed
@Qard
Stephen Belanger (Qard) deleted the max/fix-codex-first-turn-model branch August 25, 2026 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants