Conversation
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. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Empty compaction disconnects can still skip persistence, and metadata facade and Redis round-trip coverage gaps remain.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Persists manual compaction identity through cancellation and disconnect paths, preventing incorrect rerun controls.
Changes:
- Adds
compactmetadata and Redis persistence. - Marks aborted compaction content or records typed failure content.
- Adds unit, controller, and end-to-end coverage.
| File | Summary |
|---|---|
packages/api/src/types/stream.ts |
Adds compaction metadata typing. Moderate issue: the job facade does not expose compact. |
packages/api/src/stream/metadata.ts |
Sanitizes compaction metadata. |
packages/api/src/stream/interfaces/IJobStore.ts |
Extends serializable job metadata. |
packages/api/src/stream/implementations/RedisJobStore.ts |
Restores compaction metadata. Nit: add Redis round-trip coverage. |
packages/api/src/stream/GenerationJobManager.ts |
Marks aborted compaction content. |
packages/api/src/stream/__tests__/abortCompactionIdentity.spec.ts |
Tests abort persistence. |
packages/api/src/agents/compaction.ts |
Implements aborted-content identity marking. |
packages/api/src/agents/compaction.spec.ts |
Tests helper behavior. |
e2e/specs/mock/scenarios/compaction-rerun-controls.spec.ts |
Adds cancelled-compaction UI scenarios. |
api/server/controllers/agents/request.js |
Persists compaction identity on disconnect. Critical issue: empty disconnect snapshots bypass typed failure persistence. |
api/server/controllers/agents/__tests__/request.resumeMetadata.spec.js |
Updates metadata mocks. |
api/server/controllers/agents/__tests__/request.partialDisconnect.spec.js |
Tests disconnect persistence. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: 5b8ceb2f3b
ℹ️ 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".
| const persistableContent = markAbortedCompactionContent( | ||
| filterPersistableAbortContent(aggregatedContent), | ||
| isCompaction, | ||
| ); |
There was a problem hiding this comment.
When the last SSE subscriber disconnects after a compaction has emitted a persistable non-summary part (such as thinking, activity, text, or a tool call), this handler runs while generation explicitly continues, but markAbortedCompactionContent finds no summary/error and appends compaction_failed. The partial row therefore reports a false failure on reload until a successful completion overwrites it, and can remain that way if final persistence fails. Use a marker-only path here and reserve synthesized failure content for the actual terminal abort path.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6562583. The disconnect save now runs the marker-only mode (synthesizeFailure: false): it stamps what streamed and invents no failure, since the run is still live and the completing path overwrites the row. Regression in request.partialDisconnect.spec.js saves a think-only compaction partial with no error part.
| if (!markedOutcome) { | ||
| contentParts.push(...compactionFailureContent()); |
There was a problem hiding this comment.
When Stop lands after summarization has started but before its first delta, the abort snapshot can contain an empty placeholder such as { type: 'summary', content: [], summarizing: true }. The loop sets markedOutcome solely from that type, so this branch skips the promised compaction_failed part even though no summary content was produced; once submission ends, the summary renderer no longer considers it actively streaming and can label the empty interrupted row as context compacted by the user. Only count a summary as an outcome when it contains streamed text, while still stamping the placeholder for identity.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6562583. Only a summary carrying text counts as an outcome now (isSummaryPartWithText); the placeholder itself is still stamped and the typed failure is appended beside it. Covered in compaction.spec.ts and abortCompactionIdentity.spec.ts, including the manager-level abort snapshot.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bf52141f5
ℹ️ 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".
| part.initiatedBy = 'user'; | ||
| if (isSummaryPartWithText(part)) { | ||
| hasOutcome = true; |
There was a problem hiding this comment.
When Stop arrives after the summarizer has opened a summary part, this stamps initiatedBy but preserves summarizing: true and leaves failed unset. After the run settles or the page reloads, Summary.tsx computes isActivelyStreaming as false and therefore labels the part “Context compacted by you”; a partial summary with text also sets hasOutcome, so no failure is appended, while an empty placeholder's sibling error does not affect the summary component. Fresh evidence after the prior thread is that the renderer selects its success label solely from the summary part's own failed and initiatedBy fields, so the attempted fix still makes a cancelled compaction claim success.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6ed3ba8. The terminal path now applies the completed run's outcome rules: a usable summary is marked as the outcome, a text-bearing partial one keeps its text but is marked failed so the label reads Summarization failed, and an empty placeholder is replaced by the typed failure. Covered in compaction.spec.ts; the disconnect snapshot still stamps identity only since its row belongs to the live run.
| if (!hasOutcome && synthesizeFailure) { | ||
| contentParts.push(...compactionFailureContent()); |
There was a problem hiding this comment.
When a manual compaction is stopped before streaming any content, synthesizing this error makes shouldPersistAbortedTurn true, so the abort endpoint's beforePublish hook saves jobData.userMessage as a prerequisite. For a compaction that object is the already-persisted leaf projected with text: '', while the hook additionally forces sender: 'User' and isCreatedByUser: true; because saveMessage updates by the same message ID, Stop can erase a user leaf's text or convert an assistant leaf into an empty user message. Skip the prerequisite write when jobData.compact is true—the anchor already exists—while still persisting the new aborted response.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6ed3ba8. The abort route skips the prerequisite user write when jobData.compact is true (the anchor is the persisted leaf itself) while still persisting the aborted response row. Regression in abort.spec.js asserts Stop saves exactly one row for a compaction. You are right that the overwrite was not only reachable through the synthesized failure: any compaction stop after the created event hit it on dev, so the guard closes the pre-existing window too.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ed3ba8ba8
ℹ️ 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".
| const persistableContent = markAbortedCompactionContent( | ||
| filterPersistableAbortContent(aggregatedContent), | ||
| isCompaction, | ||
| { synthesizeFailure: false }, |
There was a problem hiding this comment.
When all subscribers leave after only thinking/text or a partial summary, this persists a marker-only snapshot; if the provider subsequently throws, that snapshot is not overwritten. Fresh evidence beyond the prior disconnect thread is that saveErrorTurn returns as soon as it finds the row under liveResponseMessageId (lines 437-443), so a thinking-only row remains unmarked and exposes the wrong rerun controls, while a partial summary keeps failed unset and is labeled as a successful user compaction after reload. The terminal error path needs to finalize or replace an existing disconnected snapshot.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in db2cf6e8bb. saveErrorTurn's early-return branches now finalize a compaction's partial row through resolveFinalizedCompactionTurn (packages/api): a snapshot with no summary or error part gets the typed failure, a partial summary is marked failed beside its text, and an already-terminal row is untouched. Ordinary turns keep the existing partial-preservation behavior. The mock harness cannot drive disconnect-then-provider-fault, so the rule is covered by the resolveFinalizedCompactionTurn unit tests.
| /** A compaction's `userMessage` is the already-persisted leaf | ||
| * projected for identity only; upserting it would erase a user | ||
| * leaf's text or turn an assistant leaf into an empty user row. */ | ||
| const shouldPersistAnchor = jobData?.compact !== true; |
There was a problem hiding this comment.
This adds shouldPersistAnchor and a new conditional persistence branch directly to the CJS abort route. The repository requires /api CJS files to contain wiring rather than behavior and specifically says newly added branches and persistence decisions belong in packages/api; move this compaction prerequisite policy behind a TypeScript helper and leave the route to invoke it.
AGENTS.md reference: AGENTS.md:L64-L68
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in db2cf6e8bb. The decision moved to shouldPersistAbortAnchor in packages/api/src/agents/compaction.ts; the route keeps only the wiring. Covered by unit tests plus abort.spec.js.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eabaf2b613
ℹ️ 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".
| async function finalizeFailedCompactionTurn( | ||
| req, | ||
| { userId, conversationId, messageId, partialRow }, | ||
| ) { | ||
| const finalized = resolveFinalizedCompactionTurn(partialRow, req.body); |
There was a problem hiding this comment.
Unlike the resolved abort-anchor branch, this newly added finalizeFailedCompactionTurn helper introduces another decision branch and persistence workflow directly in api/server/controllers/agents/request.js. The repository requires new CJS branches, helpers, and service-call behavior under /api to live in packages/api; move the whole finalization operation behind a TypeScript entry point with its persistence dependency supplied by the caller, leaving this controller to invoke it.
AGENTS.md reference: AGENTS.md:L64-L68
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 20856aa30f. The operation moved to persistFinalizedCompactionTurn in packages/api/src/agents/compaction.ts with the write injected; the controller supplies its persistence dependency and the loaded row, and keeps no decision branch.
| req?._agentEventBindingRetention?.expiredAt ?? req?.resolvedConversation?.expiredAt, | ||
| interfaceConfig: req?.config?.interfaceConfig, | ||
| }, | ||
| { messageId, conversationId, ...finalized }, |
There was a problem hiding this comment.
When all subscribers disconnect and the provider subsequently throws, the existing snapshot was saved with unfinished: true and error: false, but this update only supplies identity and replacement content. Because saveMessage partially updates the row, those live-snapshot flags remain after terminal finalization, so a restored session persists a terminal failure as an unfinished non-error response and downstream consumers such as search/export continue to classify it as incomplete. Include the terminal envelope fields (unfinished: false and error: true, consistently with the normal saveErrorTurn path) in the finalized update.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 20856aa30f. persistFinalizedCompactionTurn writes the terminal envelope (unfinished false, error true) beside the finalized content, matching the normal saveErrorTurn row; covered by the new unit test.
| export function shouldPersistAbortAnchor( | ||
| jobData: { compact?: boolean } | null | undefined, | ||
| ): boolean { | ||
| return jobData?.compact !== true; |
There was a problem hiding this comment.
When Stop wins after createJob but before BaseClient has loaded the branch, or when the submitted parent ID is stale, jobData.userMessage is only the unverified projection created from the request. The synthesized failure makes the abort turn persistable, while this unconditional skip lets the route write its assistant child without confirming that the anchor exists, leaving an orphaned/phantom-root row on reload. Fresh evidence beyond the resolved anchor-overwrite thread is that this early-abort window precedes branch loading and isUnpersistedPreliminaryParent does not query ordinary UUID parent IDs; check anchor existence rather than rewriting it before saving the response.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 20856aa30f. shouldPersistAbortAnchor became resolveAbortAnchorDecision(jobData, anchorExists): the route now reads the anchor before deciding, writes only the response when the leaf is persisted, and nothing at all when it is not (Stop winning the pre-branch-load race). Both compaction branches have regressions in abort.spec.js.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20856aa30f
ℹ️ 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".
| jobData?.responseMessageId && | ||
| shouldPersistAbortedTurn | ||
| shouldPersistAbortedTurn && | ||
| anchorDecision !== 'skip-turn' |
There was a problem hiding this comment.
When Stop wins before a compaction's branch has loaded, the anchor lookup returns skip-turn, and this guard silently omits both database writes. However, abortJob has already synthesized a compaction_failed part, so it classifies the abort as non-early and constructs a normal FINAL containing the conversation and response; because this callback succeeds, that FINAL is published and the client can navigate to a response that was deliberately never persisted. Signal this outcome back to abortJob so it publishes an early-abort/reconciliation event instead.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a3d7c1f558. skip-turn now pushes onto beforePublish's persistenceErrors, the established channel for suppressing the normal FINAL: the manager publishes a reconciliation frame instead, and no row is written. The abort spec asserts both.
| const finalized = resolveFinalizedCompactionTurn(partialRow, requestBody); | ||
| if (finalized == null) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
When a disconnected compaction snapshot already contains an error part or a failed summary and the live run subsequently throws, resolveFinalizedCompactionTurn returns null, causing this branch to skip the entire save rather than only skip the content rewrite. saveErrorTurn then returns immediately, leaving the snapshot's unfinished: true and error: false flags permanently intact. Fresh evidence after the prior terminal-envelope fix is that its new wrapper writes those fields only when content also needs finalization; it must still persist the terminal envelope for an already-marked outcome.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a3d7c1f558. resolveFinalizedCompactionTurn returns a discriminated write decision now: content finalization when the parts need it, an envelope-only write when they already carry the failure (error part or failed summary), and no write only for other turns' rows or a completed checkpoint. persistFinalizedCompactionTurn settles the terminal envelope in both write cases; both are unit tested.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3d7c1f558
ℹ️ 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".
| if (jobData?.compact === true && jobData?.userMessage?.messageId && req?.user?.id) { | ||
| const anchorRows = await getMessages({ |
There was a problem hiding this comment.
Fresh evidence after the prior thread is that the current head still adds both the compaction-specific branch and a new getMessages service call directly to this CJS route; resolveAbortAnchorDecision only moved the final boolean decision. Move the existence lookup and policy into a TypeScript operation in packages/api, injecting the database reader so this route only supplies dependencies and invokes it.
AGENTS.md reference: AGENTS.md:L64-L68
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2cc88ae4a0. The existence lookup and policy are now one async TypeScript operation (resolveAbortedTurnAnchorDecision) with the database reader injected; the route supplies its reader, awaits the decision, and pushes the withhold error. No compaction branch remains in the route.
| if (sawCheckpoint) { | ||
| return { write: false }; |
There was a problem hiding this comment.
When a disconnected compaction has already emitted a usable summary but terminal persistence subsequently fails before overwriting the snapshot, saveErrorTurn finds that row and this branch performs no write. The snapshot was stored with unfinished: true and error: false, so the restored conversation permanently treats an already-terminal job as live/incomplete; use the loaded row's envelope to distinguish an already-finished response from a disconnect snapshot and settle the latter while preserving its completed summary.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2cc88ae4a0. The resolver reads the row's own envelope: a completed checkpoint inside a snapshot still flagged unfinished settles its envelope while preserving the summary; an already-settled row stays untouched. Unit tested both ways.
| const anchorRows = await getMessages({ | ||
| user: req.user.id, | ||
| messageId: jobData.userMessage.messageId, | ||
| conversationId: jobData.conversationId, | ||
| }); |
There was a problem hiding this comment.
If this new anchor lookup throws because Mongo is temporarily unavailable, the exception exits beforePublish before the checkpoint-cleanup block at lines 887-899 runs. At that point abortJob has already won the terminal CAS, so the manager suppresses the normal FINAL and cleans up the job while leaving its checkpoint behind; capture the lookup failure while withholding the response write, then still execute checkpoint cleanup as the callback already does for message-save failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2cc88ae4a0. The anchor read runs inside the injected operation and a failure returns skip-turn instead of throwing, so beforePublish continues to checkpoint cleanup while the response write is withheld.
| if (!finalized.write) { | ||
| return false; | ||
| } | ||
| await saveMessage({ |
There was a problem hiding this comment.
When the injected saveMessage resolves null or undefined rather than throwing—which the production method's contract explicitly permits—this helper still returns true, and saveErrorTurn consequently lets completeJob finalize the terminal barrier as though the snapshot had been settled. The row remains unfinished: true/error: false while clients receive terminal reconciliation, so require a truthy saved row or throw just as the surrounding failed-turn persistence path does.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2cc88ae. The injected save must resolve truthy; a falsy result throws like the surrounding failed-turn persistence, and a unit test covers it.
| const existing = await getMessages({ | ||
| user: userId, | ||
| messageId: errorMessageId, | ||
| conversationId, | ||
| }); |
There was a problem hiding this comment.
For every non-compaction generation error, this changed query now materializes the entire existing response document even though finalizeFailedCompactionTurn immediately no-ops and the caller returns. Previously the _id projection avoided loading potentially large streamed content, tool results, and metadata on this error path; keep the lightweight projection for ordinary turns and request only the content/envelope fields needed when req.body.compact === true.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2cc88ae4a0. Both existing-row checks keep the id-only projection for ordinary turns; the full document is requested only for a compaction's live response row, which is the one row the finalizer writes.
| await finalizeFailedCompactionTurn(req, { | ||
| userId, | ||
| conversationId, | ||
| messageId: errorMessageId, | ||
| partialRow: existing[0], |
There was a problem hiding this comment.
When a compaction is anchored to a persisted message whose ID ends in _—a supported preliminary-response shape—getPreliminaryResponseMessageId at line 392 normalizes that anchor ID back to itself. This lookup therefore returns the anchor, and the newly added call marks that existing branch message error: true and can append compaction_failed before the code checks the distinct liveResponseMessageId; finalize only the exact live response row so a failed compaction cannot corrupt its parent.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2cc88ae4a0. The anchor-derived branch no longer finalizes anything; a compaction is finalized only under its distinct live response id, so an anchor-shaped id (underscore preliminary shape included) is never treated as the failed run's row.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cc88ae4a0
ℹ️ 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".
| /** No compaction finalization here: this id can normalize back to the | ||
| * anchor itself when the anchor ends in `_`, and the anchor is never | ||
| * the failed run's row. The run's own snapshot, if any, is checked | ||
| * under its distinct live response id below. */ | ||
| return; |
There was a problem hiding this comment.
When a failed compaction is anchored to an existing message whose ID ends in _, getPreliminaryResponseMessageId normalizes errorMessageId to that anchor, so this lookup succeeds and the return executes before the distinct liveResponseMessageId lookup below. If a disconnect snapshot was saved and the provider subsequently errors, that live row therefore remains unfinished: true/error: false, and a marked partial summary can be presented as a successful user compaction after reload. Fresh evidence after the resolved anchor-finalization thread is that the current early return still makes its promised “checked ... below” path unreachable in this supported ID shape; skip writing the anchor but continue to finalize the distinct live row.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 16da5d3. The existing-row settlement now lives in settleExistingRowsBeforeErrorTurn (packages/api): an anchor-shaped match settles the distinct live response row and never receives the error row, and the same settlement runs when nothing matched, so the collision shape no longer strands the snapshot. Covered by the anchor-shaped collision regression.
16da5d3 to
84b3896
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84b38966ca
ℹ️ 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".
| messageExists: (messageId, conversationId) => | ||
| getMessages({ user: req?.user?.id, messageId, conversationId }).then( | ||
| (rows) => rows.length > 0, | ||
| ), |
There was a problem hiding this comment.
For every stopped compaction, this existence callback calls getMessages without a projection, so Mongo materializes the anchor's complete message—including potentially large content, tool results, attachments, and metadata—while the decision only reads rows.length. Fresh evidence beyond the resolved ordinary-failure projection thread is that this newly added abort-route lookup remains unprojected; pass the _id projection so Stop does not incur avoidable latency and heap use on large conversation leaves.
AGENTS.md reference: AGENTS.md:L186-L188
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d54e8e1. The existence reader now requests the id-only projection, so Stop never materializes the anchor's content or attachments for a length check.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3e160ffc1
ℹ️ 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".
d54e8e1 to
8d53e95
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b724f5eabe
ℹ️ 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".
| return ( | ||
| jobRecord.status === 'complete' || | ||
| jobRecord.status === 'error' || | ||
| jobRecord.status === 'aborted' |
There was a problem hiding this comment.
When the last subscriber leaves after claimBeforeResponsePersistence has changed the job to complete, error, or aborted with terminalPersistencePending: true, but before the Mongo write finishes, this helper reports the job as settled and the guard in request.js drops the disconnect snapshot. If that final write then fails, the terminal path only publishes reconciliation and the streamed compaction has no fallback row, losing its summary/content. This is distinct from the previously discussed check-to-save race: the fresh evidence is that the terminal status transition explicitly precedes BaseClient response persistence, so a terminal status does not yet prove the row is durable. Await the pending persistence outcome or retain a fallback until the pending marker clears.
AGENTS.md reference: AGENTS.md:L45-L47
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fb85ea6. isSettledJobRecord now treats terminalPersistencePending as unsettled, so a disconnect in the claim-to-write window keeps the snapshot as the fallback; covered by a unit test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2702225ef0
ℹ️ 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 guard reads the same record, so the window is the settling | ||
| * path's own commit span. Ordinary turns keep the pre-change behavior | ||
| * exactly: their snapshot is the fallback row, settled or not. */ | ||
| if (isCompaction && isSettledJobRecord(jobRecord, jobCreatedAt)) { |
There was a problem hiding this comment.
When a compaction's subscribers disconnect, this newly added branch decides in the legacy CJS controller whether persistence is skipped. The repository requires new /api branches and service behavior to live in packages/api, so move the settled-job guard behind a TypeScript operation that receives the job state and persistence dependencies, leaving this listener to invoke it.
AGENTS.md reference: AGENTS.md:L73-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2485ce8. The guard became allowsDisconnectSnapshot in packages/api, combining the compaction flag with the settled-record check; the controller keeps only the invocation and the log line. Unit tests cover both turn kinds, and the disconnect specs assert a settled ordinary turn still persists while a settled compaction does not.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2485ce87e8
ℹ️ 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".
| return ( | ||
| jobRecord.status === 'complete' || | ||
| jobRecord.status === 'error' || | ||
| jobRecord.status === 'aborted' | ||
| ); |
There was a problem hiding this comment.
When the last-subscriber callback is delayed until after the terminal Mongo write fails, publishTerminalClaim(..., null) can durably store a reconciliation event and clear terminalPersistencePending; this predicate then classifies the job as settled and allowsDisconnectSnapshot discards the only streamed fallback even though no message row was persisted. Fresh evidence after the prior pending-marker fix is that finalizeTerminalPersistence clears the marker for reconciliation events as well as successful finals, so the final event or another persistence-success signal must distinguish these cases.
AGENTS.md reference: AGENTS.md:L45-L47
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 86e701b. isSettledJobRecord now also treats a record whose durable final event is a reconciliation frame as unsettled: a failed terminal write cleared the pending marker while publishing the conservative frame, and no row was persisted, so the streamed snapshot remains the turn's only fallback. Unit test distinguishes the reconcile frame from a settled final.
There was a problem hiding this comment.
Extended in 418342c: the guard is now resolveDisconnectSnapshotMode, and a reconciled record promotes the snapshot to the terminal row, persisted with the terminal marking (the typed failure when no summary streamed) and the terminal envelope. Unit tests cover the mode decision and the persisted row; live runs keep the marker-only shape.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86e701b3db
ℹ️ 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".
| if (hasDurableReconcileFrame(jobRecord.finalEvent)) { | ||
| /** The terminal write failed and a reconciliation frame was published in | ||
| * its place: nothing was persisted for the turn, so the streamed | ||
| * snapshot remains its only row. */ | ||
| return false; |
There was a problem hiding this comment.
When the last-subscriber callback runs after a compaction’s terminal Mongo write has failed and the reconciliation frame has been committed, this exception makes allowsDisconnectSnapshot accept the write, but request.js still saves the marker-only content with unfinished: true and error: false. Consequently an errored run can restore as a successful partial summary, a completed run remains unfinished, and an aborted non-summary snapshot lacks its terminal failure outcome. The new reconciliation-aware path should persist a status-appropriate terminal envelope/outcome rather than the ordinary live snapshot.
AGENTS.md reference: AGENTS.md:L45-L51
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 418342cace
ℹ️ 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".
| unfinished: snapshotMode !== 'terminal', | ||
| error: snapshotMode === 'terminal', |
There was a problem hiding this comment.
When the disconnect callback runs after a reconciled complete or aborted claim, resolveDisconnectSnapshotMode collapses both into the same terminal mode used for errors, so these lines persist every fallback as unfinished: false, error: true. A completed compaction with a usable summary is therefore restored as an error, while a stopped compaction loses the normal aborted envelope (unfinished: true, error: false). Fresh evidence after the prior thread is that the promotion now writes a terminal row but still ignores the available jobRecord.status; return a status-specific mode/envelope rather than treating every reconciliation as a failed generation.
AGENTS.md reference: AGENTS.md:L45-L51
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 929997a. resolveReconciledSnapshotEnvelope maps the claim's status to its row shape; unit and controller regressions cover aborted, complete, and error.
| const persistableContent = markAbortedCompactionContent( | ||
| filterPersistableAbortContent(aggregatedContent), | ||
| isCompaction, | ||
| { synthesizeFailure: snapshotMode === 'terminal' }, |
There was a problem hiding this comment.
When Stop or the error path reconciles after its primary write fails before any content was emitted, closing the remaining subscriber invokes this handler with aggregatedContent: [], but the initial length guard returns before this terminal-mode synthesis can append compaction_failed. The reconciliation frame is then durable while no response row records the cancelled compaction at all. Load the terminal mode before rejecting empty content so reconciled compactions can synthesize their terminal outcome, while still skipping empty snapshots for live runs.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 929997a. Empty content is rejected only for live snapshots; a promoted terminal snapshot synthesizes the typed failure from nothing, with a controller regression.
| * runs keep the marker-only shape, a failed terminal write (settled | ||
| * for a reconciliation frame) promotes the snapshot to the turn's | ||
| * terminal row, and a durably settled compaction withholds it. */ | ||
| const snapshotMode = resolveDisconnectSnapshotMode(isCompaction, jobRecord, jobCreatedAt); |
There was a problem hiding this comment.
When an early Stop finds that the submitted compaction anchor is absent, the abort route deliberately withholds both rows and publishes reconciliation, but a non-empty allSubscribersLeft callback then enters this new terminal mode and saves the snapshot using resumeState.userMessage, which is only the unverified identity projection from job metadata. That recreates the orphaned child the anchor decision was intended to suppress. Fresh evidence after the resolved anchor thread is that reconciliation promotion bypasses its existence check; verify the anchor again, or carry the route's skip-turn outcome, before permitting this fallback write.
AGENTS.md reference: AGENTS.md:L186-L189
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 929997a. resolveDisconnectSnapshotMode now takes an injected anchorExists reader and returns skip when the anchor is not persisted; both unit and controller regressions cover the withheld promotion.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 929997a8b4
ℹ️ 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".
| if (partialResponseSaved || !aggregatedContent) { | ||
| return; |
There was a problem hiding this comment.
When the first disconnect occurs while a compaction is live, this handler saves an unfinished: true snapshot and sets partialResponseSaved; if the completed response write later fails and the terminal claim settles to a reconciliation frame, there may be no further disconnect callback, and any callback that does occur returns here before resolveDisconnectSnapshotMode can promote the row. Unlike the previously reviewed delayed-first-callback ordering, the fresh evidence is this successful-earlier-callback ordering: a completed job can remain permanently stored as an unfinished live response after reload. Settle the already-saved snapshot from the terminal persistence-failure path, or allow reconciliation to reprocess it.
AGENTS.md reference: AGENTS.md:L45-L51
Useful? React with 👍 / 👎.
Pull Request
Summary
Closes berry-13#33, split out of the review of #15883.
Stopping a manual compaction mid-summarization persists the partial turn through the abort path, and that path had no way to know the run was a compaction: the job metadata flattens
compactintoisRegenerate, and the abort persistence sites build their row from stream-aggregated content alone. The row therefore carried noinitiatedBy: 'user'marker, and on a branch ending in a user message the hover row kept Regenerate, which answered the user message behind the compaction instead of redoing it.This carries
compacton the job metadata and stamps the aborted content with the compaction identity at both abort persistence sites, through one helper alongsideresolveFailedTurnContentso all three persistence routes share the rule that a compaction turn is always identifiable. A stopped compaction keeps itsunfinishedshape and any partial summary it streamed; a run stopped before any part streamed records the typed failure so the row is still identifiable.How it works
markAbortedCompactionContentstampsinitiatedBy: 'user'on every summary and error part of the aborted content, the same markerisUserInitiatedCompactionreads to withhold edit, regenerate and continue, and appends the typedcompaction_failedpart when the stopped run streamed nothing yet.Type of change
Testing
Tested environments/configuration: mock e2e harness (Playwright, in-process fake model with the fixture summarizer held silent via
delayMs, stopped through the composer's stop control); jest for the unit and controller suites; MongoDB and the in-memory job store.Automated tests:
packages/api:npx jest src/agents/compaction.spec.ts src/stream/__tests__/abortCompactionIdentity.spec.ts src/stream/__tests__/RedisJobStore.spec.ts: 97 passedapi:npx jeston the touched suites (request partial disconnect, resume metadata, abort route): 218 passedreviewctl precheck(static + jest per touched workspace) on the final head: static pass,@librechat/api2,855 related tests passed; theapiworkspace run had two suites crash on Jest worker OOM under shared-machine load with zero test failures (5,588 passed), environment rather than diffe2e/specs/mock/scenarios/compaction-rerun-controls.spec.ts, run throughreviewctl verifyon desktop light, desktop dark and mobile:cancelled-compaction-on-user-turn-offers-no-rerun-controlsandcancelled-compaction-on-user-turn-survives-reload-without-rerun-controls, both passing on all three projects on the final headScreenshots / recordings
No user-facing change beyond the withheld hover controls on a cancelled compaction row, which the new e2e scenarios assert in the DOM.
Risk / compatibility
No schema migration: the marker is a content-part field that already existed. Redis job records decode the new
compactmetadata key with absent-means-absent, so mixed-version records read as before. A stopped compaction that streamed nothing now persists a typed-failure row instead of an empty one; ordinary turns keep the early-abort behavior unchanged.Checklist