Skip to content

add turn items view to app-server turns#21063

Merged
rhan-oai merged 8 commits into
mainfrom
rhan/turn-items-view
May 5, 2026
Merged

add turn items view to app-server turns#21063
rhan-oai merged 8 commits into
mainfrom
rhan/turn-items-view

Conversation

@rhan-oai

@rhan-oai rhan-oai commented May 4, 2026

Copy link
Copy Markdown
Collaborator

Why

Turn.items currently overloads an empty array to mean either that no items exist or that the server intentionally did not load them for this response. That ambiguity blocks future lazy-loading work where clients need to distinguish unloaded, summary, and fully hydrated turn payloads.

What changed

  • add a new TurnItemsView enum with notLoaded, summary, and full variants
  • add required itemsView metadata to app-server Turn payloads
  • mark reconstructed persisted history as full and live shell-style turn payloads as notLoaded
  • keep current thread/turns/list behavior unchanged and document that it still returns full turns today
  • regenerate the JSON and TypeScript protocol fixtures

Verification

  • just write-app-server-schema
  • cargo test -p codex-app-server-protocol
  • cargo test -p codex-app-server thread_read_can_include_turns
  • cargo test -p codex-app-server thread_turns_list_can_page_backward_and_forward
  • cargo test -p codex-app-server thread_resume_rejects_history_when_thread_is_running
  • just fix -p codex-app-server-protocol
  • just fix -p codex-app-server
  • just fmt

@rhan-oai rhan-oai force-pushed the rhan/turn-items-view branch 2 times, most recently from 82f46ee to 1832cf6 Compare May 4, 2026 19:18
@rhan-oai rhan-oai force-pushed the rhan/turn-items-view branch from 1832cf6 to d648764 Compare May 4, 2026 20:59
@rhan-oai rhan-oai force-pushed the rhan/turn-items-view branch from d648764 to 7083fd8 Compare May 4, 2026 21:12
@rhan-oai rhan-oai marked this pull request as ready for review May 4, 2026 23:56
owenlin0
owenlin0 reviewed May 5, 2026
View reviewed changes

@owenlin0 owenlin0 left a comment •
edited
Loading

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.

looking good overall, just a few comments

Comment thread codex-rs/app-server/src/bespoke_event_handling.rs Outdated
@@ -163,6 +164,7 @@ pub(crate) async fn apply_bespoke_event_handling(
state.active_turn_snapshot().unwrap_or_else(|| Turn {

@owenlin0 owenlin0 May 5, 2026 •
edited
Loading

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 can make turn/started report itemsView: "full": active_turn_snapshot() is built through the history reducer, whose PendingTurn -> Turn conversion sets items_view to Full. Since turn/started is a lifecycle notification and should not claim to include the complete item history, this path should normalize the payload before sending, e.g. clear items and force items_view = TurnItemsView::NotLoaded.

let's always make sure a turn/started notification returns itemsView: notLoaded

Comment thread codex-rs/app-server/src/request_processors/turn_processor.rs Outdated
Turn {
id: turn_id,
items,
items_view: TurnItemsView::Full,

@owenlin0 owenlin0 May 5, 2026 •
edited
Loading

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.

ReviewStartResponse should not report Full here. This turn is synthesized for a start response, not reconstructed from persisted app-server history. If we want it to behave like turn/start, this should be NotLoaded; if the synthetic prompt item is intentionally a display-only preview, Summary would be more accurate than Full.

for consistency with turn/start, let's also always return itemsView: notLoaded for review/start as well

owenlin0
owenlin0 approved these changes May 5, 2026
View reviewed changes

@owenlin0 owenlin0 left a comment

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.

pre-approving

@rhan-oai rhan-oai enabled auto-merge (squash) May 5, 2026 18:44
@rhan-oai rhan-oai disabled auto-merge May 5, 2026 18:49
@rhan-oai rhan-oai enabled auto-merge (squash) May 5, 2026 19:00
@rhan-oai rhan-oai merged commit 9e0c191 into main May 5, 2026
26 checks passed
@rhan-oai rhan-oai deleted the rhan/turn-items-view branch May 5, 2026 19:17
@github-actions github-actions Bot locked and limited conversation to collaborators May 5, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@owenlin0 owenlin0 owenlin0 approved these changes

Assignees

No one assigned

Labels

code-reviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rhan-oai @owenlin0