Skip to content

fix(api-server): emit approval events on legacy chat-completions SSE stream - #51878

Open
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/api-server-chat-completions-approval-events
Open

fix(api-server): emit approval events on legacy chat-completions SSE stream#51878
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/api-server-chat-completions-approval-events

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Wire approval-request events into the legacy /v1/chat/completions SSE stream so WebUI (and compatible clients) receive approval.request / hermes.approval.request events when a guarded tool blocks for approval.

Related Issue

Fixes #51871

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • gateway/platforms/api_server.py: Add approval_notify_callback parameter to _run_agent; define _approval_notify closure in _handle_chat_completions (streaming path) that redacts credentials and pushes __approval__ tagged tuples; register approval session key in _run_approval_sessions so POST /v1/runs/{completion_id}/approval resolves pending approvals; emit both event: approval.request and event: hermes.approval.request in the SSE writer; clean up _run_approval_sessions on agent task completion.
  • tests/gateway/test_chat_completions_approval.py: 11 regression tests covering callback queue behavior, redaction pass-through, SSE dual-event emission, approval session lifecycle, and _run_agent registration/unregistration.

How to Test

  1. Run pytest tests/gateway/test_chat_completions_approval.py tests/gateway/test_approval_prompt_redaction.py -q — all 19 tests should pass.
  2. Run pytest tests/gateway/test_approve_deny_commands.py tests/tools/test_approval.py -q — all 254 existing approval tests should still pass.
  3. Manual: start Hermes gateway with a tool that requires approval (e.g., rm -rf guard), send a streaming /v1/chat/completions request, verify the SSE stream emits event: approval.request with run_id, choices, and redacted command.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/gateway/test_approval_prompt_redaction.py tests/gateway/test_approve_deny_commands.py tests/tools/test_approval.py tests/gateway/test_chat_completions_approval.py -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Code Intelligence

  • Analyzed: gateway/platforms/api_server.py (_handle_chat_completions, _run_agent, _write_sse_chat_completion, _handle_run_approval)
  • Blast radius: LOW — additive parameter to _run_agent, new tagged-tuple type in SSE queue, no changes to existing runs-API approval path
  • Related patterns: Mirrors the existing _approval_notify in the /v1/runs path (line ~3965); reuses _redact_approval_command for credential redaction; uses _run_approval_sessions for resolution lookup

…stream

The /v1/chat/completions streaming path did not register an approval
notify callback, so approval-required tool runs blocked silently with
no event reaching the WebUI. Mirror the runs-API approval wiring:

- Add approval_notify_callback parameter to _run_agent
- Define _approval_notify in _handle_chat_completions that redacts
  credentials and pushes __approval__ tagged tuples to the SSE queue
- Register the session key in _run_approval_sessions so
  POST /v1/runs/{completion_id}/approval can resolve pending approvals
- Emit both event: approval.request and hermes.approval.request on
  the SSE stream for backward compatibility
- Clean up _run_approval_sessions when the agent task completes

Fixes NousResearch#51871
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists 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 labels Jun 24, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracing the missing chat-completions approval bridge; current main still starts this stream without approval wiring at gateway/platforms/api_server.py:2311-2322, so the premise is valid.

Problems

  • gateway/platforms/api_server.py:2022 reuses gateway_session_key / session_id as the approval key. That is unsafe for concurrent requests: tools/approval.py:1459-1485 has one callback and pending queue per key. Current main deliberately changed runs to use a unique run ID in 66325a770; tests/gateway/test_api_server_runs.py:361-420 covers the cross-run approval isolation this would regress.
  • The status created at gateway/platforms/api_server.py:2024-2030 is never made terminal: _on_agent_done only removes approval state at :2057-2060. Current cleanup retains nonterminal statuses (gateway/platforms/api_server.py:4772-4779), so completed streams remain running indefinitely.

Suggested changes

  • Key chat-completion approvals by completion_id, and add the concurrent shared-session isolation regression.
  • Set a terminal status (or remove the synthetic status) when the task ends, then assert lifecycle cleanup in the integration test.

Automated hermes-sweeper review.


# Register the approval session so POST /v1/runs/{completion_id}/approval
# can resolve pending approvals on the legacy chat-completions path.
approval_session_key = gateway_session_key or session_id or completion_id

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use completion_id as the approval key. tools.approval owns one callback and queue per key, so a shared session or memory key lets concurrent streams overwrite each other's callback and resolve the wrong dangerous command. Current main fixed this exact class for runs in 66325a770 by using the unique run ID.

# chat-completions path so stale entries don't accumulate.
def _on_agent_done(_fut):
_stream_q.put(None)
self._run_approval_sessions.pop(completion_id, None)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes approval state but leaves the status created at lines 2024-2030 as running. Current status cleanup only expires terminal states, so set a terminal status here (or remove the synthetic status) to avoid retaining completed chat streams forever.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/streaming Streaming responses: gateway delivery, provider wire labels Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/streaming Streaming responses: gateway delivery, provider wire 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-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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.

Legacy chat-completions stream does not emit approval events

3 participants