fix(proxy): preserve developer-interleaved fresh resends - #1537
Conversation
|
@codex review |
![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: 033f8784b0
ℹ️ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03ca55f40f
ℹ️ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2773e0d01
ℹ️ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 917217843b
ℹ️ 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 Current head: |
|
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 review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1e305be6d
ℹ️ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c75b98a45
ℹ️ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb20e1b73e
ℹ️ 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 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Komzpa
added
🤖 codex: needs work
choi138
left a comment
choi138
left a comment
There was a problem hiding this comment.
Published replies for the superseded, outdated Codex threads after the minimized current-head rewrite.
Komzpa
added
🤖 codex: ok
|
@codex review |
|
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". |
|
Hi @Soju06, could you please take a look at this PR when you have a chance? Thank you! |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Soju06
left a comment
Soju06
left a comment
There was a problem hiding this comment.
Reviewed head bb974f4f988f73703b14d67995e5c9b4611af809. The bounded developer-transparency design (historical interleave / fresh custom-tool loop / retained-output terminal follow-up) is well-scoped and the OpenSpec delta matches the implementation on the primary classification path. However, there is one fail-closed bypass on the owner-unavailable fallback path that needs to be fixed before merge, plus a smaller historical-path gap.
Blocker: owner-unavailable fallback strips developer-ID evidence before reclassification
The initial durable-full-resend classification (streaming.py, classify_durable_full_resend) correctly passes preserve_developer_message_ids=True so that a response-owned fresh developer message (e.g. id="msg_...") is rejected by the new item.get("id") in (None, "") check.
But owner_unavailable_allows_account_neutral_replay() (streaming.py ~L1208) rebuilds the projection without the flag:
replay_projection = project_responses_input_for_account_neutral_fresh_replay(
cast(list[JsonValue], payload.input),
stored_count=durable_full_resend_anchor_count,
)The default projection executes projected_item.pop("id"), and the widened responses_input_suffix_retains_prior_output then reclassifies the stripped items. Concretely: a full resend shaped final_answer -> user -> developer(id="msg_response_owned") is rejected by the initial classifier, but once the durable owner becomes unavailable, the same payload is re-projected with the ID removed, the new retained-output branch accepts it, the failed owner is excluded, and the response-owned developer content is replayed as an instruction on another account.
This contradicts the delta spec's own requirement ("Classification MUST retain response-owned developer-message ID evidence until these checks have completed") and the PR body's fail-closed claim. Note this path only became reachable through this PR's classifier widening: previously any developer message in the fresh suffix failed the classification regardless of ID.
Suggested fix: run the fallback eligibility classification on a preserved-ID projection as well (keeping the actual serialized replay payload on the existing ID-stripping projection), or reject response-owned developer IDs before projection on that path.
Blocker (test gap, same root): positive fixtures encode the bypass as expected behavior
Both positive fixtures (tests/unit/test_replay_safety.py ~L757 and ~L1180) give the fresh developer message "id": "msg_control" and then call project_responses_input_for_account_neutral_fresh_replay without preserve_developer_message_ids=True, so the forbidden evidence is erased before the acceptance assertion. As written, these tests would keep passing even if the fail-closed check regressed entirely.
Please drop the IDs from the positive fixtures and add rejection regressions that classify a preserved-ID projection (or go through the public classifier path) for both the fresh tool-loop and retained-output shapes.
Non-blocking: explicit "type": null passes the historical interleave check
The fresh-path predicate _fresh_developer_message_is_transparent guards ("type" not in item or _is_nonblank_string(item.get("type"))), but the historical path does not: _direct_tool_call_prefix_state normalizes explicit null to None, and _historical_pending_developer_message_is_transparent accepts item_type in (None, "message"). So {"type": null, "role": "developer", ...} passes exact-manifest proof, while the delta spec says explicit null item types must fail closed. One-line guard to align it with the fresh path.
Non-blocking (fine as a follow-up): rejection reasons are unobservable
All new rejection causes collapse into safe_fresh_context=False, and the anchor-injection log line records only the response ID. When an operator later needs to explain why a resend fell back to the anchored path, the triggering rule can't be recovered without payload tracing. A structured, non-sensitive classification-reason field on the replay decision log would make this diagnosable. Happy to see this land separately.
Everything else looks good: CI is green on the current head, the shape matching is strict (exact 3-item suffix, call_id equality, single self-contained input_text, account-neutral metadata), and the fail-closed scenario coverage outside the two gaps above is thorough.
|
Hi @Soju06, I applied your review feedback in The owner-unavailable safety check now preserves developer message IDs, strips them only for the actual replay payload, and fails closed for response-owned IDs, explicit GitHub currently reports a separate conflict with the newly advanced |
Resolve the tests/unit/test_proxy_http_bridge.py assertion conflict so both behaviours hold: latest main invokes the account-neutral classifier once for missing_owner, and this PR keeps response_owned_developer fail-closed before the classifier runs. Prove the canonical Responses-Lite developer position against the original input instead of projected adjacency, so a bundle that does not start the stored prefix and a developer message made adjacent only by projection both fail closed.
Soju06
left a comment
Soju06
left a comment
There was a problem hiding this comment.
Re-reviewed head f27a0d008a4c34907be8945789529ff07fbab7e9. The 2b5d827d fixes for my previous review are correctly carried forward (preserved-ID eligibility projection on the owner-unavailable path, positive fixtures without IDs on the fresh-developer paths, type: null fail-closed on the historical path), and the merge-integration canonical_lite_developer_index design is sound in principle: anchoring the canonical position on the original input index prevents a projected-out item from collapsing a later developer message into the canonical slot. CI is green on this head.
However, the merge introduced one blocking test defect, and I want one behavioral question answered with real payload evidence before this lands.
Blocker: historical-interleave positive test exercises a production-impossible path
test_full_resend_tool_loop_manifest_tolerates_fresh_developer_interleave_after_historical_one (tests/unit/test_replay_safety.py ~L717) gives the historical developer message "id": "msg_old_control", then builds its projection without preserve_developer_message_ids=True and asserts acceptance.
Production classification now always uses the preserving projection (streaming.py L987 and L1390). Under that projection this exact input is rejected, because _historical_pending_developer_message_is_transparent requires item.get("id") is None. So the test validates a sanitized shape the production path can never produce, and asserts acceptance of an input production classification would refuse.
Please fix the fixture to match the production path (preserving projection; drop the ID if the accepted shape is ID-less), and add the preserved-ID rejection counterpart for the historical position.
Question that needs payload evidence, not just a fixture edit
The fixture bug raises a product-level question: do real Responses-Lite historical developer messages carry response-owned IDs in cumulative resends? This decides whether the PR achieves its stated goal:
- If real history arrives with IDs (as this fixture suggested), then the current ID-less transparency requirement rejects the motivating #1485 reconnect shape, and the PR fixes nothing in production while all its synthetic tests pass.
- If real history arrives without IDs, the fixture edit above is the whole fix.
Please confirm with a captured real resend payload (or equivalent evidence from request logs) and state the answer in the PR body / OpenSpec context.
Tightening request (should-fix): bound the historical interleave as the spec narrative implies
_direct_tool_call_prefix_state accepts a transparent developer message whenever the pending deque is non-empty, and accepting one does not consume anything. So call1 -> call2 -> developer -> output1 -> output2 (parallel batch) and call -> developer -> developer -> output (duplicate) both pass prefix parsing. The delta spec's fail-closed enumeration for parallel/duplicate shapes currently only binds the fresh path, so this is not a spec violation as written, but it is broader than the "verified interleaving" story and broader than what #1485 needs. Please either bound it (exactly one pending call and at most one interleaved developer message per pending window) or explicitly document the accepted multiplicity in the OpenSpec delta so the width is a decision, not an accident.
Non-blocking
canonical_lite_developer_indextravels as a bare int alongside independently-suppliedinput_items/stored_count, so the type contract lets a caller mark an arbitrary position canonical; and a preserving-mode projection ("must not be serialized") is indistinguishable by type from a replay-safe one. Passing the projection object (or a distinct classification-projection type) would make both misuses unrepresentable. Fine as a follow-up refactor.- The bounded-interleave helper hand-copies the
custom_tool_call/custom_tool_call_outputpairing that_TOOL_CALL_TYPE_BY_OUTPUT_TYPEalready declares; derive it from the registry. - Rejection-reason observability (from my first review) remains open as an agreed follow-up.
One process note: this is the fifth scope movement on this guard axis (fresh-suffix widening, hardening, canonical-lite carve-out). Each step has been individually reasonable, but I'd like the next revision to be convergent: fixture fix + payload evidence + interleave bound, and no new classification surface.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…n projection Address the second maintainer review on Soju06#1537. Classify the historical-interleave fixture through the production ID-preserving projection and drop its response-owned ID, so the accepted shape is the one production can actually produce, and add the preserved-ID rejection counterpart at the historical position. Un-mask three negatives that were failing for incidental reasons: place the stored response-owned developer message at the canonical Lite position, give the retained assistant output a final_answer phase, and pass the canonical index to the retained-output rejection. Each now fails only on the property under test. Bound the historical interleave to a pending window that holds exactly one outstanding call, never held parallel calls, and consumes at most one developer message, and fail closed when such a window later becomes parallel. Sequential single-call windows keep their interleaved message. Record the measured resend evidence in OpenSpec context, including that no observed payload reaches these allowances and why.
|
@Soju06 Addressed the re-review on Blocker: production-impossible historical-interleave fixtureFixed. Auditing all 22 call sites of the projection turned up three more negatives that were passing for the wrong reason, all now un-masked and each verified to flip when the intended cause is removed:
Payload evidence: the answer is neither branch of your either/orMeasured 2,798 real Running the production classification path — ID-preserving projection, then the stored-prefix walk — 0 of 2,798 payloads reach any developer-transparency allowance. Two gates sit upstream of the ID check you asked about:
On your specific question: real history arrives both ways. Of the unique occurrences of the motivating interleave, 40 are ID-less and 55 carry an Separately, 0 of 16,036 inline developer items carry So the honest status: the fixture edit is not the whole fix, and this PR does not yet make #1485 work in production. What it does deliver is the correct, bounded, fail-closed classification plus the evidence to decide the next step. Widening either gate is a separate decision with its own evidence, and I did not smuggle it in here. Happy to land this as the correctness and evidence step and open the follow-ups, or to hold it if you would rather see the reachable version first — your call. Tightening request: boundedChose the bound over documenting the width. A pending window opens when the pending-call deque becomes non-empty and closes when it drains; a developer message is transparent only in a window that holds exactly one outstanding call, never held parallel calls, and has not already consumed a developer message. A local Spec delta and Not in this revision
VerificationLocal, before push: |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
OpenSpec change proposing that a durable HTTP bridge session's proxy-injected previous_response_id anchor be cleared, fenced to the session's owner epoch, after its response.create proves eventless at the existing 240s client-safe deadline (missing_response_created_timeout) -- so the next reattach on that session takes the unanchored path instead of repeating the identical dead anchor forever. Scoped to clients whose incoming payload already looks like a full conversation resend (the confirmed Soju06#1534 reproduction). A genuine delta-only proxy-injected anchor, or any client-supplied anchor, is left untouched: clearing those would replace a visible, bounded timeout with a request upstream could silently accept as a new, context-free conversation. See design.md's Load-bearing assumption for the scoping rationale and the open question left for maintainer input. Refs: Soju06#1534, Soju06#1167, Soju06#1393, Soju06#1485, Soju06#1486, Soju06#1537 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Implements openspec/changes/invalidate-durable-bridge-anchor-after-stuck-timeout. - DurableBridgeRepository.clear_latest_response_anchor: a fenced UPDATE that nulls latest_response_id, latest_input_item_count, latest_input_full_fingerprint, and latest_pending_tool_calls_json for a session, scoped to owner_instance_id/owner_epoch (no-op if a newer owner already holds the row). latest_turn_state and aliases are left untouched. - DurableBridgeSessionCoordinator.clear_live_session_response_anchor wraps it, matching the existing renew/release coordinator pattern. - request_state.proxy_injected_anchor_had_full_resend_payload records whether the client's own payload already looked like a full resend at the moment codex-lb injected the anchor, captured at every injection site in streaming.py (durable fresh-reattach, owner-forward interrupted-tool-call recovery, session-level injection) and carried through the later trim re-prepare step. - In the HTTP bridge upstream reader's missing_response_created_timeout handling, clear the durable anchor -- before releasing durable ownership, so the fenced write still lands under the current owner epoch -- only when a request whose eventless deadline actually expired carries both proxy_injected_previous_response_id and proxy_injected_anchor_had_full_resend_payload. A genuine delta-only proxy-injected anchor, or a client-supplied one, is left intact. Emits durable_anchor_invalidated for observability. Deliberately does not reuse the existing fresh_upstream_request_is_retry_safe field for this gate: tracing it end to end shows it is always False at the point this gate needs to fire (including for the confirmed Soju06#1534 reproduction), because it answers a stricter, different question about in-place same-request replay safety rather than "does this client ever send complete context." Tests: fenced repository/coordinator write (clears correctly; no-ops past a newer owner epoch; leaves turn-state and the previous-response alias row untouched) in test_durable_bridge_sessions.py; a parametrized HTTP-bridge regression in test_proxy_http_bridge.py covering all three shapes (full-resend proxy-injected anchor clears, delta-only proxy-injected anchor stays, client-supplied anchor stays). Verified: full tests/unit suite (5064 passed), tests/integration/ test_http_responses_bridge.py (112 passed), Ruff, format, changed-file ty, proxy architecture checks, and openspec validate (change + all 48 specs) all pass. Refs: Soju06#1534, Soju06#1167, Soju06#1393, Soju06#1485, Soju06#1486, Soju06#1537 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 participants
Summary
Preserve a verified Responses-Lite full resend when its fingerprint-matched history or one of two bounded fresh suffixes contains an account-neutral developer control message.
This head was rebuilt independently from the current main branch and does not depend on #1527. The follow-up is ported directly onto the existing #1537 head without importing the unrelated local branch lineage.
Type of change
Linked issue: Related to #1485; follow-up to #1486.
OpenSpec
Change directory:
openspec/changes/allow-developer-interleaved-fresh-resend/Root cause
Responses-Lite keeps its
additional_toolsbundle and developer instructions inline. After reconnect, a cumulative resend can contain either the existing historical shape or one of two newly observed fresh suffixes:The replay classifier rejected every developer message in the fresh suffix. That false negative forced durable
previous_response_idinjection and prefix trimming instead of preserving the complete, account-neutral resend, which could lead to an upstream acknowledgement timeout.Changes
final_answerand exactly one explicit user message.custom_tool_call, one developer message, and its matchingcustom_tool_call_output, equal to the durable pending manifest.turn_idmetadata, one self-containedinput_textpart, known fields, no non-empty response-owned ID or phase, and no status other thancompleted./v1/responsescoverage for accepted and rejected shapes.Non-goals
input/messagesinstruction-hoisting change.User / operator impact
A reconnecting Responses-Lite conversation with one of the verified bounded control-message shapes remains on the safe fresh full-resend path instead of falling into unnecessary anchor injection. Unsafe or ambiguous shapes remain fail-closed.
Test plan
TDD evidence: the two new positive replay cases plus bridge parameters produced
4 failed, 5 passedbefore the production change; the new focused boundary set passed afterward.Independent broad review: no findings after tracing request normalization, Responses-Lite detection, replay projection/classification, durable prefix and pending-tool evidence, anchor removal/injection, owner/account selection and migration, and final upstream serialization. A focused follow-up review also found no issues in the test-only type annotation delta and confirmed AST-equivalent runtime semantics.
Simplicity
No setting, README, environment, navigation, setup, or default surface changes.
Screenshots / output
Not applicable; no dashboard-visible changes.
Checklist