From c6392902cd1b63546846d8ecbfe2dbefa30f591c Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Fri, 19 Jun 2026 13:16:39 -0400 Subject: [PATCH] fix(gateway): correct approval URL and thread session history into runs API (#4479) --- api/gateway_chat.py | 12 +- api/runner_client.py | 4 +- tests/test_gateway_approval_runs_api.py | 4 +- ...test_issue4479_gateway_approval_history.py | 119 ++++++++++++++++++ tests/test_runner_client.py | 2 +- 5 files changed, 135 insertions(+), 6 deletions(-) create mode 100644 tests/test_issue4479_gateway_approval_history.py diff --git a/api/gateway_chat.py b/api/gateway_chat.py index c4cca576..ccfbcaff 100644 --- a/api/gateway_chat.py +++ b/api/gateway_chat.py @@ -260,7 +260,7 @@ def _run_gateway_runs_api_streaming( session_id, msg_text, model, workspace, stream_id, base_url, api_key, prefill_messages, body_extras, *, put_gateway_event, cancel_event, - attachments=None, cfg=None, + attachments=None, cfg=None, session=None, ): """Submit via POST /v1/runs and relay SSE events including approval.""" url_runs = f"{base_url.rstrip('/')}/v1/runs" @@ -283,6 +283,15 @@ def _run_gateway_runs_api_streaming( message_content = str(msg_text or "") instructions_parts = [] conversation_history = [] + for entry in getattr(session, "context_messages", None) or []: + if not isinstance(entry, dict): + continue + role = str(entry.get("role") or "").strip().lower() + if role not in {"user", "assistant"}: + continue + content = entry.get("content") + if content is not None: + conversation_history.append({"role": role, "content": content}) for entry in prefill_messages or []: if not isinstance(entry, dict): continue @@ -572,6 +581,7 @@ def _run_gateway_chat_streaming( cancel_event=cancel_event, attachments=attachments, cfg=cfg, + session=s, ) except Exception as exc: put_gateway_event("apperror", { diff --git a/api/runner_client.py b/api/runner_client.py index 459bf8dd..5ba1ed62 100644 --- a/api/runner_client.py +++ b/api/runner_client.py @@ -81,8 +81,8 @@ class HttpRunnerClient: def respond_approval(self, run_id: str, approval_id: str, choice: str) -> dict[str, Any]: return self._post( - f"/v1/runs/{urllib.parse.quote(str(run_id), safe='')}/approvals/{urllib.parse.quote(str(approval_id), safe='')}/respond", - {"choice": choice}, + f"/v1/runs/{urllib.parse.quote(str(run_id), safe='')}/approval", + {"choice": choice, "approval_id": approval_id}, ) def respond_clarify(self, run_id: str, clarify_id: str, response: str) -> dict[str, Any]: diff --git a/tests/test_gateway_approval_runs_api.py b/tests/test_gateway_approval_runs_api.py index 8f87960c..ebaecfbb 100644 --- a/tests/test_gateway_approval_runs_api.py +++ b/tests/test_gateway_approval_runs_api.py @@ -470,8 +470,8 @@ def test_gateway_approval_response_relay(): from api.routes import _handle_approval_respond _handle_approval_respond(handler, body) - assert captured.get("url", "") == "http://gw:8642/v1/runs/run%20abc%2F1/approvals/appr%20x%2Fy/respond" - assert captured["body"] == {"choice": "once"} + assert captured.get("url", "") == "http://gw:8642/v1/runs/run%20abc%2F1/approval" + assert captured["body"] == {"choice": "once", "approval_id": "appr x/y"} handler.send_response.assert_called_with(200) # Cleanup. diff --git a/tests/test_issue4479_gateway_approval_history.py b/tests/test_issue4479_gateway_approval_history.py new file mode 100644 index 00000000..7a41f8c6 --- /dev/null +++ b/tests/test_issue4479_gateway_approval_history.py @@ -0,0 +1,119 @@ +"""Tests for #4479: gateway approval URL contract and conversation history threading.""" +from __future__ import annotations + +import types + + +# --------------------------------------------------------------------------- +# Bug 1: respond_approval URL and payload shape +# --------------------------------------------------------------------------- + +def test_respond_approval_url_matches_gateway_contract(): + from api.runner_client import HttpRunnerClient + + client = HttpRunnerClient(base_url="http://gw:8642") + captured = {} + + def fake_post(url, body): + captured["url"] = url + captured["body"] = body + return {} + + client._post = fake_post + client.respond_approval("run-abc", "appr-xyz", "approve") + + assert captured["url"] == "/v1/runs/run-abc/approval" + assert "/approvals/" not in captured["url"] + assert "/respond" not in captured["url"] + + +def test_respond_approval_body_includes_approval_id(): + from api.runner_client import HttpRunnerClient + + client = HttpRunnerClient(base_url="http://gw:8642") + captured = {} + + def fake_post(url, body): + captured["url"] = url + captured["body"] = body + return {} + + client._post = fake_post + client.respond_approval("run-1", "appr-2", "deny") + + assert captured["body"] == {"choice": "deny", "approval_id": "appr-2"} + + +# --------------------------------------------------------------------------- +# Bug 2: conversation history threading via session parameter +# --------------------------------------------------------------------------- + +def _build_conversation_history(context_messages, prefill_messages): + """Exercise the same logic as _run_gateway_runs_api_streaming's conversation builder.""" + session = types.SimpleNamespace(context_messages=context_messages) + instructions_parts = [] + conversation_history = [] + for entry in getattr(session, "context_messages", None) or []: + if not isinstance(entry, dict): + continue + role = str(entry.get("role") or "").strip().lower() + if role not in {"user", "assistant"}: + continue + content = entry.get("content") + if content is not None: + conversation_history.append({"role": role, "content": content}) + for entry in prefill_messages or []: + if not isinstance(entry, dict): + continue + role = str(entry.get("role") or "").strip().lower() + content = entry.get("content") + if role == "system": + if isinstance(content, str) and content.strip(): + instructions_parts.append(content) + elif content is not None: + instructions_parts.append(str(content)) + continue + if role not in {"user", "assistant"}: + continue + conversation_history.append({"role": role, "content": content}) + return conversation_history, instructions_parts + + +def test_runs_api_threads_session_context_messages(): + context = [ + {"role": "user", "content": "hello"}, + {"role": "assistant", "content": "hi there"}, + ] + prefill = [{"role": "system", "content": "You are helpful."}] + + history, instructions = _build_conversation_history(context, prefill) + + assert len(history) == 2 + assert history[0] == {"role": "user", "content": "hello"} + assert history[1] == {"role": "assistant", "content": "hi there"} + assert instructions == ["You are helpful."] + + +def test_runs_api_filters_system_from_context_messages(): + context = [ + {"role": "user", "content": "hello"}, + {"role": "system", "content": "injected system"}, + {"role": "assistant", "content": "hi"}, + {"role": "tool", "content": "tool output"}, + ] + prefill = [] + + history, _ = _build_conversation_history(context, prefill) + + roles = [m["role"] for m in history] + assert "system" not in roles + assert "tool" not in roles + assert roles == ["user", "assistant"] + + +def test_runs_api_empty_context_messages(): + history, _ = _build_conversation_history(None, [{"role": "system", "content": "sys"}]) + assert history == [] + + history2, _ = _build_conversation_history([], [{"role": "user", "content": "q"}]) + assert history2 == [{"role": "user", "content": "q"}] diff --git a/tests/test_runner_client.py b/tests/test_runner_client.py index 9b47b67c..4cbac2d4 100644 --- a/tests/test_runner_client.py +++ b/tests/test_runner_client.py @@ -111,7 +111,7 @@ def test_runner_client_maps_observe_status_and_controls(monkeypatch): ("GET", "http://runner.local/v1/runs/run%2F1/events?cursor=event%3A2", None), ("GET", "http://runner.local/v1/runs/run%2F1", None), ("POST", "http://runner.local/v1/runs/run%2F1/cancel", {}), - ("POST", "http://runner.local/v1/runs/run%2F1/approvals/approval%2F1/respond", {"choice": "once"}), + ("POST", "http://runner.local/v1/runs/run%2F1/approval", {"choice": "once", "approval_id": "approval/1"}), ("POST", "http://runner.local/v1/runs/run%2F1/clarifications/clarify%2F1/respond", {"response": "answer"}), ("POST", "http://runner.local/v1/runs/run%2F1/messages", {"message": "next", "mode": "interrupt"}), ("POST", "http://runner.local/v1/sessions/session%2F1/goal", {"action": "set", "text": "finish"}),