Skip to content

- - #45888

Open
rodboev wants to merge 1 commit into
NousResearch:mainfrom
rodboev:pr/responses-api-approval
Open

-#45888
rodboev wants to merge 1 commit into
NousResearch:mainfrom
rodboev:pr/responses-api-approval

Conversation

@rodboev

@rodboev rodboev commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Summary

The Responses API path was only half-wired for guarded approvals: _write_sse_responses() silently dropped the queued __approval_request__ tuple, no resolution endpoint existed, and the non-streaming path registered a no-op approval callback that could block indefinitely.

This update completes the Responses approval transport with proper scope isolation. Streaming responses emit response.approval.requested SSE events keyed by the generated response_id. The approval callback, event-loop queue, resolution endpoint, resolve_all, profile ownership, and worker cleanup all use response_id as the sole key, so concurrent requests sharing a session or memory key cannot overwrite each other's notifier or resolve each other's pending actions. The non-streaming path remains callback-free.

Changes

  • gateway/platforms/api_server.py: scoped all Responses approval state to the generated response_id; added event-loop queue handoff with call_soon_threadsafe; redacted event["command"] through _redact_approval_command before SSE delivery; added _response_approval_sessions profile ownership map; added POST /v1/responses/{response_id}/approval with profile-scoped authentication and sibling-profile rejection; passed approval_session_key=response_id separately from the conversation key; registered approval state through the shared callback seam and cleared it in worker completion finally; removed the non-streaming no-op callback.
  • tools/approval.py: added _register_gateway_approval to check callback identity atomically before queue insertion so a captured worker that loses its callback during disconnect teardown fails closed without entering the timeout loop; extended clear_session to evict the session key from _denial_tally.
  • tests/gateway/test_api_server.py: added one-worker approval progression, same-session concurrency and resolve_all isolation, profile rejection, redaction contract, late-callback guard, lifecycle cleanup, and Runs preservation coverage.
  • tests/tools/test_approval_interrupt.py: added callback teardown race regression.
  • tests/tools/test_denial_circuit_breaker.py: added named-key denial-tally eviction regression.

Validation

  • pytest tests/gateway/test_api_server.py tests/gateway/test_api_server_runs.py tests/gateway/test_approval_prompt_redaction.py tests/tools/test_approval_interrupt.py tests/tools/test_denial_circuit_breaker.py -v --timeout=0 — 137 passed

Not in scope

This PR does not try to invent a second approval mechanism for normal chat replies in OpenAI-compatible clients. It only completes the existing gateway approval contract for the Responses API: structured approval events plus a matching resolution endpoint keyed by response_id.

Upstream

Closes #45505.
Reported by @arnoulddw.
Thanks to @AIalliAI for the Responses approval wiring analysis.

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists labels Jun 14, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

✅ Verified — Approval callback wiring in Responses API path

Reviewed the diff for gateway/platforms/api_server.py (Responses API approval integration).

  • Callback registration: Confirmed _on_approval_request queues approval.request events to the SSE stream via _stream_q.put() — consistent with the existing _on_tool_complete pattern
  • Non-streaming path: The no-op _on_approval_request_nonstream is correctly wired for consistency; approval is handled internally by the agent's built-in mechanism for non-streaming calls
  • Cleanup in _run() finally block: Verified unregister_gateway_notifyreset_current_session_keyclear_session_vars ordering is correct; approval_token is set inside the try block before any exception-prone code, so the if approval_token is not None guard works for both success and early-failure paths
  • Backward compatibility: approval_notify_callback=None default in _run() signature ensures existing callers are unaffected

The implementation correctly separates the streaming and non-streaming approval paths. No issues found.

@harjothkhara

Copy link
Copy Markdown
Contributor

Nice — registering register_gateway_notify + set_current_session_key in _run_agent and tearing them down in the finally is the right shape and mirrors the /v1/runs path well. Two gaps look like they'd stop the streaming approval from actually reaching a client and being resolvable, plus a minor one on the non-streaming path — flagging in case they're useful:

1. The queued __approval_request__ event is silently dropped. _on_approval_request does _stream_q.put(("__approval_request__", event)), but _write_sse_responses's _dispatch only routes __tool_started__ / __tool_completed__ tuples and drops everything else (# Other types are silently dropped.). So the event is enqueued but never emitted onto the SSE stream — the client never sees it. An elif tag == "__approval_request__": branch in _dispatch that writes it as an SSE event (e.g. response.approval.requested) would close this.

2. There's no way to resolve the approval. Once the callback is registered the guard blocks the agent thread in _await_gateway_decision until gateway_timeout (default 300s) and then auto-denies — there's no POST /v1/responses/{response_id}/approval endpoint calling resolve_gateway_approval (the /v1/runs path has _handle_run_approval + the route for exactly this). Without it a streaming caller can register the block but never approve, so it hangs and denies. Note unregister_gateway_notify only runs in the worker's own finally, which can't fire while the thread is parked — so a client disconnect mid-approval also leaves the thread blocked until timeout.

3. Minor — the non-streaming pass callback likely regresses that path. Registering any non-None callback makes _run_agent call register_gateway_notify, which flips the guard from its current immediate text fallback to blocking. With a no-op notify and no resolution channel, a guarded tool on non-streaming /v1/responses would now block until timeout rather than returning right away. Might be safer to leave the non-streaming path without a callback (or fail fast with a clear message) until there's a way to resolve.

Happy to help if any of this is useful.

@rodboev

rodboev commented Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the two functional gaps from the review on the current branch head.

  1. _write_sse_responses() now handles the queued __approval_request__ tuple and emits a response.approval.requested SSE event instead of silently dropping it.
  2. The Responses API now has POST /v1/responses/{response_id}/approval, backed by the same resolve_gateway_approval() path the Runs API uses. The adapter tracks the active approval session by response_id for the lifetime of the streaming response.
  3. I also removed the no-op approval callback from the non-streaming Responses path so it no longer enters the gateway wait loop without a resolution channel.

Validation rerun:

  • pytest tests/gateway/test_api_server.py -v --timeout=0 --timeout-method=thread — 159 passed, 1 skipped

@rodboev
rodboev force-pushed the pr/responses-api-approval branch 2 times, most recently from 48a3218 to a5af237 Compare June 28, 2026 19:52
@alt-glitch alt-glitch added the sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages label Jun 28, 2026
@rodboev
rodboev force-pushed the pr/responses-api-approval branch from eb1c16c to ce372b1 Compare July 7, 2026 05:22
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for completing the missing Responses approval transport. The current main premise is real: gateway/platforms/api_server.py:2970-2979 drops non-tool tagged SSE tuples, and the streaming Responses call at :3397-3409 has no approval callback or response approval route.

Problems

  • The added approval_session_key = gateway_session_key or session_id or response_id is not isolated per response. tools/approval.py:1459-1473 stores one callback and FIFO queue per key. Concurrent clients sharing a session ID can overwrite each other's notifier and resolve each other's pending approval. Current main scopes /v1/runs approvals to generated IDs for this exact reason (gateway/platforms/api_server.py:4289-4303; 66325a770).
  • The added Responses callback forwards the raw approval command to SSE. Current API approval delivery redacts it before enqueueing (gateway/platforms/api_server.py:4364-4373), with a regression contract in tests/gateway/test_approval_prompt_redaction.py:70-128.

Suggested changes

  • Key Responses approval state exclusively by generated response_id, then add a two-concurrent-responses isolation test, including resolve_all.
  • Redact event["command"] through _redact_approval_command before enqueueing and test the emitted payload.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@rodboev
rodboev force-pushed the pr/responses-api-approval branch from ce372b1 to 942e9e3 Compare August 1, 2026 01:33
@rodboev

rodboev commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. Addressed both gaps.

  1. All Responses approval state is now scoped exclusively to the generated response_id. The new _register_gateway_approval helper registers a queue entry only if the callback captured by the worker is still the live registered callback for that key. Disconnect teardown (which calls unregister_gateway_notify) makes any concurrent queue registration fail closed and return immediately rather than entering the timeout loop. resolve_gateway_approval, clear_session, and the approval endpoint all operate on response_id as the sole key. The concurrent isolation test covers two responses sharing a session ID, including resolve_all.

  2. _on_approval_request now copies and redacts event["command"] through _redact_approval_command before the payload enters the event-loop queue. The emitted response.approval.requested event carries the redacted command.

Additionally: clear_session now evicts the response key from _denial_tally so smart-deny state does not outlive the response.

@rodboev rodboev changed the title fix(gateway): wire approval callbacks into Responses API path (#45505) - Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: OpenWebUI Responses API sessions cannot approve guarded tool/code execution

5 participants