🤖 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
Introduce an inactive pending-attachment file protocol with exact consumption and rollback receipts. Generation and boundary evidence gate which persisted snapshot can enrich a request; a consumed predecessor cannot return through a provisional successor's fallback. Ambiguous legacy bytes are preserved while uncertain attachment context is suppressed. Optional sidecar read failures load as absent. Empty directory entries at the sidecar path heal through nonrecursive removal; nonempty contents remain intact and mutation errors stay visible. Non-object JSON roots follow invalid-state recovery. Standalone preparation leaves plausible unsupported schema objects untouched and returns no receipt, so a failed boundary publication cannot erase another version's state. No new backup envelope or marker is introduced. Valid V1 handling remains compatible. Runtime activation is separate and must hold the existing history locks through preparation, boundary publication, synchronous receipt delivery, and failure cleanup. It must distinguish optional enrichment refusal from mandatory history publication. This is the middle of fixed phase stack coder#4135 → coder#4138 → coder#4148. Every member will be queued together after current Codex approval, resolved findings, and passing CI. Validation: 69 focused tests and full static checks pass on the integrated parent; independent review approved. Real filesystem cases cover exact receipts, predecessor consumption, generation/reset fences, ambiguous legacy files, directory repair and nonempty preservation, invalid JSON-root recovery, unreadable sidecars, and byte preservation across unsupported preparation and history-write outcomes. Nix formatting was skipped because Nix is unavailable. --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable -->
…der#4148) Deleting current provider rows or their boundary can leave a foreign compactor's captured generation valid, allowing stale initial, fallback, or boundary publication after cleanup. This change fences the removed occurrences before rewriting history, using the provider scanner's raw-aware suffix rules. It covers single deletion, batch deletion, archive fallback, and the private deletion used by commitPartial. Deleting a readable separator can join malformed fragments into new reset evidence; the shared scanner probe detects that change while preserving the raw bytes. Readable user/system reset floors participate in deletion classification without changing provider reads. Oversized reset evidence stays raw, and protected active identities prevent deletion or truncation from falling back to an archived duplicate. Exact-limit valid rows remain addressable because the size check excludes the trailing LF. Retained boundaries and separators, existing reset evidence, incomplete tokens, and ineffective cuts preserve generation. A later history-write failure retains the conservative fence. Protected parsed rows retain identity and sequence metadata separately from transformable rows. Partial rewrites and rename keep their cached counter above retained protected sequences. Seven counter regressions were reproduced; actual sequence reuse was not reproduced because ordinary appends already refresh durable history. Reasoning-only deletions inherit the conservative provider-context classification from merged coder#4135. The pending-state prerequisite coder#4138 has also merged; this PR is the remaining deletion layer. Runtime activation is separate. Validation: 518 affected tests and full static checks pass on the main-integrated candidate. Coverage includes raw-fragment joining, malformed-role reset floors, protected active/archive duplicate IDs, exact-limit and oversized rows, sequence accounting, real provider history, and stale initial/fallback/boundary publication. The exact-rollback fixture restores captured history bytes so its unchanged-generation assertions remain meaningful after generic deletion gains fencing. Edit/fork truncation now refuses a protected active target before archive fallback when no readable active match exists. Four regressions reproduced destructive fallback to an archived duplicate; twelve cases cover oversized and ambiguous reset rows in both keep/remove modes. Refusal preserves both files byte-for-byte, generation, and the publication journal. Readable active duplicates retain precedence, while unrelated protected IDs still allow legitimate archive truncation. The correction reuses existing identity metadata and the single-deletion guard; independent review approved it. The queued-base CI failure was isolated to four fault-injection cases after coder#4146 changed generation publication to write a staging sibling and then rename it. The fixture now matches that exact generation artifact's staging prefix, keeps history-stage matching exact, and asserts that injection fired. All four regressions, all eight write-failure controls, and the 349-test history suite pass on the previously failing merge-group base. Production behavior is unchanged by this integration correction. Nix formatting was skipped because Nix is unavailable. Linux E2E also now runs with one worker because all Electron workers share one Xvfb display and clipboard. On the exact failing queue candidate, native probes reproduced three clipboard denials in four competing-window runs: two lost focus before validation, and one during the awaited isolated-world activation check. Focusing the parent before clicking a self-closing auth popup reproduced the same closed-target error on attempt nine; authentication completed, the remote stayed connected, and the local renderer and projects remained intact. Security checks and all original tests remain unchanged. The unmodified remote-connection file passes all three tests with one Linux worker (34.9 seconds); 30 focused popup controls also pass. Reproduction used Linux arm64 with pinned Electron 40.9.3, Playwright 1.57.0, and Bun 1.3.5; the failing CI runner was x64. Full static checks and both workflow linters pass for the final CI-only correction. The worker-count repair was verified at b82705b by the full Linux x64 CI suite: 55 tests passed and 21 skipped using one worker, including all remote-connection tests. Measured test execution took 8.5 minutes; the complete job took 9m56s including setup and build ([CI result](https://github.com/coder/xum/actions/runs/34279578520/job/102240858941)). Serial execution trades some throughput for stable native focus without reducing coverage. Regression risk is in classifying which removed occurrences affect provider context while preserving malformed privacy floors and occupied sequences. Tests cover retained-floor controls, duplicate IDs, restart, and generation/history-write failure ordering using real HistoryService storage. --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable -->
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