🤖 refactor: centralize prepared history publication and origin - #4187
🤖 refactor: centralize prepared history publication and origin#4187ThomasK33 wants to merge 2 commits into
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. |
1512246 to
41f5ab3
Compare
41f5ab3 to
04977de
Compare
|
@codex review Please review current head Generated with |
|
Codex Review: Didn't find any major issues. Another round soon, 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". |
🛡️ 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. |
04977de to
2f180b0
Compare
|
@codex review Please review current head 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". |
🛡️ 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. |
2f180b0 to
7235c68
Compare
|
@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. |
![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: 7235c68bc0
ℹ️ 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".
7235c68 to
a54cba5
Compare
|
Addressed PRRT_kwDOPxxmWM6hM7oo (finding) in The foreign row still lands after the first owned prefix and before the failing write. Exact call order and persisted sequence assertions prove interleaving; rollback must preserve that row and start no provider. All 11 isolated middle tests and 15 integrated runtime tests pass, as do canonical static checks. No production change was needed for this finding. Generated with |
|
@codex review Please review current head Generated with |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
🛡️ 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. |
a54cba5 to
2c8c781
Compare
|
@codex review Please review current head Generated with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c8c7818a6
ℹ️ 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. |
2c8c781 to
7f26d64
Compare
|
Addressed 3988873763 at the HistoryService seam. The real service still writes successful prefixes and a foreign row. A spy returns the normal Err result for the failing append, and actual disk-backed rollback preserves only the foreign row. Returning Err preserves the API contract exercised here; throwing would test a different exceptional path. Filesystem append mocking was removed. All11 prepared-publication tests and canonical static checks pass; the propagated V1 tests also pass. Generated with |
|
@codex review Generated with |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
🛡️ 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. |
7f26d64 to
1a5231b
Compare
|
@codex review Generated with |
|
Codex Review: Didn't find any major issues. Delightful! 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". |
🛡️ 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. |
Keep prepared publication and manual/automatic origin at one shared boundary. Rebased unchanged owned layer onto durable acceptance review fixes. 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 -->
Preserve real successful prefixes, foreign writes and rollback assertions while injecting the normal returned failure at the HistoryService boundary. 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 -->
1a5231b to
fe7f0e7
Compare
|
@codex review Please review the current head after the complete phase was composed on current main. All known findings have fixes and replies in their owning layers; the eight-member phase stays held until all members are approved and green. 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: fe7f0e7618
ℹ️ 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".
| for (const _row of earlier.slice(1)) appends.mockImplementationOnce(append); | ||
| // HistoryService reports write failures as Err. Inject that public contract rather than | ||
| // a thrown exception, which would exercise a different preparation failure path. | ||
| appends.mockResolvedValueOnce(Err("injected write failure")); |
There was a problem hiding this comment.
Fresh evidence in the current head is that the failure is now injected with mockResolvedValueOnce(Err(...)), so these cases still do not exercise a rejected HistoryService call and can miss cleanup failures that occur when publication throws after earlier prefixes were persisted. Use the prescribed real-service spy with mockRejectedValueOnce(...) and arrange the successful/foreign writes around that rejection.
AGENTS.md reference: AGENTS.md:L117-L117
Useful? React with 👍 / 👎.
| messages.length === 1 | ||
| ? await this.historyService.appendToHistory(this.workspaceId, messages[0]) | ||
| : await this.historyService.appendManyToHistory(this.workspaceId, messages); | ||
| if (result.success) persistedCancelableMessageIds.push(...messages.map((row) => row.id)); | ||
| return result; |
There was a problem hiding this comment.
When a retained compaction cancellation exists, this shared trigger path still calls the ordinary append methods regardless of attempt.acceptanceOrigin; a repo-wide search finds no production callers of captureCompactionReplacement or acceptCompactionReplacement. Consequently a later manual send is never stamped as the durable replacement and cannot retire the retained Stop, while the newly propagated origin has no runtime effect. Route manual trigger publication (and the corresponding Resume path) through the replacement-acceptance API, preserving cancellation only for automatic work.
Useful? React with 👍 / 👎.
1 participant
Centralizes prepared history publication so manual triggers and their prefixes share one publication boundary, while preserving manual/automatic origin across preparation. Runtime layers consume receipts and rollback facts instead of inferring acceptance from a successful send result.
Publication uses #4182's guarded history operations. Origin stays independent of visibility and billing. Failure tests use a real HistoryService: successful prefixes and a foreign row persist normally, a service spy returns the public failure Result, and rollback must preserve the foreign row.
This update only advances the prerequisite base. The owned prepared-publication change is unchanged. Its 11 tests / 61 assertions and canonical static checks passed; propagated runtime tests cover guarded batches and actual receipt ordering.
Risk: publication callbacks affect queue and budget accounting. Real disk-state and foreign-writer assertions protect against lost or duplicated history.
Final phase integration on main
ad8a01b: independent composition review and full canonical static checks pass on tree3ed04896. Across 35 affected files, 2,549 tests / 28,590 assertions pass: 34 files passed before a test-only lifecycle repair, then all 28 pinned-budget tests passed after it. Two final-flush fixtures now complete their fake stream on Stop instead of waiting for later disposal; their original assertions and production behavior are unchanged.The complete cancellation phase is ordered #4214 → #4215 → #4219 → #4221 → #4182 → #4187 → #4191 → #4209. The reader prerequisites and runtime changes merge together only after every member has current-head approval and green CI.
Generated with
xum• Model:unavailable• Thinking:unavailable• Cost:$unavailable