⚓ fix: Keep a Resumed Compaction Anchored to the Turn It Summarizes - #16037
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 854d6b13cb
ℹ️ 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".
| /** The response hangs off a row the user did not write: only a compaction does | ||
| * that, and it is regenerate-shaped for every consumer of the run — no user | ||
| * turn of its own, the response parented onto an existing message. */ | ||
| const isAnchoredRun = existingSlotMessage != null && existingSlotMessage.isCreatedByUser !== true; |
There was a problem hiding this comment.
When the active branch ends in a user-created row, useCompactConversation still permits compaction because canCompact does not restrict the leaf's author, but this predicate classifies that resumed job as an ordinary turn. After a reload, the sync path therefore merges the server's identity-only projection over the stored user row, temporarily erasing its text, and a failed abort can append a duplicate anchor because compact was never restored. Detect the compaction from the projection/job shape rather than requiring a non-user anchor so restored sessions preserve this valid leaf as well.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ba855c78d.
You are right on both counts, and the repository says so itself: canCompact does not restrict the leaf's author, and isUserInitiatedCompaction documents that "Compact runs on whatever leaf the branch ends with, so its response can parent onto a user message as easily as onto the answer it summarized" — there are even two compaction-on-user-turn e2e scenarios. My isCreatedByUser !== true test recognized only the assistant-leaf kind.
Both consequences you named reproduce as failing tests on the previous head:
useResumeOnLoad: the rebuilt submission hadcompact: undefined, so it was treated as an ordinary turn.useResumableSSE: the sync path then merged the identity-only projection over the stored row and its text became""— the prompt blanks while the compaction runs.
The fix detects the anchor from the projection's shape instead of the anchor's author, as you suggested: isCompactionAnchorProjection in client/src/utils/messages.ts requires an id, no parentMessageId, and empty text. That is what separates the two producers — projectCompactionAnchor returns { messageId, conversationId, text: '' } with no parent, while getPreliminaryUserMessage always publishes the turn it created, parent included. buildResumeEventSubmission now reads the flag or the projection, and carries the resolved answer on the resumed submission, so the flagless re-attach path and the abort-error write agree with it.
Two tests added, both failing before the change: a user-leaf anchor rebuilt by useResumeOnLoad, and a user-leaf sync on a submission carrying no flag.
A compaction submits no user turn: its user-message slot names the leaf it summarizes up to. The resume paths adopted that identity-only projection as a ROW, rewriting the answer being summarized into an empty, parentless user message, which buildTree files as a phantom root: the thread folded and the pane could return on another branch. Treat the slot as an anchor everywhere rows are written, and let the anchor name the branch to restore.
854d6b1 to
a1471dd
Compare
Compact runs on whatever leaf the branch ends with and canCompact does not restrict its author, so the anchor is a user message as often as an answer. Detecting it by not-user-created recognized only the assistant-leaf kind: the other was rebuilt as an ordinary turn, so the sync path merged the identity-only projection over the stored row and blanked the prompt still on screen, and a failed abort appended a second row under its id. Judge the anchor by the projection shape instead, and carry the resolved answer on the resumed submission so a flagless re-attach agrees.
|
@codex review the latest head |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
Problem
Re-attaching to a running manual compaction folded the thread: every turn above the compaction dropped out of the visible branch, and the pane could come back on a different branch than the one being compacted. A reload, a navigation back, or a dropped SSE connection while "Compact context" was still running was enough.
A compaction submits no user turn. It hangs a summarize-only response off the branch's leaf and puts that leaf in the submission's user-message slot (
useCompactConversation), which the server projects the same way, identity only (projectCompactionAnchor: id, conversation, empty text, no parent, no author). The live path never writes that slot as a row, because every write is behindisRegenerate. The re-attach paths did:buildSubmissionFromResumeStatematched the slot only againstisCreatedByUserrows, so a compaction anchored on an assistant answer missed and a new row was synthesized in its place: empty text,isCreatedByUser: true,parentMessageId: NO_PARENT.buildResumeEventSubmissionstampedisCreatedByUser: trueover the slot, andmergeResumeMessagesthen merged it onto the row with the same id, i.e. onto the answer being summarized.buildTreefiles a parentless (or self-parented) row as a root, so the rewritten answer became a phantom root branch: the conversation above it detached, and the view landed on that branch showing an empty user row and the streaming summary.Same defect class in the abort path: the
abortConversationcatch wrotesubmission.userMessageunconditionally, duplicating the anchor's id as an empty self-parented row.Fix
One invariant, applied where rows are written: a compaction's user-message slot is an anchor, never a row.
useResumeOnLoad: resolve the slot from loaded history by id (ids are unique per row) and adopt that row as-is; the run is anchored when the slot carries the anchor's shape (an id, noparentMessageId, empty text), so mark the rebuilt submissioncompact: trueand keep it regenerate-shaped. The branch to restore falls back to the anchor itself when the slot carries no parent, so a compaction started on an older branch comes back on that branch.useResumableSSE: an anchored run keeps the identity it resumed with, andmergeResumeMessagesneither merges nor inserts the slot, only reconciling the response row.useEventHandlers: the abort-error write skips the slot for a compaction.No protocol, server, or persisted-shape change.
Tests
client/src/hooks/SSE/__tests__/useResumableSSE.spec.ts— a resumed sync carrying the anchor projection leaves the answer an assistant row parented where it was, with the summary under it. Fails before the fix (the row becomestext: '',isCreatedByUser: true, self-parented).client/src/hooks/SSE/__tests__/useResumeOnLoad.spec.tsx— resuming a compaction adopts the anchor, marks the run, keeps it in the replayed history, parents the summary onto it, and restores the compacting branch (index 1 of 2, not the default 0). A second case covers the branch fallback when no response id is published yet. Both fail before the fix.e2e/specs/mock/thread-fold.spec.ts— the visual regression: send a turn, hold the fixture summarizer open, Compact, reload into the running compaction, then assert the prompt and the answer are still on screen with no phantom-root sibling switcher, and that the summary settles under that answer with three turns. This is the user-visible failure, so it belongs in the fold suite rather than in a unit test.The abort-error guard is covered by inspection only; that callback has no unit harness in the spec.
Follow-up (
ba855c78d)Codex caught that the first pass detected the anchor by its author, which is wrong:
canCompactdoes not restrict the leaf's author, andisUserInitiatedCompactiondocuments that a compaction's response parents onto a user message as readily as onto an answer (twocompaction-on-user-turne2e scenarios exist). A compaction anchored on a user leaf was therefore still rebuilt as an ordinary turn, so the sync path blanked that prompt's text mid-run and a failed abort appended a second row under its id.The anchor is now recognized from the projection's shape, in one predicate both consumers share:
isCompactionAnchorProjection(id present, noparentMessageId, empty text) inclient/src/utils/messages.ts.projectCompactionAnchoremits exactly that;getPreliminaryUserMessagealways publishes a parent, which is what separates them.buildResumeEventSubmissionreads the flag or the projection and carries the resolved answer forward, so the flagless re-attach path agrees.Two more tests, both failing before the change: a user-leaf anchor through
useResumeOnLoad, and a user-leaf sync on a submission with no flag.