fix(sessions): reconcile pending input appends - #4776
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. |
![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: b2e3048e02
ℹ️ 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 review |
|
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: ebacb6c356
ℹ️ 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 review |
|
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: b35f30006d
ℹ️ 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 review |
|
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
Prefer the exact committed tail before overlap checks
When the pre-append tail overlaps the pending batch—for example, a one-item Session containing "same" receives another identical message—and add_items() commits before raising, the observed [same, same] tail equals expected while its last len(before) items also equal before. This makes both committed and unchanged true, so the next line reports an ambiguous history on every retry even though the exact expected tail proves the append committed. Handle committed first and only test the unchanged suffix otherwise.
AGENTS.md reference: AGENTS.md:L202-L202
ℹ️ 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 review |
|
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: cb9fcdb750
ℹ️ 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".
|
Follow-up to the duplicate-tail concern in review 5059982645: 97c9a62 adds the safe committed-tail preference, but only when the initial m+1 read proves that the complete pre-append history was observed. In that complete-history case, a lost acknowledgment after appending an item identical to the old tail is recognized exactly once. If the bounded pre-append window was saturated, committed and unchanged can both match after a pre-commit failure, so the retry remains fail-closed instead of blindly accepting committed. Regression tests cover both the one-old-identical lost-ack success and the three-old-identical ambiguous atomic-failure case; the checkpoint and history remain intact in the latter. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! 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. |
linhongyu510
left a comment
linhongyu510
left a comment
There was a problem hiding this comment.
One remaining ownership issue in the latest reconciliation path: item_reference.id is payload identity, not a response-only envelope ID. The existing sanitizer test explicitly includes item_reference among item types whose required IDs must be preserved, but reconciliation removes that ID unconditionally.
| dict[str, Any], | ||
| _sanitize_openai_conversation_item(normalized), | ||
| ) | ||
| clean.pop("id", None) |
There was a problem hiding this comment.
P1: Preserve semantic IDs for item_reference during reconciliation
This unconditional removal makes distinct references canonicalize identically:
{"type": "item_reference", "id": "item_A"}
{"type": "item_reference", "id": "item_B"}Both become {"type": "item_reference"}. After a lost ACK, a checkpoint for item_A can therefore treat a Session tail containing item_B as the committed batch, clear the pending write, and consume the owned pending input even though the referenced item differs.
This conflicts with test_sanitize_preserves_ids_required_by_openai_conversation_items, which correctly lists item_reference as requiring its ID. Please preserve id for item_reference (and ideally remove IDs only for item types confirmed to receive output-only server IDs), with a resume regression asserting item_A vs item_B fails closed before checkpoint/pending-input cleanup.
There was a problem hiding this comment.
Confirmed against current head 97c9a62b in an isolated worktree. The existing sanitizer suite passes (38 passed), but the direct reconciliation regression fails exactly as described:
{"a": {"type": "item_reference"},
"b": {"type": "item_reference"},
"equal": true}
AssertionError: distinct item_reference IDs canonicalized equally
So the existing required-ID sanitizer test does not cover the later reconciliation canonicalizer. This is reproducible without a remote Session and should be fixable with a focused unit test before the full lost-ACK resume case.
2 participants
Summary
RunState.pending_inputto_state()and normalize assistant-role ownership across JSON round tripsTest plan
Passed locally:
pytest -q tests/test_run_state_pending_input.py tests/test_run_impl_resume_paths.py tests/test_runner_guardrail_resume.py— 122 passedgit diff --checkFull locked verification could not complete locally because
make syncstalled twice while downloading optionallitellm,botocore, andtemporaliopackages. The fallback reused environment isopenai-agents==0.17.0while this tree is 0.22.0 and lacks multiple optional extras. In that same environment, exact base89c02c82reports 8743 passed / 26 failed / 24 collection errors; this branch reports 8753 passed / 27 failed / 24 collection errors. The failure groups are unchanged version-skew or missing-extra surfaces plus one nondeterministic Runloop import failure; none touch the changed files or focused suites. Upstream CI can exercise the frozen lock environment.Issue number
Fixes #4775
Checks
.agents/skills/code-change-verification/scripts/run.shin the frozen all-extras environment