From 490bb6d6c968d0240f3034318e05f9561baf4334 Mon Sep 17 00:00:00 2001 From: Sertac Ozercan Date: Mon, 14 Sep 2026 12:48:59 -0700 Subject: [PATCH] fix(runtime): preserve Orka tool approval outcomes Signed-off-by: Sertac Ozercan --- docs/foundry-hosted-brokered.md | 31 ++- docs/orka-human-approval.md | 144 +++++++++++ docs/orka.md | 11 +- docs/runtime-adapters.md | 3 +- docs/runtime-capabilities.md | 6 + .../common/agentkit_serve_common/foundry.py | 5 + .../foundry_model_loop.py | 20 ++ .../agentkit_serve_common/tool_errors.py | 26 ++ .../common/tests/test_foundry_approvals.py | 140 +++++++++++ .../tests/test_foundry_brokered_protocol.py | 8 +- .../agentkit_serve/agent_factory.py | 37 ++- .../tests/test_orka_approvals.py | 230 ++++++++++++++++++ 12 files changed, 642 insertions(+), 19 deletions(-) create mode 100644 docs/orka-human-approval.md create mode 100644 runtimes/common/agentkit_serve_common/tool_errors.py create mode 100644 runtimes/common/tests/test_foundry_approvals.py create mode 100644 runtimes/microsoft-agent-framework/tests/test_orka_approvals.py diff --git a/docs/foundry-hosted-brokered.md b/docs/foundry-hosted-brokered.md index e1ec034..2f007b8 100644 --- a/docs/foundry-hosted-brokered.md +++ b/docs/foundry-hosted-brokered.md @@ -152,6 +152,15 @@ Canonical denied/error payload: } ``` +Both `approved` values describe final results. Neither value represents a +pending human review. Orka holds its original tool call while the review is +pending, then sends exactly one final result for that call. The hosted adapter +uses fixed messages for Orka's `approval_declined`, `approval_expired`, +`approval_cancelled`, `approval_stale`, `tool_execution_failed`, and +`tool_outcome_unknown` codes. A tool failure remains distinct from a declined +review. An unknown outcome finishes the continuation without another model +request, preventing an automatic retry of an uncertain action. + `function_call_output` is privileged continuation input. It is rejected unless a known `previous_response_id` has a pending matching `call_id` **and** the request uses the Orka-only continuation path. Configure @@ -205,9 +214,11 @@ construction. The public continuation must still carry the same `agent_session_id` so Foundry routes it to that sandbox. An immediate initial/continuation pair proves routing affinity, not file recovery. -To prove persistence, stop the Foundry session between the two requests and show -that a new process reloads the pending state. Configure the state TTL and Orka -approval timeout longer than that test. Deployments without a session-persisted +To prove persistence, restart the AgentKit process between the two requests in +the same hosted session and show that it reloads the pending state. Preserve the +state file and keep the test inside the configured state TTL and Orka task/session +limits. This does not establish recovery of a lost Foundry runtime session. +Deployments without a session-persisted file, shared file, or platform-managed store must pin one replica or use sticky routing; otherwise a continuation that lands on a different/restarted container fails safely with `unknown_previous_response_id`. Pending state expires after @@ -221,6 +232,14 @@ they are persisted, replayed, embedded in deterministic responses, or sent back through the model loop. A platform-managed state backend is still required before treating multi-replica production as fully supported. +For Orka-managed human approval, configure +`AGENTKIT_FOUNDRY_RESPONSE_STATE_TTL_SECONDS=1800` and a hosted-session idle +timeout of at least 1800 seconds. The review can consume 600 seconds and tool +execution another 240 seconds, followed by result delivery and model continuation. +The default 900-second state lifetime leaves too little margin at that limit. +Task deadlines, active leases, and platform session limits still apply. See +[Human approval for Orka tools](orka-human-approval.md) for the full wait budget. + The file is sensitive runtime state, not harmless metadata. In model-loop mode it includes model messages such as system instructions, conversation history, and user prompts, in addition to brokered arguments/outputs and cached final @@ -426,8 +445,10 @@ checking ownership, the active lease, and the expected response and call. It also translates MCP results into AgentKit's approved/error envelope. Keep the proof out of the ACP child configuration. The gateway must forward the proof field; local tests cannot establish that a public Foundry deployment does so. -See the shared-proof limitations above. Human tool approvals for external v2 -runtimes remain unsupported by Orka. +See the shared-proof limitations above. Human tool approvals require a qualified +Orka controller, supervisor, Foundry broker, and AgentKit image that support +`supportsBrokeredToolApprovals`. Older combinations remain unsupported. Review +policy, reviewer permissions, cancellation, and execution decisions stay in Orka. ## Hosted follow-up questions diff --git a/docs/orka-human-approval.md b/docs/orka-human-approval.md new file mode 100644 index 0000000..6bd4b3a --- /dev/null +++ b/docs/orka-human-approval.md @@ -0,0 +1,144 @@ +# Human approval for Orka tools + +AgentKit can wait while a person reviews an Orka-managed tool call. Orka stores +the proposed action, checks reviewer permissions, and decides whether the tool +runs. AgentKit receives the final result for the original call and continues +without resubmitting the prompt or repeating completed tool steps. + +This requires matching Orka, AgentKit, and, for hosted execution, Foundry broker +images qualified for `supportsBrokeredToolApprovals`. The direct path uses the +Microsoft Agent Framework ACP adapter and Orka's loopback MCP server. The hosted +path uses AgentKit's brokered Responses adapter through `agent-runtime-foundry`. +Do not advertise approval support for other runtime combinations without their +matching acceptance checks. + +AgentKit's local `tools[].approval` setting is separate. Omission and `never` +remain valid; `auto` and `always` remain unsupported. `supportsPermissions` +stays false. Approval of shell commands or files inside the runtime is outside +this feature. + +## Wait limits and setup + +| Limit | Qualified setup | +|---|---| +| Human review | At most 600 seconds, subject to tighter task/session limits | +| Tool execution after approval | At most 240 seconds | +| Direct MCP `tools/call` | Orka injects `AGENTKIT_MCP_TIMEOUT=900` into the ACP child | +| Hosted AgentKit pending state | Set `AGENTKIT_FOUNDRY_RESPONSE_STATE_TTL_SECONDS=1800` | +| Foundry hosted-session idle timeout | Configure at least 1800 seconds | + +The complete task needs time for initial model work, review, tool execution, +result delivery, and model continuation. A larger MCP or state timeout does not +extend a task deadline, an expired lease, or a platform session limit. A review +that reaches its deadline expires without executing the proposed action. + +The direct MAF default remains 120 seconds outside the qualified Orka child. +Confirm the child receives the 900-second override, rather than setting it only +on the supervisor process. The MAF adapter applies that value to both its MCP +request timeout and HTTP client. It does not retry a tool call after connection +loss because the action might already have run. + +Hosted AgentKit's `/readiness` response reports +`foundryResponses.stateTtlSeconds`; check that it is 1800 in the configured +deployment. Also verify the idle timeout on the deployed Foundry agent version. +[Foundry's hosted-session documentation](https://learn.microsoft.com/azure/foundry/agents/how-to/manage-hosted-sessions) +describes that platform setting. Keep the same `agent_session_id` and the +authenticated continuation proof on subsequent requests. + +For AgentKit process recovery, configure `AGENTKIT_FOUNDRY_RESPONSE_STATE_FILE` +on access-controlled storage that survives the process restart. Use one writer +and preserve the same hosted session identity. Missing or expired state rejects +the continuation; it never authorizes restarting the original action. This +does not provide recovery of an entire lost Foundry runtime session. + +## Outcomes + +While review is pending, the tool execution count remains zero. The direct MCP +request stays open. Hosted AgentKit retains its pending `function_call`; Orka +sends no `function_call_output` until the result is final. `approved:false` +must never be sent merely because the person has not decided. + +| Final outcome | AgentKit behavior | +|---|---| +| Approved and executed | Continue the original call with its actual output | +| `approval_declined` | Report that the person declined the call | +| `approval_expired` | Report that the review expired | +| `approval_cancelled` | Report cancellation when a final result can be delivered | +| `approval_stale` | Report that the approval no longer authorizes the call | +| `tool_execution_failed` | Report execution failure, distinct from a human decline | +| `tool_outcome_unknown` | Stop automatic continuation; never retry the uncertain action | + +The direct adapter accepts these codes only from an MCP error result's +`structuredContent.code`. It replaces tool-controlled messages with fixed text. +An unknown outcome fails the direct prompt. Hosted AgentKit returns a fixed +unknown-outcome response and caches it, without asking the model for another +tool call. Identical hosted result delivery returns the cached response; +conflicting results are rejected. + +Orka controls cancellation. Cancelling a task or losing its authority stops +the waiting runtime. A later approval must not revive it. The AgentKit hosted +endpoint does not independently grant permission to resume a cancelled task. + +## Acceptance through Orka + +Use Orka's +[`examples/human-approval-v2`](https://github.com/orka-agents/orka/tree/main/examples/human-approval-v2) +fixture with automatic `read-inventory` and approval-required +`create-work-order`. Both are simulated; the action records an execution count. +Follow its setup and Task fixture for each qualified runtime. Keep separate +runtime capacity available for an independent task while the first waits. + +Inspect the pending review in Orka's task approval panel or through the normal +API. The following commands operate on an already-created disposable task. +`ORKA_CURL_CONFIG` names a private curl configuration containing the required +authentication. Keep its credential out of shell history and command arguments. + +```sh +curl --fail --silent --show-error --config "$ORKA_CURL_CONFIG" \ + "$ORKA_API_URL/api/v1/tasks/$TASK_NAME/approvals?namespace=$ORKA_NAMESPACE" | + jq '{taskName, approvals: [.approvals[] | { + id, action, targetTool, targetArgsPreview, targetArgsDigest, + status, expiresAt, executionOutcome + }]}' +``` + +Check the exact tool, safe argument preview, task, and expiry before selecting +`APPROVAL_ID`. Confirm the simulated action count is zero, then approve it: + +```sh +curl --fail --silent --show-error --config "$ORKA_CURL_CONFIG" \ + -H 'Content-Type: application/json' \ + --data '{"decision":"approve","reason":"Simulated action acceptance"}' \ + "$ORKA_API_URL/api/v1/tasks/$TASK_NAME/approvals/$APPROVAL_ID/decision?namespace=$ORKA_NAMESPACE" +``` + +The action must run once and its receipt must reach the original conversation. +Repeat with a fresh task and `"decision":"decline"`; the action count must stay +zero. Use fresh disposable tasks for cancellation, review expiry, a tool error +after approval, and duplicate or competing decisions. Check both execution +count and Orka's stored `executionOutcome`. Do not treat a successful API request +alone as evidence that a tool ran. + +For direct execution, include a review longer than 120 seconds and within the +600-second limit. For hosted execution, restart only the AgentKit process with +saved pending state and prove the matching continuation works. Repeat without +saved state and confirm it fails without a new action. Verify an unrelated +conversation progresses while review is pending. + +## Local compatibility tests + +The deterministic tests cover waiting, named outcomes, cancellation, independent +work, saved-state recovery, and duplicate continuations. The HTTP test uses the +production MAF client and MCP SDK with a counted simulated broker. Run it with +a real wait longer than the old default: + +```sh +AGENTKIT_TEST_APPROVAL_WAIT_SECONDS=121 \ + uv run --directory runtimes/microsoft-agent-framework --extra dev \ + pytest -q tests/test_orka_approvals.py -k http_mcp +uv run --directory runtimes/common --extra dev \ + pytest -q tests/test_foundry_approvals.py +``` + +These tests establish runtime compatibility. Live Orka approval API execution +and public Foundry gateway behavior require the separate acceptance run above. diff --git a/docs/orka.md b/docs/orka.md index 7a71466..9af6c55 100644 --- a/docs/orka.md +++ b/docs/orka.md @@ -55,11 +55,18 @@ config. The registration's `adapterName` must be `agentkit-serve-acp`. Its `adapterDigest` and the composition build's `AGENTKIT_ADAPTER_DIGEST` must both equal the `sha256:` digest from `AGENTKIT_RUNTIME_IMAGE`. Set the profile's `providerKind` to `agentkit` and advertise -`supportsAgentSessionConfiguration: false`. `approvalRequiredTools` must stay -empty because the AgentKit ACP child does not implement permission callbacks. +`supportsAgentSessionConfiguration: false` and `supportsPermissions: false`. +Orka-managed MCP tool approval uses the existing waiting call, without a local +ACP permission callback. A qualified Microsoft Agent Framework composition can +advertise `supportsBrokeredToolApprovals: true` and configure Orka's +`approvalRequiredTools`. The controller, supervisor, and AgentKit image must all +support that contract. Older images and unqualified adapters remain unsupported. If the registration allows brokered tools, the Task must submit that exact `allowedTools` list. +See [Human approval for Orka tools](orka-human-approval.md) for wait limits, +result handling, and acceptance checks through Orka's approval API. + Set `ORKA_ACP_CONTROLLER_EPOCH` from Orka's current `ControllerEpoch` record. Select by `spec.name` because the resource name is hashed. This lookup requires exactly one matching record with a positive integer epoch: diff --git a/docs/runtime-adapters.md b/docs/runtime-adapters.md index 8a543c1..91f48b9 100644 --- a/docs/runtime-adapters.md +++ b/docs/runtime-adapters.md @@ -118,7 +118,8 @@ Startup-scoped model clients and long-lived MCP sessions still resolve their own startup credentials at runtime initialization; they are not rebuilt for every turn. - a missing or empty command fails before serving, -- `AGENTKIT_MCP_TIMEOUT` controls MCP initialization timeout, +- `AGENTKIT_MCP_TIMEOUT` controls MCP initialization timeout; MAF also uses it + for tool requests, including [Orka approval waits](orka-human-approval.md), - each tool subprocess receives only env vars declared in that tool's `env`, - undeclared `${VAR}` interpolation inside a declared env value is rejected, and - tool sessions are entered once for the app lifespan and reused across requests, diff --git a/docs/runtime-capabilities.md b/docs/runtime-capabilities.md index 8fb6cfe..a0d9c03 100644 --- a/docs/runtime-capabilities.md +++ b/docs/runtime-capabilities.md @@ -43,6 +43,12 @@ and auth wiring. Orka-specific names here describe the protocol contract AgentKi exposes; Orka remains responsible for policy, approval, idempotency, and side-effect governance. +Orka v2's `supportsBrokeredToolApprovals` registration capability covers +controller-managed approvals over the ACP child's MCP connection. It is separate +from AgentKit's unsupported local `tool-approval` capability and the legacy v1 +brokered hooks below. See [Human approval for Orka tools](orka-human-approval.md) +for the qualified direct MAF and hosted Foundry paths. + ## Current support | Runtime | Capabilities | diff --git a/runtimes/common/agentkit_serve_common/foundry.py b/runtimes/common/agentkit_serve_common/foundry.py index e79c2e9..7282585 100644 --- a/runtimes/common/agentkit_serve_common/foundry.py +++ b/runtimes/common/agentkit_serve_common/foundry.py @@ -49,6 +49,7 @@ from .conversation import FORWARDED_ROLES, ConversationTurn, RunRequest from .runtime import AgentRunError, BrokeredToolDefinition, RunResult, RuntimeFactory from .server import make_auth_dependency +from .tool_errors import orka_tool_error_details logger = logging.getLogger(__name__) @@ -2324,6 +2325,10 @@ def _advance_brokered_state( def _final_text_from_tool_output(call: _PendingCall, output: dict[str, Any]) -> str: if not output.get("approved"): error = output.get("error") if isinstance(output.get("error"), dict) else {} + details = orka_tool_error_details(error) + if details is not None: + code, message = details + return f"Brokered tool {call.tool.name}: {code}: {message}" code = str(error.get("code") or "brokered_tool_denied") message = str(error.get("message") or "brokered tool was not performed") return f"Brokered tool {call.tool.name} was not performed: {code}: {message}" diff --git a/runtimes/common/agentkit_serve_common/foundry_model_loop.py b/runtimes/common/agentkit_serve_common/foundry_model_loop.py index bcf1129..fe902d9 100644 --- a/runtimes/common/agentkit_serve_common/foundry_model_loop.py +++ b/runtimes/common/agentkit_serve_common/foundry_model_loop.py @@ -28,6 +28,7 @@ from .conversation import FORWARDED_ROLES, RunRequest from .runtime import AgentRunError, BrokeredToolDefinition from .skills import SkillCatalog +from .tool_errors import orka_tool_error_details _MAX_ARGUMENT_DEPTH = 128 _MAX_RATE_LIMIT_RETRIES = 2 @@ -211,8 +212,27 @@ async def resume( ) from exc if len(output_bytes) > self.max_output_bytes: raise AgentRunError("brokered tool output is too large for model resume", status=413, code="brokered_output_too_large") + try: + payload = json.loads(output) + except (ValueError, RecursionError): + payload = None + details = ( + orka_tool_error_details(payload.get("error")) + if isinstance(payload, dict) and payload.get("approved") is False + else None + ) + if details is not None: + code, message = details + output = json.dumps({"approved": False, "error": {"code": code, "message": message}}, separators=(",", ":")) + if len(output.encode("utf-8")) > self.max_output_bytes: + raise AgentRunError("brokered tool output is too large for model resume", status=413, code="brokered_output_too_large") resumed = [dict(message) for message in messages] resumed.append({"role": "tool", "tool_call_id": call_id, "content": output}) + if details is not None and details[0] == "tool_outcome_unknown": + # Another model round could repeat an action whose effect is unknown. + # Finish this continuation and let Orka retain the execution evidence. + text = f"{details[0]}: {details[1]}" + return ModelLoopFinal(text=text, messages=[*resumed, {"role": "assistant", "content": text}]) return await self._advance(resumed, call_id=next_call_id or f"call_{uuid.uuid4().hex}") async def validate_credentials(self) -> None: diff --git a/runtimes/common/agentkit_serve_common/tool_errors.py b/runtimes/common/agentkit_serve_common/tool_errors.py new file mode 100644 index 0000000..5e92a24 --- /dev/null +++ b/runtimes/common/agentkit_serve_common/tool_errors.py @@ -0,0 +1,26 @@ +"""Fixed, non-sensitive outcomes from Orka's governed tool broker.""" + +from __future__ import annotations + +from collections.abc import Mapping + + +_ORKA_TOOL_ERRORS = { + "approval_declined": "The tool call was declined.", + "approval_expired": "The tool approval expired.", + "approval_cancelled": "The tool call was cancelled.", + "approval_stale": "The tool approval is no longer valid.", + "tool_execution_failed": "MCP tool execution failed.", + "tool_outcome_unknown": "The tool execution outcome is unknown; do not retry.", +} + + +def orka_tool_error_details(value: object) -> tuple[str, str] | None: + """Read an explicit broker code without forwarding tool-controlled text.""" + if not isinstance(value, Mapping): + return None + code = value.get("code") + if not isinstance(code, str): + return None + message = _ORKA_TOOL_ERRORS.get(code) + return (code, message) if message is not None else None diff --git a/runtimes/common/tests/test_foundry_approvals.py b/runtimes/common/tests/test_foundry_approvals.py new file mode 100644 index 0000000..655c76e --- /dev/null +++ b/runtimes/common/tests/test_foundry_approvals.py @@ -0,0 +1,140 @@ +"""Hosted continuation compatibility with Orka's final approval outcomes.""" + +from __future__ import annotations + +import json +from types import SimpleNamespace + +import pytest +from fastapi.testclient import TestClient + +import agentkit_serve_common.foundry as foundry_module +from test_foundry_brokered_protocol import ( + CONTINUATION_AUTH, + _FakeChatTransport, + _app, + _call, + _chat_response, + _continuation, + _message_text, + _model_loop_app, + _multi_tool_spec, + _spec, +) +from test_foundry_tool_workflows import _tool + + +_ERRORS = { + "approval_declined": "The tool call was declined.", + "approval_expired": "The tool approval expired.", + "approval_cancelled": "The tool call was cancelled.", + "approval_stale": "The tool approval is no longer valid.", + "tool_execution_failed": "MCP tool execution failed.", + "tool_outcome_unknown": "The tool execution outcome is unknown; do not retry.", +} + + +def _result(response, output, *, session="review-session"): + request = _continuation(response["id"], _call(response)["call_id"], output) + request["agent_session_id"] = session + return request + + +@pytest.mark.parametrize("outcome", ["approved", *_ERRORS]) +def test_review_wait_and_process_restart_preserve_completed_steps_and_final_result(tmp_path, monkeypatch, outcome): + now = [1000.0] + monkeypatch.setattr(foundry_module, "time", SimpleNamespace(time=lambda: now[0])) + monkeypatch.setenv("AGENTKIT_FOUNDRY_RESPONSE_STATE_TTL_SECONDS", "1800") + state_file = tmp_path / "responses.json" + spec = _multi_tool_spec() + spec.brokered_tools[1].name = "create-simulated-work-order" + spec.brokered_tools[1].brokered_class = "write" + first_model = _FakeChatTransport([ + _tool("check-network-telemetry", {"site": "site-a"}), + _tool("create-simulated-work-order"), + _tool("check-network-telemetry", {"site": "site-b"}), + _chat_response({"role": "assistant", "content": "The independent lookup finished."}), + ]) + with TestClient(_model_loop_app(spec, first_model, response_state_file=state_file)) as client: + initial = client.post("/responses", json={"input": "Check site-a, then create a simulated work order.", "agent_session_id": "review-session"}).json() + first_output = {"approved": True, "output": {"site": "site-a", "signal": "low"}} + pending_response = client.post("/responses", json=_result(initial, first_output), headers=CONTINUATION_AUTH) + assert pending_response.status_code == 200, pending_response.text + pending = pending_response.json() + assert _call(pending)["name"] == "create-simulated-work-order" + assert all(item["type"] != "message" for item in pending["output"]) + assert len(first_model.requests) == 2 + assert client.get("/readiness").json()["foundryResponses"]["stateTtlSeconds"] == 1800 + + # Waiting does not require another response or keep the app's request busy. + now[0] += 599 + other = client.post("/responses", json={"input": "Check site-b.", "agent_session_id": "other-session"}).json() + other_final = client.post("/responses", json=_result(other, {"approved": True, "output": {"signal": "normal"}}, session="other-session"), headers=CONTINUATION_AUTH) + assert other_final.status_code == 200 + assert _message_text(other_final.json()) == "The independent lookup finished." + assert len(first_model.requests) == 4 + + # Restart only the AgentKit process with saved state, within the same hosted + # session. Account for the remaining review second and 240s execution budget. + now[0] += 241 + second_model = _FakeChatTransport([] if outcome == "tool_outcome_unknown" else [ + _chat_response({"role": "assistant", "content": f"Final outcome: {outcome}"}), + ]) + output = ( + {"approved": True, "output": {"receipt": "simulated-work-order-1"}} + if outcome == "approved" + else {"approved": False, "error": {"code": outcome, "message": "UNSAFE_TOOL_DETAIL", "details": "UNSAFE_TOOL_DETAIL"}} + ) + payload = _result(pending, output) + with TestClient(_model_loop_app(spec, second_model, response_state_file=state_file)) as client: + wrong_session = dict(payload, agent_session_id="other-session") + assert client.post("/responses", json=wrong_session, headers=CONTINUATION_AUTH).status_code == 409 + assert client.post("/responses", json=payload).status_code == 403 + assert second_model.requests == [] + final = client.post("/responses", json=payload, headers=CONTINUATION_AUTH) + assert final.status_code == 200, final.text + duplicate = client.post("/responses", json=payload, headers=CONTINUATION_AUTH) + assert duplicate.json() == final.json() + conflict = _result(pending, {"approved": True, "output": {"receipt": "different"}}) + assert client.post("/responses", json=conflict, headers=CONTINUATION_AUTH).status_code == 409 + if outcome == "tool_outcome_unknown": + assert second_model.requests == [] + assert _message_text(final.json()) == f"{outcome}: {_ERRORS[outcome]}" + else: + assert len(second_model.requests) == 1 + messages = second_model.requests[0]["messages"] + assert len([message for message in messages if message["role"] == "user"]) == 1 + tool_results = [message for message in messages if message["role"] == "tool"] + assert len(tool_results) == 2 + assert json.loads(tool_results[0]["content"]) == first_output + assert tool_results[1]["tool_call_id"] == _call(pending)["call_id"] + expected = output if outcome == "approved" else {"approved": False, "error": {"code": outcome, "message": _ERRORS[outcome]}} + assert json.loads(tool_results[1]["content"]) == expected + assert "UNSAFE_TOOL_DETAIL" not in json.dumps(messages) + + +@pytest.mark.parametrize("saved_state", [False, True]) +def test_lost_or_expired_review_state_never_restarts_the_original_action(tmp_path, monkeypatch, saved_state): + now = [1000.0] + monkeypatch.setattr(foundry_module, "time", SimpleNamespace(time=lambda: now[0])) + state_file = tmp_path / "responses.json" if saved_state else None + spec = _spec(tool_name="create-simulated-work-order", brokered_class="write") + first_model = _FakeChatTransport([_tool("create-simulated-work-order")]) + with TestClient(_model_loop_app(spec, first_model, response_state_file=state_file, state_ttl_seconds=1800)) as client: + initial = client.post("/responses", json={"input": "Create a simulated work order.", "agent_session_id": "review-session"}).json() + now[0] += 1801 + second_model = _FakeChatTransport([]) + with TestClient(_model_loop_app(spec, second_model, response_state_file=state_file, state_ttl_seconds=1800)) as client: + response = client.post("/responses", json=_result(initial, {"approved": True, "output": {"receipt": "late"}}), headers=CONTINUATION_AUTH) + assert response.status_code in {404, 410} + assert response.json()["error"]["code"] in {"unknown_previous_response_id", "response_state_expired"} + assert second_model.requests == [] + + +@pytest.mark.parametrize("code", _ERRORS) +def test_deterministic_final_error_preserves_code_without_claiming_an_action_did_not_run(code): + with TestClient(_app(_spec(tool_name="create-simulated-work-order", brokered_class="write"))) as client: + initial = client.post("/responses", json={"input": "create-simulated-work-order", "agent_session_id": "review-session"}).json() + response = client.post("/responses", json=_result(initial, {"approved": False, "error": {"code": code, "message": "UNSAFE_TOOL_DETAIL"}}), headers=CONTINUATION_AUTH) + assert response.status_code == 200 + assert _message_text(response.json()) == f"Brokered tool create-simulated-work-order: {code}: {_ERRORS[code]}" diff --git a/runtimes/common/tests/test_foundry_brokered_protocol.py b/runtimes/common/tests/test_foundry_brokered_protocol.py index b1db2f4..32785b2 100644 --- a/runtimes/common/tests/test_foundry_brokered_protocol.py +++ b/runtimes/common/tests/test_foundry_brokered_protocol.py @@ -1852,15 +1852,15 @@ def test_foundry_brokered_decline_policy_rejection_and_execution_error_are_truth cases = [ ( {"approved": False, "error": {"code": "approval_declined", "message": "Human declined dispatch-work-order"}}, - "approval_declined: Human declined dispatch-work-order", + "Brokered tool dispatch-work-order: approval_declined: The tool call was declined.", ), ( {"approved": False, "error": {"code": "tool_policy_rejected", "message": "writes are disabled"}}, - "tool_policy_rejected: writes are disabled", + "Brokered tool dispatch-work-order was not performed: tool_policy_rejected: writes are disabled", ), ( {"approved": False, "error": {"code": "tool_execution_failed", "message": "downstream timed out"}}, - "tool_execution_failed: downstream timed out", + "Brokered tool dispatch-work-order: tool_execution_failed: MCP tool execution failed.", ), ] @@ -1871,7 +1871,7 @@ def test_foundry_brokered_decline_policy_rejection_and_execution_error_are_truth call = _call(initial) resp = client.post("/responses", json=_continuation(initial["id"], call["call_id"], payload), headers=CONTINUATION_AUTH) assert resp.status_code == 200, resp.text - assert _message_text(resp.json()) == f"Brokered tool dispatch-work-order was not performed: {expected}" + assert _message_text(resp.json()) == expected def test_foundry_brokered_rejects_multiple_tool_outputs_deterministically(): diff --git a/runtimes/microsoft-agent-framework/agentkit_serve/agent_factory.py b/runtimes/microsoft-agent-framework/agentkit_serve/agent_factory.py index 050e7b3..e83b7c7 100644 --- a/runtimes/microsoft-agent-framework/agentkit_serve/agent_factory.py +++ b/runtimes/microsoft-agent-framework/agentkit_serve/agent_factory.py @@ -64,6 +64,7 @@ RuntimeSession, offline_orka_echo_enabled, ) +from agentkit_serve_common.tool_errors import orka_tool_error_details _AUTH_WORKLOAD_IDENTITY = "workload-identity-token" _CONTEXT_TYPE_SEARCH = "search" @@ -77,13 +78,14 @@ _DEFAULT_SESSION_CACHE_MAX = 256 # Invocation tasks share a fatal-error signal with their run owner. Concurrent # Sessions on the same Agent have independent signals. -_run_failure: ContextVar[asyncio.Future[str] | None] = ContextVar("agentkit_maf_run_failure", default=None) +_run_failure: ContextVar[asyncio.Future[AgentRunError] | None] = ContextVar("agentkit_maf_run_failure", default=None) +_ORKA_TOOL_ERROR_CONTEXT_KEY = "agentkit_orka_tool_error" -def _fail_run(message: str) -> None: +def _fail_run(message: str, *, code: str | None = None) -> None: failure = _run_failure.get() if failure is not None and not failure.done(): - failure.set_result(message) + failure.set_result(AgentRunError(message, code=code)) def _mcp_request_timeout() -> int: @@ -285,6 +287,15 @@ class _MCPToolError(Exception): """A validated MCP result reports an admitted tool execution failure.""" +class _OrkaToolError(_MCPToolError): + """A broker result whose code and message come from the fixed allowlist.""" + + def __init__(self, code: str, message: str) -> None: + super().__init__(f"{code}: {message}") + self.code = code + self.message = message + + class _MCPProtocolError(Exception): """An MCP call failed without an admitted tool error result.""" @@ -312,6 +323,9 @@ def _parse_tool_result_from_mcp(self, result): if not isinstance(result, CallToolResult): raise _MCPProtocolError("MCP tool protocol failed") if result.isError: + details = orka_tool_error_details(result.structuredContent) + if details is not None: + raise _OrkaToolError(*details) raise _MCPToolError("MCP tool execution failed") return super()._parse_tool_result_from_mcp(result) @@ -383,9 +397,17 @@ async def process( ) -> None: failure = _run_failure.get() if failure is not None and failure.done(): - raise MiddlewareTermination(failure.result()) + raise MiddlewareTermination(str(failure.result())) try: await call_next() + except _OrkaToolError as exc: + context.metadata[_ORKA_TOOL_ERROR_CONTEXT_KEY] = exc + if exc.code == "tool_outcome_unknown": + _fail_run(str(exc), code=exc.code) + raise MiddlewareTermination(str(exc)) from None + # The SDK hides ordinary exception messages from the model. Project + # only these fixed broker outcomes through its normal tool result. + context.result = {"isError": True, "code": exc.code, "message": exc.message} except _MCPProtocolError: _fail_run("MCP tool protocol failed") # MiddlewareTermination stops the model loop even on MAF 1.9, @@ -825,7 +847,8 @@ async def process( except Exception: await self._emit(ToolCallEvent(call_id, name, "failed")) raise - await self._emit(ToolCallEvent(call_id, name, "completed")) + status = "failed" if isinstance(context.metadata.get(_ORKA_TOOL_ERROR_CONTEXT_KEY), _OrkaToolError) else "completed" + await self._emit(ToolCallEvent(call_id, name, status)) async def run_agent( @@ -840,7 +863,7 @@ async def run_agent( kwargs = {} if request.on_tool_event is not None: kwargs["middleware"] = [_ToolEventMiddleware(request.on_tool_event)] - failure: asyncio.Future[str] = asyncio.get_running_loop().create_future() + failure: asyncio.Future[AgentRunError] = asyncio.get_running_loop().create_future() token = _run_failure.set(failure) async def execute(): @@ -862,7 +885,7 @@ async def execute(): running.cancel() await asyncio.gather(running, return_exceptions=True) if failure.done(): - raise AgentRunError(failure.result()) + raise failure.result() return RunResult(text=_result_text(result), usage=_result_usage(result)) finally: failure.cancel() diff --git a/runtimes/microsoft-agent-framework/tests/test_orka_approvals.py b/runtimes/microsoft-agent-framework/tests/test_orka_approvals.py new file mode 100644 index 0000000..6905136 --- /dev/null +++ b/runtimes/microsoft-agent-framework/tests/test_orka_approvals.py @@ -0,0 +1,230 @@ +"""Orka result compatibility through the real MAF function-invocation loop.""" + +from __future__ import annotations + +import asyncio +import json +import logging +import os +import socket +from contextlib import AsyncExitStack +from unittest import mock + +import pytest +import uvicorn +from mcp import types +from mcp.server.fastmcp import FastMCP + +from agentkit_serve import agent_factory +from agentkit_serve_common.config import AgentSpec, ToolSpec +from agentkit_serve_common.conversation import RunRequest +from agentkit_serve_common.runtime import AgentRunError +from test_mcp_failures import _Client, _setup, _success + + +_PRIVATE = "PRIVATE_UNTRUSTED_TOOL_ERROR" +_ERRORS = { + "approval_declined": "The tool call was declined.", + "approval_expired": "The tool approval expired.", + "approval_cancelled": "The tool call was cancelled.", + "approval_stale": "The tool approval is no longer valid.", + "tool_execution_failed": "MCP tool execution failed.", + "tool_outcome_unknown": "The tool execution outcome is unknown; do not retry.", +} + + +def _error(code): + return types.CallToolResult( + isError=True, + content=[types.TextContent(type="text", text=_PRIVATE)], + structuredContent={"code": code, "message": _PRIVATE, "detail": _PRIVATE}, + ) + + +def _function_results(messages): + return [content for message in messages for content in message.contents if content.type == "function_result"] + + +@pytest.mark.parametrize("outcome", ["approved", *_ERRORS]) +def test_waiting_call_continues_once_with_real_outcome_while_another_agent_progresses(monkeypatch, caplog, outcome): + async def exercise(): + waiting, decision = asyncio.Event(), asyncio.Event() + executions = [] + events = [] + + async def broker(): + waiting.set() + await decision.wait() + if outcome in {"approved", "tool_execution_failed", "tool_outcome_unknown"}: + executions.append("simulated-action") + return _success() if outcome == "approved" else _error(outcome) + + async def observe(event): + events.append(event) + + async with AsyncExitStack() as stack: + agent, server, session, model = await _setup(stack, monkeypatch, [broker]) + run = asyncio.create_task(agent_factory.run_agent(agent, RunRequest("reviewed", on_tool_event=observe))) + try: + await asyncio.wait_for(waiting.wait(), 3) + assert executions == [] and not run.done() + assert len(model.requests["reviewed"]) == 1 + assert [event.status for event in events] == ["in_progress"] + other, _, other_session, _ = await _setup(stack, monkeypatch, [_success()]) + result = await asyncio.wait_for(agent_factory.run_agent(other, RunRequest("independent")), 3) + assert result.text == "done" and len(other_session.calls) == 1 + assert executions == [] and not run.done() + decision.set() + if outcome == "tool_outcome_unknown": + with pytest.raises(AgentRunError, match="tool_outcome_unknown") as caught: + await asyncio.wait_for(asyncio.shield(run), 3) + assert caught.value.code == "tool_outcome_unknown" + assert len(model.requests["reviewed"]) == 1 + else: + result = await asyncio.wait_for(asyncio.shield(run), 3) + assert result.text == "done" and len(model.requests["reviewed"]) == 2 + results = _function_results(model.requests["reviewed"][-1]) + assert len(results) == 1 + if outcome == "approved": + assert json.loads(results[0].result)["text"] == "héllo 世界 🌍" + else: + assert json.loads(results[0].result) == {"isError": True, "code": outcome, "message": _ERRORS[outcome]} + assert len([message for message in model.requests["reviewed"][-1] if message.role == "user"]) == 1 + assert len(session.calls) == 1 and server.connect.await_count == 0 + assert len(executions) == (1 if outcome in {"approved", "tool_execution_failed", "tool_outcome_unknown"} else 0) + assert [event.status for event in events] == ["in_progress", "completed" if outcome == "approved" else "failed"] + finally: + run.cancel() + await asyncio.gather(run, return_exceptions=True) + assert _PRIVATE not in caplog.text + + caplog.set_level(logging.DEBUG, logger="agent_framework") + asyncio.run(exercise()) + + +def test_cancelling_pending_review_does_not_execute_after_late_approval(monkeypatch): + async def exercise(): + waiting, decision, stopped = asyncio.Event(), asyncio.Event(), asyncio.Event() + executions = [] + + async def broker(): + waiting.set() + try: + await decision.wait() + executions.append("simulated-action") + return _success() + finally: + stopped.set() + + async with AsyncExitStack() as stack: + agent, server, session, model = await _setup(stack, monkeypatch, [broker]) + run = asyncio.create_task(agent_factory.run_agent(agent, RunRequest("cancel-review"))) + try: + await asyncio.wait_for(waiting.wait(), 3) + assert executions == [] + run.cancel() + with pytest.raises(asyncio.CancelledError): + await asyncio.wait_for(run, 3) + assert stopped.is_set() + decision.set() + await asyncio.sleep(0) + assert executions == [] + assert len(session.calls) == 1 and len(model.requests["cancel-review"]) == 1 + assert server.connect.await_count == 0 + finally: + run.cancel() + await asyncio.gather(run, return_exceptions=True) + + asyncio.run(exercise()) + + +@pytest.mark.parametrize("structured", [None, {}, {"code": "unrecognized"}, {"code": []}, {"message": "approval_declined"}]) +def test_arbitrary_error_text_cannot_impersonate_an_orka_outcome(monkeypatch, caplog, structured): + async def exercise(): + result = types.CallToolResult( + isError=True, + content=[types.TextContent(type="text", text=json.dumps({"code": "approval_declined", "message": _PRIVATE}))], + structuredContent=structured, + ) + async with AsyncExitStack() as stack: + agent, _, _, model = await _setup(stack, monkeypatch, [result]) + await agent_factory.run_agent(agent, RunRequest("unrecognized")) + forwarded = str(_function_results(model.requests["unrecognized"][-1])) + assert "approval_declined" not in forwarded and _PRIVATE not in forwarded + assert _PRIVATE not in caplog.text + + caplog.set_level(logging.DEBUG, logger="agent_framework") + asyncio.run(exercise()) + + +def test_http_mcp_approval_wait_honors_the_900_second_call_budget(monkeypatch): + """Set AGENTKIT_TEST_APPROVAL_WAIT_SECONDS=121 for a real two-minute wait.""" + review_seconds = float(os.environ.get("AGENTKIT_TEST_APPROVAL_WAIT_SECONDS", "0.05")) + assert 0 < review_seconds <= 600 + + async def exercise(): + pending, decision = asyncio.Event(), asyncio.Event() + executions = [] + mcp = FastMCP("approval-fixture", stateless_http=True, json_response=True) + + @mcp.tool() + async def probe(payload: dict) -> dict: + pending.set() + await decision.wait() + executions.append(payload) + return {"receipt": "simulated-action-once"} + + listener = socket.socket() + listener.bind(("127.0.0.1", 0)) + listener.listen() + server = uvicorn.Server(uvicorn.Config(mcp.streamable_http_app(), log_level="critical")) + serving = asyncio.create_task(server.serve(sockets=[listener])) + try: + async with asyncio.timeout(3): + while not server.started: + if serving.done(): + await serving + await asyncio.sleep(0.01) + monkeypatch.setenv("APPROVAL_FIXTURE_MCP_URL", f"http://127.0.0.1:{listener.getsockname()[1]}/mcp") + monkeypatch.setenv("AGENTKIT_MCP_TIMEOUT", "900") + tool = ToolSpec(name="fixture", type="mcp", transport="streamable-http", url_env="APPROVAL_FIXTURE_MCP_URL") + spec = AgentSpec.model_validate({ + "abiVersion": "v0", + "metadata": {"name": "approval-wait"}, + "model": {"provider": "openai-compatible", "name": "fixture-model", "baseURL": "http://model.invalid/v1"}, + "instructions": "Use the simulated action.", + "tools": [tool.model_dump()], + "expose": {"openai": True, "port": 8080}, + }) + async with AsyncExitStack() as stack: + remote = agent_factory.build_tool(tool, stack=stack) + assert remote.request_timeout == 900 + assert remote._httpx_client.timeout.read == 900 + await stack.enter_async_context(remote) + model = _Client(remote.functions[0].name) + with mock.patch.object(agent_factory, "build_tool", return_value=remote.functions[0]): + agent = await stack.enter_async_context(agent_factory.build_agent(spec, client=model)) + run = asyncio.create_task(agent_factory.run_agent(agent, RunRequest("review-over-http"))) + try: + await asyncio.wait_for(pending.wait(), 3) + await asyncio.sleep(review_seconds) + assert executions == [] and not run.done() + assert len(model.requests["review-over-http"]) == 1 + decision.set() + assert (await asyncio.wait_for(asyncio.shield(run), 5)).text == "done" + assert len(executions) == 1 and len(model.requests["review-over-http"]) == 2 + results = _function_results(model.requests["review-over-http"][-1]) + assert len(results) == 1 + assert json.loads(results[0].result) == {"receipt": "simulated-action-once"} + finally: + run.cancel() + await asyncio.gather(run, return_exceptions=True) + finally: + decision.set() + server.should_exit = True + try: + await asyncio.wait_for(serving, 5) + finally: + listener.close() + + asyncio.run(exercise())