Repository navigation
fix(client): late answers no longer vanish when only part of a thread is loaded - #16437
yaacovcorcos wants to merge 1 commit into
Conversation
… is loaded With partial thread history, an item that arrived after a later item of the same run had raised the ordinal watermark was dropped as old history, so an answer could arrive and never appear. Keep a missing item when the loaded window already shows an earlier local item of the same run. Items from runs outside the window are still dropped. Co-Authored-By: Codex (gpt-6.1-sol) <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a narrowly scoped client-side bug fix that preserves existing behavior outside partial timelines and adds regression coverage for both retained and dropped late items. Its runtime impact is isolated to correctly inserting late items from runs already present in the loaded window. You can add or adjust custom eligibility rules. Learn more. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe projection now preserves certain late turn items in partial timelines when a visible item belongs to the same run. Tests cover items from loaded and unloaded runs. ChangesPartial Timeline Projection
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No merge-blocking issue is established for partial-timeline late-item handling; the change is mergeable after normal checks. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Note Grok responding on behalf of Julius. Thanks for this, @yaacovcorcos. #17764 just merged and covers the same case: |
Problem
When a thread's history is only partly loaded,
applyOrchestrationV2ProjectionEventdrops any newturn item whose ordinal is at or below
latestLocalTurnOrdinal, treating it as unloaded history.Every applied
turn-item.updatedraises that watermark (threads.ts). So if a later item of therunning turn arrives first (for example an approval request at ordinal 13), the answer at ordinal 12
that arrives next is dropped by
shouldDropMissingPartialTurnItemand never appears, even though itsrun is on screen.
Fix
In
shouldDropMissingPartialTurnItem, keep a missing item when the loaded window already shows anearlier local item of the same run. It is then inserted by ordinal as usual. Items from runs outside
the loaded window, and items older than the window, are still dropped.
Why this qualifies
A very small, focused fix for an obvious bug: one guard in one client reducer, plus a test. No
contract, server, or UI change.
Verification
Test:
packages/client-runtime/src/state/orchestrationV2Projection.test.ts,partial timeline: $name that arrives after a later sibling, two cases:keeps an item from a loaded run: window holds items 11 and 13 of one run, watermark 13; item 12 ofthat run arrives and must sit between them.
drops an item from an unloaded run: the same item 12 from a run that is not in the window is stilldropped (reducer returns the same projection).
Before the fix (
cd packages/client-runtime && vp test run src/state/orchestrationV2Projection.test.ts):After the fix, the same command passes, along with the neighbouring partial-history suites:
vp test run src/state/orchestrationV2Projection.test.ts src/state/threads-sync.test.ts src/state/threadHistoryMerge.test.ts: all pass.vp run typecheckinpackages/client-runtime: exit 0.vp lint --report-unused-disable-directives packages/client-runtime/src/state: no findings in the touched files.vp fmt --check packages/client-runtime/src/state: clean.Not checked
Not reproduced in a running app. The test drives the reducer directly with the event order that
triggers the drop; I did not capture a live session where a provider emits a later item before the
answer.
Found and fixed in a downstream fork (Scient), then ported to current main. Ported and verified with
Claude Opus 5.5 in Claude Code; the original fix was written with Codex (gpt-6.1-sol).
🤖 Generated with Claude Code