Skip to content

Suppress duplicate same-response side-effect tool calls - #1567

Open
Komzpa wants to merge 1 commit into
Soju06:mainfrom
Komzpa:fix/same-response-side-effect-dedupe-20260802
Open

Suppress duplicate same-response side-effect tool calls#1567
Komzpa wants to merge 1 commit into
Soju06:mainfrom
Komzpa:fix/same-response-side-effect-dedupe-20260802

Conversation

@Komzpa

@Komzpa Komzpa commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • suppress duplicate named side-effect tool calls in the same upstream response when canonical arguments match but the model emits a new call_id
  • keep existing replay-burst behavior across response IDs and still allow intentional repeats after an intervening side-effect boundary
  • update HTTP/WebSocket coverage so duplicate side-effect calls produce a retryable stream_incomplete instead of silently executing twice

Tests

  • uv run ruff format tests/unit/test_proxy_tool_call_dedupe.py tests/unit/test_proxy_utils.py tests/integration/test_proxy_websocket_responses.py
  • uv run ruff check app/modules/proxy/tool_call_dedupe.py tests/unit/test_proxy_tool_call_dedupe.py tests/unit/test_proxy_utils.py tests/integration/test_proxy_websocket_responses.py
  • uv run pytest -q tests/unit/test_proxy_utils.py::test_stream_responses_suppresses_same_response_http_tool_calls_with_distinct_call_ids tests/unit/test_proxy_utils.py::test_process_upstream_websocket_text_suppresses_same_response_distinct_tool_call_ids tests/unit/test_proxy_tool_call_dedupe.py tests/unit/test_proxy_http_bridge.py -k 'duplicate or tool_call or replay' tests/integration/test_proxy_websocket_responses.py -k 'duplicate or tool_call or replay'

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@Komzpa
Komzpa force-pushed the fix/same-response-side-effect-dedupe-20260802 branch from 129584e to 06b6993 Compare August 2, 2026 00:27
@Komzpa

Komzpa commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@Soju06

Soju06 commented Aug 4, 2026

Copy link
Copy Markdown
Owner

The flat-call part of this is a real spec-compliance fix: openspec/specs/responses-api-compat/spec.md ("Same-response side-effect tool-call replays are suppressed") already mandates that a changed call_id alone must not make a same-response replay distinct, and main currently violates that for sequential flat calls. But as written the new check overreaches into identity-scoped calls:

  1. P1 — collapses distinct namespaced/code-mode call identities. The new same-response check (app/modules/proxy/tool_call_dedupe.py:140) runs before identity_scoped_call is computed at line 149, and same_response_argument_key deliberately zeroes the call_id slot. So two collaboration.spawn_agent custom_tool_calls in one response with identical input and different call_ids — the exact scenario the "Namespaced side-effect replay dedupe preserves call identity" requirement pins down ("Calls with different namespaces or different nonblank call IDs MUST remain distinct, even when their names and canonical arguments match"; scenario "Distinct namespaced spawns use identical arguments") — now suppress the second spawn and convert the whole turn into a stream_incomplete failure. Same for code-mode exec with identical source and distinct call_ids (2026-07-12 harden delta: "without collapsing distinct call identities"). Since the client retry will regenerate the same two spawns, this is a deterministic retry loop for legit multi-agent v2 parallel spawns. CI is green only because downstream same-response distinct-call_id coverage exists for flat calls, not namespaced ones. Fix: move the check below line 149 and gate it on not identity_scoped_call (identity-scoped calls already get cross-response protection via retained call_id), and add a downstream test mirroring the spec scenario.

  2. P3 — burst clear also drops response-scoped parallel_tool_use keys. _clear_downstream_side_effect_burst_keys (line 202) pops every response-scoped key with a None call_id, which includes the per-tool_use keys written by _mark_duplicate_parallel_tool_call_downstream_event. On the response-scoped WS path, parallel([exec X]) -> exec Y -> parallel([exec X]) in one response previously trimmed the second X; now it executes twice. If the intervening-boundary reset is meant to apply to parallel batches too, fine, but it should be deliberate.

  3. P3 — the intervening-boundary carve-out itself is new semantics. The canonical requirement says forward only the first event, with no boundary exception; the burst reset is a reasonable refinement but deserves a small openspec delta modifying that requirement rather than shipping as silent drift (that drift is how main ended up out of spec here in the first place).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants