🤖 fix: fence history resets, truncation and rejected context - #4135
Conversation
This comment has been minimized.
This comment has been minimized.
a6cead8 to
579b4ad
Compare
![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: a6cead8cbd
ℹ️ 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".
This comment has been minimized.
This comment has been minimized.
579b4ad to
ed0c0f2
Compare
|
Addressed both findings:
Both failures reproduced before the fixes. All 323 tests across seven suites pass with default timeouts after serializing local validation; all 22 focused regressions and full static checks pass. Independent review approved the two-file fix, with 28 production additions and 15 deletions. @codex review Generated with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed0c0f2b12
ℹ️ 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".
2283373 to
297b169
Compare
|
Addressed 3957729509 with one shared predicate across both tail-cut paths. Removing a durable reset, rollover, or compaction boundary advances publication even when its text is empty. Retained boundaries, invalid empty metadata, display-only changes, and preserved raw reset evidence keep their prior behavior. The new matrix reproduced 12 failures before the fix while five controls passed. All 340 tests across seven affected suites and final static checks pass with default timeouts. Independent review approved the 13 changed production lines. Scope reassessment kept this round to the existing context-change invariant. @codex review 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.
297b169 to
f369285
Compare
|
@codex review The journal prerequisite #4133 merged. GitHub rebased this layer onto main at f369285; the complete tree is identical to previously approved 297b169. Please confirm the current head. The validated fencing delta and 347-test evidence are unchanged. 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: f369285d4f
ℹ️ 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".
ThomasK33
changed the title
|
Finding 3958805613 is valid missing coverage in the existing deletion paths. It predates this PR's reset/truncation changes. The concrete fix is now #4148: 43 added and 8 removed production lines covering deleteMessage, deleteMessages, archive fallback, and private deletion used by commitPartial. It uses the shared raw-aware provider suffix and fences before rewriting history. That follow-up passed independent review, 426 tests across six affected suites, final deletion/publication-barrier cases, and full static checks. Its failure tests verify generation-write failure preserves history/counters and later history-write failure retains conservative invalidation. Fresh publications remain possible after deletion. This PR's title and description now state its actual scope: resets, truncation, and rejected context. Keeping the distinct deletion family in #4148 follows the requested small-PR delivery; it is not being dropped or claimed fixed by this branch. #4148 is based on this PR and will follow it onto main. @codex review Please review this reset/truncation layer within that explicit scope; the cleanup-deletion finding is implemented and reviewable in #4148. Generated with |
|
Codex Review: Didn't find any major issues. Keep them coming! 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.
|
@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. All three reset/truncation commits remain identical by git range-diff. All 347 affected tests and the full static-check components pass on the combined tree. No new behavior or scope was added. The previously reviewed scope remains unchanged: reset, truncation, and rejected-context fencing here; cleanup deletion is implemented separately in #4148. That follow-up and the pending-attachment layer remain untouched until this parent lands. Generated with |
f369285 to
b258ce2
Compare
|
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.
|
Addressed the shared classification issue found in #4148 review 3960203170 at its owning lower layer. Anthropic thinking requests can retain reasoning-only assistant messages, so destructive-history fences must count those removals conservatively. This opt-in now covers active/archive tail cuts, percentage truncation, and context-budget rejection; unrelated helper callers keep their defaults. Six regressions reproduced the missing fence. All 374 affected tests and full static-check components pass; independent review approved. The production fix is 25 changed lines across the shared helper and history service. F2 will retain deletion-specific regression coverage and inherit this implementation after integration. The fixed phase remains #4135 → #4138 → #4148, queued together only after every member is ready. @codex review Generated with |
|
Codex Review: Didn't find any major issues. 🎉 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.
The TokenBudget story immediately checks visibility after opening a warning. It can find and click the control while an ancestor is still transparent, making the one-shot assertion fail. Await the existing visibility assertion with waitFor, retaining the same requirement and failure timeout. Stacked above coder#4143: full unit CI on this otherwise unchanged one-line patch reproduced that PR's confirmed reconnect version-fixture leak. After integrating the prerequisite, both affected API/analytics suites pass in the proven analytics-first order (seed 1): 37 tests, 1,949 assertions. Final full static checks also pass; the Storybook patch is identical. This changes one test line; application behavior is unchanged. The HighUsage smoke test failed on backend-only PRs coder#4133 and coder#4135. CI logs do not capture ancestor styles, so the original opacity cause remains unconfirmed. Validation: a controlled real-browser test held the actual entrance wrapper at zero opacity. The old assertion failed; the new play remained pending, passed after release, and still failed when permanently held. All 10 TokenBudget Storybook tests passed. All 20 built-Storybook manager cases (10 stories, dark/light) emitted explicit success events at their declared desktop or 375px mobile viewport. Production Storybook build and full static checks passed; Nix formatting skipped because Nix is unavailable. --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable -->
Advance the publication generation under the history lock before reset, rollover and destructive edits. Preserve publication for refused and non-destructive operations. 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: I4d2655f52391e903a03fb60a5418b3d5a1d93a1c
Advance publication generation before context-budget rejection removes provider context. Reuse the raw-reset-aware active history suffix when classifying prefix truncation, preserving publication and usage for sealed-only cuts. 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: I5d12687dde1a58d5dda26c0185c8d69fe9978182
Treat a removed reset or compaction boundary as a provider-context change even when it contains no replayable content. Share the predicate across active and archived tail cuts while preserving retained raw reset evidence. 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: I387fae5c06ea5b37f0c3d52eecac0e4fa1117228
Conservatively include reasoning-only assistant messages in destructive-history eligibility, since Anthropic thinking requests preserve them. Apply the opt-in to active/archive tail cuts, percentage truncation, and context-budget rejection; unrelated helper callers retain their defaults. Six regressions fail before the fix and pass after it. All 374 affected tests and full static components pass; independent review approved. 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: I66025f7b1876072451738c8db4ce5fbd10ca58b5
|
@codex review Integrated current main so this phase includes merged CI fix #4149. GitHub confirmed the previous top-layer unit job exceeded its old 15-minute limit; the merged workflow supplies the corrected limit and comment gate. Every own-layer commit remains identical by git range-diff. Validation on the integrated base: 374 fencing tests, 63 pending-protocol tests, and 482 combined top-layer tests pass; full static checks pass at the phase top. No source changes were needed during integration. The complete phase remains #4135 → #4138 → #4148. Please confirm the current head; all three will be queued together after refreshed review and CI pass. Generated with |
2569167 to
e644e5e
Compare
1 participant
A compactor in another backend can retain a snapshot after reset, rollover, truncation or context-budget rejection. Advance the journal publication generation under the same history lock before those mutations, so stale work cannot publish or fold into the changed history.
This is the reset/truncation/context-budget fencing layer above #4133, split from closed #4121. Refused operations, no-ops, sealed-prefix or targeted display-only cuts, and ordinary compaction preserve publication ownership. Cleanup deletions through deleteMessage/deleteMessages and their private helper are a separate F2 follow-up; this PR does not claim to fence every history mutation.
Context-budget rejection also retires publication when it removes eligible provider context. Tail cuts recognize removal of a durable reset or compaction boundary even when it contains no replayable text. Prefix cuts use the raw-reset-aware active suffix, preserving generation and usage metadata for rows sealed by malformed reset evidence.
Validation: 347 tests pass across seven history/compaction suites with default limits after rebasing onto the journal review fixes. They cover foreign stale publication/fold rejection, fresh publication success, failure before destructive writes, lock ordering, context-budget rejection, removal of empty boundaries, and raw reset privacy floors. Full static checks and independent local review pass.
Risk: a later history-write failure can conservatively leave the generation advanced, discarding an in-flight compaction while retaining history. Durable Stop and attachment persistence remain separate layers.
Generated with
xum• Model:unavailable• Thinking:unavailable• Cost:$unavailable