diff --git a/bot_bottle/supervise.py b/bot_bottle/supervise.py index 0acd593..a1bc0aa 100644 --- a/bot_bottle/supervise.py +++ b/bot_bottle/supervise.py @@ -47,6 +47,7 @@ from .supervise_types import ( STATUS_MODIFIED, STATUS_REJECTED, TOOLS, + TOOL_CHECK_PROPOSAL, TOOL_EGRESS_ALLOW, TOOL_EGRESS_BLOCK, TOOL_EGRESS_TOKEN_ALLOW, @@ -263,6 +264,7 @@ __all__ = [ "TOOLS", "EGRESS_FORWARD_PROXY", "EGRESS_INTROSPECT_URL", + "TOOL_CHECK_PROPOSAL", "TOOL_EGRESS_ALLOW", "TOOL_EGRESS_BLOCK", "TOOL_GITLEAKS_ALLOW", diff --git a/bot_bottle/supervise_server.py b/bot_bottle/supervise_server.py index 3cfcf02..e054d7e 100644 --- a/bot_bottle/supervise_server.py +++ b/bot_bottle/supervise_server.py @@ -2,14 +2,24 @@ Per-bottle MCP server exposing tools the agent calls to propose egress config changes when stuck. The tools are `egress-allow`, -`egress-block`, and `list-egress-routes`. +`egress-block`, `list-egress-routes`, and `check-proposal`. -Each queued tool call: +Each queued proposal tool call: 1. Validates the proposed file syntactically. 2. Writes a Proposal to the host SQLite database. - 3. Blocks polling for a matching Response row. - 4. Returns the operator's `{status, notes}` to the agent. + 3. Blocks polling for a matching Response row, up to a short grace + window (`SUPERVISE_RESPONSE_TIMEOUT_SECONDS`, default 30s). + 4. On a decision within the window, returns the operator's + `{status, notes}`. On timeout, returns `status: pending` **with the + proposal id** and leaves the proposal queued — the flow is + non-blocking past the grace window (PRD prd-new / issue #412). + +`check-proposal` is the non-blocking companion: given a `proposal_id` +returned by a `pending` response, it reports the current decision +(`pending` | `approved` | `modified` | `rejected`) without re-proposing, +so an approval made out-of-band (e.g. a web review console) can be resumed +without holding an HTTP request open. One shared server fronts every bottle (PRD 0070) and attributes each proposal to the calling bottle by source IP, resolved from the orchestrator @@ -22,7 +32,9 @@ Speaks MCP over HTTP+JSON-RPC. Methods handled: * `initialize` — handshake; returns server info + caps. * `notifications/initialized` — ack-only. * `tools/list` — returns the tool definitions. - * `tools/call` — validates, queues, blocks, returns. + * `tools/call` — validates, queues, waits out the grace + window, returns (pending past it); or, for + `check-proposal`, a non-blocking status poll. Everything else returns JSON-RPC error -32601 (method not found). @@ -232,6 +244,31 @@ TOOL_DEFINITIONS: list[dict[str, object]] = [ ), "inputSchema": _proposal_input_schema(), }, + { + "name": _sv.TOOL_CHECK_PROPOSAL, + "description": ( + "Poll a previously queued proposal for the operator's decision " + "WITHOUT blocking or re-proposing. Pass the `proposal_id` you " + "got back when an `egress-allow`/`egress-block` call returned " + "`status: pending`. Returns the current status: `pending` (no " + "decision yet — poll again later), `approved`, `modified`, " + "`rejected`, or `unknown` (no such queued proposal — wrong id, " + "or it was already resolved and read)." + ), + "inputSchema": { + "type": "object", + "properties": { + "proposal_id": { + "type": "string", + "description": ( + "The proposal id from a `pending` response." + ), + }, + }, + "required": ["proposal_id"], + "additionalProperties": False, + }, + }, ] @@ -353,7 +390,7 @@ def handle_tools_call( deadline=deadline, ) except TimeoutError: - text = format_pending_response_text(config.response_timeout_seconds) + text = format_pending_response_text(proposal.id, config.response_timeout_seconds) return { "content": [{"type": "text", "text": text}], "isError": False, @@ -370,6 +407,54 @@ def handle_tools_call( } +def handle_check_proposal( + params: dict[str, object], + config: ServerConfig, +) -> dict[str, object]: + """Non-blocking poll of a queued proposal's decision, by id. + + Never creates a Proposal (so `check-proposal` isn't in `TOOLS`); it only + reads the queue. Resolution order mirrors the synchronous path's terminal + step — a decided proposal is archived here exactly as `handle_tools_call` + archives it after `wait_for_response`, so `pending` proposals stay visible + to the operator until they're both decided *and* polled.""" + args_raw = params.get("arguments", {}) + if not isinstance(args_raw, dict): + raise _RpcClientError(ERR_INVALID_PARAMS, "tools/call 'arguments' must be an object") + proposal_id = args_raw.get("proposal_id") + if not isinstance(proposal_id, str) or not proposal_id.strip(): + raise _RpcClientError( + ERR_INVALID_PARAMS, + "check-proposal: 'proposal_id' is required and must be a non-empty string", + ) + proposal_id = proposal_id.strip() + + try: + response = _sv.read_response(config.bottle_slug, proposal_id) + except FileNotFoundError: + # No decision yet — distinguish "still queued" from "unknown id". + try: + _sv.read_proposal(config.bottle_slug, proposal_id) + except FileNotFoundError: + return { + "content": [{"type": "text", "text": format_unknown_proposal_text(proposal_id)}], + "isError": True, + } + return { + "content": [{"type": "text", "text": format_still_pending_text(proposal_id)}], + "isError": False, + } + + try: + _sv.archive_proposal(config.bottle_slug, proposal_id) + except OSError as e: + raise _RpcInternalError(f"failed to archive proposal: {e}") from e + return { + "content": [{"type": "text", "text": format_response_text(response)}], + "isError": response.status == _sv.STATUS_REJECTED, + } + + def format_response_text(response: "_sv.Response") -> str: """Pretty-print a Response for the tool's text content. The agent reads the text and decides whether to retry / give up / surface.""" @@ -382,12 +467,35 @@ def format_response_text(response: "_sv.Response") -> str: return "\n".join(lines) -def format_pending_response_text(timeout_seconds: float) -> str: +def format_pending_response_text(proposal_id: str, timeout_seconds: float) -> str: + """Grace-window timeout: the proposal stays queued, and the agent is + told the id so it can `check-proposal` instead of re-proposing.""" return "\n".join([ "status: pending", + f"proposal_id: {proposal_id}", ( - "notes: operator response timed out after " - f"{timeout_seconds:g}s; proposal remains queued" + f"notes: no operator decision within {timeout_seconds:g}s; the " + "proposal remains queued. Poll it (do not re-propose) by calling " + f"`check-proposal` with proposal_id={proposal_id!r}." + ), + ]) + + +def format_still_pending_text(proposal_id: str) -> str: + return "\n".join([ + "status: pending", + f"proposal_id: {proposal_id}", + "notes: still queued; no operator decision yet. Call `check-proposal` again later.", + ]) + + +def format_unknown_proposal_text(proposal_id: str) -> str: + return "\n".join([ + "status: unknown", + f"proposal_id: {proposal_id}", + ( + "notes: no queued proposal with this id for this bottle — the id " + "may be wrong, or the proposal was already resolved and read." ), ]) @@ -482,6 +590,11 @@ class MCPHandler(http.server.BaseHTTPRequestHandler): # — silently dropping base routes like api.anthropic.com on approval. if req.params.get("name") == _sv.TOOL_LIST_EGRESS_ROUTES: return self._resolved_routes_payload() + # `check-proposal` is a non-blocking read of the calling bottle's + # own queue — attributed by source IP like a proposal, but it + # never queues or blocks. + if req.params.get("name") == _sv.TOOL_CHECK_PROPOSAL: + return handle_check_proposal(req.params, self._attributed_config(config)) # Attribute the proposal to the source-IP-resolved bottle, so the one # shared server queues each bottle's proposal under its own slug. return handle_tools_call(req.params, self._attributed_config(config)) diff --git a/bot_bottle/supervise_types.py b/bot_bottle/supervise_types.py index d62343d..f88203a 100644 --- a/bot_bottle/supervise_types.py +++ b/bot_bottle/supervise_types.py @@ -20,6 +20,10 @@ TOOL_EGRESS_ALLOW = "egress-allow" TOOL_GITLEAKS_ALLOW = "gitleaks-allow" TOOL_EGRESS_TOKEN_ALLOW = "egress-token-allow" TOOL_LIST_EGRESS_ROUTES = "list-egress-routes" +# Read-only agent tool: poll a queued proposal for the operator's decision +# without blocking or re-proposing. It never becomes a `Proposal.tool` (no +# queue record is created for it), so it is intentionally NOT in `TOOLS`. +TOOL_CHECK_PROPOSAL = "check-proposal" TOOLS: tuple[str, ...] = ( TOOL_EGRESS_ALLOW, TOOL_EGRESS_BLOCK, @@ -156,6 +160,7 @@ __all__ = [ "TOOLS", "TOOL_EGRESS_ALLOW", "TOOL_EGRESS_BLOCK", + "TOOL_CHECK_PROPOSAL", "TOOL_EGRESS_TOKEN_ALLOW", "TOOL_GITLEAKS_ALLOW", "TOOL_LIST_EGRESS_ROUTES", diff --git a/docs/prds/prd-new-nonblocking-supervise.md b/docs/prds/prd-new-nonblocking-supervise.md new file mode 100644 index 0000000..f069f8d --- /dev/null +++ b/docs/prds/prd-new-nonblocking-supervise.md @@ -0,0 +1,125 @@ +# PRD prd-new: Non-blocking supervise (async approval + proposal polling) + +- **Status:** Draft +- **Author:** didericis +- **Created:** 2026-07-18 +- **Issue:** #412 + +## Summary + +The per-bottle supervise MCP server (`bot_bottle/supervise_server.py`) +answers `tools/call` **synchronously**: it queues the agent's proposal and +blocks the tool call polling for the operator's decision. On timeout it +returns `status: pending` and leaves the proposal queued — but it hands the +agent **no proposal id** and offers **no way to poll a specific pending +proposal**, so the only way to learn the outcome is to re-propose (a +duplicate). + +This PRD makes the MCP flow non-blocking and pollable, so an approval can +happen out-of-band (a human taking minutes-to-hours in a review console) +without holding an HTTP request open or wedging the agent: + +1. Include the `proposal_id` in the `pending` response. +2. Add a `check-proposal` MCP tool: a non-blocking status lookup by + proposal id. +3. Keep the short synchronous grace window for the common "operator is + right there" fast path. + +## Problem + +`handle_tools_call` → `_sv.wait_for_response(...)` blocks up to +`SUPERVISE_RESPONSE_TIMEOUT_SECONDS` (default 30s). Two problems follow: + +- **Human latency ≠ tool-call latency.** A real review — rendered diff, + RBAC routing to an approver, someone tapping approve on their phone — is + minutes-to-hours. Holding the MCP request open that long is fragile + (proxy/keepalive timeouts, the mitmproxy egress hop, and the agent + harness's own tool-call timeout, which a long block can trip and stall + the whole turn). +- **No resume path.** The pending fallback already exists, but without a + proposal id and a poll tool the agent can't reconnect to that specific + decision — it re-proposes, duplicating the queue entry. + +This is also the precondition for the planned web-console human-review +flow (RBAC, audit retention, mobile) — see issue #412. + +**Safety note:** the MCP tools only *propose* policy changes; enforcement +stays at the egress proxy and the git-gate. Returning early on `pending` +therefore opens no hole — the agent still cannot egress or push anything +unapproved. + +## Goals / success criteria + +- A `pending` MCP response carries the `proposal_id`. +- An agent can call `check-proposal(proposal_id)` and get the current + state (`pending` | `approved` | `modified` | `rejected`) **without + blocking** and **without creating a new proposal**. +- The synchronous fast path (operator approves within the grace window) is + unchanged: the first `tools/call` still returns the decision directly. +- No change to enforcement, attribution (source-IP → bottle), or the + operator-side queue/response schema. + +## Non-goals + +- The git-gate `pre-receive` path (it is synchronous by nature and cannot + poll — its async variant is reject-fast + re-push; tracked as a + follow-up). +- Backpressure / in-flight-proposal caps. +- MCP server→client notifications (event-driven resume). +- Any web-console UI (this PRD is the protocol groundwork it needs). + +## Design + +### `pending` response carries the id + +`handle_tools_call`'s timeout branch formats the pending text with the +`proposal.id` and a pointer to `check-proposal`, so the agent knows what to +poll. + +### `check-proposal` tool + +A new read-only MCP tool (`TOOL_CHECK_PROPOSAL = "check-proposal"`), +attributed to the calling bottle by source IP exactly like the proposal +tools. Input: `{ "proposal_id": string }`. Behavior: + +1. `read_response(slug, id)` → + - **found**: archive the proposal (same terminal step the synchronous + path takes) and return the decision via `format_response_text`; + `isError` iff rejected. +2. **not found** → `read_proposal(slug, id)` → + - **found**: still queued → return `status: pending`. + - **not found**: unknown id, or already resolved-and-archived (e.g. a + second poll) → return `status: unknown`, `isError: true`. + +Both lookups already raise `FileNotFoundError` when absent +(`queue_store.py`), so the handler needs no new store methods. `check-` +`proposal` is the only path (besides the synchronous response) that +archives, so a proposal that times out to `pending` stays visible to the +operator until it is decided and then polled. + +### Grace window + +Left at the existing 30s default (`SUPERVISE_RESPONSE_TIMEOUT_SECONDS`), +which doubles as the instant-approve fast path. Tuning it down is an +operator setting, not a code change; noted for the console rollout. + +## Implementation chunks + +1. **(this PR)** `TOOL_CHECK_PROPOSAL` constant; `check-proposal` tool + definition + `handle_check_proposal`; dispatch wiring; `proposal_id` in + the pending text; unit tests. Files: `bot_bottle/supervise_types.py`, + `bot_bottle/supervise.py` (re-export), `bot_bottle/supervise_server.py`, + `tests/unit/test_supervise_server.py`. +2. **(follow-up)** git-gate `pre-receive` reject-fast + re-push. +3. **(follow-up)** per-bottle in-flight-proposal backpressure cap. +4. **(follow-up)** MCP notifications for event-driven resume; web-console + review flow (RBAC, audit retention) on top. + +## Open questions + +- Should a resolved-but-unpolled proposal auto-archive after some TTL, or + only on poll? (Leaning: only on poll, so a decision is never lost to a + reaper before the agent sees it.) +- Does the agent harness need an explicit "you have a pending proposal" + nudge, or is returning `pending` from the original call enough? (Deferred + to the notifications chunk.) diff --git a/tests/unit/test_supervise_server.py b/tests/unit/test_supervise_server.py index 1ba0f77..115b7e0 100644 --- a/tests/unit/test_supervise_server.py +++ b/tests/unit/test_supervise_server.py @@ -32,7 +32,9 @@ from bot_bottle.supervise_server import ( _RpcError, _RpcInternalError, _response_timeout_from_env, + format_pending_response_text, format_response_text, + handle_check_proposal, handle_initialize, handle_tools_call, handle_tools_list, @@ -218,6 +220,7 @@ class TestHandleToolsList(unittest.TestCase): _sv.TOOL_EGRESS_ALLOW, _sv.TOOL_EGRESS_BLOCK, _sv.TOOL_LIST_EGRESS_ROUTES, + _sv.TOOL_CHECK_PROPOSAL, ]), sorted(names), ) @@ -484,9 +487,10 @@ class TestFormatResponseText(unittest.TestCase): class TestFormatPendingResponseText(unittest.TestCase): def test_formats_timeout_message(self): - text = supervise_server.format_pending_response_text(12.5) + text = supervise_server.format_pending_response_text("prop-9", 12.5) self.assertIn("status: pending", text) self.assertIn("12.5s", text) + self.assertIn("proposal_id: prop-9", text) # --- End-to-end HTTP sanity ------------------------------------------------ @@ -685,5 +689,129 @@ class TestResolvedRoutesPayload(unittest.TestCase): _handler(None)._resolved_routes_payload() +class TestNonBlockingSupervise(unittest.TestCase): + """PRD prd-new / issue #412: pending responses carry the proposal id, and + `check-proposal` polls a queued proposal without blocking or re-proposing.""" + + _ROUTES = "routes:\n - host: example.com\n" + + def setUp(self): + self._tmp = tempfile.TemporaryDirectory(prefix="supervise-nonblock-test.") + self._home_patch = use_bottle_root(Path(self._tmp.name) / ".bot-bottle") + self.config = ServerConfig(bottle_slug="dev") + _qs.QueueStore("dev").migrate() + _as.AuditStore().migrate() + + def tearDown(self): + self._home_patch() + self._tmp.cleanup() + + def _seed_proposal(self) -> "_sv.Proposal": + p = _sv.Proposal.new( + bottle_slug="dev", + tool=_sv.TOOL_EGRESS_ALLOW, + proposed_file=self._ROUTES, + justification="need example.com", + current_file_hash=_sv.sha256_hex(self._ROUTES), + ) + _sv.write_proposal(p) + return p + + def _check(self, proposal_id: str) -> dict[str, object]: + return handle_check_proposal({"arguments": {"proposal_id": proposal_id}}, self.config) + + # --- pending response carries the id --- + + def test_pending_text_includes_id_and_pointer(self): + text = format_pending_response_text("abc-123", 30.0) + self.assertIn("status: pending", text) + self.assertIn("proposal_id: abc-123", text) + self.assertIn("check-proposal", text) + + def test_tools_call_timeout_returns_pending_with_id_and_stays_queued(self): + # No responder → the grace window expires → pending, not blocked forever. + result = handle_tools_call( + { + "name": _sv.TOOL_EGRESS_ALLOW, + "arguments": {"routes_yaml": self._ROUTES, "justification": "x"}, + }, + ServerConfig(bottle_slug="dev", response_timeout_seconds=0.05), + ) + self.assertFalse(result["isError"]) # type: ignore[index] + text = result["content"][0]["text"] # type: ignore[index] + self.assertIn("status: pending", text) + pending = _sv.list_pending_proposals("dev") + self.assertEqual(1, len(pending)) # still queued, not archived + self.assertIn(pending[0].id, text) # agent got the id to poll + + # --- check-proposal poll --- + + def test_check_returns_approved_and_archives(self): + p = self._seed_proposal() + _sv.write_response("dev", _sv.Response(proposal_id=p.id, status=_sv.STATUS_APPROVED, notes="ok")) + result = self._check(p.id) + self.assertFalse(result["isError"]) + text = result["content"][0]["text"] # type: ignore[index] + self.assertIn("status: approved", text) + self.assertIn("notes: ok", text) + with self.assertRaises(FileNotFoundError): # archived on read + _sv.read_proposal("dev", p.id) + + def test_check_rejected_sets_isError(self): + p = self._seed_proposal() + _sv.write_response("dev", _sv.Response(proposal_id=p.id, status=_sv.STATUS_REJECTED, notes="no")) + result = self._check(p.id) + self.assertTrue(result["isError"]) + self.assertIn("status: rejected", result["content"][0]["text"]) # type: ignore[index] + + def test_check_pending_when_no_decision_yet(self): + p = self._seed_proposal() + result = self._check(p.id) + self.assertFalse(result["isError"]) + text = result["content"][0]["text"] # type: ignore[index] + self.assertIn("status: pending", text) + self.assertIn(p.id, text) + self.assertEqual(1, len(_sv.list_pending_proposals("dev"))) # not archived + + def test_check_unknown_id_is_error(self): + result = self._check("no-such-proposal") + self.assertTrue(result["isError"]) + self.assertIn("status: unknown", result["content"][0]["text"]) # type: ignore[index] + + def test_check_missing_id_raises(self): + with self.assertRaises(_RpcClientError) as cm: + handle_check_proposal({"arguments": {}}, self.config) + self.assertEqual(ERR_INVALID_PARAMS, cm.exception.code) + + def test_check_empty_id_raises(self): + with self.assertRaises(_RpcClientError) as cm: + handle_check_proposal({"arguments": {"proposal_id": " "}}, self.config) + self.assertEqual(ERR_INVALID_PARAMS, cm.exception.code) + + def test_check_arguments_must_be_object(self): + with self.assertRaises(_RpcClientError) as cm: + handle_check_proposal({"arguments": []}, self.config) + self.assertEqual(ERR_INVALID_PARAMS, cm.exception.code) + + def test_full_nonblocking_round_trip(self): + # 1. tools/call times out → pending with id + result = handle_tools_call( + { + "name": _sv.TOOL_EGRESS_ALLOW, + "arguments": {"routes_yaml": self._ROUTES, "justification": "x"}, + }, + ServerConfig(bottle_slug="dev", response_timeout_seconds=0.05), + ) + pid = _sv.list_pending_proposals("dev")[0].id + self.assertIn(pid, result["content"][0]["text"]) # type: ignore[index] + # 2. operator decides out-of-band + _sv.write_response("dev", _sv.Response(proposal_id=pid, status=_sv.STATUS_APPROVED, notes="ok")) + # 3. agent resumes by polling — no re-proposing + poll = self._check(pid) + self.assertFalse(poll["isError"]) + self.assertIn("status: approved", poll["content"][0]["text"]) # type: ignore[index] + self.assertEqual([], _sv.list_pending_proposals("dev")) # resolved + archived + + if __name__ == "__main__": unittest.main()