feat(api-server): support approval and stop on session chat stream - #58856
feat(api-server): support approval and stop on session chat stream#58856baleian wants to merge 1 commit into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment (not blocking)
New feature: API server support for approval and stop on session chat stream. This is a 3-file PR with moderate additions (~24K diff). The feature adds meaningful new API surface to the session streaming layer.
Suggestion: The PR description is minimal. Please confirm:
- Whether this feature has corresponding test coverage
- Whether any new endpoints are added to the OpenAPI/API schema
Surface area note: With 3 files changed and new stream control behavior, this warrants a human reviewer to confirm the session stop semantics are correct before merge.
|
Tests Line coverage over the changed hunks in API schema No new endpoints — grep over the diff shows zero route registrations. It reuses |
100yenadmin
left a comment
There was a problem hiding this comment.
Reviewed against #58853's accepted design ("reuse the existing runs-API control endpoints, no new endpoints") — this implements it faithfully: approval.request emitted on the session SSE stream with the same redaction shaping as the runs path, the run registered in _active_run_agents/_active_run_tasks/_run_approval_sessions so POST /v1/runs/{run_id}/approval and /stop just work, and cleanup bound to the task future. The test that introspects the source to assert the redaction line sits before the sink is a nice guard against silent regressions during future refactors.
One question and one follow-up thought:
_cleanup_runpops_run_statuses[run_id]— after the stream closes, can a client still query the terminal status (completed/failed/stopped) of a session-stream run, or does it 404 immediately? If the classic runs path retains a terminal status for some window, mirroring that here would keep the two control surfaces symmetric; if it doesn't, ignore me.clarifyprompts park the agent thread the same way exec-approvals do (tools/clarify_gateway.py, same blocking-queue shape) — a session-stream turn that hits a clarify still has no path to answer. The seam this PR builds (approval.requestevent + runs-API resolution) extends to clarify almost mechanically (clarify.requestevent + a resolve route toresolve_gateway_clarify). Natural follow-up rather than scope for this PR — flagging it because we're actively working the clarify surfaces on other channels (#61940, #61943) and would happily build that follow-up on top of this once it lands.
Would like to see this merge — it closes the sharpest gap in #58853.
|
Thanks for targeting a real session-stream control gap: current main's handler calls Problems
Suggested changes
Automated hermes-sweeper review. |
What does this PR do?
Wires interactive approval and stop into the streaming session
endpoint
POST /api/sessions/{id}/chat/stream.That endpoint reaches the agent through
_run_agent, which — unlike the/v1/runspath — registered nothing in the shared run maps. As a result:registered for the turn, so a guarded tool hit the "no callback" fallback in
tools/approval.pyand returnedapproval_pendingto the agent while theSSE stream emitted no
approval.requestevent and offered no way toresolve it. This is the session-endpoint sibling of Legacy chat-completions stream does not emit approval events #51871 (same root cause:
the notifier is only wired on the runs path) and complements fix(api-server): emit approval events on legacy chat-completions SSE stream #51878, which
fixes it for
/v1/chat/completions.run_idwas never published in_active_run_agents/_active_run_tasks, soPOST /v1/runs/{run_id}/stopcould not find the agent to interrupt.
Rather than add new endpoints, this reuses the existing runs-API control
endpoints (
/v1/runs/{run_id}/approval,/v1/runs/{run_id}/stop) unchanged —they key purely off shared
run_idmaps and are agnostic to which handlerpopulated them. The streaming session turn now registers itself in those maps
and surfaces
approval.requeston its SSE stream. Full session-store historyfidelity (tool results, tool_calls, reasoning) comes for free because the
session path already replays the native conversation.
Only the streaming endpoint is wired; the synchronous
/api/sessions/{id}/chatcannot do interactive approval (single blocking response, no event channel) and
is left unchanged.
Related Issue
Fixes #58853
Related: #51871 (chat-completions sibling), #51878.
Type of Change
Changes Made
gateway/platforms/api_server.py:_run_agent: add two optional, backward-compatible params —approval_notify_callback(register/unregister the gateway approvalnotifier around
run_conversation, mirroring the/v1/runsinlinewiring) and
run_id(publish the liveAIAgentin_active_run_agents[run_id]and clean it up infinallyso/v1/runs/{run_id}/stopcan interrupt it). Existing callers(
/v1/chat/completions) pass neither and are unaffected._handle_session_chat_stream: define an_approval_notifyclosure thatredacts credentials (
_redact_approval_command) and emitsapproval.requeston the SSE stream; register the run in_run_approval_sessions/_run_statuses/_active_run_tasks(and_run_streams_createdfor the orphan sweeper); set terminal run status;tear all registrations down on turn completion.
tests/gateway/test_session_chat_approval_stop.py: new — 7 tests covering_run_agentrun_id publish/cleanup, approval callback register/unregister,the
_approval_notifyclosure (redaction +approval.requestshape), and anend-to-end streaming turn that emits
approval.requestand populates theshared control maps, then cleans them up.
tests/gateway/test_approval_prompt_redaction.py: harden the redact-before-sink guard to check every
_approval_notifyin the module (not just thefirst), so additional approval-notify transports can't skip redaction.
How to Test
pytest tests/gateway/test_session_chat_approval_stop.py tests/gateway/test_approval_prompt_redaction.py -q— 15 pass.pytest tests/gateway/test_api_server_runs.py tests/gateway/test_api_server.py tests/run_agent/ -q— all pass (2111 passed, 3 skipped locally).API_SERVER_ENABLED=1and a tool that requires approval.POST /api/sessions/{id}/chat/streamwith a prompt that triggers the guarded tool → the SSE stream now emitsevent: approval.request.POST /v1/runs/{run_id}/approval {"choice":"once"}(run_id from therun.startedevent) → the tool proceeds.POST /v1/runs/{run_id}/stop→ the agent is interrupted.Checklist
Code
feat(api-server): …)Documentation & Housekeeping
_run_agent) — no user-facing docs needed