Skip to content

fix(proxy): demote quarantined bridge reattach keys - #1730

Open
Komzpa wants to merge 1 commit into
Soju06:mainfrom
Komzpa:fix/quarantine-full-resend-account-neutral-20260813
Open

fix(proxy): demote quarantined bridge reattach keys#1730
Komzpa wants to merge 1 commit into
Soju06:mainfrom
Komzpa:fix/quarantine-full-resend-account-neutral-20260813

Conversation

@Komzpa

@Komzpa Komzpa commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • route quarantined full-resend HTTP bridge reattaches onto a fresh account-neutral replay key
  • clear stale session affinity inputs before session creation so poisoned hard keys cannot rebuild the same wedged bridge
  • tighten the quarantine regression test to assert the fresh key and stripped headers

Validation

  • uv run --frozen pytest -q tests/unit/test_proxy_http_bridge.py::test_stream_http_bridge_quarantined_full_resend_stays_unanchored_when_reattach_gate_already_false
  • uv run --frozen pytest -q tests/unit/test_proxy_http_bridge.py -q

Notes

This is the focused public carrier for the post-compaction live hang class where a quarantined hard session key could suppress the durable anchor but still create/reuse the same poisoned bridge key.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ddbc1fdf3f

ℹ️ 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".

Comment thread app/modules/proxy/_service/http_bridge/streaming.py Outdated
Comment thread app/modules/proxy/_service/http_bridge/streaming.py Outdated
@Komzpa

Komzpa commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fd602a99a0

ℹ️ 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".

Comment thread app/modules/proxy/_service/http_bridge/quarantine.py Outdated
@Komzpa Komzpa added 🤖 codex: needs work [@codex review] raised an issue and removed 🤖 codex: needs work [@codex review] raised an issue labels Aug 13, 2026
@Komzpa

Komzpa commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4123c19249

ℹ️ 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".

Comment thread app/modules/proxy/_service/http_bridge/streaming.py Outdated
Comment thread app/modules/proxy/_service/http_bridge/upstream_events.py Outdated
@Komzpa
Komzpa force-pushed the fix/quarantine-full-resend-account-neutral-20260813 branch from 4123c19 to 234ccf0 Compare August 14, 2026 04:32
@Komzpa

Komzpa commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 234ccf0bb4

ℹ️ 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".

Comment thread app/modules/proxy/_service/http_bridge/streaming.py
Comment thread app/modules/proxy/_service/http_bridge/upstream_events.py Outdated
Comment thread app/modules/proxy/_service/http_bridge/upstream_events.py
Comment thread app/modules/proxy/_service/http_bridge/streaming.py
@Komzpa Komzpa added the 🤖 codex: needs work [@codex review] raised an issue label Aug 14, 2026
@Komzpa
Komzpa force-pushed the fix/quarantine-full-resend-account-neutral-20260813 branch 3 times, most recently from b8c31a9 to 3284e05 Compare August 14, 2026 15:19
@github-actions github-actions Bot added the db migration PR changes Alembic database migrations; maintainer must coordinate merge order label Aug 14, 2026
@Komzpa
Komzpa force-pushed the fix/quarantine-full-resend-account-neutral-20260813 branch from 7d62f78 to 3530210 Compare August 14, 2026 16:00
@Komzpa

Komzpa commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3530210400

ℹ️ 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".

Comment thread app/core/openai/requests.py
Comment thread app/core/openai/requests.py
@Komzpa Komzpa added the needs rebase Needs rebase or conflict repair against current main label Aug 14, 2026
@Komzpa
Komzpa force-pushed the fix/quarantine-full-resend-account-neutral-20260813 branch from 3530210 to e4820fa Compare August 14, 2026 19:02
@Komzpa Komzpa removed the needs rebase Needs rebase or conflict repair against current main label Aug 14, 2026
@Komzpa

Komzpa commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e4820fa56a

ℹ️ 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".

Comment thread app/modules/proxy/_service/response_create.py
@Komzpa Komzpa removed the 🤖 codex: needs work [@codex review] raised an issue label Aug 15, 2026
@Komzpa

Komzpa commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

Unknown error
ℹ️ 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".

@Soju06

Soju06 commented Aug 16, 2026

Copy link
Copy Markdown
Owner

