fix(compact): preserve active skill recovery state

[Komzpa]

Summary

• preserve active <skill>...</skill> blocks as compact-trimming state anchors
• add an explicit remote compaction recovery message when compact input contains active skill blocks
• keep ordinary skill catalog mentions from being treated as active mode state

Root Cause

Remote compaction can return only the encrypted compaction item. Active skill mode is runtime state carried in <skill>...</skill> blocks, so trimming or normalizing compact output without that state leaves the next turn aware of the skill catalog but not of the currently active skill.

Validation

• python -m pytest -q tests/unit/test_openai_requests.py -k 'active_skill_context_anchor or plain_skill_catalog_mentions or preserves_codex_goal_context_anchor or poisoned_local_compact or compaction'
• python -m pytest -q tests/integration/test_proxy_compact.py -k 'normalizes_summary_output_for_codex_remote_v2 or adds_active_skill_recovery_context'
• python -m pytest -q tests/integration/test_proxy_responses.py -k 'compaction_trigger'
• live image canary: running codex-lb emits source="remote_compaction_v2_recovery", A remote compaction just happened, <active_skill_context>, and <name>grill-me</name> for an active grill-me compact payload

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4a8cd360d7

ℹ️ 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]

Thanks for continuing to dig into the post-compaction stall class — the replay-safety core here (treating completed compaction items and completed tool_search_call/tool_search_output pairs as self-contained account-neutral replay context, with the fail-closed guards for owner-bound ids, execution != "client", and non-neutral tool manifests) looks genuinely useful and is well covered by the new tests. That said, I don't think this PR is mergeable in its current shape:

1. CI is red. ty fails at app/modules/proxy/api.py:5744: "output": output_items assigns list[dict[str, JsonValue]] where JsonValue is expected. The output_items loop in _synthetic_compaction_response_stream only ever holds one item now that the multi-item recovery output was reverted, so the simplest fix is to restore the original single-item form.

2. The skill-recovery feature this PR is named after no longer exists at head. After the earlier bot review, the head revision asserts skill blocks are not anchored and compact output stays single-item — but the title, body (<active_skill_context>, source="remote_compaction_v2_recovery"), and the listed validation test names describe the removed revision. The revert also left dead code: _compaction_output_item_id and _compaction_item_texts (api.py:5681/5690) are never called, and the new compact_request parameter on _normalize_codex_remote_compaction_v2_result is unused. Please drop the residue and retitle/rescope.

3. The reconnect ownership change reverses current main semantics. Main deliberately pins account-neutral recovery to the same account and fails closed with a typed owner requirement, and fix(http-bridge): preserve goal-restart recovery across reconnects #1680 has since strengthened that exact line (require_same_account = account_neutral_recovery or (require_same_account and not goal_restart)). Dropping the pin so recovery can rebind to another account after a silent owner also risks duplicate turn execution if the silent owner actually processed the already-dispatched response.create. If you believe fallback is safe for account-neutral replay keys specifically, that deserves its own PR with that argument made explicitly, rather than arriving folded into a compact fix.

4. Part of this is already on main. The response-event-evidence recompute landed via fix(http-bridge): keep idle retirements out of retry circuit #1677 (now inside _retire_stale_pending_http_bridge_session in request_submit.py), so _http_bridge_pending_response_events_seen is unused here and superseded.

5. The branch is conflicting — the http_bridge package was restructured on main since your merge base, so the mixin/streaming hunks need a full rebase anyway.

Suggestion: extract the replay_safety.py compaction + tool-search self-containment work (plus its unit tests and the classify_durable_full_resend wiring it needs) into a focused PR on top of current main, and open the reconnect-ownership relaxation separately if you still want to argue for it. As folded, this PR mixes a reverted feature, a closed PR (#1733), and several independent behavior changes, which makes it very hard to review safely.

[Soju06]

Status check: the 08-16 verdict was effectively 'split this PR' — that's a restructure decision, not a mechanical rebase, and it's been quiet since. Are you up for extracting the replay-safety core into its own PR, or should we close this one and track the pieces separately?

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 10 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?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

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: b755a6c3-3816-48a7-b737-3c5cc6c1f4fb

📥 Commits

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

📒 Files selected for processing (13)

• app/modules/proxy/_service/http_bridge/helpers.py
• app/modules/proxy/_service/http_bridge/streaming.py
• app/modules/proxy/api.py
• app/modules/proxy/replay_safety.py
• app/modules/proxy/service.py
• openspec/changes/recover-post-compact-bridge-replays/proposal.md
• openspec/changes/recover-post-compact-bridge-replays/specs/responses-api-compat/spec.md
• openspec/changes/recover-post-compact-bridge-replays/tasks.md
• tests/integration/test_http_responses_bridge.py
• tests/integration/test_proxy_compact.py
• tests/unit/test_openai_requests.py
• tests/unit/test_proxy_http_bridge.py
• tests/unit/test_replay_safety.py

---

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

[Soju06]

Thanks for the rebase — CI is green and the branch is clean against current main now, so I've removed the needs-rebase label. The replay-safety core (compaction self-containment, the pre-compaction boundary guard, the tool-search pair handling, and the pending-tool-manifest safe-context extension) still looks solid and well tested. However, most of the 08-16 review items survive verbatim at this head, so I can't merge it as-is:

1. Title/body still describe the removed skill-recovery feature. The head's own tests now assert skill blocks are NOT anchored and compact output stays single-item, so a squash merge would produce a misleading commit message. Please retitle and rewrite the body to describe what the PR actually does.
2. The dead code called out on 08-16 is still here: the unused compact_request parameter on _normalize_codex_remote_compaction_v2_result, the never-called _compaction_output_item_id / _compaction_item_texts, and the output_items loop in _synthetic_compaction_response_stream that only ever holds one item (please restore the original single-item form rather than casting around it). Additionally, _http_bridge_pending_response_events_seen in helpers.py is never called — fix(http-bridge): keep idle retirements out of retry circuit #1677 already landed this recompute inside _retire_stale_pending_http_bridge_session, so please drop it, along with the tasks.md/proposal lines claiming that work.
3. The ownership semantic changes are still folded in: skipping the owner-first attempt for durable model transitions (durable_model_transition_uses_fresh_replay) and making excluded_account_ids transient in rollback_pre_dispatch_recovery_claim both reverse deliberate main behavior (the existing test comments document why exclusions accumulate). Each may be defensible, but they need their own PRs with the argument made explicitly — not silently inside a compact fix whose body doesn't mention them.

Concretely: narrow this PR to the replay_safety.py work plus its unit tests and the pending-tool-manifest wiring in durable_full_resend_allows_account_neutral_replay, with an accurate title/body, and I'll merge it quickly. If you'd rather not, let me know and I'll extract that core myself with attribution and close this one.

[Komzpa]

Absorbed by #1720: the current #1720 head contains the compact recovery and replay-safety scope from this PR.