🤖 refactor: capture post-compaction snapshots for request consumption - #4134
Conversation
This comment has been minimized.
This comment has been minimized.
|
@codex review Please review the exact local preparation/write/consumption owners and snapshot capture before terminal awaits. This layer preserves the persisted V1 format; cross-backend receipts and durable cancellation are follow-up layers. Generated with |
This comment has been minimized.
This comment has been minimized.
![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: cfc3cb196e
ℹ️ 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".
|
Addressed both findings:
The original failures reproduced before the fix. All 25 ownership tests pass, including 12 new regressions; the broader 409-test run passed with a 30-second CLI timeout while the machine was under concurrent validation load. Full static checks and independent review pass. No test timeout constants were changed. @codex review Generated with |
3c0f6a8 to
00cf436
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00cf436aa7
ℹ️ 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".
|
Reassessing this boundary after the second substantive review round. The new findings show that preparation takeover, publication and rollback restoration form a coupled protocol; they should not remain bundled with request snapshot consumption. This PR is being narrowed to request-captured snapshots and exact local acknowledgment/discard while preserving baseline preparation/rollback behavior. The preparation/publication/rollback protocol will be a separate delivery. The full current attempt is preserved locally at The current diff is not ready to merge. The findings remain unresolved until the narrowed candidate is implemented, validated and reviewed; dependent cleanup and handoff layers will be rebased onto that candidate. Generated with |
00cf436 to
7b06ea8
Compare
ThomasK33
changed the title
|
The PR has been narrowed to C1 rather than continuing the preparation/rollback review loop. Findings 3957739385 and 3957739390 concern the removed preparation-identity cleanup protocol. Finding 3957739393 concerns its removed predecessor-file restoration protocol. Those paths are no longer introduced by this PR; preparation/publication/heartbeat rollback use their main-branch implementations. The full attempt is preserved locally on The new delta covers request-captured acknowledgement/discard, periodic carryover, and serialized local sidecar cleanup. Independent review approved this narrower boundary. @codex review Please review the replacement C1 diff against main. Generated with |
7b06ea8 to
2faff0c
Compare
|
@codex review Rebased this unchanged four-file layer onto current main so dependent history-publication layers can validate their real integration base. git range-diff confirms the C1 patch is identical. All 414 tests across 11 affected suites and full static checks pass on the rebased head. Generated with |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
This comment has been minimized.
This comment has been minimized.
Wait for the warning's visible state after opening it so the assertion can tolerate the app entrance transition without accepting persistent invisibility. Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable --> Change-Id: Ieeb9c6b96af089dab27156d43a7f54fcb9b38dbe
Bind acknowledgement and context-exceeded discard to the snapshot injected into that request. Serialize local sidecar writes and cleanup so a late consumer cannot delete replacement state. Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable --> Change-Id: Ic4c8b849c3a49f7269591d140df156251240020b
|
@codex review Prioritizing this bottom layer for merge. Integrated the shared CI fixes: #4143 is merged and the validated one-line Storybook fix #4145 is queued immediately ahead. The C1 implementation patch is identical by git range-diff; no new behavior or scope was added. All 414 affected tests and full static checks pass on the combined tree. This avoids rerunning the known broken reconnect fixture in each unrelated refactor PR. Upper layers remain untouched until this parent lands. Generated with |
2faff0c to
369b1a6
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 369b1a6f52
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 369b1a6f52
ℹ️ 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".
Retire provisional pending state when manual boundary publication fails, including read paths. Preserve the original captured owner when a continuous preparation rolls back, so acknowledgment and discard remain valid even if restoration persistence fails. Five regressions reproduce both findings before the fix. All 214 targeted tests and full static checks pass; independent review approved the 17-line production fix. Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable --> Change-Id: I64df09b02161295313177bf6c3acf0b66873392f
|
Addressed both findings:
Five regressions reproduced the failures before the fix. All 214 targeted tests and full static checks pass; independent review approved. The production delta is 17 changed lines. No broader producer protocol or runtime activation was added. @codex review Generated with |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95530cff7c
ℹ️ 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".
Capture the predecessor owner for heartbeat preparation and preserve it through failed append and contention rollback, including restoration write failure. Existing request acknowledgment/discard authority then survives restoration just as it does for continuous compaction. All restoration call sites audited. Eight regressions reproduced before fixing; 222 targeted tests, 17 dispatch tests and full static checks pass. Independent review approved the nine-line production fix. Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable --> Change-Id: Iaf0e58405faee72c8073bd0dad40cc5a3d50df7e
|
Addressed finding 3960175478: heartbeat rollback now captures and restores its predecessor's original owner on failed append and contention rollback, including failed restoration writes. A request that already captured that snapshot can still acknowledge or discard it. Audited all three persistence call sites: heartbeat and continuous restoration preserve owners; fresh preparation creates a new owner. Eight regressions reproduced the failure before the nine-line production fix. All 222 targeted tests, 17 dispatch tests, and full static checks pass; independent review approved. @codex review Generated with |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Queued compaction cleanup can resume after a replacement turn and overwrite newer summary metadata or delete its heartbeat boundary. Make these two cleanup operations conditional under the existing history lock: match the captured summary ID, sequence and pending request, transform the current row, and recheck local ownership immediately before rename. Skipped heartbeat cleanup restores no pending state and emits no deletion. Ordinary history update/delete APIs and serialized cleanup waiting remain unchanged. This small prerequisite above coder#4134 replaces part of closed coder#4121. Correlated continuation handoff will be a separate PR; this change does not enable overlapping retirement or add durable cancellation. Validation: the original broader run passed 303 tests; the final adapted layer passes all 176 affected tests and full static checks. Coverage includes exact cleanup, 10 corrupt-sequence cases, duplicate-identity skips, same-ID/different-sequence controls, and session races while cleanup is queued or its committed result is delayed. Invalid or ambiguous identity evidence returns a recoverable skip without permitting deletion by ID alone. Independent review approved the adaptation; coordinator review approved the final uniqueness guard. Risk: an incorrect match could clear the wrong pending follow-up or skip a valid cleanup. Tests cover current-owner success, stale-owner rejection, unrelated metadata preservation, staging failure, and preserved history sequence/archive behavior. --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable -->
Add inactive continuation-ownership primitives to TurnCoordinator. A claimed follow-up has an exact identity tied to its source turn, can transfer that identity through its own admission, and retains its physical slot until its owner finishes. Admission and edit reservations prevent conflicting claims; Stop cancels only an existing claim, and an edit reservation temporarily excludes work without retiring intent before history commits. The final diff contains only the coordinator and its tests. No production caller claims a follow-up token or passes a compaction handoff, so AgentSession dispatch, queue, Stop, edit, and recovery behavior remain unchanged from coder#4137. Runtime activation is deferred until targeted contention, failed-recovery queue ordering, durable acceptance, and committed-edit retirement have one coherent scheduling contract. This is the remaining member of the continuation phase. Prerequisites coder#4134 and coder#4137 have merged; the five D1 commits are now rebased onto main with identical patches and an unchanged complete tree. The coordinator will submit this member through the native stack queue after current-head review and CI pass. Scope remains limited to these inactive primitives and review fixes. Validation: 50 coordinator tests and 175 affected tests pass, covering admission exclusion, fresh claims after Stop, temporary edit vetoes, exact physical settlement, own-admission transfer, reentrant PREPARING callbacks, and unchanged behavior without tokens. Tagged handoffs cannot borrow an edit reservation's exemption. Independent review confirmed the two-file diff and absence of production claimants. Full static checks pass on current head 184097c. Nix formatting was skipped because Nix is unavailable. The prior merge-group Storybook run hit an inherited BrowserWithoutBridge visibility race: a diagnostic observed the connected button under AppLoader at opacity zero before its entrance frame. The exact intermittent assertion did not reproduce in five local runs (35 tests passed), and current-head Storybook CI passes. D1 leaves the renderer and story unchanged; future queue runs retain that inherited timing risk. --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable -->
1 participant
A request now acknowledges or discards the post-compaction snapshot it actually injected. A late stream completion or context-exceeded retry cannot consume a newer snapshot, including a replacement with identical contents. Periodic reinjection keeps the identity of its consumed carryover.
This is C1 of the agent-loop refactor, narrowed from the earlier preparation-ownership proposal. It adds local snapshot identities and serializes sidecar writes against cleanup. Preparation, boundary publication, heartbeat rollback, and continuous-compaction rollback retain their baseline behavior; their stronger transaction protocol is a separate change.
Validation: all 397 tests across 11 compaction/session suites passed with default timeouts, and static-check passed. Tests cover delayed load, queued cleanup versus replacement, failed successor writes, stream completion, context-exceeded discard, and periodic reinjection with real history and controlled I/O. Independent review approved the scoped production delta: 170 changed production lines and 401 test lines.
Risk: acknowledgement and carryover handling change across asynchronous stream completion. This layer protects local consumers; it does not establish cross-process publication or rollback guarantees.
Generated with
xum• Model:unavailable• Thinking:unavailable• Cost:$unavailable