Thanks — the quarantine-demotion core of this PR is in good shape after the review iterations: the generation fence, the projected account-neutral dispatch, the deterministic replay nonce, and the claim→rebind(clear_continuity)→renew advance of the original durable row all trace correctly, and the test coverage for them is thorough. Two blocking items remain in the bundled slimming/namespace scope, plus a scoping request:

  1. Orphaned archived-spec rewrite. openspec/changes/archive/2026-08-13-fix-replayed-namespaced-function-call/specs/responses-api-compat/spec.md is rewritten to say replayed tool-call namespaces "MUST be preserved" on live Responses egress, but the final head no longer contains the serializer change that would implement it: app/core/openai/requests.py::to_payload() still strips namespaces by default, the live spec (openspec/specs/responses-api-compat/spec.md:4595) still says "MUST omit", and this PR's own integration test (test_v1_responses_http_bridge_preserves_agent_wait_output_through_wire_normalization) asserts the upstream function_call item has no namespace key. Please revert the archive edit (it's the historical record of what actually landed); if the requirement is genuinely being flipped, that should be its own openspec change with the serializer work.

  2. Core WebSocket slimmer not covered. The new live-spec requirement ("Historical response.create slimming MUST preserve outputs for namespaced agent-control calls") is only implemented in the service-facade slimmer. The duplicate _slim_historical_response_input_item in app/core/clients/proxy.py (~L2244), used when the HTTP session bridge is disabled, still replaces collaboration/multi_agent_v1 outputs with the omission notice — the last completed Codex round flagged this and the final push didn't address it (the follow-up review errored out). Either extend the protection there too or scope the spec text to the bridge path.

  3. Scope. The title/body describe a focused quarantined-reattach fix, but the PR also carries the agent-control slimming protection (+ two spec files), a _matching_websocket_request_states_for_previous_response_error heuristic change, and test-flake fixes. Could you split the slimming/spec work into its own PR? The quarantine fix looks close to merge-ready on its own, and separating them keeps the spec question from blocking it.

Minor, non-blocking: _AGENT_CONTROL_TOOL_NAMES matches on bare name with no namespace, so a user tool that happens to be called send_input gets its large outputs permanently exempted from slimming and can newly fail size enforcement — consider requiring the namespace for the name-only legacy case or documenting the tradeoff. Also, in the reroute branch the fresh_reattach_anchor_skipped_quarantined event now logs the freshly minted recovery key because bridge_session_key is reassigned before the log call; logging the original quarantined key (with the new key in detail=) would keep the event useful for tracing.

@Soju06

Soju06 commented Aug 19, 2026

Copy link
Copy Markdown
Owner

Rebase nudge: per the 08-16 assessment this is close to mergeable — two scoped blockers plus the post-0816 conflict. Not superseded by anything on main; a rebase + those two fixes gets it over the line.

Rebased onto current main; squashed the branch's review-iteration
commits into one reviewable change.
@Komzpa
Komzpa force-pushed the fix/quarantine-full-resend-account-neutral-20260813 branch from 5e495a0 to f38b650 Compare August 19, 2026 16:51
@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@Komzpa, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 59 minutes

Limit details: You’ve used all 3 included reviews currently available.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5f36ac4b-deb1-40c1-9272-dc660596e325

📥 Commits

Reviewing files that changed from the base of the PR and between f839952 and f38b650.

📒 Files selected for processing (14)
  • app/modules/proxy/_service/http_bridge/quarantine.py
  • app/modules/proxy/_service/http_bridge/request_submit.py
  • app/modules/proxy/_service/http_bridge/service_stubs.py
  • app/modules/proxy/_service/http_bridge/streaming.py
  • app/modules/proxy/_service/http_bridge/upstream_events.py
  • app/modules/proxy/_service/response_create.py
  • app/modules/proxy/_service/support.py
  • app/modules/proxy/_service/websocket/helpers.py
  • openspec/changes/archive/2026-08-13-fix-replayed-namespaced-function-call/specs/responses-api-compat/spec.md
  • openspec/specs/responses-api-compat/spec.md
  • tests/integration/test_http_responses_bridge.py
  • tests/unit/test_durable_bridge_sessions.py
  • tests/unit/test_proxy_http_bridge.py
  • tests/unit/test_proxy_utils.py

Comment @coderabbitai help to get the list of available commands.

@Soju06

Soju06 commented Aug 20, 2026

Copy link
Copy Markdown
Owner

Thanks for the rebase onto beta.2 — CI is fully green and the quarantine-demotion core still traces correctly. However, checking head f38b650, both blocking items from the 08-16 assessment came through the rebase unchanged:

  1. The archived-spec rewrite (openspec/changes/archive/2026-08-13-fix-replayed-namespaced-function-call/specs/responses-api-compat/spec.md) is still present. On current main, app/core/openai/requests.py::to_payload() still strips replayed tool-call namespaces by default, and this PR's own integration test (test_v1_responses_http_bridge_preserves_agent_wait_output_through_wire_normalization) asserts the upstream function_call item has no namespace key — so the rewritten archive describes behavior that never shipped. Please revert the archive edit; if the requirement is genuinely being flipped, that needs its own openspec change with the serializer work.

  2. The new live-spec MUST ("Historical response.create slimming MUST preserve outputs for namespaced agent-control calls") is still only implemented in the service-facade slimmer. The duplicate _slim_historical_response_input_item in app/core/clients/proxy.py (~L2740), used when the HTTP session bridge is disabled, still replaces collaboration/multi_agent_v1 outputs with the omission notice. Either extend the protection there or scope the spec text to the bridge path.

And the scoping ask stands: the quarantined-reattach fix itself is essentially merge-ready — splitting the slimming/spec work (plus the _matching_websocket_request_states_for_previous_response_error heuristic and flake fixes) into a follow-up PR would let us land the quarantine fix immediately. If splitting is inconvenient, say the word and we can carve out the quarantine core on our side with attribution preserved.

(Also noting the db migration label appears stale — this head touches no migrations/ files — so I'll drop it.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

db migration PR changes Alembic database migrations; maintainer must coordinate merge order

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants