🤖 refactor: add inactive pending attachment file primitives - #4138
Conversation
This comment has been minimized.
This comment has been minimized.
|
@codex review Please review this unactivated protocol layer against #4135: receipt identity, fallback consumption, generation-qualified rollback, and final ownership checks. Runtime adapters and producer/consumer activation are separate. Generated with |
42bb258 to
06bee05
Compare
This comment has been minimized.
This comment has been minimized.
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
06bee05 to
b40db4e
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: b40db4ef71
ℹ️ 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: 6dea0c7688
ℹ️ 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
|
Addressed 3958201268 with best-effort rejected-state pruning, and 3958201275 by preserving unsupported schema bytes. Exact/mandatory cleanup errors remain visible. 3958484081 and 3958484087 identify missing evidence in legacy writers. Existing metadata cannot distinguish fresh downgrade output from stale pre-reset state. The explicit policy preserves ambiguous bytes, suppresses uncertain enrichment and recovers after fresh compaction. Two real-history regressions verify these cases without weakening fences. 3958201263 is valid for a complete publisher. This PR is explicitly narrowed to inactive file/receipt primitives. Its API contract now requires one transaction spanning prepare, history publication, synchronous receipt and failure cleanup. The separate G2b transaction layer will provide this before runtime activation; standalone prepare is not advertised as atomic history publication. 59 focused tests and full static checks pass; final legacy cases also pass focused lint. Please review the narrowed primitive contract and load fixes. @codex review Generated with |
be19fcc to
9621303
Compare
|
@codex review GitHub rebased this layer after #4133 merged; current head is 9621303. Its complete tree matches be19fcc. The preceding response covers all five findings and the explicit inactive primitive/legacy policy contract. All addressed threads are resolved; please review this current head. 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.
9621303 to
36d7d4c
Compare
|
@codex review Integrated the shared CI fixes through the current F1 base. All three G1 commits and the final protocol/test blobs are unchanged; 59 affected tests and full static checks pass on 36d7d4c. Runtime activation remains outside this PR. This is the middle layer of the fixed phase stack #4135 → #4138 → #4148. All phase members will be queued together after current Codex approval, resolved findings, and passing CI. Scope is frozen to review and required integration fixes. 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: 36d7d4c482
ℹ️ 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".
36d7d4c to
df69f39
Compare
|
Addressed both findings with a nine-line production fix:
This protocol remains inactive. Its future atomic activation must separate optional enrichment refusal from mandatory history publication; unsupported data is preserved rather than overwritten before that boundary exists. Integrated F1's reviewed reasoning-context classification. All four own commits remain equivalent; 60 focused tests and full static checks pass on df69f39. Independent review approved the narrowed compatibility fix. @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: df69f396b1
ℹ️ 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 3960609110 in the shared read path: an empty directory at the sidecar path is removed nonrecursively, allowing subsequent load, prepare, and discard operations to recover. Nonempty directories retain their contents; failed removal remains visible to mutation/reset callers while optional loading keeps its existing best-effort behavior. Three regressions failed before the fix. All 63 tests and full static checks pass; independent review approved the seven changed production lines. Unknown-version preservation and the inactive protocol scope are unchanged. @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.
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
Introduce an inactive file protocol for exact snapshot receipts, publication, consumption and qualified rollback. Preserve V1 loading and guard ownership immediately before publication. Keep runtime integration in a separate layer. 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: Ia49b1d38afce332d9adc3113275a690a4db7cad0
Distinguish known absence, identified boundaries and unreadable reset floors. Persist fallback evidence and centralize generation eligibility so rollback, restart and cleanup cannot revive legacy attachments across a newer boundary or destructive edit. 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: I37d2763e3072b5dbddcebd69bfb951d19c1ff3ee
Make rejected-state pruning best-effort, preserve newer schema bytes, and document the inactive file primitives' transaction and conservative legacy contracts. 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: I3b8aadf43ba7dee9092bf9d83df17e1fe4e698d0
Treat optional sidecar read failures as absent, and refuse standalone preparation without modifying unsupported newer-format bytes. This keeps failed boundary publication from losing state owned by another version, without adding a format older readers could misinterpret. Valid V1 behavior is unchanged. All 60 focused tests and targeted lint pass; independent review approved. Atomic activation must keep optional enrichment refusal separate from mandatory history publication. 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: Ica841c16829b5bc996cb8ad3d71be5ca9c494a9b
Repair empty directory entries through the shared read path. Nonrecursive removal preserves unrelated nonempty contents and mutation errors remain visible. Validation: 63 tests, full static checks, and independent review pass. 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: I969b6ffe9fbf5d927a70c8ba76d8c15e60a92b51
|
@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 |
6fe914a to
ca0a5a5
Compare
This comment has been minimized.
This comment has been minimized.
|
Security 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: ca0a5a5096
ℹ️ 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".
Only object roots can represent unsupported schemas. Treat null, arrays, and primitive roots as invalid state so later preparation can recover, while preserving plausible future objects. Validation: 69 focused tests and independent review pass; full static validation recorded before publication. 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: Ie666f6a1402822b24095ec4d9301217268f4b448
|
Addressed 3961174517 with one shared parser correction: only object roots can represent an unsupported schema. Null, arrays, and primitive JSON now follow existing invalid-state recovery, allowing fresh preparation with or without a prior load. Plausible unsupported object schemas remain preserved and declined. All six recovery cases failed before the fix. All 69 focused tests and final static checks pass; independent review approved. Three production lines changed, including the rationale comment. No format, interface, or activation changes were added. @codex review Generated with |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
|
Security 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. |
…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
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 #4135 → #4138 → #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