Skip to content

code-mode: preserve initial yield at completion#29289

Merged
cconger merged 1 commit into
mainfrom
cconger/code-mode-runtime-compact-03e3-initial-yield-boundary
Jun 21, 2026
Merged

code-mode: preserve initial yield at completion#29289
cconger merged 1 commit into
mainfrom
cconger/code-mode-runtime-compact-03e3-initial-yield-boundary

Conversation

@cconger

@cconger cconger commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Retain the first pre-observation yield_control() boundary when a cell completes before observation.
  • Deliver the preserved yield before the buffered completion.
  • Keep later unattached yields as no-ops.

Why

Create followed by the initial wait must preserve the former execute response boundary even when the script runs to completion first.

Impact

The first wait observes the same initial yield boundary as before create and observe were decoupled.

Validation

  • Focused initial-yield signature regression passed.
  • Stack-tip validation: just test -p codex-code-mode -p codex-code-mode-protocol (70 passed).
  • Parent branch: cconger/code-mode-runtime-compact-03e2-observation-delivery.

@cconger cconger force-pushed the cconger/code-mode-runtime-compact-03e2-observation-delivery branch from f2cf22e to 78a5993 Compare June 21, 2026 06:24
@cconger cconger force-pushed the cconger/code-mode-runtime-compact-03e3-initial-yield-boundary branch from 289b79d to 4439ee8 Compare June 21, 2026 06:24
.commit_completion(
HashMap::new(),
event,
/*pending_initial_yield_items*/ None,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This slice never supplies a real initial yield: both production completion call sites still pass None, and only the direct state tests construct Some(...)
Is this expected? This does not seems aligned with the PR title

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.

This is unfortunately preparatory for #29290 they could likely have been merged.

@cconger cconger force-pushed the cconger/code-mode-runtime-compact-03e2-observation-delivery branch from 78a5993 to aeac9d7 Compare June 21, 2026 19:24
@cconger cconger force-pushed the cconger/code-mode-runtime-compact-03e3-initial-yield-boundary branch from 4439ee8 to acb5941 Compare June 21, 2026 19:24
@cconger cconger force-pushed the cconger/code-mode-runtime-compact-03e2-observation-delivery branch from aeac9d7 to 01fa5b8 Compare June 21, 2026 20:18
@cconger cconger marked this pull request as ready for review June 21, 2026 20:20

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

Copy link
Copy Markdown
Contributor

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: acb5941852

ℹ️ 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".

.commit_completion(
stored_value_writes,
event,
/*pending_initial_yield_items*/ None,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Badge Preserve the first dropped yield at completion

When the initial YieldAfter response is dropped before the actor can deliver a yield_control() boundary and the cell then runs to completion before another observation, this path still commits the completion with pending_initial_yield_items set to None. The failed yield delivery is only folded back into content_items, so the next YieldAfter observe receives one merged Completed event instead of the preserved Yielded boundary followed by the buffered completion; the new CellState buffering is only exercised by tests that call commit_completion(..., Some(...)) directly, not by the actor. Capture the first undelivered initial yield, including an empty yield, and pass it into the completion commit here.

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.

We address this later, good catch though.

Base automatically changed from cconger/code-mode-runtime-compact-03e2-observation-delivery to main June 21, 2026 20:53
@cconger cconger force-pushed the cconger/code-mode-runtime-compact-03e3-initial-yield-boundary branch from acb5941 to 7dc6da8 Compare June 21, 2026 20:55
@cconger cconger merged commit eb8c1ee into main Jun 21, 2026
46 of 47 checks passed
@cconger cconger deleted the cconger/code-mode-runtime-compact-03e3-initial-yield-boundary branch June 21, 2026 22:35
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 21, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants