Merge Agent runs runtime adapter contract - #5450
MerverliPy wants to merge 18 commits into
Conversation
Phase 1: Stable RuntimeEvent and RuntimeStatus contract with: - RuntimeEvent dataclass: event_id, seq, run_id, session_id, type, created_at, terminal, payload with secret redaction - RuntimeStatus dataclass: full reconnect/mobile fields including controls, pending_approval_ids, pending_clarify_ids, error, result - make_event() / make_status() factory helpers - Event type and status validation helpers - docs/rfcs/runtime-api-contract.md with Hermex/mobile usage pattern - 16 contract tests covering serialization, validation, redaction No live streaming or route changes. Dependency-light: no imports from api/streaming.py or live runtime globals.
Update agent-runs adapter and runtime routes to handle new Agent approval/clarify response shapes: - respond_approval/respond_clarify map not_found, conflict, not_supported - unified _control_result_response helper maps status to HTTP codes - new TestApprovalClarifyErrorMapping covering all error states - no secrets leaked in any error response path
Phase 15 confirms WebUI agent-runs adapter correctly proxies all Agent runtime endpoints. No code changes required - existing test suite comprehensively covers the contract. Verification: - Run status, events, cancel, approval, clarify all proxy correctly - Error mapping: not_found->404, conflict->409, success->200 - Secret redaction preserved end-to-end - Mobile pending actions resolve correctly in agent-runs mode - 138 tests passed (default mode), 130 passed (agent-runs mode) - 345 Agent runtime tests pass with the same contract
Add live HTTP smoke script and pytest tests for the WebUI agent-runs adapter smoke harness. New files: - scripts/smoke_agent_runs_live.sh — live smoke script - tests/test_agent_runs_live_http_smoke.py — 8 tests Live smoke verified (cross-repo): 1. Runtime capabilities -> agent-runs mode 2. Proxied run status -> terminal state 3. Proxied events -> done event 4. Cancel/stop -> proxies correctly 5. Deployment health -> agent-runs adapter Tests: 146 passed (default), 138 passed/8 expected (agent-runs env) No architecture changes. agent-runs remains opt-in.
No code changes. Verifies: - Deterministic cross-repo smoke (--fake): 11/11 PASSED - Default tests: 146 passed, 0 failed - Agent-runs env tests: 138 passed, 8 expected failures - Real DeepSeek smoke: SKIPPED (no key) - Agent-side approval/clarify deterministic trigger wired - Messaging-adapter smoke plan documented in hermes-agent repo
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if parsed.path.startswith("/api/runs/") and not parsed.path.endswith("/events"): | ||
| return handle_run_status(handler, parsed) |
There was a problem hiding this comment.
startswith("/api/runs/") and not endswith("/events")) also matches action sub-paths such as /api/runs/{id}/cancel, /api/runs/{id}/approval, and /api/runs/{id}/clarify when called via GET. The path then lands in handle_run_status, which returns 400 "invalid run_id" because of the "/" check — technically safe, but misleading. Adding an explicit check that the remainder contains no / makes the intent clear.
| if parsed.path.startswith("/api/runs/") and not parsed.path.endswith("/events"): | |
| return handle_run_status(handler, parsed) | |
| if parsed.path.startswith("/api/runs/") and not parsed.path.endswith("/events") and "/" not in parsed.path[len("/api/runs/"):]: | |
| return handle_run_status(handler, parsed) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Phase 31 review-response update: Pushed repair commit Fixes included:
Verification:
Current PR status:
Ready for re-review. |
| try: | ||
| health_url = agent_base_url.rstrip("/") + "/v1/health" | ||
| req = urllib.request.Request(health_url, method="GET") | ||
| api_key = os.getenv("HERMES_WEBUI_AGENT_RUNS_API_KEY", "").strip() | ||
| if api_key: | ||
| req.add_header("Authorization", "Bearer " + api_key) | ||
| with urllib.request.urlopen(req, timeout=5) as resp: | ||
| agent_runtime_reachable = resp.status < 500 | ||
| if agent_runtime_reachable and resp.status < 400: | ||
| import json | ||
|
|
||
| try: | ||
| body = json.loads(resp.read().decode("utf-8", errors="replace")) | ||
| agent_api_version = str( | ||
| body.get("version") or body.get("api_version") or "" | ||
| ) or None | ||
| except Exception: | ||
| pass | ||
| except Exception: |
There was a problem hiding this comment.
urllib.request.urlopen follows HTTP redirects by default and sends all original headers — including Authorization: Bearer <api_key> — to the redirect destination. AgentRunsClient deliberately prevents this with a custom _NoRedirect opener (HTTPRedirectHandler that returns None). That protection is absent here: a 301/302 from the configured agent health URL would silently forward the API key to a third-party host. If the agent base URL ever serves a cross-origin redirect (e.g., HTTP→HTTPS misconfiguration or a compromised DNS record), the bearer token is leaked.
| if runtime_adapter_agent_runs_enabled(): | ||
| adapter = _adapter() | ||
| if adapter is None: | ||
| return json_response( | ||
| handler, | ||
| {"error": "agent_runtime_unreachable", "message": "agent-runs adapter is not configured."}, | ||
| status=502, | ||
| ) | ||
| run_id = str(body.get("run_id") or "").strip() | ||
| approval_id = str(body.get("approval_id") or "").strip() | ||
| choice = str(body.get("choice") or "accept").strip() | ||
| result = adapter.respond_approval(run_id, approval_id, choice) |
There was a problem hiding this comment.
run_id validation in approval and clarify handlers
handle_run_cancel explicitly returns 400 when run_id is empty, but the parallel handle_run_approval and handle_run_clarify handlers forward an empty string straight to the remote adapter. A POST to /api/runs//approval sets run_id="" in the body (via the routing code), which then calls adapter.respond_approval("", ...) and fires an HTTP request to the agent at /v1/runs//approval, an invalid path. handle_run_cancel already has the correct guard — the same check is needed here.
| if runtime_adapter_agent_runs_enabled(): | |
| adapter = _adapter() | |
| if adapter is None: | |
| return json_response( | |
| handler, | |
| {"error": "agent_runtime_unreachable", "message": "agent-runs adapter is not configured."}, | |
| status=502, | |
| ) | |
| run_id = str(body.get("run_id") or "").strip() | |
| approval_id = str(body.get("approval_id") or "").strip() | |
| choice = str(body.get("choice") or "accept").strip() | |
| result = adapter.respond_approval(run_id, approval_id, choice) | |
| if runtime_adapter_agent_runs_enabled(): | |
| adapter = _adapter() | |
| if adapter is None: | |
| return json_response( | |
| handler, | |
| {"error": "agent_runtime_unreachable", "message": "agent-runs adapter is not configured."}, | |
| status=502, | |
| ) | |
| run_id = str(body.get("run_id") or "").strip() | |
| if not run_id: | |
| return bad(handler, "run_id is required", 400) | |
| approval_id = str(body.get("approval_id") or "").strip() | |
| if not approval_id: | |
| return bad(handler, "approval_id is required", 400) | |
| choice = str(body.get("choice") or "accept").strip() | |
| result = adapter.respond_approval(run_id, approval_id, choice) |
Cross-repo contract review — two
|
|
Thanks for the substantial effort here, @MerverliPy — but I'm going to decline this in its current form. The honest reasoning: this is ~9,400 lines adding a whole new remote-runtime + mobile-API architectural layer (runtime contract/journal/routes/adapter, a I'm not dismissing the work — if there's a concrete use case I'm missing, I'd genuinely like to hear it. If you can make the case for the specific problem this solves for WebUI users (what can't be done today that this enables, and who needs it), please open an issue laying that out. If the direction lands, the right path would be decomposing this into small, individually-reviewable PRs (each subsystem on its own) with a full security review of the external HTTP contract, and dropping the committed Closing for now — happy to reconsider a scoped, motivated version. Appreciate the ambition. |
2 participants
Adds the Agent runs runtime adapter, runtime routes, mobile pending-action support, deployment health reporting, live smoke harness, runtime contract documentation, and cross-repo integration support for Hermes Agent runtime runs.
Verification completed locally:
Related Agent PR:
Credential-gated checks not run: