🤖 fix: activate durable compaction cancellation across turn admission - #4191
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ab5348384
ℹ️ 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 all three findings in 38b907a:
Validation: 701 tests across cancellation runtime/core/storage, exact cleanup, and WorkspaceService; 743 tests across AgentSession callers and history truncation (overlapping the runtime suite); TypeScript, no-cache ESLint, and full Generated with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38b907ac6b
ℹ️ 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".
38b907a to
1ac4cea
Compare
|
Addressed the three commit-boundary findings in their owning layers:
The lower layer also fixes the Integration CI value-comparison failure: capture CAS compares primitive nonce/generation values, and exact resume comparisons use a realm-local JSON snapshot. The original mock-only IPC test reproduced the normal-send rejection; dedicated append and resume acceptance tests now pass under Jest's VM and complete mock streaming. Final validation: lower commit Generated with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ac4cea9b7
ℹ️ 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".
Keep Stop authoritative through failed manual preparation, and retire it only after durable replacement acceptance or exact canceled-follow-up cleanup. Preserve fresh automatic monitor/family work after ordinary Stop while retained Stop waits for manual replacement. Carry original Stop admission through recovery and compaction producer awaits. 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: Ia7f59680bde10259475c31fafbee04f8012ed832
Preserve successful stream-stop evidence across cancellation persistence failure so terminal accounting and hard-stop cleanup still finish before reporting durability debt. Reconcile ordinary cancellation after its exact summary follow-up was already cleared, preserving retained floors and foreign Stops. Allow explicit downgrade intervention to install a guarded retained fence over unsupported records while automatic readers preserve them. 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: I90bc366783839b19b42311a18b1a6afab4821091 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Recheck every captured admission fence while automatic single and batch appends remain eligible for rollback. Preserve concurrent rows and the existing committed manual-replacement boundary. Recheck edit ownership at the final active-history rename. Stage archive transaction outputs before a synchronous marker, tombstone, archive, and chat commit using the existing recovery hashes. Stop during staging leaves original history intact, and failed transaction recovery verifies physical ownership before touching successor files. Committed cleanup failure remains recoverable on restart. 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: I32c84c86a73826143f232779f6cdf09d71a6b466 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Send now previously carried the queued entry's pre-Stop generation into dispatch and silently refused its user row. Refresh only the selected manual entry's compaction capture; automatic entries, caller cancellation, and a later Stop remain fenced. Explicit manual recovery can replace the exact still-current oversized narrowing debt with a retained Stop. Automatic reads remain blocked, and a newer Stop, retry, or ordinary I/O failure cannot be displaced by that fallback. Finish subscription replay before the resume test's error window. Share the existing helper with mock regressions that distinguish old replayed errors from a new resumed failure; preserve the provider test's continuation assertions. Validation: 833 surrounding unit tests, seven mock IPC regressions, all five original dropdown tests, the original queued-send IPC case under mock routing, make typecheck, and no-cache ESLint pass. Parent comparisons reproduce the runtime-only failures. Outbound network is denied in mock test commands. 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: Icb526da23c82b687fdf461c3da3f347761f99a8a Signed-off-by: Thomas Kosiewski <tk@coder.com>
1ac4cea to
639a4d7
Compare
|
Addressed both findings in
The resume CI failure had a separate subscription window: the helper counted the previous attempt's replayed stream-error as a failure of the resumed attempt. The shared helper now waits for initial subscription replay and clears those events before resume. Existing prefix/continuation assertions are unchanged; mock tests prove both successful resume and rejection of a fresh resumed error. Final validation on this exact combined head: 2,665 tests across 52 files with 13,488 assertions; 15 mock IPC cases; five original Send Mode UI cases; the original queued Send Now case; Generated with |
|
@codex review Please review current head Generated with |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 639a4d78cb
ℹ️ 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".
Keep per-add compaction probes and manual refresh authority beside acceptance origin, combining only the remaining entries at dispatch. Ordinary queued manual additions batch again; caller-specific probes still seal their entries. Validation: the real ServiceContainer Send Now regression reproduces a missing second addition before repair, then persists one joined user row. All 94 queue tests and eight mock IPC cases pass, including caller cancellation, automatic identity, second Stop, and keyed withdrawal controls. Types, no-cache ESLint, formatting, and diff checks pass. Independently reviewed by Boyle and root. 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: I113b01af73d7fc80ad11cd54d48a34f914c4956d Signed-off-by: Thomas Kosiewski <tk@coder.com>
Publish the cancellation fence before neutralizing legacy-visible follow-ups, then recheck the exact nonce and generation after the captured engine and terminal policy settle. Existing history cleanup preserves folds, boundaries, tails, and unrelated metadata; failures keep the durable fence and retry debt. Recheck blocking debt after history reads before narrowing from both automatic admission paths. Otherwise an old captured Stop can join a newer Stop that is waiting for the same terminal policy, deadlocking interruption. Validation: 795 focused tests, both deterministic cycle regressions, and four actual older-parent recovery cases pass. The combined repair pair also passes 94 queue tests, 16 mock IPC cases, five original Send Mode UI cases, and the original queued Send Now case. Jest denies all outbound provider requests. Build, types, no-cache lint, formatting, and full static checks pass. Independently reviewed by Leibniz; committed under coordinator authorization. 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: I51d80c9914e1d2c1208546fd28d861f768defb3c Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
Addressed both findings in the runtime repair pair ending at
The actual older parent Validation: 795 focused tests, 94 queue tests, 16 mock IPC cases, five original Send Mode UI cases, the original queued Send Now case, and four actual-parent compatibility cases; build, types, lint, formatting, and diff checks pass. Fresh full Generated with |
|
@codex review Please review current head Generated with |
There was a problem hiding this comment.
💡 Codex Review
xum/src/node/services/historyService.ts
Lines 491 to 495 in 1843fe9
When a hard Stop that preserves partials (such as ACP cancellation or the CLI budget stop) encounters a malformed partial.json, this unconditional parse failure occurs after the cancellation sidecar has been committed. The mutation remains blocking, and every subsequent manual send retries the same cleanup and fails before it can publish a replacement, permanently bricking the workspace until the user manually deletes the partial. Quarantine or discard malformed partial state, or allow explicit replacement to supersede this cleanup debt.
AGENTS.md reference: AGENTS.md:L110-L111
ℹ️ 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".
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Activates durable compaction cancellation across Stop, manual send/edit/resume, queued input, and automatic recovery. Stopped compaction continuations stay canceled across restart and downgrade. Failed manual preparation keeps Stop in force; a successful manual replacement becomes durable before retained Stop cleanup. Ordinary queued manual additions still form one follow-up, including when dispatched with Send Now.
Based on #4187, using the replacement-acceptance and prepared-publication seams from #4182 and #4187.
Validation on
1843fe91e5163e6962acb47c37077600882f1272: 795 focused cancellation, history-cleanup, admission, queue-dispatch, and WorkspaceService tests pass with 3,234 assertions; the separate batching selection passes 94 queue tests. Sixteen mock IPC cases, all five original Send Mode UI cases, and the original queued Send Now regression pass. All final Jest runs deny outbound fetch/HTTP/HTTPS.make build-main, types, no-cache lint, formatting, and diff checks pass. Fresh fullmake static-checkpasses on the corrected candidate. Build, IPC, and UI checks preceded the final two post-history-read debt guards; the final focused tests, types, lint, and fresh full static cover those guards.The actual older parent
2f180b0cwas run against sanitized real Stop artifacts. With the former disk shape it dispatches the canceled continuation despite the sidecar; with repaired artifacts it starts no stream and preserves the fold and tail. Four cases cover summary startup and real partial commit followed by startup. Deterministic tests also hold engine/policy writers across initial cleanup, protect newer nonce/generation successors, inject failure in both cleanups, and reproduce both captured-old-Stop/new-Stop policy cycles without timeouts.The main risks are shared admission behavior and additional history cleanup before Stop is acknowledged. Physical interruption begins immediately; unreadable or unsafe recovery artifacts retain cancellation and return an error rather than claim successful durable Stop. Validation covers real local storage and the frozen older reader, but does not establish live-provider or multi-backend end-to-end behavior.
Generated with
xum• Model:unavailable• Thinking:unavailable• Cost:$unavailable