fix(compact): preserve tool search pairs - #1606
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Verified the core fix against main and it is correct: Two items before this can merge:
Heads-up: your open #1235 rewrites these same frozensets into |
af42123 to
56b7b81
Compare
|
@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: 433d75e535
ℹ️ 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".
| request = ResponsesCompactRequest.model_validate(payload) | ||
| dumped = request.to_payload() |
There was a problem hiding this comment.
Add regression coverage through /backend-api/codex/responses/compact rather than only calling ResponsesCompactRequest.to_payload() directly. The reported failure occurred at the externally visible compact endpoint, so this helper-level test does not verify that routing and final upstream preparation preserve the pair; the repository explicitly requires bug-fix coverage at the failing product path.
AGENTS.md reference: AGENTS.md:L125-L128
Useful? React with 👍 / 👎.
| async def update_last_used(self, key_id: str, *, commit: bool = True) -> None: | ||
| """Compatibility touch for maintenance and durability checks.""" | ||
| await self._session.execute(update(ApiKey).where(ApiKey.id == key_id).values(last_used_at=utcnow())) |
There was a problem hiding this comment.
Remove this unrelated API-key persistence API from the compact-trimming change. It adds a public database mutation with commit-on-by-default behavior that is neither described nor covered by this commit's compact regression, coupling two independently reviewable and revertible concerns despite the repository's one-concern rule.
AGENTS.md reference: AGENTS.md:L121-L124
Useful? React with 👍 / 👎.
| _TOOL_TEXT_PART_TYPES = frozenset({"text", "input_text", "output_text", "refusal"}) | ||
| _COMPACT_STATE_TOOL_NAMES = frozenset({"create_goal", "get_goal", "update_goal", "update_plan"}) | ||
| _TOOL_CALL_ITEM_TYPES = frozenset({"function_call", "custom_tool_call", "apply_patch_call"}) | ||
| _TOOL_CALL_ITEM_TYPES = frozenset({"function_call", "custom_tool_call", "apply_patch_call", "tool_search_call"}) |
There was a problem hiding this comment.
Use a dedicated compact-pairing call-type set instead of adding tool_search_call to _TOOL_CALL_ITEM_TYPES. When a standard, compact, or configured-source request contains a tool_search_call with a namespace, the shared set now makes strip_replayed_tool_call_namespaces_from_payload() silently remove that field even though this fix only needs pairing during trimming; openspec/changes/fix-replayed-namespaced-function-call/context.md:9 explicitly limits that normalization to the established replayed call types and requires other input-item types to remain unchanged.
AGENTS.md reference: AGENTS.md:L24-L26
Useful? React with 👍 / 👎.
|
|
||
| async def update_last_used(self, key_id: str, *, commit: bool = True) -> None: | ||
| """Compatibility touch for maintenance and durability checks.""" | ||
| await self._session.execute(update(ApiKey).where(ApiKey.id == key_id).values(last_used_at=utcnow())) |
There was a problem hiding this comment.
Guard this update with the same last_used_at IS NULL OR last_used_at < :new predicate used by the coalescer, or route the touch through that coalescer. If another replica has already stored a later timestamp—for example because replica clocks differ or writes arrive out of observation order—this unconditional assignment moves last_used_at backwards, violating the monotonic cross-replica contract in openspec/changes/coalesce-api-key-last-used-writes/specs/api-keys/spec.md:5.
AGENTS.md reference: AGENTS.md:L24-L26
Useful? React with 👍 / 👎.
|
Thanks — the core fix here is real and worth landing: pairing
Once those are in (and the Codex threads resolved), this looks mergeable — CI is already green. |
Komzpa
added
the
needs rebase
9c23412 to
7f83b33
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f83b330dc
ℹ️ 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".
| if account_neutral_recovery: | ||
| request_state.preferred_account_id = session.account.id | ||
| request_state.preferred_account_id = None | ||
| request_state.excluded_account_ids.add(session.account.id) |
There was a problem hiding this comment.
This branch changes an account-neutral pre-created retry from reconnecting to the same preferred account to excluding that account and selecting another one, which is a proxy-routing and failover contract change. The only OpenSpec delta touched by this commit documents tool-search replay-prefix trimming, not cross-account recovery, so this behavior needs its own active OpenSpec change and regression scenarios before landing.
AGENTS.md reference: AGENTS.md:L92-L98
Useful? React with 👍 / 👎.
| "tool_search_output": frozenset( | ||
| { | ||
| "call_id", | ||
| "caller", | ||
| "execution", | ||
| "id", | ||
| _INTERNAL_CHAT_MESSAGE_METADATA_FIELD, | ||
| "output", | ||
| "status", | ||
| "tools", |
There was a problem hiding this comment.
Handle tool_search_output.tools in the self-contained output validator instead of only allowing the field here. The owning proposal explicitly records that this item carries a tools list rather than the synthetic output shape (proposal.md:47-54), but pairing it through _TOOL_CALL_TYPE_BY_OUTPUT_TYPE sends it to _tool_output_is_self_contained(), which accepts only an output string/content-part list. Consequently a real compact history with {type: "tool_search_output", tools: [...]} is still classified as non-account-neutral, preventing the new fresh-replay recovery path from engaging; the new test masks this by using output: "Found codex-lb".
AGENTS.md reference: AGENTS.md:L24-L26
Useful? React with 👍 / 👎.
| if item_type == "compaction" and _compaction_item_is_self_contained(current): | ||
| continue |
There was a problem hiding this comment.
Do not skip the account-scoped reference check for compaction ciphertext without a verified portability guarantee. For an unanchored full resend containing a completed compaction item, this continue bypasses _mapping_has_account_scoped_reference(), whose existing invariant classifies every non-empty encrypted_content value as account-scoped; the request can therefore be marked account-neutral and the new retry branch can resend account A's encrypted context on account B. That risks an upstream rejection or unusable recovered context, so compaction state should remain owner-pinned unless the owning specification and product-path coverage prove that its encryption is cross-account portable.
AGENTS.md reference: AGENTS.md:L105-L110
Useful? React with 👍 / 👎.
| ### Requirement: Previous-response replay trimming handles tool-search output pairs | ||
| When a Responses HTTP bridge or WebSocket continuation carries `previous_response_id` and replays already-stored response output items before a fresh `tool_search_output`, the service MUST trim the replayed `tool_search_call` prefix and preserve the `tool_search_output` plus the fresh turn. The service MUST NOT forward both the replayed `tool_search_call` and its `tool_search_output` on top of the `previous_response_id` anchor. |
There was a problem hiding this comment.
Do not add the new tool-search trimming contract only to an already archived delta. openspec/specs/responses-api-compat/spec.md contains no corresponding requirement, so the repository's normative SSOT does not actually describe the behavior implemented here and future spec validation or changes will operate from stale requirements. Create or reopen an active change and sync this requirement into the main capability spec before archiving it.
AGENTS.md reference: AGENTS.md:L24-L28
Useful? React with 👍 / 👎.
Komzpa
removed
the
needs rebase
7f83b33 to
5831738
Compare
|
Folded into #1720: tool-search pair preservation joins the compact payload sanitization carrier; commits replayed onto a clean base to shed stale branch ancestry. |
2 participants
Summary
Preserve Codex
tool_search_call/tool_search_outputpairs during compact input trimming so oversized sessions do not forward an orphantool_search_outputupstream. This fixes a live compact failure where OpenAI rejected session019fcc77-8e08-7f02-b5d2-3388c5122004withNo tool call found for tool search output with call_id call_d2chifMNZfXqfFWin5jz0dQf.Type of change
fix:— bug fix (no behavior change beyond the bug)Linked issue: none
OpenSpec
Change directory: n/a
Changes
tool_search_callandtool_search_outputto the compact tool pair reconciliation sets.tool_search_call/tool_search_outputhistory item pair.Test plan